linux-kernel - Re: [PATCH 01/11] Platform: add a dev_groups pointer to struct platform

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190704104311.GA16681@kroah.com>
Date:   Thu, 4 Jul 2019 12:43:11 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Johan Hovold <johan@...nel.org>
Cc:     linux-kernel@...r.kernel.org,
        Richard Gong <richard.gong@...ux.intel.com>,
        Romain Izard <romain.izard.pro@...il.com>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Mans Rullgard <mans@...sr.com>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Randy Dunlap <rdunlap@...radead.org>
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@...ux.intel.com>
> > Cc: Romain Izard <romain.izard.pro@...il.com>
> > Cc: "Rafael J. Wysocki" <rafael@...nel.org>
> > Cc: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> > Cc: Mans Rullgard <mans@...sr.com>
> > Cc: Bartosz Golaszewski <bgolaszewski@...libre.com>
> > Cc: Randy Dunlap <rdunlap@...radead.org>
> > Cc: linux-kernel@...r.kernel.org
> > Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.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...