[RFC PATCH] Handle hotplugged devices from xorg.conf

[Dan Nicholson]

On Wed, Dec 3, 2008 at 7:28 PM, Peter Hutterer <peter.hutterer at who-t.net> wrote:
> On Wed, Dec 03, 2008 at 07:20:26AM -0800, Dan Nicholson wrote:
>> Hotplug hooks are added to the HAL core to allow the DDX to handle device
>> hotplugging. If a registered hook returns False, config/hal will continue
>> processing the event. This allows the DDX to examine the type of device
>> and deciding whether to handle it or not.
>
> I'm not quite sure if that approach really fixes the issue - namely to avoid
> duplicated device.
> Duplicate devices can only occur if we have devices in the xorg.conf and we're
> using HAL hotplugging. HAL will always provide a /dev/input/eventX path for
> the device.

The crucial feature here (to me) is not the avoidance of duplicate
devices. That's just a nice side benefit. It's that you can continue
configuring devices in xorg.conf, which is the Xorg global
configuration store. As opposed to an fdi file.

> So a duplicate device can only occur if one of the following is true:
> 1. /dev/input/mice or /dev/input/mouseX devices in the config
> 2. /dev/input/by-id or /dev/input/by-path devices in the config
> 3. /dev/input/eventX devices in the config
>
> Your patch can't address 1 because you cannot know the link between event and
> mouse device You can only say you won't add eventX devices if /dev/input/mice
> is configured, but what's the point of using HAL then...
> Anyway, this problem has already been addressed in the server by deactivating
> the mouse driver if HAL is in use.

Yeah, I don't really care about /dev/input/mice. HAL will never give
us that device, anyway.

> The patch does address 2 by using stat, which AFAIK is the only way to tell
> whether /dev/input/by-id/foobar-event is the same as /dev/input/event0.
> This is already part of evdev however, and I think the driver is the correct
> place for it. There's at least one (out-of-tree) driver that can open the same
> device up to 4 times and it will give you different information each time,
> although it's the same device file. I don't think we should prevent this in
> the server?

I hadn't been thinking about any wackiness like that. It was just the
most reliable way to answer the question "Does this device match
anything that's specified in xorg.conf"? I guess this isn't totally
sufficient, but can't think of a better heuristic off-hand.

> The patch does address 3 through strcmp, but I claim that this is a marginal
> use-case only. Anything that uses a /dev/input/eventX device in the config is
> likely to be break on reboot, since there's no guarantee that the number will
> stay the same.

I'm not concerned about this use case. If you're foolish enough to use
a non-persistent name in your configuration, then you get what you
asked for.

> As it stands now, I think your effort would be better spent on finding a way
> to modify hal setting through a proper tool so that people don't have to
> resort to adding their options to a fdi file. If we had such a tool, we could
> tell people to just stop using xorg.conf completely.

I definitely think that tool needs to be made and would spend time
working on it. However, there is something to be said for having a
global configuration. If I setup my session to work right for the
trackpoint on my laptop, I don't want to then go and create that setup
for every other user, too.

I mean, why not xorg.conf? That is Xorg's configuration file. I agree
that Xorg should be smart enough that I don't need a configuration
file in most cases. However, when I do, isn't it preferable to have it
be xorg.conf rather than for a seemingly orthogonal tool (HAL)?

>>  Issues:
>>  * config_hal_hotplug_hook should probably be a linked list, but this is
>>    all I needed for now.
>>  * I think there's race with HandleConfigFile. config_init happens early
>>    in main and HandleConfigFile doesn't happen until later in InitOutput.
>>    I didn't hit a problem with this in my testing, but I don't know what
>>    would prevent it from happening. One thing I thought of is just to busy
>>    wait in xf86HotplugAddDevice until xf86configptr becomes non-NULL.
>
> config_init only sets up the fds, but they aren't polled until later in
> dispatch. By then we already finished parsing the config file, i.e. by the
> time you get data from HAL, you're well into the first server generation
> already (which btw. is the reason that you can't easily "fall back" to default
> devices, because you never know when the device list may come)

OK, good to know.

>>  * I'm not sure the logic with allowEmptyInput and autoEnableDevices is
>>    correct. Peter?
>
> AutoAddDevices - add devices from hal. Turn this off if you don't want HAL do
> do anything.
> AutoEnableDevices - enable the devices added by hal. I don't think there's a
> real use-case for that either, but it's trivial enough to have.
> AllowEmptyInput - (AAD && AED) by default. If off, the server forces a core
> pointer and a core keyboard through the default configuration (mouse + kbd
> driver). If on, mouse + kbd devices are ignored and the server boots up
> with VCP and VCK only.

Alright. My question is mostly with handling of AllowEmptyInput and
core pointer/keyboard. I think I got it, though.

> A few comments regarding the code, I only did a quick read:
>
>> +
>> +/* Figure out whether an InputDevice in xorg.conf will become a core device.
>> + * This can happen in 3 ways:
>> + * 1. The InputDevice section has Core(Pointer|Keyboard).
>> + * 2. The device was given on the command line from -pointer or -keyboard.
>> + * 3. The active ServerLayout section lists the InputDevice as core.
>> + */
>
> These days, the term CoreDevice only refers to devices that send core events.
> Strictly after the old meaning, the current implementation (1.5, 1.6) has VCP
> and VCK as the Core Pointer and the Core Keyboard and all physical devices as
> "SendCoreEvents". Core now just means that this device will generate a core
> event on the pointer/keyboard focus when it moves. any device that doesn't,
> won't move the visible cursor and won't be able to interact with the GUI.
>
> Master blows that out of the water again, core basically has no meaning
> anymore. Any MD will generate core events, SDs don't generate core events.

Alright. This part is still confusing to me, but I guess I'll just
have to think about it some more.

>> +static Bool
>> +device_is_core(XF86ConfInputPtr conf, IDevPtr idev)
>
> The rest of the file uses CamelCase notation, we should stick with that.

Makes sense.

>
>> +{
>> +    IDevPtr *devs;
>> +
>> +    /* Is this a core device in the configuration? */
>> +    if (xf86CheckBoolOption(conf->inp_option_lst, "CorePointer", FALSE))
>> +        return True;
>> +    if (xf86CheckBoolOption(conf->inp_option_lst, "CoreKeyboard", FALSE))
>> +        return True;
>> +
>> +    /* Was this the device given on the command line? */
>> +    if (xf86PointerName && strcmp(idev->identifier, xf86PointerName) == 0)
>> +        return True;
>> +    if (xf86KeyboardName && strcmp(idev->identifier, xf86KeyboardName) == 0)
>> +        return True;
>> +
>> +    /* Is the InputDevice specified as a core device in the active layout? */
>> +    for (devs = xf86ConfigLayout.inputs; devs && *devs; devs++) {
>> +        IDevPtr idp = *devs;
>> +        if (strcmp(idev->identifier, idp->identifier) != 0)
>> +            continue;
>> +        if (idp->commonOptions &&
>> +            (xf86CheckBoolOption(idp->commonOptions, "CorePointer", FALSE) ||
>> +             xf86CheckBoolOption(idp->commonOptions, "CoreKeyboard", FALSE)))
>> +            return True;
>> +        if (idp->extraOptions &&
>> +            (xf86CheckBoolOption(idp->extraOptions, "CorePointer", FALSE) ||
>> +             xf86CheckBoolOption(idp->extraOptions, "CoreKeyboard", FALSE)))
>> +            return True;
>> +    }
>
> you really really want TRUE as default here.

You mean as the default arg to CheckBoolOption? I want it to return
FALSE unless it's actually set to TRUE in the configuration. From my
understanding, passing TRUE would mean that I would get that back if
the Option wasn't set. Then I would incorrectly match devices that
weren't marked as CorePointer/Keyboard.

What am I missing here?

>> +/* Add a hotplugged device from the configuration. It it's already been
>> + * added or is a core device, skip it. */
>> +static Bool
>> +add_hotplug_device(XF86ConfInputPtr conf, const char *udi)
>> +{
>> +    IDevPtr idev = NULL;
>> +    DeviceIntPtr pdev;
>> +    char *config_info = NULL;
>> +    Bool ret = False;
>> +    int rc;
>> +
>> +    idev = xcalloc(1, sizeof(*idev));
>> +    if (!idev)
>> +        goto unwind;
>> +
>> +    if (!configInput(idev, conf, X_CONFIG))
>> +        goto unwind;
>> +
>> +    config_info = xalloc(strlen(udi) + 6); /* "xf86:" and NULL */
>> +    if (!config_info) {
>> +        xf86Msg(X_ERROR, "xf86: Could not allocate name\n");
>> +        goto unwind;
>> +    }
>> +    sprintf(config_info, "xf86:%s", udi);
>> +
>> +    /* Skip duplicate devices. */
>> +    if (device_is_duplicate(config_info)) {
>> +        xf86Msg(X_WARNING, "xf86: Device %s already added. Ignoring.\n",
>> +                idev->identifier);
>> +        ret = True;
>> +        goto unwind;
>> +    }
>> +
>> +    /* Skip core devices when AllowEmptyInput is false. */
>> +    if (!xf86Info.allowEmptyInput && device_is_core(conf, idev)) {
>> +        xf86MsgVerb(X_INFO, 4, "xf86: Device %s listed as core. Ignoring.\n",
>> +                    idev->identifier);
>> +        ret = True;
>> +        goto unwind;
>> +    }
>
> this bit should - if correctly implemented - leave you with no input
> devices that control the visible sprite.

I'm not sure I'm following. I'm basically just trying to emulate the
logic of checkCoreInputDevices. But it looks like I didn't do that
correctly since allowEmptyInput is only taken into account when an
InputDevice has Option Core*.

>> +
>> +    /* Add the device and enable it if AutoEnableDevices is true. */
>> +    xf86Msg(X_INFO, "xf86: Adding input device %s\n", idev->identifier);
>> +    rc = xf86NewInputDevice(idev, &pdev, xf86Info.autoEnableDevices);
>> +    if (rc != Success) {
>> +        xf86Msg(X_ERROR, "xf86: xf86NewInputDevice failed (%d)\n", rc);
>> +        goto unwind;
>> +    }
>> +
>> +    /* Store the config_info so that we can skip duplicates later. */
>> +    for(; pdev; pdev = pdev->next) {
>> +        if (pdev->config_info)
>> +            xfree(pdev->config_info);
>> +        pdev->config_info = xstrdup(config_info);
>> +    }
>> +    xfree(config_info);
>> +
>> +    return True;
>> +
>> +unwind:
>> +    if (idev)
>> +        xfree(idev);
>> +    if (config_info)
>> +        xfree(config_info);
>> +    return ret;
>> +}
>> +
>> +/* See if we can find an InputDevice in the configuration whose Device
>> + * matches the one being hotplugged. If so, add it. */
>> +Bool
>> +xf86HotplugAddDevice(const char *path, const char *udi)
>> +{
>> +    struct stat device_stat;
>> +    XF86ConfInputPtr conf;
>> +
>> +    if (!xf86configptr) {
>> +        xf86Msg(X_ERROR,
>> +                "xf86: Cannot access global config data structure\n");
>> +        return False;
>> +    }
>> +
>> +    if ((stat(path, &device_stat) < 0) || !S_ISCHR(device_stat.st_mode))
>> +        return False;
>> +
>> +    for (conf = xf86configptr->conf_input_lst; conf; conf = conf->list.next)
>> +    {
>> +        char *conf_path;
>> +        struct stat conf_stat;
>> +
>> +        if (!conf->inp_option_lst)
>> +            continue;
>> +
>> +        conf_path = xf86FindOptionValue(conf->inp_option_lst, "Device");
>> +        if (!conf_path)
>> +            conf_path = xf86FindOptionValue(conf->inp_option_lst, "Path");
>
> is there a driver that supports the Path option? Evdev doesn't anymore,
> synaptics auto-picks the device anyway and I don't think the other drivers do.

Don't know. It was just an easy safeguard, but it could be removed.

Thanks for the quick review.

--
Dan