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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] lockdep: recursive locking splat in rp1_pwm_read_tachometer
--

commit 2891771f3f49717037220ec4f19e7c24529f99b5
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. This commit adds the supporting driver.

> diff --git a/drivers/pwm/pwm-rp1.c b/drivers/pwm/pwm-rp1.c
> new file mode 100644
> index 0000000000000..cfd38e46cc589
> --- /dev/null
> +++ b/drivers/pwm/pwm-rp1.c

[ ... ]

> +int rp1_pwm_read_tachometer(struct device *dev)
> +{
> +	struct pwm_chip *chip;
> +	struct rp1_pwm *rp1;
> +	u32 tach_val;
> +	int ret;
> +
> +	if (!dev)
> +		return -EINVAL;
> +
> +	device_lock(dev);
> +
> +	chip = dev_get_drvdata(dev);

[Severity: Medium]
Is it safe to call device_lock() inside this exported API?

When an external consumer (like a fan driver) calls this during its probe
or remove callbacks, the driver core already holds the consumer device's
lock. When this function then calls device_lock(), it acquires the PWM
device's lock. 

Because both locks belong to the exact same default device lock class, this
sequence triggers a lockdep recursive locking splat.

Could this access be protected using the driver's own internal
synchronization (such as rp1->lock) instead of relying on the driver core's
device_lock()?

> +	if (!chip) {
> +		ret = -ENODEV;
> +		goto err_dev_unlock;
> +	}
> +

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1789724999.git.andrea.porta@suse.com?part=2