Message-ID: <20170214042245.GA19688@dtor-ws>
Date:   Mon, 13 Feb 2017 20:22:45 -0800
From:   Dmitry Torokhov <dmitry.torokhov@...il.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     linux-kernel@...r.kernel.org, Guenter Roeck <linux@...ck-us.net>
Subject: Re: [PATCH 1/2] driver core: emit uevents when device is bound to a
 driver

On Mon, Feb 13, 2017 at 04:52:07PM -0800, Greg Kroah-Hartman wrote:
> 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?

Anyone who needs to act on driver-specific attribute. For example, you
could tie firmware updates for some devices to them.

> 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.

No, I mean that current systemd drops events that it does not know about
on the floor. I.e. if it is not in the add, remove, change, move,
offline, or online list, it will get dropped on the floor instead of
being passed on to rules engine. Why? Not sure. Older udev versions did
not need any changes as far as I can see.

Anyway, if we come to an agreement on this I will look into getting
systemd handle this nicely.

> 
> 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, 

"Yeah, well, you know, that’s just, like, your opinion, man." ;)

> 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.

I think we just have fundamental disagreement here. I believe these
attributes belong to the device that is closest to the actual hardware.
This way, even if we come up with brand new input subsystem, inputNG,
and switch over to it, the attributes affecting hardware behavior,
rather than class abstractions, would stay the same. And if we keep
input v1.0 alongside of inputNG for a while we would not need to
duplicate this in both places.

Also, consider attributes such as firmware checksum or configuration
checksum and firmware update triggers that are used to flash persistent
firmware on some devices (Elan touchscreens and touchpads, Atmel touch
controllers, Weida touch controllers, and so on). Quite often, if device
comes up in so-called bootloader mode (because nvram is messed up or
something) there might not even be input device you want to attach
attributes to. The only thing that is there is i2c_client or spi device
or USB interface.

> 
> > 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... :)

Some of them can be fixed, some can't (becaue ABI).

> 
> > 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?

I do not think there would be much confusion between ADD and BIND,
because the attributes are really driver specific (otherwise they'd be
pushed into subsystem) so whoever is creating the rules would know quite
a bit about the device and driver.

> What about just using the existing
> "CHANGE" uevent?

I thought BIND would convey the event more clearly than overloading
CHANGE.

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

We might want to look into switching our firmware updaters to trigger
off udev events (ChromeOS). Also, here is an example of people trying to
automatically configure trackpoint and it being unreliable:

https://wiki.archlinux.org/index.php/TrackPoint#Configuration_at_boot

BTW, what I really want now is to get the patch 2/2 in, as it will allow
cleaning up bunch of drivers. And it does not depend on this patch, I
just sent them together.

Thanks.

-- 
Dmitry

Powered by blists - more mailing lists