[Cbe-oss-dev] [patch 10/13] powerpc: sysfs fix compiler warning

[Michael Ellerman]

On Thu, 2007-07-19 at 23:28 -0700, Greg KH wrote:
> On Fri, Jul 20, 2007 at 04:03:09PM +1000, Michael Ellerman wrote:
> > On Thu, 2007-07-19 at 22:09 -0700, Greg KH wrote:
> > > On Thu, Jul 19, 2007 at 10:02:29AM +1000, Michael Ellerman wrote:
> > > > On Thu, 2007-07-19 at 00:32 +0200, Arnd Bergmann wrote:
> > > > > On Wednesday 18 July 2007, Andrew Morton wrote:
> > > > > > >       for_each_possible_cpu(cpu) {
> > > > > > >               sysdev = get_cpu_sysdev(cpu);
> > > > > > > -             sysfs_create_group(&sysdev->kobj, attrs);
> > > > > > > +             error = sysfs_create_group(&sysdev->kobj, attrs);
> > > > > > > +
> > > > > > > +             if (error) {
> > > > > > > +                     for_each_possible_cpu(cpu) {
> > > > > > > +                             sysdev = get_cpu_sysdev(cpu);
> > > > > > > +                             sysfs_remove_group(&sysdev->kobj, attrs);
> > > > > > > +                     }
> > > > > > > +                     break;
> > > > > > > +             }
> > > > > > >       }
> > > > > > >  
> > > > > > >       mutex_unlock(&cpu_mutex);
> > > > > > > -     return 0;
> > > > > > > +     return error;
> > > > > > >  }
> > > > > > >  EXPORT_SYMBOL_GPL(cpu_add_sysdev_attr_group);
> > > > > > 
> > > > > > This will attempt to remove groups which were never added.  I don't know
> > > > > > what will happen then.
> > > > 
> > > > It might BUG if the directory lookup fails, otherwise it will happily do
> > > > nothing AFAICT.
> > > > 
> > > > > I looked at the code and it doesn't look too good. Let's drop this patch for
> > > > > now. Christian, please submit a new patch when you've found a solution.
> > > > 
> > > > 
> > > > Given that we __must_check the create, it'd be nice if the sysfs API
> > > > gave us a bit of help, how about:
> > > > 
> > > > 
> > > > diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
> > > > index 52eed2a..cba7a2c 100644
> > > > --- a/fs/sysfs/group.c
> > > > +++ b/fs/sysfs/group.c
> > > > @@ -65,15 +65,16 @@ int sysfs_create_group(struct kobject * kobj,
> > > >  	return error;
> > > >  }
> > > >  
> > > > -void sysfs_remove_group(struct kobject * kobj, 
> > > > -			const struct attribute_group * grp)
> > > > +static int __sysfs_remove_group(struct kobject * kobj, 
> > > > +			 const struct attribute_group * grp)
> > > >  {
> > > >  	struct dentry * dir;
> > > >  
> > > >  	if (grp->name) {
> > > >  		dir = lookup_one_len_kern(grp->name, kobj->dentry,
> > > >  				strlen(grp->name));
> > > > -		BUG_ON(IS_ERR(dir));
> > > > +		if (IS_ERR(dir))
> > > > +			return PTR_ERR(dir);
> > > >  	}
> > > >  	else
> > > >  		dir = dget(kobj->dentry);
> > > > @@ -83,8 +84,22 @@ void sysfs_remove_group(struct kobject * kobj,
> > > >  		sysfs_remove_subdir(dir);
> > > >  	/* release the ref. taken in this routine */
> > > >  	dput(dir);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +void sysfs_remove_group(struct kobject * kobj, 
> > > > +			const struct attribute_group * grp)
> > > > +{
> > > > +	BUG_ON(__sysfs_remove_group(kobj, grp));
> > > >  }
> > > > +EXPORT_SYMBOL_GPL(sysfs_remove_group);
> > > >  
> > > > +void sysfs_remove_group_please(struct kobject * kobj, 
> > > > +			const struct attribute_group * grp)
> > > > +{
> > > 
> > > Heh, this is a joke, right?
> > 
> > Well yes and no.
> > 
> > > Funny, you have to ask nicely in order for the code to do what you
> > > want...
> > 
> > A bit of politeness never hurts.
> > 
> > > I don't see the problem with the original code that you are creating
> > > this patch for.  Can you explain please?
> > 
> > The problem is you can't do this:
> > 
> > for_each_thing(thing) {
> > 	error = sysfs_create_group(&thing->kobj, attrs);
> > 	if (error) {
> > 		for_each_thing(thing)
> > 			sysfs_remove_group(&thing->kobj, attrs);
> > 	}
> > }
> > 
> > Because calling sysfs_remove_group() on something you haven't created
> > groups for might BUG.
> 
> It does?  Where is the BUG at?  We should allow this as we do allow it
> for individual files.

void sysfs_remove_group(struct kobject * kobj,
                        const struct attribute_group * grp)
{
        struct dentry * dir;

        if (grp->name) {
                dir = lookup_one_len_kern(grp->name, kobj->dentry,
                                strlen(grp->name));
                BUG_ON(IS_ERR(dir));
        }
        else ...

Which I assume will hit if the lookup fails, ie. when the directory
isn't there yet.


> > So you have to keep track by hand, or not use the
> > for_each_ macros, both of which are suboptimal.
> > 
> > So my patch adds a remove routine that is safe to call (I think) even if
> > you've never done the create. And that makes the driver code simpler.
> > QED :)
> 
> I say remove the BUG as it doesn't sound correct.

Cool, I'll send a new patch that just returns if the lookup fails.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/cbe-oss-dev/attachments/20070720/21bf0049/attachment.pgp>