From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932149AbbBGRUf (ORCPT ); Sat, 7 Feb 2015 12:20:35 -0500 Received: from mail.linuxfoundation.org ([140.211.169.12]:59424 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932098AbbBGRUd (ORCPT ); Sat, 7 Feb 2015 12:20:33 -0500 Date: Sat, 7 Feb 2015 18:10:48 +0800 From: Greg Kroah-Hartman To: Takashi Iwai Cc: linux-kernel@vger.kernel.org Subject: Re: [PATCH 0/2] Add device_create_files() and device_remove_files() helpers Message-ID: <20150207101048.GA13928@kroah.com> References: <1422477974-8369-1-git-send-email-tiwai@suse.de> <20150128210547.GA18649@kroah.com> <20150128213421.GA20488@kroah.com> <20150128222851.GA24074@kroah.com> <20150130042626.GA19001@kroah.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Jan 30, 2015 at 05:31:51PM +0100, Takashi Iwai wrote: > At Thu, 29 Jan 2015 20:26:26 -0800, > Greg Kroah-Hartman wrote: > > > > On Thu, Jan 29, 2015 at 12:11:21AM +0100, Takashi Iwai wrote: > > > At Wed, 28 Jan 2015 14:28:51 -0800, > > > Greg Kroah-Hartman wrote: > > > > > > > > On Wed, Jan 28, 2015 at 11:18:57PM +0100, Takashi Iwai wrote: > > > > > At Wed, 28 Jan 2015 13:34:21 -0800, > > > > > Greg Kroah-Hartman wrote: > > > > > > > > > > > > On Wed, Jan 28, 2015 at 10:26:28PM +0100, Takashi Iwai wrote: > > > > > > > At Wed, 28 Jan 2015 13:05:47 -0800, > > > > > > > Greg Kroah-Hartman wrote: > > > > > > > > > > > > > > > > On Wed, Jan 28, 2015 at 09:46:12PM +0100, Takashi Iwai wrote: > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > this is a simple patch to add device_create_files() and > > > > > > > > > device_remove_files() to replace multiple device_create_file() or > > > > > > > > > _remove() calls with a single shot with the device_attr list. > > > > > > > > > > > > > > > > > > It's basically just a clean up, but also helps to simplify the error > > > > > > > > > handling a lot in many existing codes since the function itself does > > > > > > > > > rollback at error. > > > > > > > > > > > > > > > > > > The series contains a patch to apply these to drivers/base/node.c. > > > > > > > > > I have lots of patches (up to 30) to use these in the whole tree, but > > > > > > > > > maybe it'd be easier too apply once after this stuff is merged at > > > > > > > > > first. It's just a cleanup so no urgent task, after all. > > > > > > > > > > > > > > > > I'd like to some day be able to drop device_create_file entirely, as it > > > > > > > > is almost always used in a racy way (but not always, so we can't get rid > > > > > > > > of it today.) > > > > > > > > > > > > > > > > A driver should be using an attribute group and be created/registered > > > > > > > > with it if they want any files associated with it, so giving people the > > > > > > > > ability to add large numbers of files all at once seems like the wrong > > > > > > > > thing to do :) > > > > > > > > > > > > > > Well, through the glance over many codes using device_create_file(), > > > > > > > I think the problem of the attribute group is that there is little > > > > > > > help for generating the entries dynamically. For example, if you have > > > > > > > two groups you want to enable conditionally, what would be the best > > > > > > > way to implement? > > > > > > > > > > > > Use the is_visable() function callback, that's what it is there for. > > > > > > > > > > But if the entries are determined dynamically? Selecting the enabled > > > > > elements from the static list is one way, it'd work in many cases, but > > > > > it's not always the most straightforward way. It often would be > > > > > easier to build up the list dynamically. > > > > > > > > Do you have an example of this? Wouldn't it be the same thing to list > > > > them all in an attribute group, but only say "this is valid" in the > > > > is_visable() callback for those that would have been built up > > > > dynamically? > > > > > > One common scene is the case where a device has already the static > > > group defined in the core helper module while a driver wants to put > > > additional sysfs entries on it. > > > > > > A complex case is something in drivers/leds/trigger/ledtrig-*.c. > > > > > > Another interesting example is drivers/regulator/core.c. It creates a > > > bunch of various sysfs files depending on the client's ops presence. > > > It might be implemented via is_visible, but then it'd become more > > > lengthy (too many small callback functions). > > > > Yeah, I'm not saying it's easy, or simple, it's just the only way I know > > how to do this in a race-free way. We have to create the files before > > the uevent happens, not after, like these drivers are doing. > > > > If you can think of a way that we can do this in a simpler way, that > > would be great. > > The latter one (regulator/core.c) is actually a case where is_visible > callback would work better, I noticed after studying mode code. > Thanks for hints. I'm going to submit a patch later. > > OTOH, the leds class looks not intuitive. Need more investigation. > > > > Also, multiple drivers seem calling device_create_file() from the > > > array of attributes in a loop. One reason might be that it's easier > > > to write for a bunch of entries, without defining many piece of > > > structs. An example is found in drivers/gpu/drm/drm_sysfs.c. > > > > That one should just be adding the whole attribute group, using > > device_add_groups, which we have in the driver core, but I didn't export > > publicly. That is if those are being added in a race-free way, I > > couldn't unwind the drm mess to see if the uevent is happening after the > > files are added or before. > > If we export device_add_groups() and device_remove_groups(), is it > safe to call it before device_add()? If yes, some drivers/subsystems > can have a code flow like: > > some_subsystem_init(struct device *dev) > { > device_initialize(dev); > devs->groups = subsystem_groups; > .... > } > > driver_init(struct device *dev) > { > some_subsystem_init(dev); > device_add_groups(dev, additional_groups); > .... > device_add(dev); > .... > } > > The network device has a own multi dev_groups array so that the driver > can put an own group while the net core fills common groups > dynamically just before the device registration call. I though of > implementing similar for others (including the sound stuff), but if > the scheme above works, the rewrite will become smaller. > > Of corse, the drawback of the explicit device_add_groups() call would > be that you'll have to call device_remove_groups() at removal or error > paths. Right now, no, you can't call device_add_groups() until after device_add() happens, as it device_initialize() doesn't do enough sysfs work in order to be able to create the files. > > > > > What if having a link to the chained group for appending entries > > > > > dynamically? Just a wild idea, but it might make things easier. > > > > > > > > We have the ability to pass a group list pointer to device_create > > > > already, and the attribute pointer is a list of groups as well, how can > > > > we change this to be "easier"? > > > > > > I guess the order is the problem. In many cases, you know the > > > additional entries only after the device creation. The device > > > creation is often done by a helper code. So the driver has no control > > > to it, just gets the resultant device. > > > > Yeah, that's the problem. And another problem is drivers adding > > attributes to devices after they are bound to a device, which is kind of > > pointless, as the uevent is long past at that point in time. I've > > cleaned up a bunch of those, but odds are there are still more to fix. > > Right, there are a bunch of drivers doing it. I guess partly because > they don't need uevents for creation, but also partly because there is > no way to give attribute groups properly in some cases. For example, > misc_register() or register_framebuffer() calls device_create() so the > caller can't pass groups. > > It'd be trivial to extend struct miscdevice to carry an optional group > field and change the call to device_create_with_groups(). But, > fb_info has also common sysfs entries, so it'd need also the solution > above with device_add_groups() in addition. Your patch to do that looks good, I'll queue them all up after 3.20-rc1 is out as it's too close to 3.19 at the moment. thanks, greg k-h