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);
+ 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);
+ return ret;
+ }
+ kobject_uevent(&_dev->kobj, KOBJ_CHANGE);
}
out:
@@ -632,19 +657,6 @@ static int platform_drv_probe_fail(struct device *_dev)
return -ENXIO;
}
-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);
- dev_pm_domain_detach(_dev, true);
-
- return ret;
-}
-
static void platform_drv_shutdown(struct device *_dev)
{
struct platform_driver *drv = to_platform_driver(_dev->driver);
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index cc464850b71e..027f1e1d7af8 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -190,6 +190,7 @@ struct platform_driver {
int (*resume)(struct platform_device *);
struct device_driver driver;
const struct platform_device_id *id_table;
+ const struct attribute_group **dev_groups;
bool prevent_deferred_probe;
};
--
2.22.0
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.
> + 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.
> + return ret;
> + }
> + kobject_uevent(&_dev->kobj, KOBJ_CHANGE);
> }
Johan
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...
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: Johan Hovold <[email protected]>
Cc: [email protected]
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
v2: addressed Johan's comments by reordering when we remove the files
from the device, and clean up on an error in a nicer way. Ended up
making the patch smaller overall, always nice.
drivers/base/platform.c | 16 +++++++++++++++-
include/linux/platform_device.h | 1 +
2 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 713903290385..74428a1e03f3 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -614,8 +614,20 @@ 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) {
+ if (drv->remove)
+ drv->remove(dev);
dev_pm_domain_detach(_dev, true);
+ return ret;
+ }
+ kobject_uevent(&_dev->kobj, KOBJ_CHANGE);
}
out:
@@ -638,6 +650,8 @@ static int platform_drv_remove(struct device *_dev)
struct platform_device *dev = to_platform_device(_dev);
int ret = 0;
+ if (drv->dev_groups)
+ device_remove_groups(_dev, drv->dev_groups);
if (drv->remove)
ret = drv->remove(dev);
dev_pm_domain_detach(_dev, true);
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index cc464850b71e..027f1e1d7af8 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -190,6 +190,7 @@ struct platform_driver {
int (*resume)(struct platform_device *);
struct device_driver driver;
const struct platform_device_id *id_table;
+ const struct attribute_group **dev_groups;
bool prevent_deferred_probe;
};
--
2.22.0
Hi Greg,
On Thu, Jul 4, 2019 at 5:15 AM Greg Kroah-Hartman
<[email protected]> 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.
Why is this limited to platform bus? Drivers for other buses also
often want to augment list of their attributes during probe(). I'd
move it to generic probe handling.
>
> 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: Johan Hovold <[email protected]>
> Cc: [email protected]
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
> ---
> v2: addressed Johan's comments by reordering when we remove the files
> from the device, and clean up on an error in a nicer way. Ended up
> making the patch smaller overall, always nice.
>
> drivers/base/platform.c | 16 +++++++++++++++-
> include/linux/platform_device.h | 1 +
> 2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 713903290385..74428a1e03f3 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -614,8 +614,20 @@ 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) {
> + if (drv->remove)
> + drv->remove(dev);
> dev_pm_domain_detach(_dev, true);
> + return ret;
> + }
> + kobject_uevent(&_dev->kobj, KOBJ_CHANGE);
We already emit KOBJ_BIND when we finish binding device to a driver,
regardless of the bus. I know we still need to teach systemd to handle
it properly, but I think it is better than sprinkling KOBJ_CHANGE
around.
Thanks.
--
Dmitry
On Thu, Jul 04, 2019 at 02:17:22PM -0700, Dmitry Torokhov wrote:
> Hi Greg,
>
> On Thu, Jul 4, 2019 at 5:15 AM Greg Kroah-Hartman
> <[email protected]> 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.
>
> Why is this limited to platform bus? Drivers for other buses also
> often want to augment list of their attributes during probe(). I'd
> move it to generic probe handling.
This is not limited to the platform at all, the driver core supports
this for any bus type today, but it's then up to the bus-specific code
to pass that on to the driver core. That's usually set for the
bus-specific attributes that they want exposed for all devices of that
bus type (see the bus_groups, dev_groups, and drv_groups pointers in
struct bus_type).
For the platform devices, the problem is that this is something that the
individual drivers want after they bind to the device. And as all
platform devices are "different" they can't be a "common" set of
attributes, so they need to be created after the device is bound to the
driver.
> > 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: Johan Hovold <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: Greg Kroah-Hartman <[email protected]>
> > ---
> > v2: addressed Johan's comments by reordering when we remove the files
> > from the device, and clean up on an error in a nicer way. Ended up
> > making the patch smaller overall, always nice.
> >
> > drivers/base/platform.c | 16 +++++++++++++++-
> > include/linux/platform_device.h | 1 +
> > 2 files changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index 713903290385..74428a1e03f3 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -614,8 +614,20 @@ 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) {
> > + if (drv->remove)
> > + drv->remove(dev);
> > dev_pm_domain_detach(_dev, true);
> > + return ret;
> > + }
> > + kobject_uevent(&_dev->kobj, KOBJ_CHANGE);
>
> We already emit KOBJ_BIND when we finish binding device to a driver,
> regardless of the bus. I know we still need to teach systemd to handle
> it properly, but I think it is better than sprinkling KOBJ_CHANGE
> around.
But the object's attributes did just change, which is what KOBJ_CHANGE
tells userspace, so this should be the correct thing to say to
userspace.
And yes, ideally KOBJ_BIND would be handled, and it will be sent once
the device's probe function succeeds, but we have to deal with old
userspaces as well, right?
thanks,
greg k-h
Hi Greg,
On Sat, Jul 6, 2019 at 1:32 AM Greg Kroah-Hartman
<[email protected]> wrote:
>
> On Thu, Jul 04, 2019 at 02:17:22PM -0700, Dmitry Torokhov wrote:
> > Hi Greg,
> >
> > On Thu, Jul 4, 2019 at 5:15 AM Greg Kroah-Hartman
> > <[email protected]> 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.
> >
> > Why is this limited to platform bus? Drivers for other buses also
> > often want to augment list of their attributes during probe(). I'd
> > move it to generic probe handling.
>
> This is not limited to the platform at all, the driver core supports
> this for any bus type today, but it's then up to the bus-specific code
> to pass that on to the driver core. That's usually set for the
> bus-specific attributes that they want exposed for all devices of that
> bus type (see the bus_groups, dev_groups, and drv_groups pointers in
> struct bus_type).
>
> For the platform devices, the problem is that this is something that the
> individual drivers want after they bind to the device. And as all
> platform devices are "different" they can't be a "common" set of
> attributes, so they need to be created after the device is bound to the
> driver.
I believe that your assertion that only platform devices want to
install custom attributes is incorrect. Drivers for devices attached
to serio, i2c, USB, spi, etc, etc, all have additional attributes:
dtor@dtor-ws:~/kernel/work (master *)$ grep -l '\(i2c\|usb\|spi\)'
`git grep -l '\(device_add_group\|sysfs_create_group\)' -- drivers` |
wc -l
170
I am pretty sure some of this count is false positives, but majority
is actually proper hits.
...
> >
> > We already emit KOBJ_BIND when we finish binding device to a driver,
> > regardless of the bus. I know we still need to teach systemd to handle
> > it properly, but I think it is better than sprinkling KOBJ_CHANGE
> > around.
>
> But the object's attributes did just change, which is what KOBJ_CHANGE
> tells userspace, so this should be the correct thing to say to
> userspace.
>
> And yes, ideally KOBJ_BIND would be handled, and it will be sent once
> the device's probe function succeeds, but we have to deal with old
> userspaces as well, right?
Not for the new functionality, I do not think so. Newer kernels should
be compatible with older userspace as it not breaking it, but new
functionality is not guaranteed to be available with older userspace.
Thanks.
--
Dmitry
On Sat, Jul 06, 2019 at 10:04:39AM -0700, Dmitry Torokhov wrote:
> Hi Greg,
>
> On Sat, Jul 6, 2019 at 1:32 AM Greg Kroah-Hartman
> <[email protected]> wrote:
> >
> > On Thu, Jul 04, 2019 at 02:17:22PM -0700, Dmitry Torokhov wrote:
> > > Hi Greg,
> > >
> > > On Thu, Jul 4, 2019 at 5:15 AM Greg Kroah-Hartman
> > > <[email protected]> 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.
> > >
> > > Why is this limited to platform bus? Drivers for other buses also
> > > often want to augment list of their attributes during probe(). I'd
> > > move it to generic probe handling.
> >
> > This is not limited to the platform at all, the driver core supports
> > this for any bus type today, but it's then up to the bus-specific code
> > to pass that on to the driver core. That's usually set for the
> > bus-specific attributes that they want exposed for all devices of that
> > bus type (see the bus_groups, dev_groups, and drv_groups pointers in
> > struct bus_type).
> >
> > For the platform devices, the problem is that this is something that the
> > individual drivers want after they bind to the device. And as all
> > platform devices are "different" they can't be a "common" set of
> > attributes, so they need to be created after the device is bound to the
> > driver.
>
> I believe that your assertion that only platform devices want to
> install custom attributes is incorrect.
Sorry, I didn't mean to imply that only platform drivers want to do
this, as you say, many other drivers do as well.
> Drivers for devices attached
> to serio, i2c, USB, spi, etc, etc, all have additional attributes:
>
> dtor@dtor-ws:~/kernel/work (master *)$ grep -l '\(i2c\|usb\|spi\)'
> `git grep -l '\(device_add_group\|sysfs_create_group\)' -- drivers` |
> wc -l
> 170
>
> I am pretty sure some of this count is false positives, but majority
> is actually proper hits.
Yeah, I know, we need to add this type of functionality to those busses
as well. I don't see a way of doing it other than this bus-by-bus
conversion, do you?
> > > We already emit KOBJ_BIND when we finish binding device to a driver,
> > > regardless of the bus. I know we still need to teach systemd to handle
> > > it properly, but I think it is better than sprinkling KOBJ_CHANGE
> > > around.
> >
> > But the object's attributes did just change, which is what KOBJ_CHANGE
> > tells userspace, so this should be the correct thing to say to
> > userspace.
> >
> > And yes, ideally KOBJ_BIND would be handled, and it will be sent once
> > the device's probe function succeeds, but we have to deal with old
> > userspaces as well, right?
>
> Not for the new functionality, I do not think so. Newer kernels should
> be compatible with older userspace as it not breaking it, but new
> functionality is not guaranteed to be available with older userspace.
I agree, but again, this is a kobject change (adding attributes), so
I think the event type I picked here is the correct one.
thanks,
greg k-h
On Sat, Jul 6, 2019 at 10:19 AM Greg Kroah-Hartman
<[email protected]> wrote:
>
> On Sat, Jul 06, 2019 at 10:04:39AM -0700, Dmitry Torokhov wrote:
> > Hi Greg,
> >
> > On Sat, Jul 6, 2019 at 1:32 AM Greg Kroah-Hartman
> > <[email protected]> wrote:
> > >
> > > On Thu, Jul 04, 2019 at 02:17:22PM -0700, Dmitry Torokhov wrote:
> > > > Hi Greg,
> > > >
> > > > On Thu, Jul 4, 2019 at 5:15 AM Greg Kroah-Hartman
> > > > <[email protected]> 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.
> > > >
> > > > Why is this limited to platform bus? Drivers for other buses also
> > > > often want to augment list of their attributes during probe(). I'd
> > > > move it to generic probe handling.
> > >
> > > This is not limited to the platform at all, the driver core supports
> > > this for any bus type today, but it's then up to the bus-specific code
> > > to pass that on to the driver core. That's usually set for the
> > > bus-specific attributes that they want exposed for all devices of that
> > > bus type (see the bus_groups, dev_groups, and drv_groups pointers in
> > > struct bus_type).
> > >
> > > For the platform devices, the problem is that this is something that the
> > > individual drivers want after they bind to the device. And as all
> > > platform devices are "different" they can't be a "common" set of
> > > attributes, so they need to be created after the device is bound to the
> > > driver.
> >
> > I believe that your assertion that only platform devices want to
> > install custom attributes is incorrect.
>
> Sorry, I didn't mean to imply that only platform drivers want to do
> this, as you say, many other drivers do as well.
>
> > Drivers for devices attached
> > to serio, i2c, USB, spi, etc, etc, all have additional attributes:
> >
> > dtor@dtor-ws:~/kernel/work (master *)$ grep -l '\(i2c\|usb\|spi\)'
> > `git grep -l '\(device_add_group\|sysfs_create_group\)' -- drivers` |
> > wc -l
> > 170
> >
> > I am pretty sure some of this count is false positives, but majority
> > is actually proper hits.
>
> Yeah, I know, we need to add this type of functionality to those busses
> as well. I don't see a way of doing it other than this bus-by-bus
> conversion, do you?
Can't you push the **dev_groups from platform driver down to the
generic driver structure and handle them in driver_sysfs_add()?
>
> > > > We already emit KOBJ_BIND when we finish binding device to a driver,
> > > > regardless of the bus. I know we still need to teach systemd to handle
> > > > it properly, but I think it is better than sprinkling KOBJ_CHANGE
> > > > around.
> > >
> > > But the object's attributes did just change, which is what KOBJ_CHANGE
> > > tells userspace, so this should be the correct thing to say to
> > > userspace.
> > >
> > > And yes, ideally KOBJ_BIND would be handled, and it will be sent once
> > > the device's probe function succeeds, but we have to deal with old
> > > userspaces as well, right?
> >
> > Not for the new functionality, I do not think so. Newer kernels should
> > be compatible with older userspace as it not breaking it, but new
> > functionality is not guaranteed to be available with older userspace.
>
> I agree, but again, this is a kobject change (adding attributes), so
> I think the event type I picked here is the correct one.
I guess once you push it all into core you'll end up with 2 uevents
being emitted back-to-back and this seems inefficient.
If you really want KOBJ_CHANGE maybe have some additional attribute
like "CHANGE=driver-specific-attrs" in it? It's all quite ugly though.
Thanks.
--
Dmitry
On Sat, Jul 06, 2019 at 10:39:38AM -0700, Dmitry Torokhov wrote:
> On Sat, Jul 6, 2019 at 10:19 AM Greg Kroah-Hartman
> <[email protected]> wrote:
> >
> > On Sat, Jul 06, 2019 at 10:04:39AM -0700, Dmitry Torokhov wrote:
> > > Hi Greg,
> > >
> > > On Sat, Jul 6, 2019 at 1:32 AM Greg Kroah-Hartman
> > > <[email protected]> wrote:
> > > >
> > > > On Thu, Jul 04, 2019 at 02:17:22PM -0700, Dmitry Torokhov wrote:
> > > > > Hi Greg,
> > > > >
> > > > > On Thu, Jul 4, 2019 at 5:15 AM Greg Kroah-Hartman
> > > > > <[email protected]> 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.
> > > > >
> > > > > Why is this limited to platform bus? Drivers for other buses also
> > > > > often want to augment list of their attributes during probe(). I'd
> > > > > move it to generic probe handling.
> > > >
> > > > This is not limited to the platform at all, the driver core supports
> > > > this for any bus type today, but it's then up to the bus-specific code
> > > > to pass that on to the driver core. That's usually set for the
> > > > bus-specific attributes that they want exposed for all devices of that
> > > > bus type (see the bus_groups, dev_groups, and drv_groups pointers in
> > > > struct bus_type).
> > > >
> > > > For the platform devices, the problem is that this is something that the
> > > > individual drivers want after they bind to the device. And as all
> > > > platform devices are "different" they can't be a "common" set of
> > > > attributes, so they need to be created after the device is bound to the
> > > > driver.
> > >
> > > I believe that your assertion that only platform devices want to
> > > install custom attributes is incorrect.
> >
> > Sorry, I didn't mean to imply that only platform drivers want to do
> > this, as you say, many other drivers do as well.
> >
> > > Drivers for devices attached
> > > to serio, i2c, USB, spi, etc, etc, all have additional attributes:
> > >
> > > dtor@dtor-ws:~/kernel/work (master *)$ grep -l '\(i2c\|usb\|spi\)'
> > > `git grep -l '\(device_add_group\|sysfs_create_group\)' -- drivers` |
> > > wc -l
> > > 170
> > >
> > > I am pretty sure some of this count is false positives, but majority
> > > is actually proper hits.
> >
> > Yeah, I know, we need to add this type of functionality to those busses
> > as well. I don't see a way of doing it other than this bus-by-bus
> > conversion, do you?
>
> Can't you push the **dev_groups from platform driver down to the
> generic driver structure and handle them in driver_sysfs_add()?
Sorry for the delay, got busy with the merge window...
Anyway, no, we can't call this then, because driver_sysfs_add() is
called before probe() is called. So if probe() fails, we don't bind the
device to the driver. We also should not be creating sysfs files for a
driver that has not had probe() called yet, as internal structures will
not be set up at that time.
> > > > > We already emit KOBJ_BIND when we finish binding device to a driver,
> > > > > regardless of the bus. I know we still need to teach systemd to handle
> > > > > it properly, but I think it is better than sprinkling KOBJ_CHANGE
> > > > > around.
> > > >
> > > > But the object's attributes did just change, which is what KOBJ_CHANGE
> > > > tells userspace, so this should be the correct thing to say to
> > > > userspace.
> > > >
> > > > And yes, ideally KOBJ_BIND would be handled, and it will be sent once
> > > > the device's probe function succeeds, but we have to deal with old
> > > > userspaces as well, right?
> > >
> > > Not for the new functionality, I do not think so. Newer kernels should
> > > be compatible with older userspace as it not breaking it, but new
> > > functionality is not guaranteed to be available with older userspace.
> >
> > I agree, but again, this is a kobject change (adding attributes), so
> > I think the event type I picked here is the correct one.
>
> I guess once you push it all into core you'll end up with 2 uevents
> being emitted back-to-back and this seems inefficient.
It's not the nicest, I agree. But, this is not all that common for
drivers to do, so it should not happen often enough to cause many
issues. Not all devices in the system will have this happen for them.
> If you really want KOBJ_CHANGE maybe have some additional attribute
> like "CHANGE=driver-specific-attrs" in it? It's all quite ugly though.
What would that addition help out with? You still need to rescan the
device attributes no matter what. Trying to compare a list of
attributes with what is "new" is probably slower than just reading them
all over again "from scratch".
thanks,
greg k-h
On Fri, Jul 19, 2019 at 08:52:20PM +0900, Greg Kroah-Hartman wrote:
> On Sat, Jul 06, 2019 at 10:39:38AM -0700, Dmitry Torokhov wrote:
> > On Sat, Jul 6, 2019 at 10:19 AM Greg Kroah-Hartman
> > <[email protected]> wrote:
> > >
> > > On Sat, Jul 06, 2019 at 10:04:39AM -0700, Dmitry Torokhov wrote:
> > > > Hi Greg,
> > > >
> > > > On Sat, Jul 6, 2019 at 1:32 AM Greg Kroah-Hartman
> > > > <[email protected]> wrote:
> > > > >
> > > > > On Thu, Jul 04, 2019 at 02:17:22PM -0700, Dmitry Torokhov wrote:
> > > > > > Hi Greg,
> > > > > >
> > > > > > On Thu, Jul 4, 2019 at 5:15 AM Greg Kroah-Hartman
> > > > > > <[email protected]> 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.
> > > > > >
> > > > > > Why is this limited to platform bus? Drivers for other buses also
> > > > > > often want to augment list of their attributes during probe(). I'd
> > > > > > move it to generic probe handling.
> > > > >
> > > > > This is not limited to the platform at all, the driver core supports
> > > > > this for any bus type today, but it's then up to the bus-specific code
> > > > > to pass that on to the driver core. That's usually set for the
> > > > > bus-specific attributes that they want exposed for all devices of that
> > > > > bus type (see the bus_groups, dev_groups, and drv_groups pointers in
> > > > > struct bus_type).
> > > > >
> > > > > For the platform devices, the problem is that this is something that the
> > > > > individual drivers want after they bind to the device. And as all
> > > > > platform devices are "different" they can't be a "common" set of
> > > > > attributes, so they need to be created after the device is bound to the
> > > > > driver.
> > > >
> > > > I believe that your assertion that only platform devices want to
> > > > install custom attributes is incorrect.
> > >
> > > Sorry, I didn't mean to imply that only platform drivers want to do
> > > this, as you say, many other drivers do as well.
> > >
> > > > Drivers for devices attached
> > > > to serio, i2c, USB, spi, etc, etc, all have additional attributes:
> > > >
> > > > dtor@dtor-ws:~/kernel/work (master *)$ grep -l '\(i2c\|usb\|spi\)'
> > > > `git grep -l '\(device_add_group\|sysfs_create_group\)' -- drivers` |
> > > > wc -l
> > > > 170
> > > >
> > > > I am pretty sure some of this count is false positives, but majority
> > > > is actually proper hits.
> > >
> > > Yeah, I know, we need to add this type of functionality to those busses
> > > as well. I don't see a way of doing it other than this bus-by-bus
> > > conversion, do you?
> >
> > Can't you push the **dev_groups from platform driver down to the
> > generic driver structure and handle them in driver_sysfs_add()?
>
> Sorry for the delay, got busy with the merge window...
>
> Anyway, no, we can't call this then, because driver_sysfs_add() is
> called before probe() is called. So if probe() fails, we don't bind the
> device to the driver. We also should not be creating sysfs files for a
> driver that has not had probe() called yet, as internal structures will
> not be set up at that time.
Ah, yes, I got confused by the fact that driver_sysfs_remove is called
early. Anyway, I think you want something like this:
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 0df9b4461766..61d9d650d890 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -515,9 +515,17 @@ static int really_probe(struct device *dev, struct device_driver *drv)
goto probe_failed;
}
+ if (device_add_groups(dev, drv->dev_groups)) {
+ printk(KERN_ERR "%s: device_add_groups(%s) failed\n",
+ __func__, dev_name(dev));
+ goto dev_groups_failed;
+ }
+
if (test_remove) {
test_remove = false;
+ device_remove_groups(dev, drv->dev_groups);
+
if (dev->bus->remove)
dev->bus->remove(dev);
else if (drv->remove)
@@ -545,6 +553,11 @@ static int really_probe(struct device *dev, struct device_driver *drv)
drv->bus->name, __func__, dev_name(dev), drv->name);
goto done;
+dev_groups_failed:
+ if (dev->bus->remove)
+ dev->bus->remove(dev);
+ else if (drv->remove)
+ drv->remove(dev);
probe_failed:
if (dev->bus)
blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
@@ -1075,6 +1088,8 @@ static void __device_release_driver(struct device *dev, struct device *parent)
pm_runtime_put_sync(dev);
+ device_remove_groups(dev, drv->dev_groups);
+
if (dev->bus && dev->bus->remove)
dev->bus->remove(dev);
else if (drv->remove)
diff --git a/include/linux/device.h b/include/linux/device.h
index 4a295e324ac5..12aa8c687404 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -259,6 +259,8 @@ enum probe_type {
* @resume: Called to bring a device from sleep mode.
* @groups: Default attributes that get created by the driver core
* automatically.
+ * @dev_groups: Additional attributes attached to device instance once the
+ * it is bound to the driver.
* @pm: Power management operations of the device which matched
* this driver.
* @coredump: Called when sysfs entry is written to. The device driver
@@ -293,6 +295,7 @@ struct device_driver {
int (*suspend) (struct device *dev, pm_message_t state);
int (*resume) (struct device *dev);
const struct attribute_group **groups;
+ const struct attribute_group **dev_groups;
const struct dev_pm_ops *pm;
void (*coredump) (struct device *dev);
Thanks.
--
Dmitry
On Sat, Jul 20, 2019 at 07:38:57AM +0300, Dmitry Torokhov wrote:
> On Fri, Jul 19, 2019 at 08:52:20PM +0900, Greg Kroah-Hartman wrote:
> > On Sat, Jul 06, 2019 at 10:39:38AM -0700, Dmitry Torokhov wrote:
> > > On Sat, Jul 6, 2019 at 10:19 AM Greg Kroah-Hartman
> > > <[email protected]> wrote:
> > > >
> > > > On Sat, Jul 06, 2019 at 10:04:39AM -0700, Dmitry Torokhov wrote:
> > > > > Hi Greg,
> > > > >
> > > > > On Sat, Jul 6, 2019 at 1:32 AM Greg Kroah-Hartman
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > On Thu, Jul 04, 2019 at 02:17:22PM -0700, Dmitry Torokhov wrote:
> > > > > > > Hi Greg,
> > > > > > >
> > > > > > > On Thu, Jul 4, 2019 at 5:15 AM Greg Kroah-Hartman
> > > > > > > <[email protected]> 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.
> > > > > > >
> > > > > > > Why is this limited to platform bus? Drivers for other buses also
> > > > > > > often want to augment list of their attributes during probe(). I'd
> > > > > > > move it to generic probe handling.
> > > > > >
> > > > > > This is not limited to the platform at all, the driver core supports
> > > > > > this for any bus type today, but it's then up to the bus-specific code
> > > > > > to pass that on to the driver core. That's usually set for the
> > > > > > bus-specific attributes that they want exposed for all devices of that
> > > > > > bus type (see the bus_groups, dev_groups, and drv_groups pointers in
> > > > > > struct bus_type).
> > > > > >
> > > > > > For the platform devices, the problem is that this is something that the
> > > > > > individual drivers want after they bind to the device. And as all
> > > > > > platform devices are "different" they can't be a "common" set of
> > > > > > attributes, so they need to be created after the device is bound to the
> > > > > > driver.
> > > > >
> > > > > I believe that your assertion that only platform devices want to
> > > > > install custom attributes is incorrect.
> > > >
> > > > Sorry, I didn't mean to imply that only platform drivers want to do
> > > > this, as you say, many other drivers do as well.
> > > >
> > > > > Drivers for devices attached
> > > > > to serio, i2c, USB, spi, etc, etc, all have additional attributes:
> > > > >
> > > > > dtor@dtor-ws:~/kernel/work (master *)$ grep -l '\(i2c\|usb\|spi\)'
> > > > > `git grep -l '\(device_add_group\|sysfs_create_group\)' -- drivers` |
> > > > > wc -l
> > > > > 170
> > > > >
> > > > > I am pretty sure some of this count is false positives, but majority
> > > > > is actually proper hits.
> > > >
> > > > Yeah, I know, we need to add this type of functionality to those busses
> > > > as well. I don't see a way of doing it other than this bus-by-bus
> > > > conversion, do you?
> > >
> > > Can't you push the **dev_groups from platform driver down to the
> > > generic driver structure and handle them in driver_sysfs_add()?
> >
> > Sorry for the delay, got busy with the merge window...
> >
> > Anyway, no, we can't call this then, because driver_sysfs_add() is
> > called before probe() is called. So if probe() fails, we don't bind the
> > device to the driver. We also should not be creating sysfs files for a
> > driver that has not had probe() called yet, as internal structures will
> > not be set up at that time.
>
> Ah, yes, I got confused by the fact that driver_sysfs_remove is called
> early. Anyway, I think you want something like this:
Ah, nice, this looks good. Let me try this and see how it goes...
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 0df9b4461766..61d9d650d890 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -515,9 +515,17 @@ static int really_probe(struct device *dev, struct device_driver *drv)
> goto probe_failed;
> }
>
> + if (device_add_groups(dev, drv->dev_groups)) {
> + printk(KERN_ERR "%s: device_add_groups(%s) failed\n",
> + __func__, dev_name(dev));
dev_err() of course :)
thanks for the review, much appreciated.
greg k-h
Hi Greg,
On 7/25/19 8:44 AM, Greg Kroah-Hartman wrote:
> On Sat, Jul 20, 2019 at 07:38:57AM +0300, Dmitry Torokhov wrote:
>> On Fri, Jul 19, 2019 at 08:52:20PM +0900, Greg Kroah-Hartman wrote:
>>> On Sat, Jul 06, 2019 at 10:39:38AM -0700, Dmitry Torokhov wrote:
>>>> On Sat, Jul 6, 2019 at 10:19 AM Greg Kroah-Hartman
>>>> <[email protected]> wrote:
>>>>>
>>>>> On Sat, Jul 06, 2019 at 10:04:39AM -0700, Dmitry Torokhov wrote:
>>>>>> Hi Greg,
>>>>>>
>>>>>> On Sat, Jul 6, 2019 at 1:32 AM Greg Kroah-Hartman
>>>>>> <[email protected]> wrote:
>>>>>>>
>>>>>>> On Thu, Jul 04, 2019 at 02:17:22PM -0700, Dmitry Torokhov wrote:
>>>>>>>> Hi Greg,
>>>>>>>>
>>>>>>>> On Thu, Jul 4, 2019 at 5:15 AM Greg Kroah-Hartman
>>>>>>>> <[email protected]> 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.
>>>>>>>>
>>>>>>>> Why is this limited to platform bus? Drivers for other buses also
>>>>>>>> often want to augment list of their attributes during probe(). I'd
>>>>>>>> move it to generic probe handling.
>>>>>>>
>>>>>>> This is not limited to the platform at all, the driver core supports
>>>>>>> this for any bus type today, but it's then up to the bus-specific code
>>>>>>> to pass that on to the driver core. That's usually set for the
>>>>>>> bus-specific attributes that they want exposed for all devices of that
>>>>>>> bus type (see the bus_groups, dev_groups, and drv_groups pointers in
>>>>>>> struct bus_type).
>>>>>>>
>>>>>>> For the platform devices, the problem is that this is something that the
>>>>>>> individual drivers want after they bind to the device. And as all
>>>>>>> platform devices are "different" they can't be a "common" set of
>>>>>>> attributes, so they need to be created after the device is bound to the
>>>>>>> driver.
>>>>>>
>>>>>> I believe that your assertion that only platform devices want to
>>>>>> install custom attributes is incorrect.
>>>>>
>>>>> Sorry, I didn't mean to imply that only platform drivers want to do
>>>>> this, as you say, many other drivers do as well.
>>>>>
>>>>>> Drivers for devices attached
>>>>>> to serio, i2c, USB, spi, etc, etc, all have additional attributes:
>>>>>>
>>>>>> dtor@dtor-ws:~/kernel/work (master *)$ grep -l '\(i2c\|usb\|spi\)'
>>>>>> `git grep -l '\(device_add_group\|sysfs_create_group\)' -- drivers` |
>>>>>> wc -l
>>>>>> 170
>>>>>>
>>>>>> I am pretty sure some of this count is false positives, but majority
>>>>>> is actually proper hits.
>>>>>
>>>>> Yeah, I know, we need to add this type of functionality to those busses
>>>>> as well. I don't see a way of doing it other than this bus-by-bus
>>>>> conversion, do you?
>>>>
>>>> Can't you push the **dev_groups from platform driver down to the
>>>> generic driver structure and handle them in driver_sysfs_add()?
>>>
>>> Sorry for the delay, got busy with the merge window...
>>>
>>> Anyway, no, we can't call this then, because driver_sysfs_add() is
>>> called before probe() is called. So if probe() fails, we don't bind the
>>> device to the driver. We also should not be creating sysfs files for a
>>> driver that has not had probe() called yet, as internal structures will
>>> not be set up at that time.
>>
>> Ah, yes, I got confused by the fact that driver_sysfs_remove is called
>> early. Anyway, I think you want something like this:
>
> Ah, nice, this looks good. Let me try this and see how it goes...
>
I tried Dmitry's patch on Intel Stratix10 platform and it works.
I added one minor change on the top of Dmitry's patch, since I think we
need add one additional check prior to device_add_groups(). To align
with Dmitry's patch, I also change my code to use the new dev_groups
pointer in the struct of device_driver.
My changes are below,
diff --git a/include/linux/device.h b/include/linux/device.h
index 6717ade..9207aea3 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -296,6 +296,7 @@ struct device_driver {
int (*suspend) (struct device *dev, pm_message_t state);
int (*resume) (struct device *dev);
const struct attribute_group **groups;
+ const struct attribute_group **dev_groups;
const struct dev_pm_ops *pm;
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 994a907..a91e69f 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -554,9 +554,19 @@ static int really_probe(struct device *dev, struct
device_driver *drv)
goto probe_failed;
}
+ if (drv->dev_groups) {
+ if (device_add_groups(dev, drv->dev_groups)) {
+ dev_err(dev, "device_add_groups(%s) failed\n",
+ dev_name(dev));
+ goto dev_groups_failed;
+ }
+ }
+
if (test_remove) {
test_remove = false;
+ device_remove_groups(dev, drv->dev_groups);
+
if (dev->bus->remove)
dev->bus->remove(dev);
else if (drv->remove)
@@ -584,6 +594,12 @@ static int really_probe(struct device *dev, struct
device_driver *drv)
drv->bus->name, __func__, dev_name(dev), drv->name);
goto done;
+dev_groups_failed:
+ if (dev->bus->remove)
+ dev->bus->remove(dev);
+ else if (drv->remove)
+ drv->remove(dev);
+
probe_failed:
if (dev->bus)
blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
@@ -1114,6 +1130,8 @@ static void __device_release_driver(struct device
*dev, struct device *parent)
pm_runtime_put_sync(dev);
+ device_remove_groups(dev, drv->dev_groups);
+
if (dev->bus && dev->bus->remove)
dev->bus->remove(dev);
else if (drv->remove)
diff --git a/drivers/firmware/stratix10-rsu.c
b/drivers/firmware/stratix10-rsu.c
index 98d8030..93b44e9 100644
--- a/drivers/firmware/stratix10-rsu.c
+++ b/drivers/firmware/stratix10-rsu.c
@@ -391,9 +391,9 @@ static int stratix10_rsu_remove(struct
platform_device *pdev)
static struct platform_driver stratix10_rsu_driver = {
.probe = stratix10_rsu_probe,
.remove = stratix10_rsu_remove,
.driver = {
.name = "stratix10-rsu",
+ .dev_groups = rsu_groups,
},
};
Regards,
Richard
>>
>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
>> index 0df9b4461766..61d9d650d890 100644
>> --- a/drivers/base/dd.c
>> +++ b/drivers/base/dd.c
>> @@ -515,9 +515,17 @@ static int really_probe(struct device *dev, struct device_driver *drv)
>> goto probe_failed;
>> }
>>
>> + if (device_add_groups(dev, drv->dev_groups)) {
>> + printk(KERN_ERR "%s: device_add_groups(%s) failed\n",
>> + __func__, dev_name(dev));
>
> dev_err() of course :)
>
> thanks for the review, much appreciated.
>
> greg k-h
>
On Thu, Jul 25, 2019 at 02:02:03PM -0500, Richard Gong wrote:
> Hi Greg,
>
> On 7/25/19 8:44 AM, Greg Kroah-Hartman wrote:
> > On Sat, Jul 20, 2019 at 07:38:57AM +0300, Dmitry Torokhov wrote:
> > > On Fri, Jul 19, 2019 at 08:52:20PM +0900, Greg Kroah-Hartman wrote:
> > > > On Sat, Jul 06, 2019 at 10:39:38AM -0700, Dmitry Torokhov wrote:
> > > > > On Sat, Jul 6, 2019 at 10:19 AM Greg Kroah-Hartman
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > On Sat, Jul 06, 2019 at 10:04:39AM -0700, Dmitry Torokhov wrote:
> > > > > > > Hi Greg,
> > > > > > >
> > > > > > > On Sat, Jul 6, 2019 at 1:32 AM Greg Kroah-Hartman
> > > > > > > <[email protected]> wrote:
> > > > > > > >
> > > > > > > > On Thu, Jul 04, 2019 at 02:17:22PM -0700, Dmitry Torokhov wrote:
> > > > > > > > > Hi Greg,
> > > > > > > > >
> > > > > > > > > On Thu, Jul 4, 2019 at 5:15 AM Greg Kroah-Hartman
> > > > > > > > > <[email protected]> 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.
> > > > > > > > >
> > > > > > > > > Why is this limited to platform bus? Drivers for other buses also
> > > > > > > > > often want to augment list of their attributes during probe(). I'd
> > > > > > > > > move it to generic probe handling.
> > > > > > > >
> > > > > > > > This is not limited to the platform at all, the driver core supports
> > > > > > > > this for any bus type today, but it's then up to the bus-specific code
> > > > > > > > to pass that on to the driver core. That's usually set for the
> > > > > > > > bus-specific attributes that they want exposed for all devices of that
> > > > > > > > bus type (see the bus_groups, dev_groups, and drv_groups pointers in
> > > > > > > > struct bus_type).
> > > > > > > >
> > > > > > > > For the platform devices, the problem is that this is something that the
> > > > > > > > individual drivers want after they bind to the device. And as all
> > > > > > > > platform devices are "different" they can't be a "common" set of
> > > > > > > > attributes, so they need to be created after the device is bound to the
> > > > > > > > driver.
> > > > > > >
> > > > > > > I believe that your assertion that only platform devices want to
> > > > > > > install custom attributes is incorrect.
> > > > > >
> > > > > > Sorry, I didn't mean to imply that only platform drivers want to do
> > > > > > this, as you say, many other drivers do as well.
> > > > > >
> > > > > > > Drivers for devices attached
> > > > > > > to serio, i2c, USB, spi, etc, etc, all have additional attributes:
> > > > > > >
> > > > > > > dtor@dtor-ws:~/kernel/work (master *)$ grep -l '\(i2c\|usb\|spi\)'
> > > > > > > `git grep -l '\(device_add_group\|sysfs_create_group\)' -- drivers` |
> > > > > > > wc -l
> > > > > > > 170
> > > > > > >
> > > > > > > I am pretty sure some of this count is false positives, but majority
> > > > > > > is actually proper hits.
> > > > > >
> > > > > > Yeah, I know, we need to add this type of functionality to those busses
> > > > > > as well. I don't see a way of doing it other than this bus-by-bus
> > > > > > conversion, do you?
> > > > >
> > > > > Can't you push the **dev_groups from platform driver down to the
> > > > > generic driver structure and handle them in driver_sysfs_add()?
> > > >
> > > > Sorry for the delay, got busy with the merge window...
> > > >
> > > > Anyway, no, we can't call this then, because driver_sysfs_add() is
> > > > called before probe() is called. So if probe() fails, we don't bind the
> > > > device to the driver. We also should not be creating sysfs files for a
> > > > driver that has not had probe() called yet, as internal structures will
> > > > not be set up at that time.
> > >
> > > Ah, yes, I got confused by the fact that driver_sysfs_remove is called
> > > early. Anyway, I think you want something like this:
> >
> > Ah, nice, this looks good. Let me try this and see how it goes...
> >
>
> I tried Dmitry's patch on Intel Stratix10 platform and it works.
>
> I added one minor change on the top of Dmitry's patch, since I think we need
> add one additional check prior to device_add_groups(). To align with
> Dmitry's patch, I also change my code to use the new dev_groups pointer in
> the struct of device_driver.
Thanks for testing!
> My changes are below,
<snip>
> --- a/drivers/firmware/stratix10-rsu.c
> +++ b/drivers/firmware/stratix10-rsu.c
> @@ -391,9 +391,9 @@ static int stratix10_rsu_remove(struct platform_device
> *pdev)
> static struct platform_driver stratix10_rsu_driver = {
> .probe = stratix10_rsu_probe,
> .remove = stratix10_rsu_remove,
> .driver = {
> .name = "stratix10-rsu",
> + .dev_groups = rsu_groups,
I'd prefer to leave the dev_groups in the platform driver code, as no
one should have to do this crazy "sub structure definition" that
platform drivers seem to love to do.
Here's the patch that I currently have on top of Dmitry's that is
getting run through 0-day right now.
I'll resend the whole patch series once it passes (hopefully tomorrow).
thanks,
greg k-h
From 6ad595541f81407f401d992b89ae1269e88cb3be Mon Sep 17 00:00:00 2001
From: Greg Kroah-Hartman <[email protected]>
Date: Thu, 25 Jul 2019 15:54:24 +0200
Subject: [PATCH 02/13] platform: add a dev_groups pointer to struct
platform_driver
As the driver core now provides the ability to directly add/remove
device groups when a driver is bound/unbound to a device, just pass that
pointer along to the driver core.
This allows us to fix up platform drivers to not need to create/remove
groups "by hand" anymore.
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]
Cc: Dmitry Torokhov <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
drivers/base/platform.c | 1 +
include/linux/platform_device.h | 1 +
2 files changed, 2 insertions(+)
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 506a0175a5a7..21b3817569cd 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -667,6 +667,7 @@ int __platform_driver_register(struct platform_driver *drv,
drv->driver.probe = platform_drv_probe;
drv->driver.remove = platform_drv_remove;
drv->driver.shutdown = platform_drv_shutdown;
+ drv->driver.dev_groups = drv->dev_groups;
return driver_register(&drv->driver);
}
diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
index 9bc36b589827..9945a08b872a 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -189,6 +189,7 @@ struct platform_driver {
int (*resume)(struct platform_device *);
struct device_driver driver;
const struct platform_device_id *id_table;
+ const struct attribute_group **dev_groups;
bool prevent_deferred_probe;
};
--
2.22.0
On Thu, Jul 25, 2019 at 09:04:43PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Jul 25, 2019 at 02:02:03PM -0500, Richard Gong wrote:
> > Hi Greg,
> >
> > On 7/25/19 8:44 AM, Greg Kroah-Hartman wrote:
> > > On Sat, Jul 20, 2019 at 07:38:57AM +0300, Dmitry Torokhov wrote:
> > > > On Fri, Jul 19, 2019 at 08:52:20PM +0900, Greg Kroah-Hartman wrote:
> > > > > On Sat, Jul 06, 2019 at 10:39:38AM -0700, Dmitry Torokhov wrote:
> > > > > > On Sat, Jul 6, 2019 at 10:19 AM Greg Kroah-Hartman
> > > > > > <[email protected]> wrote:
> > > > > > >
> > > > > > > On Sat, Jul 06, 2019 at 10:04:39AM -0700, Dmitry Torokhov wrote:
> > > > > > > > Hi Greg,
> > > > > > > >
> > > > > > > > On Sat, Jul 6, 2019 at 1:32 AM Greg Kroah-Hartman
> > > > > > > > <[email protected]> wrote:
> > > > > > > > >
> > > > > > > > > On Thu, Jul 04, 2019 at 02:17:22PM -0700, Dmitry Torokhov wrote:
> > > > > > > > > > Hi Greg,
> > > > > > > > > >
> > > > > > > > > > On Thu, Jul 4, 2019 at 5:15 AM Greg Kroah-Hartman
> > > > > > > > > > <[email protected]> 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.
> > > > > > > > > >
> > > > > > > > > > Why is this limited to platform bus? Drivers for other buses also
> > > > > > > > > > often want to augment list of their attributes during probe(). I'd
> > > > > > > > > > move it to generic probe handling.
> > > > > > > > >
> > > > > > > > > This is not limited to the platform at all, the driver core supports
> > > > > > > > > this for any bus type today, but it's then up to the bus-specific code
> > > > > > > > > to pass that on to the driver core. That's usually set for the
> > > > > > > > > bus-specific attributes that they want exposed for all devices of that
> > > > > > > > > bus type (see the bus_groups, dev_groups, and drv_groups pointers in
> > > > > > > > > struct bus_type).
> > > > > > > > >
> > > > > > > > > For the platform devices, the problem is that this is something that the
> > > > > > > > > individual drivers want after they bind to the device. And as all
> > > > > > > > > platform devices are "different" they can't be a "common" set of
> > > > > > > > > attributes, so they need to be created after the device is bound to the
> > > > > > > > > driver.
> > > > > > > >
> > > > > > > > I believe that your assertion that only platform devices want to
> > > > > > > > install custom attributes is incorrect.
> > > > > > >
> > > > > > > Sorry, I didn't mean to imply that only platform drivers want to do
> > > > > > > this, as you say, many other drivers do as well.
> > > > > > >
> > > > > > > > Drivers for devices attached
> > > > > > > > to serio, i2c, USB, spi, etc, etc, all have additional attributes:
> > > > > > > >
> > > > > > > > dtor@dtor-ws:~/kernel/work (master *)$ grep -l '\(i2c\|usb\|spi\)'
> > > > > > > > `git grep -l '\(device_add_group\|sysfs_create_group\)' -- drivers` |
> > > > > > > > wc -l
> > > > > > > > 170
> > > > > > > >
> > > > > > > > I am pretty sure some of this count is false positives, but majority
> > > > > > > > is actually proper hits.
> > > > > > >
> > > > > > > Yeah, I know, we need to add this type of functionality to those busses
> > > > > > > as well. I don't see a way of doing it other than this bus-by-bus
> > > > > > > conversion, do you?
> > > > > >
> > > > > > Can't you push the **dev_groups from platform driver down to the
> > > > > > generic driver structure and handle them in driver_sysfs_add()?
> > > > >
> > > > > Sorry for the delay, got busy with the merge window...
> > > > >
> > > > > Anyway, no, we can't call this then, because driver_sysfs_add() is
> > > > > called before probe() is called. So if probe() fails, we don't bind the
> > > > > device to the driver. We also should not be creating sysfs files for a
> > > > > driver that has not had probe() called yet, as internal structures will
> > > > > not be set up at that time.
> > > >
> > > > Ah, yes, I got confused by the fact that driver_sysfs_remove is called
> > > > early. Anyway, I think you want something like this:
> > >
> > > Ah, nice, this looks good. Let me try this and see how it goes...
> > >
> >
> > I tried Dmitry's patch on Intel Stratix10 platform and it works.
> >
> > I added one minor change on the top of Dmitry's patch, since I think we need
> > add one additional check prior to device_add_groups(). To align with
> > Dmitry's patch, I also change my code to use the new dev_groups pointer in
> > the struct of device_driver.
>
> Thanks for testing!
>
> > My changes are below,
>
> <snip>
>
> > --- a/drivers/firmware/stratix10-rsu.c
> > +++ b/drivers/firmware/stratix10-rsu.c
> > @@ -391,9 +391,9 @@ static int stratix10_rsu_remove(struct platform_device
> > *pdev)
> > static struct platform_driver stratix10_rsu_driver = {
> > .probe = stratix10_rsu_probe,
> > .remove = stratix10_rsu_remove,
> > .driver = {
> > .name = "stratix10-rsu",
> > + .dev_groups = rsu_groups,
>
> I'd prefer to leave the dev_groups in the platform driver code, as no
> one should have to do this crazy "sub structure definition" that
> platform drivers seem to love to do.
Heh.
dtor@penguin:~/kernel/work $ git grep -A15 "static struct spi_driver" --
drivers/ | grep "\.driver.*=" | wc -l
272
You will find the similar counts for i2c, and other buses, mostly
because we need to specify driver name, OF/ACPI match tables, and
pointer to PM ops.
So please just keep it in the generic driver structure.
Thanks.
--
Dmitry
Hi Richard,
On Thu, Jul 25, 2019 at 02:02:03PM -0500, Richard Gong wrote:
> Hi Greg,
>
> On 7/25/19 8:44 AM, Greg Kroah-Hartman wrote:
> > On Sat, Jul 20, 2019 at 07:38:57AM +0300, Dmitry Torokhov wrote:
> > > On Fri, Jul 19, 2019 at 08:52:20PM +0900, Greg Kroah-Hartman wrote:
> > > > On Sat, Jul 06, 2019 at 10:39:38AM -0700, Dmitry Torokhov wrote:
> > > > > On Sat, Jul 6, 2019 at 10:19 AM Greg Kroah-Hartman
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > On Sat, Jul 06, 2019 at 10:04:39AM -0700, Dmitry Torokhov wrote:
> > > > > > > Hi Greg,
> > > > > > >
> > > > > > > On Sat, Jul 6, 2019 at 1:32 AM Greg Kroah-Hartman
> > > > > > > <[email protected]> wrote:
> > > > > > > >
> > > > > > > > On Thu, Jul 04, 2019 at 02:17:22PM -0700, Dmitry Torokhov wrote:
> > > > > > > > > Hi Greg,
> > > > > > > > >
> > > > > > > > > On Thu, Jul 4, 2019 at 5:15 AM Greg Kroah-Hartman
> > > > > > > > > <[email protected]> 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.
> > > > > > > > >
> > > > > > > > > Why is this limited to platform bus? Drivers for other buses also
> > > > > > > > > often want to augment list of their attributes during probe(). I'd
> > > > > > > > > move it to generic probe handling.
> > > > > > > >
> > > > > > > > This is not limited to the platform at all, the driver core supports
> > > > > > > > this for any bus type today, but it's then up to the bus-specific code
> > > > > > > > to pass that on to the driver core. That's usually set for the
> > > > > > > > bus-specific attributes that they want exposed for all devices of that
> > > > > > > > bus type (see the bus_groups, dev_groups, and drv_groups pointers in
> > > > > > > > struct bus_type).
> > > > > > > >
> > > > > > > > For the platform devices, the problem is that this is something that the
> > > > > > > > individual drivers want after they bind to the device. And as all
> > > > > > > > platform devices are "different" they can't be a "common" set of
> > > > > > > > attributes, so they need to be created after the device is bound to the
> > > > > > > > driver.
> > > > > > >
> > > > > > > I believe that your assertion that only platform devices want to
> > > > > > > install custom attributes is incorrect.
> > > > > >
> > > > > > Sorry, I didn't mean to imply that only platform drivers want to do
> > > > > > this, as you say, many other drivers do as well.
> > > > > >
> > > > > > > Drivers for devices attached
> > > > > > > to serio, i2c, USB, spi, etc, etc, all have additional attributes:
> > > > > > >
> > > > > > > dtor@dtor-ws:~/kernel/work (master *)$ grep -l '\(i2c\|usb\|spi\)'
> > > > > > > `git grep -l '\(device_add_group\|sysfs_create_group\)' -- drivers` |
> > > > > > > wc -l
> > > > > > > 170
> > > > > > >
> > > > > > > I am pretty sure some of this count is false positives, but majority
> > > > > > > is actually proper hits.
> > > > > >
> > > > > > Yeah, I know, we need to add this type of functionality to those busses
> > > > > > as well. I don't see a way of doing it other than this bus-by-bus
> > > > > > conversion, do you?
> > > > >
> > > > > Can't you push the **dev_groups from platform driver down to the
> > > > > generic driver structure and handle them in driver_sysfs_add()?
> > > >
> > > > Sorry for the delay, got busy with the merge window...
> > > >
> > > > Anyway, no, we can't call this then, because driver_sysfs_add() is
> > > > called before probe() is called. So if probe() fails, we don't bind the
> > > > device to the driver. We also should not be creating sysfs files for a
> > > > driver that has not had probe() called yet, as internal structures will
> > > > not be set up at that time.
> > >
> > > Ah, yes, I got confused by the fact that driver_sysfs_remove is called
> > > early. Anyway, I think you want something like this:
> >
> > Ah, nice, this looks good. Let me try this and see how it goes...
> >
>
> I tried Dmitry's patch on Intel Stratix10 platform and it works.
>
> I added one minor change on the top of Dmitry's patch, since I think we need
> add one additional check prior to device_add_groups().
sysfs_create_groups() returns success when NULL is passed, and
device_add_groups() is a simple wrapper around it.
If anything I'd prefer to codify the behavior of
device_add_groups()/sysfs_create_groups() via comments rather than
adding unneeded checks.
Thanks.
--
Dmitry