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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] pwm: rp1: uninitialized variable base used in probe error check
- [Medium] pwm: rp1: missing hardware duty cycle cap in normal polarity
--

commit e4b281ff49d6f4c4ce04ab56fa923f8ea44e3ba3
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.

> diff --git a/drivers/pwm/pwm-rp1.c b/drivers/pwm/pwm-rp1.c
> new file mode 100644
> index 0000000000000..60b02d7c85ee8
> --- /dev/null
> +++ b/drivers/pwm/pwm-rp1.c
[ ... ]
> @@ -184,15 +184,15 @@ static int rp1_pwm_round_waveform_fromhw(struct pwm_chip *chip,
>  					 struct pwm_device *pwm,
>  					 const void *_wfhw,
>  					 struct pwm_waveform *wf)
>  {
[ ... ]
> +	if (!wfhw->inverted_polarity) {
> +		wf->duty_length_ns = DIV_ROUND_UP_ULL((u64)wfhw->duty_ticks * NSEC_PER_SEC,
> +						      (u32)clk_rate);
> +	} else {
> +		if (wfhw->duty_ticks > (u64)wfhw->period_ticks + 1) {
> +			/* 100% duty cycle case */
> +			ticks = 0;
> +		} else {
> +			ticks = (u64)wfhw->period_ticks + 1 - wfhw->duty_ticks;
> +		}

[Severity: Medium]
Could wfhw->duty_ticks exceed wfhw->period_ticks (e.g., if pre-configured by
a bootloader for a 100% duty cycle)?

If so, since rp1_pwm_round_waveform_fromhw() doesn't cap wfhw->duty_ticks
for normal polarity, could this result in duty_length_ns > period_length_ns,
violating the PWM API contract and creating an invalid initial waveform state?

[ ... ]
> @@ -295,12 +295,12 @@ static int rp1_pwm_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	unsigned long clk_rate;
>  	struct pwm_chip *chip;
>  	void __iomem	*base;
>  	struct rp1_pwm *rp1;
>  	int ret;
>  
>  	chip = devm_pwmchip_alloc(dev, RP1_PWM_NUM_PWMS, sizeof(*rp1));
>  	if (IS_ERR(chip))
>  		return PTR_ERR(chip);
>  
>  	rp1 = pwmchip_get_drvdata(chip);
>  
>  	rp1->base = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(base))
>  		return PTR_ERR(base);

[Severity: High]
Does this code check the uninitialized local variable base instead of
rp1->base? 

If devm_platform_ioremap_resource() fails, could this check allow the probe
to proceed with an error pointer in rp1->base, potentially leading to a kernel
panic when dereferenced later?

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