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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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@linuxfoundation.org> wrote:
> 
> > On Fri, Jul 24, 2015 at 08:48:11AM +0200, Petr Tesarik wrote:
> > > From: Petr Tesarik <ptesarik@suse.cz>
> > > 
> > > Make it possible to read the cp210x part number from userspace by making
> > > it a sysfs attribute.
> > > 
> > > Signed-off-by: Petr Tesarik <ptesarik@suse.com>
> > 
> > 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.

> 
> > > ---
> > >  drivers/usb/serial/cp210x.c | 21 ++++++++++++++++++++-
> > >  1 file changed, 20 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/usb/serial/cp210x.c
> > > b/drivers/usb/serial/cp210x.c index dbfc722..66de350 100644
> > > --- a/drivers/usb/serial/cp210x.c
> > > +++ b/drivers/usb/serial/cp210x.c
> > > @@ -871,11 +871,24 @@ static void cp210x_break_ctl(struct
> > > tty_struct *tty, int break_state) cp210x_set_config(port,
> > > CP210X_SET_BREAK, &state, 2); }
> > >  
> > > +static ssize_t part_number_show(struct device *dev,
> > > +				struct device_attribute *attr,
> > > char *buf) +{
> > > +	struct usb_interface *intf = to_usb_interface(dev);
> > > +	struct usb_serial *serial = usb_get_intfdata(intf);
> > > +	struct cp210x_serial_private *spriv =
> > > usb_get_serial_data(serial); +
> > > +	return sprintf(buf, "%i\n", spriv->bPartNumber);
> > > +}
> > > +
> > > +static DEVICE_ATTR_RO(part_number);
> > > +
> > >  static int cp210x_startup(struct usb_serial *serial)
> > >  {
> > >  	struct usb_host_interface *cur_altsetting;
> > >  	struct cp210x_serial_private *spriv;
> > >  	unsigned int partnum;
> > > +	int result;
> > >  
> > >  	spriv = kzalloc(sizeof(*spriv), GFP_KERNEL);
> > >  	if (!spriv)
> > > @@ -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/

> > 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".
> 
> Well, I hesitated with adding it to the USB interface, but adding it to
> the tty device is definitely wrong. This is indeed an attribute of the
> device, not of the tty. If you look at the other CP210x thread, there's
> also a gpio device in the chip. I think it's totally silly to look
> inside a tty interface to see if there are any GPIO pins...
> 
> 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.

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?

thanks,

greg k-h