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

[Peter Hutterer]

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)

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.

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.

Cheers,
  Peter