Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754604AbbGXVAs (ORCPT ); Fri, 24 Jul 2015 17:00:48 -0400 Received: from smtp2.provo.novell.com ([137.65.250.81]:49128 "EHLO smtp2.provo.novell.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752134AbbGXVAr (ORCPT ); Fri, 24 Jul 2015 17:00:47 -0400 Date: Fri, 24 Jul 2015 23:00:38 +0200 From: Petr Tesarik To: Greg Kroah-Hartman Cc: Johan Hovold , "open list:USB SERIAL SUBSYSTEM" , open list Subject: Re: [PATCH 4/4] cp210x: Expose the part number in sysfs Message-ID: <20150724230038.623ff7e5@hananiah.suse.cz> In-Reply-To: <20150724181755.GA9835@kroah.com> References: <1437720491-28702-1-git-send-email-ptesarik@suse.com> <1437720491-28702-5-git-send-email-ptesarik@suse.com> <20150724181755.GA9835@kroah.com> Organization: SUSE Linux X-Mailer: Claws Mail 3.11.0 (GTK+ 2.24.28; x86_64-suse-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3262 Lines: 85 On Fri, 24 Jul 2015 11:17:55 -0700 Greg Kroah-Hartman wrote: > On Fri, Jul 24, 2015 at 08:48:11AM +0200, Petr Tesarik wrote: > > From: Petr Tesarik > > > > Make it possible to read the cp210x part number from userspace by making > > it a sysfs attribute. > > > > Signed-off-by: Petr Tesarik > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/