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

[Greg KH]

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.

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

thanks,

greg k-h