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 _______________________________________________ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg