Re: [PATCH v7 2/3] pwm: rp1: Add RP1 PWM controller driver

[Andrea della Porta <andrea.porta@suse.com>]

Hi Gary,

On 19:42 Tue 25 Aug     , Gary Guo wrote:
> On Fri Jul 24, 2026 at 4:04 PM BST, Andrea della Porta wrote:
> > Hi,
> >
> > On 10:02 Mon 20 Jul     , sashiko-bot@kernel.org wrote:
> >> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> >> - [High] Missing `.remove` callback leads to Use-After-Free in the PWM and Syscon cores and leaks clock resources on driver unbind.
> >> - [High] Register context loss on suspend/resume silently breaks PWM output.
> >> - [High] Unclamped hardware duty ticks lead to integer underflow in the PWM core for normal polarity.
> >> --
> >> 
> >> --- Patch [2]: [PATCH v7 2/3] pwm: rp1: Add RP1 PWM controller driver ---
> >> commit 0c98703d857577902e57d25b42a3d46dfbde5795
> >> Author: Naushir Patuck <naush@raspberrypi.com>
> >> 
> >> pwm: rp1: Add RP1 PWM controller driver
> >> 
> >> The Raspberry Pi RP1 southbridge features an embedded PWM controller with
> >> 4 output channels, alongside an RPM interface to read the fan speed on the
> >> Raspberry Pi 5. Add the supporting driver.
> > 
> >> > +static struct platform_driver rp1_pwm_driver = {
> >> > +	.probe = rp1_pwm_probe,
> >> > +	/*
> >> > +	 * .remove is not needed because this driver cannot be removed/unbound
> >> > +	 * cause it relies on syscon which cannot be removed
> >> > +	 */
> >> > +	.driver = {
> >> > +		.name = "rp1-pwm",
> >> 
> >> [Severity: High]
> >> Is it safe to omit the remove callback here?
> >> 
> >> Even with suppress_bind_attrs set on the driver, if the parent RP1 PCIe
> >> device is removed, reset, or unbound via sysfs, it will forcibly unbind all
> >> child devices.
> >> 
> >> Without a remove callback to run cleanup, pwmchip_remove() is never called
> >> and the clock is left enabled. Because the backing memory is devm-managed,
> >> the devres subsystem will eventually free the pwm_chip and regmap memory.
> >> This can leave dangling pointers in the core pwm_chips IDR and the global
> >> syscon_list, which could result in a use-after-free.
> >
> > This is true. Uwe, I guess I should reintroduce the .remove callback,
> > what do you think?
> >
> 
> How is remove going to work with syscon? The previous version seems to just
> ignore it completely.

True, unfortunately we have not much choice with syscon because it does not
contemplate a corresponding remove function. The best well-behaved code could
just deal with clocks and resources that can be released, while syscon is
unfortunately not among them.

On the other hand, I'd like to emphasize the following:

- the driver could not be unbinded nor unloaded (directly) by the user, and...
- ...PWM driver removal as a consequence of the pci bus device being forcibly
  removed will almost surely leave the system unusable.

As a matter of fact, even though it's not impossible for the remove callback
to be called, it seems highly unlikely. So maybe it's not that big deal.

> 
> Also, do you actually need syscon here? Given that the pwm fan device is going
> to depend on the PWM fan device anyway, you could just EXPORT_SYMBOL_NS an API
> from the rp1 driver that returns the tachometer reading when supplied with a rp1
> pwm device pointer?

I'm not devoted to syscon so if everyone agrees on the EXPORT_SYMBOL_NS solution
I'm perfectly fine. It seems to be used throughout several drivers and as long as
it's not considered to couple the consumer and the PWM producer too much, I think
it's an easy alternative. Uwe, are you fine with that?

Many thanks,
Andrea

> 
> Best,
> Gary
>