Messages in this thread | | | Date | Thu, 4 Jul 2019 12:43:11 +0200 | From | Greg Kroah-Hartman <> | Subject | Re: [PATCH 01/11] Platform: add a dev_groups pointer to struct platform_driver |
| |
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@linux.intel.com> > > Cc: Romain Izard <romain.izard.pro@gmail.com> > > Cc: "Rafael J. Wysocki" <rafael@kernel.org> > > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Cc: Mans Rullgard <mans@mansr.com> > > Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > Cc: Randy Dunlap <rdunlap@infradead.org> > > Cc: linux-kernel@vger.kernel.org > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > --- > > 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...
| |