Re: A potential broken at platform driver?

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Jun 04, 2019 at 12:33:03PM +0200, Romain Izard wrote:
> On Mon, Jun 03, 2019 at 08:02:55PM +0200, Greg KH wrote:
> > > @@ -394,7 +432,7 @@ static struct platform_driver stratix10_rsu_driver = {
> > >  	.remove = stratix10_rsu_remove,
> > >  	.driver = {
> > >  		.name = "stratix10-rsu",
> > > -		.groups = rsu_groups,
> > > +//		.groups = rsu_groups,
> > 
> > Are you sure this is the correct pointer?  I think that might be
> > pointing to the driver's attributes, not the device's attributes.
> > 
> > If platform drivers do not have a way to register groups properly, then
> > that really needs to be fixed, as trying to register it by yourself as
> > you are doing, is ripe for racing with userspace.
>  
> This is a very common issue with platform drivers, and it seems to me that
> it is not possible to add device attributes when binding a device to a
> driver without entering the race condition.
> 
> My understanding is the following one:
> 
> The root cause is that the device has already been created and reported
> to the userspace with a KOBJ_ADD uevent before the device and the driver
> are bound together. On receiving this event, userspace will react, and
> it will try to read the device's attributes. In parallel the kernel will
> try to find a matching driver. If a driver is found, the kernel will
> call the probe function from the driver with the device as a parameter,
> and if successful a KOBJ_BIND uevent will be sent to userspace, but this
> is a recent addition.
> 
> Unfortunately, not all created devices will be bound to a driver, and the
> existing udev code relies on KOBJ_ADD uevents rather than KOBJ_BIND uevents.
> If new per-device attributes have been added to the device during the
> binding stage userspace may or may not see them, depending on when userspace
> tries to read the device's attributes.
> 
> I have this possible workaround, but I do not know if it is a good solution:
> 
> When binding the device and the driver together, create a new device as a
> child to the current device, and fill its "groups" member to point to the
> per-device attributes' group. As the device will be created with all the
> attributes, it will not be affected by the race issues. The functions
> handling the attributes will need to be modified to use the parents of their
> "device" parameter, instead of the device itself. Additionnaly, the sysfs
> location of the attributes will be different, as the child device will show
> up in the sysfs path. But for a newly introduced device this will not be
> a problem.
> 
> Is this a good compromise ?

Not really.  You just want the attributes on the platform device itself.

Given the horrible hack that platform devices are today, what's one more
hack!

Here's a patch below of what should probably be done here.  Richard, can
you change your code to use the new dev_groups pointer in the struct
platform_driver and this patch and let me know if that works or not?

Note, I've only compiled this code, not tested it...

thanks,

greg k-h

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 4d1729853d1a..3dd4b73a9b30 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -598,6 +598,7 @@ struct platform_device *platform_device_register_full(
 }
 EXPORT_SYMBOL_GPL(platform_device_register_full);
 
+static int platform_drv_remove(struct device *_dev);
 static int platform_drv_probe(struct device *_dev)
 {
 	struct platform_driver *drv = to_platform_driver(_dev->driver);
@@ -614,8 +615,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:
@@ -640,6 +651,8 @@ static int platform_drv_remove(struct device *_dev)
 
 	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;
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;
 };