On Wed, Dec 3, 2008 at 7:28 PM, Peter Hutterer <[EMAIL PROTECTED]> 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 _______________________________________________ xorg mailing list xorg@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/xorg