[RFC PATCH] Handle hotplugged devices from xorg.conf

[Peter Hutterer]

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.

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.

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?

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.

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.
 
>  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)

>  * 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.


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.

> +static Bool
> +device_is_core(XF86ConfInputPtr conf, IDevPtr idev)

The rest of the file uses CamelCase notation, we should stick with that.

> +{
> +    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.

> +/* 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.

> +
> +    /* 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.

Cheers,
  Peter