| Message ID | 20201015114346.15743-1-nsaenzjulienne@suse.de |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [RFC] of/platform: Create device link between simple-mfd and its children | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 5 warnings, 16 lines checked |
On Thu, Oct 15, 2020 at 4:43 AM Nicolas Saenz Julienne <nsaenzjulienne@suse.de> wrote: > > 'simple-mfd' usage implies there might be some kind of resource sharing > between the parent device and its children. By creating a device link > with DL_FLAG_AUTOREMOVE_CONSUMER we make sure that at no point in time > the parent device is unbound while leaving its children unaware that > some of their resources disappeared. Doesn't the parent child relationship already ensure that? If not, maybe that's what needs fixing? > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> > > --- > > Some questions: > > - To what extent do we care about cleanly unbinding platform devices at > runtime? My rationale here is: "It's a platform device, for all you > know you might be unbinding someting essential to the system. So if > you're doing it, you better know what you're doing." > > - Would this be an abuse of device links? Feels like it. > > - If applying this to all simple-mfd devices is a bit too much, would > this be acceptable for a specific device setup. For example RPi4's > firmware interface (simple-mfd user) is passed to consumer drivers > trough a custom API (see rpi_firmware_get()). So, when unbound, > consumers are left with a firmware handle that points to nothing. You can always create device link between the real suppliers and consumers. > > drivers/of/platform.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c > index b557a0fcd4ba..8d5b55b81764 100644 > --- a/drivers/of/platform.c > +++ b/drivers/of/platform.c > @@ -390,8 +390,14 @@ static int of_platform_bus_create(struct device_node *bus, > } > > dev = of_platform_device_create_pdata(bus, bus_id, platform_data, parent); > - if (!dev || !of_match_node(matches, bus)) > - return 0; > + if (!dev) > + return 0; > + > + if (parent && of_device_is_compatible(parent->of_node, "simple-mfd")) > + device_link_add(&dev->dev, parent, DL_FLAG_AUTOREMOVE_CONSUMER); > + > + if (!of_match_node(matches, bus)) > + return 0; Even if we think we should add this between parent and child (this still seems like not a good place to do it). Matching it by compatible string and doing special stuff doesn't feel right inside here. -Saravana > > for_each_child_of_node(bus, child) { > pr_debug(" create child: %pOF\n", child); > -- > 2.28.0 >
On Thu, Oct 15, 2020 at 6:43 AM Nicolas Saenz Julienne <nsaenzjulienne@suse.de> wrote: > > 'simple-mfd' usage implies there might be some kind of resource sharing > between the parent device and its children. It does? No! The reason behind simple-mfd was specifically because there was no parent driver or dependency on the parent. No doubt simple-mfd has been abused. Rob > By creating a device link > with DL_FLAG_AUTOREMOVE_CONSUMER we make sure that at no point in time > the parent device is unbound while leaving its children unaware that > some of their resources disappeared. > > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
On Fri, 2020-10-16 at 09:38 -0500, Rob Herring wrote: > On Thu, Oct 15, 2020 at 6:43 AM Nicolas Saenz Julienne > <nsaenzjulienne@suse.de> wrote: > > 'simple-mfd' usage implies there might be some kind of resource sharing > > between the parent device and its children. > > It does? No! The reason behind simple-mfd was specifically because > there was no parent driver or dependency on the parent. No doubt > simple-mfd has been abused. Fair enough, so we're doing things wrong. Just for the record, I'm looking at RPi´s firmware interface: firmware: firmware { compatible = "raspberrypi,bcm2835-firmware", "simple-mfd"; #address-cells = <1>; #size-cells = <1>; mboxes = <&mailbox>; firmware_clocks: clocks { compatible = "raspberrypi,firmware-clocks"; #clock-cells = <1>; }; reset: reset { compatible = "raspberrypi,firmware-reset"; #reset-cells = <1>; }; [...] }; Note that "raspberrypi,bcm2835-firmware" has a driver, it's not just a placeholder. Consumer drivers get a handle to RPi's firmware interface through the supplier's API, rpi_firmware_get(). The handle to firmware becomes meaningless if it is unbinded, which I want to protect myself against. A simpler solution would be to manually create a device link between both devices ("raspberrypi,bcm2835-firmware" and "raspberrypi,firmware-clocks" for example) upon calling rpi_firmware_get(). But I wanted to try addressing the problem in a generic way first. Regards, Nicolas
Hi Saravana, thanks for your comments. On Thu, 2020-10-15 at 09:52 -0700, Saravana Kannan wrote: > On Thu, Oct 15, 2020 at 4:43 AM Nicolas Saenz Julienne > <nsaenzjulienne@suse.de> wrote: > > 'simple-mfd' usage implies there might be some kind of resource sharing > > between the parent device and its children. By creating a device link > > with DL_FLAG_AUTOREMOVE_CONSUMER we make sure that at no point in time > > the parent device is unbound while leaving its children unaware that > > some of their resources disappeared. > > Doesn't the parent child relationship already ensure that? If not, > maybe that's what needs fixing? Well as Rob puts it, we're not using simple-mfd as it was intended. So that pretty much settles it for generic solutions. > > > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> [...] > > > - If applying this to all simple-mfd devices is a bit too much, would > > this be acceptable for a specific device setup. For example RPi4's > > firmware interface (simple-mfd user) is passed to consumer drivers > > trough a custom API (see rpi_firmware_get()). So, when unbound, > > consumers are left with a firmware handle that points to nothing. > > You can always create device link between the real suppliers and consumers. RPi's firmware consumers use a custom API to get a handle to the firmware interface itself, rpi_firmware_get(). So no trace of the relationship is expressed in DT. If the firmware interface device, the supplier, is unbinded, that firmware handle now points nowhere and consumers will end up triggering kernel panics. Would it make sense to make a device link between the two in that case? Or I'd be, again, abusing the concept? Regards, Nicolas
On Fri, Oct 16, 2020 at 05:26:56PM +0200, Nicolas Saenz Julienne wrote: > On Fri, 2020-10-16 at 09:38 -0500, Rob Herring wrote: > > On Thu, Oct 15, 2020 at 6:43 AM Nicolas Saenz Julienne > > <nsaenzjulienne@suse.de> wrote: > > > 'simple-mfd' usage implies there might be some kind of resource sharing > > > between the parent device and its children. > > > > It does? No! The reason behind simple-mfd was specifically because > > there was no parent driver or dependency on the parent. No doubt > > simple-mfd has been abused. > > Fair enough, so we're doing things wrong. Just for the record, I'm looking at > RPi´s firmware interface: > > firmware: firmware { > compatible = "raspberrypi,bcm2835-firmware", "simple-mfd"; > #address-cells = <1>; > #size-cells = <1>; > mboxes = <&mailbox>; > > firmware_clocks: clocks { > compatible = "raspberrypi,firmware-clocks"; > #clock-cells = <1>; > }; > > reset: reset { > compatible = "raspberrypi,firmware-reset"; > #reset-cells = <1>; > }; > [...] > }; > > Note that "raspberrypi,bcm2835-firmware" has a driver, it's not just a > placeholder. Consumer drivers get a handle to RPi's firmware interface through > the supplier's API, rpi_firmware_get(). The handle to firmware becomes > meaningless if it is unbinded, which I want to protect myself against. > > A simpler solution would be to manually create a device link between both > devices ("raspberrypi,bcm2835-firmware" and "raspberrypi,firmware-clocks" for > example) upon calling rpi_firmware_get(). But I wanted to try addressing the > problem in a generic way first. IMHO rpi_firmware_get() should get a reference on the firmware device (and call try_module_get()) which prevents unbinding it. Best regards Uwe
Hi Uwe, Sorry for the late reply, got distracted with other stuff. On Mon, 2020-10-19 at 08:52 +0200, Uwe Kleine-König wrote: > On Fri, Oct 16, 2020 at 05:26:56PM +0200, Nicolas Saenz Julienne wrote: > > On Fri, 2020-10-16 at 09:38 -0500, Rob Herring wrote: > > > On Thu, Oct 15, 2020 at 6:43 AM Nicolas Saenz Julienne > > > <nsaenzjulienne@suse.de> wrote: > > > > 'simple-mfd' usage implies there might be some kind of resource sharing > > > > between the parent device and its children. > > > > > > It does? No! The reason behind simple-mfd was specifically because > > > there was no parent driver or dependency on the parent. No doubt > > > simple-mfd has been abused. > > > > Fair enough, so we're doing things wrong. Just for the record, I'm looking at > > RPi´s firmware interface: > > > > firmware: firmware { > > compatible = "raspberrypi,bcm2835-firmware", "simple-mfd"; > > #address-cells = <1>; > > #size-cells = <1>; > > mboxes = <&mailbox>; > > > > firmware_clocks: clocks { > > compatible = "raspberrypi,firmware-clocks"; > > #clock-cells = <1>; > > }; > > > > reset: reset { > > compatible = "raspberrypi,firmware-reset"; > > #reset-cells = <1>; > > }; > > [...] > > }; > > > > Note that "raspberrypi,bcm2835-firmware" has a driver, it's not just a > > placeholder. Consumer drivers get a handle to RPi's firmware interface through > > the supplier's API, rpi_firmware_get(). The handle to firmware becomes > > meaningless if it is unbinded, which I want to protect myself against. > > > > A simpler solution would be to manually create a device link between both > > devices ("raspberrypi,bcm2835-firmware" and "raspberrypi,firmware-clocks" for > > example) upon calling rpi_firmware_get(). But I wanted to try addressing the > > problem in a generic way first. > > IMHO rpi_firmware_get() should get a reference on the firmware device > (and call try_module_get()) which prevents unbinding it. Yes, it seems the way to go. Just one last question WRT this, since 'drv->remove(dev)' can't fail should I just block until the reference count hits zero? Regards, Nicolas
diff --git a/drivers/of/platform.c b/drivers/of/platform.c index b557a0fcd4ba..8d5b55b81764 100644 --- a/drivers/of/platform.c +++ b/drivers/of/platform.c @@ -390,8 +390,14 @@ static int of_platform_bus_create(struct device_node *bus, } dev = of_platform_device_create_pdata(bus, bus_id, platform_data, parent); - if (!dev || !of_match_node(matches, bus)) - return 0; + if (!dev) + return 0; + + if (parent && of_device_is_compatible(parent->of_node, "simple-mfd")) + device_link_add(&dev->dev, parent, DL_FLAG_AUTOREMOVE_CONSUMER); + + if (!of_match_node(matches, bus)) + return 0; for_each_child_of_node(bus, child) { pr_debug(" create child: %pOF\n", child);
'simple-mfd' usage implies there might be some kind of resource sharing between the parent device and its children. By creating a device link with DL_FLAG_AUTOREMOVE_CONSUMER we make sure that at no point in time the parent device is unbound while leaving its children unaware that some of their resources disappeared. Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> --- Some questions: - To what extent do we care about cleanly unbinding platform devices at runtime? My rationale here is: "It's a platform device, for all you know you might be unbinding someting essential to the system. So if you're doing it, you better know what you're doing." - Would this be an abuse of device links? - If applying this to all simple-mfd devices is a bit too much, would this be acceptable for a specific device setup. For example RPi4's firmware interface (simple-mfd user) is passed to consumer drivers trough a custom API (see rpi_firmware_get()). So, when unbound, consumers are left with a firmware handle that points to nothing. drivers/of/platform.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)