Re: [PATCH 01/11] Platform: add a dev_groups pointer to struct platform_driver

[Greg Kroah-Hartman]

On Thu, Jul 04, 2019 at 11:32:00AM +0200, Johan Hovold wrote:
> On Thu, Jul 04, 2019 at 10:46:07AM +0200, Greg Kroah-Hartman wrote:
> > Platform drivers like to add sysfs groups to their device, but right now
> > they have to do it "by hand".  The driver core should handle this for
> > them, but there is no way to get to the bus-default attribute groups as
> > all platform devices are "special and unique" one-off drivers/devices.
> > 
> > To combat this, add a dev_groups pointer to platform_driver which allows
> > a platform driver to set up a list of default attributes that will be
> > properly created and removed by the platform driver core when a probe()
> > function is successful and removed right before the device is unbound.
> > 
> > Cc: Richard Gong <richard.gong@xxxxxxxxxxxxxxx>
> > Cc: Romain Izard <romain.izard.pro@xxxxxxxxx>
> > Cc: "Rafael J. Wysocki" <rafael@xxxxxxxxxx>
> > Cc: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > Cc: Mans Rullgard <mans@xxxxxxxxx>
> > Cc: Bartosz Golaszewski <bgolaszewski@xxxxxxxxxxxx>
> > Cc: Randy Dunlap <rdunlap@xxxxxxxxxxxxx>
> > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > ---
> >  drivers/base/platform.c         | 40 +++++++++++++++++++++------------
> >  include/linux/platform_device.h |  1 +
> >  2 files changed, 27 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index 713903290385..d81fcd435b52 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -598,6 +598,21 @@ struct platform_device *platform_device_register_full(
> >  }
> >  EXPORT_SYMBOL_GPL(platform_device_register_full);
> >  
> > +static int platform_drv_remove(struct device *_dev)
> > +{
> > +	struct platform_driver *drv = to_platform_driver(_dev->driver);
> > +	struct platform_device *dev = to_platform_device(_dev);
> > +	int ret = 0;
> > +
> > +	if (drv->remove)
> > +		ret = drv->remove(dev);
> > +	if (drv->dev_groups)
> > +		device_remove_groups(_dev, drv->dev_groups);
> 
> Shouldn't you remove the groups before calling driver remove(), which
> could be releasing resources used by the attribute implementations?
> 
> This would also be the reverse of how what you do at probe.

Good point, probably doesn't really matter, but I'll reverse it.

> > +	dev_pm_domain_detach(_dev, true);
> > +
> > +	return ret;
> > +}
> > +
> >  static int platform_drv_probe(struct device *_dev)
> >  {
> >  	struct platform_driver *drv = to_platform_driver(_dev->driver);
> > @@ -614,8 +629,18 @@ static int platform_drv_probe(struct device *_dev)
> >  
> >  	if (drv->probe) {
> >  		ret = drv->probe(dev);
> > -		if (ret)
> > +		if (ret) {
> >  			dev_pm_domain_detach(_dev, true);
> > +			goto out;
> > +		}
> > +	}
> > +	if (drv->dev_groups) {
> > +		ret = device_add_groups(_dev, drv->dev_groups);
> > +		if (ret) {
> > +			platform_drv_remove(_dev);
> 
> This would lead to device_remove_groups() being called for the never
> added attribute groups. Looks like that may trigger a warning in the
> sysfs code judging from a quick look.

Hm, let me look at this some more...