Re: [PATCH v24 2/2] pwm: Add OpenCores PTC PWM driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Low] pwm: ocores: clk_get_rate() called before enabling clock in
  ocores_pwm_probe()
- [Low] pwm: ocores: unlocked and pre-enable clk_get_rate() in
  ocores_pwm_probe()
--

commit 33c73933621c3feed0e1d70460454bd0821561cc
Author: Hal Feng <hal.feng@starfivetech.com>

pwm: Add OpenCores PTC PWM driver

This commit adds a PWM driver for the OpenCores PTC IP core.

> diff --git a/drivers/pwm/pwm-ocores.c b/drivers/pwm/pwm-ocores.c
> new file mode 100644
> index 0000000000000..4c7e87cb009f6
> --- /dev/null
> +++ b/drivers/pwm/pwm-ocores.c
[ ... ]
> +static int ocores_pwm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct ocores_pwm_device *ddata;
> +	struct pwm_chip *chip;
> +	int ret;
> +
> +	chip = devm_pwmchip_alloc(dev, 1, sizeof(*ddata));
> +	if (IS_ERR(chip))
> +		return PTR_ERR(chip);
> +
> +	chip->ops = &ocores_pwm_ops;
> +	ddata = pwmchip_get_drvdata(chip);
> +
> +	ddata->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(ddata->base))
> +		return dev_err_probe(dev, PTR_ERR(ddata->base),
> +				     "Failed to map IO resources\n");
> +
> +	ddata->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(ddata->clk))
> +		return dev_err_probe(dev, PTR_ERR(ddata->clk),
> +				     "Failed to get clock\n");
> +
> +	ddata->clk_rate = clk_get_rate(ddata->clk);
> +	if (!ddata->clk_rate || ddata->clk_rate > NSEC_PER_SEC)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Invalid clock rate: %lu\n", ddata->clk_rate);

[Severity: Low]
Does caching ddata->clk_rate here without locking the clock rate via
devm_clk_rate_exclusive_get() and after enabling the clock leave the
driver vulnerable to an invalid initial rate or runtime clock rate
changes?  If the parent clock rate changes after probe, ocores_pwm_apply()
and ocores_pwm_get_state() will compute inaccurate periods and duty cycles
from the stale rate, or violate the NSEC_PER_SEC bound.

> +
> +	ddata->rst = devm_reset_control_get_optional_shared(dev, NULL);
> +	if (IS_ERR(ddata->rst))
> +		return dev_err_probe(dev, PTR_ERR(ddata->rst),
> +				     "Failed to get reset\n");
> +
> +	platform_set_drvdata(pdev, ddata);
> +
> +	ret = ocores_pwm_runtime_resume(dev);
> +	if (ret)
> +		return ret;

[Severity: Low]
Should clk_get_rate(ddata->clk) be called after
ocores_pwm_runtime_resume() enables the clock?  The clk API only
guarantees a valid rate once the clock is enabled, so querying it before
clk_prepare_enable() can return 0 (failing probe with -EINVAL) or a stale
rate on clock providers that compute their rate on enable.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107031251.98972-1-hal.feng@starfivetech.com?part=2