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

[Petr Tesarik]

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

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

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

Petr T
--
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/