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

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

Hi Gary,

On 15:05 Fri 04 Sep     , Gary Guo wrote:
> On Fri Sep 4, 2026 at 2:53 PM BST, Andrea della Porta wrote:
> > 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,
> 
> Oops, it should have been "PWM fan device depend on PWM device". But you got it.
> 
> >> 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?
> 
> I think that's pretty typical for platform devices, provided that the consumer
> and the producer are closely related.
> 
> I checked the RPi vendor kernel code and it looks like they're modifying the PWM
> fan driver code directly -- in which case the coupling would definitely be
> problematic. But I think that modification is problematic itself and not
> upstreamable. I suppose you'll add a separate, dedicated RP1 fan driver?

You nailed it. I've changed downstream driver approach because it was too invasive
to the pwm-fan driver and, as you mentioned, not upstreamable.
A separate RP1-custom fan driver was in plan from teh beginning.

> 
> In which case I don't see the coupling being an issue.

Perfect!

Many thanks,
Andrea

> 
> Best,
> Gary
> 
> >
> > Many thanks,
> > Andrea
> >
> >> 
> >> Best,
> >> Gary
> >> 
> 
>