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 <[email protected]>
> > Cc: Romain Izard <[email protected]>
> > Cc: "Rafael J. Wysocki" <[email protected]>
> > Cc: Andy Shevchenko <[email protected]>
> > Cc: Mans Rullgard <[email protected]>
> > Cc: Bartosz Golaszewski <[email protected]>
> > Cc: Randy Dunlap <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: Greg Kroah-Hartman <[email protected]>
> > ---
> > 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...
