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

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

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.

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?

Best,
Gary