Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754623AbbGXVrJ (ORCPT ); Fri, 24 Jul 2015 17:47:09 -0400 Received: from smtp2.provo.novell.com ([137.65.250.81]:38639 "EHLO smtp2.provo.novell.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753897AbbGXVrH (ORCPT ); Fri, 24 Jul 2015 17:47:07 -0400 Date: Fri, 24 Jul 2015 23:46:51 +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: <20150724234651.6b049256@hananiah.suse.cz> In-Reply-To: <20150724213323.GA14110@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> <20150724230038.623ff7e5@hananiah.suse.cz> <20150724213323.GA14110@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: 3539 Lines: 86 On Fri, 24 Jul 2015 14:33:23 -0700 Greg Kroah-Hartman 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 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... > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/