Re: [PATCH 2/2] pwm: add Axiado AX3000 PWM driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unclocked MMIO write leading to bus error on driver unbind or probe failure.
- [High] PWM hardware fails to re-enable when transitioning from a 0% duty cycle back to a positive duty cycle.
--

commit a497ac78be310d772f657a548c4a134269cc8d5a
Author: Petar Stepanovic <pstepanovic@axiado.com>

pwm: add Axiado AX3000 PWM driver
    
The Axiado AX3000 and AX3005 SoCs include PWM controllers that can be
used to generate configurable PWM output signals.
    
Add a PWM driver with support for configuring period, duty cycle, and
enable state through the Linux PWM framework.

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

[ ... ]

> +static int axiado_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			    const struct pwm_state *state)
> +{
> +	struct axiado_pwm_chip *axpwm = pwmchip_get_drvdata(chip);
> +	int ret;
> +
> +	if (state->polarity != PWM_POLARITY_NORMAL)
> +		return -EINVAL;
> +
> +	if (!state->enabled || !state->duty_cycle) {
> +		if (pwm->state.enabled)
> +			writel(AX_PWM_CTRL_DISABLE, axpwm->base + AX_PWM_CNTRL_REG);
> +
> +		return 0;
> +	}
> +
> +	ret = axiado_pwm_config(chip, pwm, state->duty_cycle, state->period);
> +	if (ret)
> +		return ret;
> +
> +	if (!pwm->state.enabled)
> +		writel(AX_PWM_CTRL_ENABLE, axpwm->base + AX_PWM_CNTRL_REG);

[Severity: High]
Will this logic fail to re-enable the hardware if transitioning from a 0%
duty cycle back to a positive duty cycle?

When the user requests a 0% duty cycle, the hardware is turned off by
writing AX_PWM_CTRL_DISABLE, but the PWM framework maintains
pwm->state.enabled = true.

If a positive duty cycle is then requested, the driver relies on checking
!pwm->state.enabled to re-enable the hardware. Since the cached state is
still true from the 0% duty cycle request, the check evaluates to false
and the AX_PWM_CTRL_ENABLE write is skipped, leaving the physical hardware
disabled.

[ ... ]

> +static void axiado_pwm_disable(void *data)
> +{
> +	struct axiado_pwm_chip *axpwm = data;
> +
> +	writel(AX_PWM_CTRL_DISABLE, axpwm->base + AX_PWM_CNTRL_REG);
> +}
> +
> +static int axiado_pwm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct axiado_pwm_chip *axpwm;
> +	struct pwm_chip *chip;
> +	int ret;
> +
> +	chip = devm_pwmchip_alloc(dev, AX_PWM_NUM, sizeof(*axpwm));
> +	if (IS_ERR(chip))
> +		return PTR_ERR(chip);
> +
> +	axpwm = pwmchip_get_drvdata(chip);
> +
> +	axpwm->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(axpwm->base))
> +		return dev_err_probe(dev, PTR_ERR(axpwm->base),
> +				     "failed to map registers\n");
> +
> +	ret = devm_add_action_or_reset(dev, axiado_pwm_disable, axpwm);
> +	if (ret)
> +		return ret;
> +
> +
> +	axpwm->clk = devm_clk_get_enabled(dev, "pwm");

[Severity: High]
Can this cause an unclocked MMIO write during driver unbind or probe failure?

The devres cleanup action axiado_pwm_disable() is registered via
devm_add_action_or_reset() before the hardware clock is acquired and
enabled via devm_clk_get_enabled().

Because devres executes actions in LIFO order, on driver unbind the
clock will be disabled before axiado_pwm_disable() is called. Similarly,
if devm_clk_get_enabled() fails, axiado_pwm_disable() will execute while
the clock is off.

Since axiado_pwm_disable() writes to the AX_PWM_CNTRL_REG MMIO register,
accessing it without the clock active could cause a bus error.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618-axiado-ax3000-pwm-v1-0-c9797a909414@axiado.com?part=2