Re: [PATCH 1/2] driver core: emit uevents when device is bound to a driver

[Greg Kroah-Hartman]

On Mon, Feb 13, 2017 at 10:46:16AM -0800, Dmitry Torokhov wrote:
> On Mon, Feb 13, 2017 at 04:07:01AM -0800, Greg Kroah-Hartman wrote:
> > On Sun, Feb 12, 2017 at 04:36:18PM -0800, Dmitry Torokhov wrote:
> > > Majority of standard for a subsystem device attributes are created at the
> > > same time devices are created, before KOBJECT_ADD uevent is emitted by the
> > > driver core. This means that attributes are there when userspace is
> > > notified about new device appearance.
> > > 
> > > However many drivers create additional driver-specific device attributes
> > > when binding to the device, to provide userspace with additional controls,
> > > and such attributes may not be there yet when userpsace receives
> > > KOBJECT_ADD event.
> > 
> > How about we fix those drivers instead?
> 
> They haven't been fixed so far (for many years), and there are ABI
> concerns that would prevent us from "fixing" them.

I agree, I have a bunch listed that should be fixed up (with groups),
but I don't have the time to do so :(

> > Are you going to change userspace (i.e. libudev) to refresh with this
> > new kobject uevent type?
> 
> Refresh? I do not think we need to refresh anything, as there attributes
> are very devise specific so there would be rules listening for these
> "bind" events and adjust the attributes as needed.

What rules?  Who needs/wants this?  I think libudev caches attributes,
but I could be wrong, when it gets a uevent about a new device being
present.  That would need to be updated with this new uevent.

> But if we do agree on these 2 new actions I'll prepare a patch for
> systemd so that it recognizes them (although why systemd believes that
> it needs to "verify" actions reported by the kernel is beyond me).

It doesn't "verify" anything, it just makes it easier for programs using
libudev to access attributes.  But it's been years since I looked at
that code, it might not be caching anything.  But it needs to be
verified.

Who is going to be relying on this new uevent?

> > The 'groups' field for drivers should handle this, but yes, there are
> > some subsystems that don't really do it, and there are drivers that like
> > to add random sysfs files to their device's directories which I would
> > argue is the correct solution here, but you don't like this, because you
> > say:
> > 
> > > Changing the drivers to introduce intermediate "dummy" device as a
> > > container for such attributes would be wasteful, and in many cases,
> > > braking our sysfs ABI.
> > 
> > I'd argue that you are adding random sysfs files to random device types
> > (i.e. a PCI device gets a random set of sysfs files just depending on
> > what driver bound to it.)  And that's wrong, and is why classes were
> > created.
> 
> Classes are good when you have several devices with common
> characteristics and purpose, they do not fit in the cases when we use
> sysfs to create a device-specific knob. I.e. there s only one device
> implementing IBM Trackpoint protocol. It has the following attributes
> controlling hardware behavior:
> 
> TRACKPOINT_INT_ATTR(sensitivity, TP_SENS, TP_DEF_SENS);
> TRACKPOINT_INT_ATTR(speed, TP_SPEED, TP_DEF_SPEED);
> TRACKPOINT_INT_ATTR(inertia, TP_INERTIA, TP_DEF_INERTIA);
> TRACKPOINT_INT_ATTR(reach, TP_REACH, TP_DEF_REACH);
> TRACKPOINT_INT_ATTR(draghys, TP_DRAGHYS, TP_DEF_DRAGHYS);
> TRACKPOINT_INT_ATTR(mindrag, TP_MINDRAG, TP_DEF_MINDRAG);
> TRACKPOINT_INT_ATTR(thresh, TP_THRESH, TP_DEF_THRESH);
> TRACKPOINT_INT_ATTR(upthresh, TP_UP_THRESH, TP_DEF_UP_THRESH);
> TRACKPOINT_INT_ATTR(ztime, TP_Z_TIME, TP_DEF_Z_TIME);
> TRACKPOINT_INT_ATTR(jenks, TP_JENKS_CURV, TP_DEF_JENKS_CURV);
> TRACKPOINT_INT_ATTR(drift_time, TP_DRIFT_TIME, TP_DEF_DRIFT_TIME);
> 
> TRACKPOINT_BIT_ATTR(press_to_select, TP_TOGGLE_PTSON, TP_MASK_PTSON, 0,
>                     TP_DEF_PTSON);
> TRACKPOINT_BIT_ATTR(skipback, TP_TOGGLE_SKIPBACK, TP_MASK_SKIPBACK, 0,
>                     TP_DEF_SKIPBACK);
> TRACKPOINT_BIT_ATTR(ext_dev, TP_TOGGLE_EXT_DEV, TP_MASK_EXT_DEV, 1,
>                     TP_DEF_EXT_DEV);
> 
> They are not applicable to a generic input device, and thus they do not
> belong to 'inptut' class (I keep only attributes that are common to all
> input devices in input class). Since they control hardware properties of
> device on given port they are attached to serio port of that device.

Ick, that sucks, why can't it be part of a 'misc' input device?  This
makes userspace programs have to do more work by having to look at
non-input devices to find the trackpoint.

> We also have other PS/2 mice protocol having their very own quirks; same
> goes for touchscreens, miscellaneous button devices, etc. If you look
> outside of input, you will see a ton of drivers using
> device_create_file() and sysfs_create_group(). Some of them can be
> changed to use attribute groups attached to a device (although it will
> take some time), and for some they are not in charge of creating the
> device instance, and so they can't:
> 
> drivers/usb/misc/cypress_cy7c63.c
> drivers/usb/misc/cytherm.c
> drivers/usb/misc/trancevibrator.c
> drivers/usb/class/cdc-acm.c
> drivers/usb/atm/cxacru.c
> <...more usb stuff...>

Ick, those USB drivers should be fixed up, if only the maintainer of
that subsystem wasn't so lazy... :)

> drivers/pcmcia/yenta_socket.c
> drivers/pcmcia/soc_common.c
> drivers/rtc/<quite a few>
> drivers/power/supply/<a few>
> drivers/spi/spi-tle62x0.c
> drivers/spi/spi-tle62x0.c
> drivers/ssb/pci.c
> drivers/ssb/pcmcia.c
> 
> ... network drivers, OF and ACPI-instantiated platform devices, media
> devices...
> 
> Some driver create links (for compatibility I guess). These also not
> going to be created with the devices.

Yeah, lots of things get it wrong.

But now you just created 2 different times when the files get created,
so userspace has to monitor two different uevents?  How does it know
what to wait for, a ADD or a BIND?  What about just using the existing
"CHANGE" uevent?

But really, I don't like this, what tools are currently broken that you
are wanting to fix up?

thanks,

greg k-h