Re: [PATCH 4/4] cp210x: Expose the part number in sysfs

[Petr Tesarik]

On Fri, 24 Jul 2015 14:33:23 -0700
Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:

> On Fri, Jul 24, 2015 at 11:00:38PM +0200, Petr Tesarik wrote:
> > On Fri, 24 Jul 2015 11:17:55 -0700
> > Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > 
> > > On Fri, Jul 24, 2015 at 08:48:11AM +0200, Petr Tesarik wrote:
> > > > From: Petr Tesarik <ptesarik@xxxxxxx>
> > > > 
> > > > Make it possible to read the cp210x part number from userspace by making
> > > > it a sysfs attribute.
> > > > 
> > > > Signed-off-by: Petr Tesarik <ptesarik@xxxxxxxx>
> > > 
> > > All sysfs files need to be documented in Documentation/ABI/
> > 
> > Is this a recently added requirement? FWIW there are many undocumented
> > sysfs attributes, even in code maintained by you. E.g. each usbserial
> > (ttyUSB*) device has an attribute called "port_number" which is not
> > documented. Or I'm blind...
> 
> It's been a requirement for years.  If we have missed any, please let me
> know and we will add them.  Sometimes we miss this when adding new
> attributes, and many very old attributes never got documented.

Fair enough. The example I gave is ancient...

>[...]
> > > > @@ -892,13 +905,19 @@ static int cp210x_startup(struct usb_serial
> > > > *serial) &partnum, 1, USB_CTRL_GET_TIMEOUT);
> > > >  	spriv->bPartNumber = partnum & 0xFF;
> > > >  
> > > > -	return 0;
> > > > +	result = device_create_file(&serial->interface->dev,
> > > > +				    &dev_attr_part_number);
> > > 
> > > You just raced with userspace, it will not properly see this attribute
> > > :(
> > 
> > Can you elaborate on this, please? AFAICS the file is created after all
> > required objects had been instantiated already. Where's the race?
> 
> That's the race.  See this blog post for all the details:
> 	http://kroah.com/log/blog/2013/06/26/how-to-create-a-sysfs-file-correctly/

Thank you for the link!

> > > Please never use device_create_file, use an attribute group assigned
> > > to the tty device.  Not the USB interface, that is only for USB
> > > interface "things".
>[...]
> > OK, if the USB interface is the wrong place, what's a good place for
> > such a thing? I don't insist on a sysfs attribute, but I don't agree
> > with the tty device.
> 
> Being a usb-serial driver, you don't have "access" to the main USB
> device, so you are kind of violating some layering rules by taking this
> on to the interface.

True. This is still one of my concerns, but the GPIO functionality is
minor compared to the serial bridge, so strictly following layering
rules would be overkill. See Johan Hovold's answer in the thread about
GPIO support for Silicon Labs cp210x USB serial:

  Indeed, you should just hang the gpio device directly off the
  usb-serial device [...]

> Who / what is going to use this file?  What is going to be done with
> the information and to what device is that information going to be
> associated with?

Many thanks again. Thinking about it some more, once I'm done with my
work, userspace can enumerate the gpio devices using sysfs without
having to care about the specific model. The part number is only
relevant internally to the cp210x driver.

In short, there is in fact no user of this file, so the best option is
to report the model in syslog and forget about a sysfs file. Simple.

Petr
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/