[PATCH] Add a "flags" field to DeleteInputDeviceRequest.

[Dan Nicholson]

On Tue, May 25, 2010 at 4:39 AM, Peter Hutterer
<peter.hutterer at who-t.net> wrote:
> On Thu, May 20, 2010 at 07:11:16PM -0700, Dan Nicholson wrote:
>> On Thu, May 20, 2010 at 6:05 PM, Peter Hutterer
>> <peter.hutterer at who-t.net> wrote:
>> > On Thu, May 20, 2010 at 03:33:41PM -0700, Dan Nicholson wrote:
>> >> On Thu, May 20, 2010 at 1:07 AM, Peter Hutterer
>> >> <peter.hutterer at who-t.net> wrote:
>> >> > Some input drivers need to implement an internal hotplugging scheme for
>> >> > dependent devices to provide multiple X devices off one kernel device file.
>> >> > Such dependent devices can be added with NewInputDeviceRequest() but they are
>> >> > not removed when the config backend calls DeleteInputDeviceRequest(),
>> >> > leaving the original device to clean up.
>> >> >
>> >> > Example of the wacom driver:
>> >> >
>> >> > config/udev calls NewInputDeviceRequest("stylus")
>> >> >
>> >> > wacom PreInit calls
>> >> >        NewInputDeviceRequest("eraser")
>> >> >        NewInputDeviceRequest("pad")
>> >> >        NewInputDeviceRequest("cursor")
>> >> >        PreInit finishes.
>> >> >
>> >> > When the device is removed, the config backend only calls
>> >> > DeleteInputDeviceRequest for "stylus". The driver needs to call
>> >> > DeleteInputDeviceRequest for the dependent devices eraser, pad and cursor to
>> >> > clean up properly.
>> >> > However, when the server terminates, DeleteInputDeviceRequest is called for
>> >> > all devices - the driver must not remove the dependent devices to avoid
>> >> > double-frees. There is no method for the driver to detect why a device is
>> >> > being removed, leading to elaborate guesswork and some amount of wishful
>> >> > thinking.
>> >> >
>> >> > Though the input driver's UnInit already supports flags, they are unused.
>> >> > This patch uses the flags to supply information where the
>> >> > DeleteInputDeviceRequest request originates from, allowing a driver to
>> >> > selectively call DeleteInputDeviceRequest when necessary.
>> >>
>> >> Here's a question that's always bothered me about this. If you set
>> >> DeviceIntPtr->config_info equal to the parent in the subdevices, won't
>> >> a single call to DeleteInputDeviceRequest in the server clean up all
>> >> the devices? See remove_devices() in config/config.c.
>> >>
>> >> Then you wouldn't have to call DeleteInputDeviceRequest from the
>> >> driver at all. Either we'd get a uevent, call DIDP for all devices
>> >> matching config_info, or the server terminates and we call DIDP for
>> >> all devices.
>> >>
>> >> Is there a reason this approach wasn't taken?
>> >
>> > hmm, good suggestion. I need to investigate this, I just never thought of
>> > it. AFAICT from a glance at the source, this still requires server patches
>> > though. config_info on the device isn't set until after NIDR which spawns
>> > off the dependent devices.
>> >
>> > So unless we set a Block or Wakeup handler to create the devices, I'm not
>> > quite sure how to do this. Of course, we could also pass this as an
>> > InputOption into NIDR and just set it there, we already have a few special
>> > options anyway.
>>
>> Yeah, I thought of this later. I think we should pass the config_info
>> into NIDR and set it there regardless. Right now, you have the config
>> backend stupidly resetting after returning from the driver. You could
>> pass it in as an InputOption. Or, if you're breaking ABI, make it a
>> member of InputAttributes and drop DeviceIntPtr->config_info since you
>> want to copy the InputAttributes into DeviceIntPtr anyway. A third ABI
>> breaking option is just to have config_info be a parameter to NIDR.
>>
>> I'm not opposed to a flags for DIDR since there could easily be other
>> things signal to the driver, but it seems like there are better
>> solutions to this particular problem.
>
> our input API is a bit of a mess... ("no, really?" I hear you say? yes. yes
> indeed.)
>
> We have 3 structs that represent an input device. Two in the DDX and one in
> the DIX.
> * IDevRec, which is the most basic one, just identifer, driver an options.
> * LocalDeviceRec (also called InputInfoRec, because, well, yes, we can)
>  which is essentially the one that counts for drivers. It contains the
>  callbacks used by the drivers. and a couple of other variables that aren't
>  really used anymore. The LocalDeviceRec holds a pointer to the IDevRec.
> * DeviceIntRec, the DIX one, is the one that holds most of the interesting
>  information, including a "private" pointer to the LocalDeviceRec (which in
>  turn has a pointer to the DeviceIntRec)

First, let me say thanks for walking through this. I had always
wondered why there were so many structs for the input devices.

>
> In the driver, local and device are mostly interchangable though the
> majority of driver functions are called with local, not device. Notable
> exception are device_control (for DEVICE_ON, DEVICE_OFF, etc.) and the
> property callbacks.
>
> Now, the device initialization process works like this:
>
> config backend gets data from hal, udev, creates InputOptions and passes
> them into NewInputDeviceRequest().
>
> That callocs an IDevRec and fills it with the options. This IDevRec is then
> passed into the driver's PreInit(). The driver is expected to allocate a
> LocalDeviceRec, assign the IDevRec to it and pass the lot back to the DDX.
>
> Now the DDX asks the DIX for a DeviceIntRec and introduces the two to each
> other. "local, meet device", "device, meet local". Rings^WPointers are
> exchanged and the happy couple goes off into the DIX to be activated,
> enabled and whatever else you do as a bunch of interlinked memory chunks.
>
> The abstraction ensures that
> * the config backend only gets the DeviceIntRec, but _after_ it has been
>  initialized. so anything the config backend does won't be noticed by the
>  driver until the device is disabled and/or removed.
> * the DDX has no access to the InputInfoRec until after the driver's PreInit
>  (which is where most of the interesting stuff happens).
> * the driver doesn't have access to the DeviceIntPtr until device activation
>  time, after PreInit.
> * When the driver's UnInit is called, the DeviceIntRec has already been
>  destroyed.
>
> So to get the config_info somehow meaningfully into the DeviceIntRec, we'd
> have to pass it to NewInputDeviceRequest, add it to the InputInfoRec,
> require the driver to assign it from the IDevRec to the InputInfoRec, then
> reassign the pointer to the DeviceIntRec in the DDX and make sure that
> either we're duplicating the same string three times or coordinate really
> nicely that only one of the parties involved (and what a party it is) frees
> the string, preferably at the right time.
>
> This seems...complicated.

Agreed. I guess I'd say to make it easier with the current API so that
it's almost entirely handled in NIDR and drivers only get involved
when they want:

1. Pass config_info to NIDR either as a parameter or part of
InputAttributes or an InputOption
2. Dup it into a new field in the IDevRec so it can be passed to the driver
3. Drivers that care can inspect it and/or make a local copy of it
4. Drivers that care can set config_info in DeviceIntRec to something
else they want with the knowledge that they're probably breaking the
config backend
5. Upon returning from PreInit, NIDR dups config_info into the
DeviceIntRec if it's not set by the driver, which would be the vast
majority of times

> I'm somewhat tempted to just remove the weird abstraction of requiring
> drivers to call xf86AllocateInput and handling all that in the DDX. The
> driver's PreInit would then only get the LocalDevicePtr which at least saves
> us one duplication and guarantees the driver's have config_info available in
> PreInit and only managed by one entity.
>
> Of course, it breaks all drivers and writing this a bit of time that's not
> really available just now. Whoop. dee. doo.

Alright. Let's do the flagged DIDR and call it a day with a mental
TODO on trying harder to get the config info to the drivers.

Reviewed-by: Dan Nicholson <dbn.lists at gmail.com>