Re: [PATCH v5 5/7] pwm: tegra: Parametrize duty and scale field widths

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Division by zero kernel panic in `tegra_pwm_probe` if the retrieved clock rate is less than `TEGRA_PWM_DEPTH` (256).
- [High] Integer truncation of 64-bit PWM state values to 32-bit signed integers in `tegra_pwm_config` leading to a division by zero kernel panic.
--

commit 0c0ba991124cb1c177969041379fb6a95cd2a4c9
Author: Mikko Perttunen <mperttunen@nvidia.com>

pwm: tegra: Parametrize duty and scale field widths

This patch parametrizes the scale field width for the Tegra PWM driver to
accommodate wider fields in Tegra264. It separates the depth value from the
duty field width and removes the duty field width definition.

> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> index c82d537436a0f..7c7b884d44363 100644
> --- a/drivers/pwm/pwm-tegra.c
> +++ b/drivers/pwm/pwm-tegra.c

[ ... ]

> @@ -105,22 +107,22 @@ static int tegra_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  
>  	/*
>  	 * Convert from duty_ns / period_ns to a fixed number of duty ticks
> -	 * per (1 << TEGRA_PWM_DUTY_WIDTH) cycles and make sure to round to the
> +	 * per TEGRA_PWM_DEPTH cycles and make sure to round to the
>  	 * nearest integer during division.
>  	 */
> -	c *= (1 << TEGRA_PWM_DUTY_WIDTH);
> +	c *= TEGRA_PWM_DEPTH;
>  	c = DIV_ROUND_CLOSEST_ULL(c, period_ns);

[Severity: High]
This is a pre-existing issue, but could this division result in a kernel
panic if period_ns becomes zero?

The core PWM framework provides 64-bit unsigned integers for the duty cycle
and period. When tegra_pwm_apply() passes these to tegra_pwm_config(), they
are cast to 32-bit signed integers (int duty_ns, int period_ns).

If a user sets a period of 4294967296 ns (2^32 ns, roughly 4.29 seconds) via
sysfs, the 64-bit value gets truncated to period_ns = 0. 

Since the DIV_ROUND_CLOSEST_ULL() division occurs before any bounds checking
against min_period_ns, does this allow an unprivileged user with sysfs access
to trigger a division by zero exception and panic the kernel?

[ ... ]

> @@ -319,7 +321,7 @@ static int tegra_pwm_probe(struct platform_device *pdev)
>  
>  	/* Set minimum limit of PWM period for the IP */
>  	pc->min_period_ns =
> -	    (NSEC_PER_SEC / (pc->clk_rate >> TEGRA_PWM_DUTY_WIDTH)) + 1;
> +	    (NSEC_PER_SEC / (pc->clk_rate / TEGRA_PWM_DEPTH)) + 1;

[Severity: High]
This isn't a bug introduced by this patch, but can this lead to a division
by zero exception during probe?

If clk_get_rate() returns a rate less than TEGRA_PWM_DEPTH (256) or fails
and returns 0, the denominator (pc->clk_rate / TEGRA_PWM_DEPTH) evaluates
to 0.

Will the subsequent NSEC_PER_SEC / 0 operation cause an immediate kernel panic
during boot? Should the driver validate pc->clk_rate and return an error if
it is too low?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529-t264-pwm-v5-0-7bf9e405a96a@nvidia.com?part=5