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

["Gary Guo" <gary@garyguo.net>]

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?

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

Best,
Gary

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