Message-ID: <20170213184616.GB20057@dtor-ws>
Date:   Mon, 13 Feb 2017 10:46:16 -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: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.

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

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

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

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

> 
> Is there a specific type of devices that you have that you wish to fix
> up using this new uevent type?

As I shown above, there is not a particular class but rather quite a few
drivers across the kernel.

Thanks.

-- 
Dmitry

Powered by blists - more mailing lists