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

[Michael Ellerman]

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. 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 :)

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/612c770f/attachment.pgp>