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-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html