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

[Nicolas Saenz Julienne <nsaenzjulienne@suse.de>]

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

Hi Uwe,

On Tue, 2020-10-13 at 14:17 +0200, Uwe Kleine-König wrote:
> Hello Nicolas,
> 
> 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:
> > > > +	depends on RASPBERRYPI_FIRMWARE || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE)
> > > 
> > > This is more complicated than necessary.
> > > 
> > > 	depends on RASPBERRYPI_FIRMWARE || COMPILE_TEST
> > > 
> > > is logically equivalent.
> > 
> > It's not exactly the same, see patch 7ed915059c300 ("gpio: raspberrypi-ext: fix
> > firmware dependency ") which explains why this pattern is needed.

I'll add a comment.

> Hmm, this is strange, but if Arnd doesn't know a better solution, then
> be it so. Is this idiom usual enough to not justify a comment?
> 
> > > What happens if duty_cycle happens to be bigger than RPI_PWM_MAX_DUTY?
> > > 
> > > I think the right thing to do here is:
> > > 
> > > 	if (state->period < RPI_PWM_PERIOD_NS ||
> > 
> > Why not using state->period != RPI_PWM_PERIOD_NS here?
> 
> From the consumer's point of view having to hit the only correct period
> is hard. So the usual convention is to provide the biggest period not
> bigger than the requested one. (The idea for the future is to provide a
> pwm_round_state() function which allows to find out the effect of
> pwm_apply_state() with the same arguments. This then allows to
> efficiently find the best setting for the consumer.)

Fair enough.

> > > Huh, why do you have to do this twice, just with different error
> > > messages? I assume you want to set RPI_PWM_DEF_DUTY_REG? What is the
> > > effect of writing this property?
> > 
> > Obviously, I failed to change the register name.
> > 
> > This is supposed to set the default duty cycle after resetting the board. I
> > added it so as to keep compatibility with the downstream version of this.
> > 
> > I'll add a comment to explain this.
> 
> fine.
> 
> > > > +	int ret;
> > > > +
> > > > +	firmware_node = of_get_parent(dev->of_node);
> > > > +	if (!firmware_node) {
> > > > +		dev_err(dev, "Missing firmware node\n");
> > > > +		return -ENOENT;
> > > > +	}
> > > > +
> > > > +	firmware = rpi_firmware_get(firmware_node);
> > > > +	of_node_put(firmware_node);
> > > > +	if (!firmware)
> > > > +		return -EPROBE_DEFER;
> > > 
> > > 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.

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) {

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

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.

Any ideas/comments?

Regards,
Nicolas


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