Re: [PATCH 3/3] pwm: Add Raspberry Pi Firmware based PWM bus

[Uwe Kleine-König]

Hello Nicolas,

[Cc: += Greg as base driver maintainer]

On Tue, Oct 13, 2020 at 06:50:47PM +0200, Nicolas Saenz Julienne wrote:
> On Tue, 2020-10-13 at 14:17 +0200, Uwe Kleine-König wrote:
> > On Tue, Oct 13, 2020 at 01:20:00PM +0200, Nicolas Saenz Julienne wrote:
> > > On Mon, 2020-10-12 at 09:06 +0200, Uwe Kleine-König wrote:
> > > > I don't see a mechanism that prevents the driver providing the firmware
> > > > going away while the PWM is still in use.
> > > 
> > > There isn't an explicit one. But since you depend on a symbol from the firmware
> > > driver you won't be able to remove the kernel module before removing the PMW
> > > one.
> > 
> > this prevents the that the module is unloaded, but not that the driver
> > is unbound.
> 
> Yes, if you were to unbind the firmware device all devices that depend on it
> (there are a bunch of them) would access freed memory. Yet again, there is no
> hotplug functionality, so short of being useful for development it'd be very
> rare for someone to unbind it. We've been living with it as such for a long
> time. Not to say that is something not to fix, but from my perspective it's
> just a corner-case.

I agree, that's a corner case. However in my eyes it is one that should
be get right. Did you try if this is indeed a problem?

> We are using 'simple-mfd' in order to probe all devices under the
> firmware interface, so my first intuition would be to add support for
> automatically unbinding of consumer devices in of/platform.c. See:
> 
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index b557a0fcd4ba..d24f2412d518 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -390,7 +390,13 @@ 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))
> +       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) {

This looks wrong for generic code. A solution local to simple-mfd (or
even the firmware device?) should be done (and doable). I think the
straight forward approach would be to add reference counting and make
.remove of the firmware device block if there are still users.
(Returning an error doesn't prevent the device going away IIRC. Last
time I looked .remove returning something non-0 didn't make any
difference. Maybe we should change it to return void?)

> If this is too much for OF maintainers, we could simply create the link upon
> calling rpi_firmware_get().

I don't know how DL_FLAG_AUTOREMOVE_CONSUMER works, but this sounds
better.

> This solves the problem of getting a kernel panic because of the use after
> free, but you'll still get some warnings after unbinding from the GPIO
> subsystem, for example, as we just removed a gpiochip that still has consumers
> up. I guess device links only go so far.

If this is indeed a real problem, lets take this to the GPIO
maintainers.

Best regards
Uwe

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

Attachment: signature.asc
Description: PGP signature