[RFC] of/platform: Create device link between simple-mfd and its children

[RFC] of/platform: Create device link between simple-mfd and its children

[Nicolas Saenz Julienne]

'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(-)

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);
-- 
2.28.0

Re: [RFC] of/platform: Create device link between simple-mfd and its children

[Saravana Kannan]

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
>

Re: [RFC] of/platform: Create device link between simple-mfd and its children

[Rob Herring]

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>

Re: [RFC] of/platform: Create device link between simple-mfd and its children

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 1522 bytes --]

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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC] of/platform: Create device link between simple-mfd and its children

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 1771 bytes --]

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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC] of/platform: Create device link between simple-mfd and its children

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1964 bytes --]

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


-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC] of/platform: Create device link between simple-mfd and its children

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 2186 bytes --]

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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]