Re: [PATCH] pwm: meson: Convert to waveform API

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Truncation error in hardware waveform quantization breaks legacy polarity inversion.
- [Medium] Independent rounding in waveform readback produces an invalid state where duty + offset > period.
--

--- Patch [1]: [PATCH] pwm: meson: Convert to waveform API ---
commit 205d0aee1231456b923515acf95a461a9f0a217f
Author: Alexandre Mergnat <amergnat@baylibre.com>

pwm: meson: Convert to waveform API

The PWM subsystem introduced a new waveform abstraction that describes a PWM
signal by period length, duty length and duty offset instead of period, duty
cycle and polarity. Adopt the new API by implementing .round_waveform_tohw(),
.round_waveform_fromhw(), .read_waveform() and .write_waveform() with a
private meson_pwm_waveform struct holding the hardware representation.

> diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c
> index 20e6cd07127c4..633b384358c08 100644
> --- a/drivers/pwm/pwm-meson.c
> +++ b/drivers/pwm/pwm-meson.c

[ ... ]

> +static int meson_pwm_round_waveform_tohw(struct pwm_chip *chip,
> +					 struct pwm_device *pwm,
> +					 const struct pwm_waveform *wf,
> +					 void *_wfhw)
>  {

[ ... ]

> +	if (meson->data->has_polarity && duty_cnt) {
> +		u64 offset_cnt = mul_u64_u64_div_u64(fin_freq, wf->duty_offset_ns,
> +						     NSEC_PER_SEC);
> +
> +		/*
> +		 * Waveforms with duty_offset_ns + duty_length_ns ==
> +		 * period_length_ns are implemented by inverting the output:
> +		 * The signal then is low for hi counts (the offset) followed
> +		 * by high for lo counts (the duty cycle). Other nonzero
> +		 * duty_offset_ns values are rounded down to 0.
> +		 */
> +		if (offset_cnt && duty_cnt + offset_cnt >= cnt) {

[Severity: High]
Could integer division truncation cause this check to fail for inverted
waveforms?

If duty_cnt and offset_cnt are calculated with independent truncation, their
sum might end up strictly less than cnt even when the original lengths sum to
the period.

Would this cause the driver to incorrectly fall back to normal polarity with a
zero offset?

> +			swap(wfhw->hi, wfhw->lo);
> +			wfhw->inverted = true;
> +		}
>  	}

[ ... ]

> +static int meson_pwm_round_waveform_fromhw(struct pwm_chip *chip,
> +					   struct pwm_device *pwm,
> +					   const void *_wfhw,
> +					   struct pwm_waveform *wf)
> +{

[ ... ]

> +	/*
> +	 * wfhw->hi and wfhw->lo are u16, so their sum doesn't overflow and
> +	 * NSEC_PER_SEC * (hi + lo) fits into the u64 dividend in
> +	 * meson_pwm_cnt_to_ns().
> +	 */
> +	wf->period_length_ns = meson_pwm_cnt_to_ns(wfhw->rate, wfhw->hi + wfhw->lo);
> +
> +	if (wfhw->inverted && wfhw->lo == 0) {
> +		/*
> +		 * The output is constant low. Report a normal constant-low
> +		 * waveform instead of duty_offset_ns == period_length_ns
> +		 * which wouldn't be a valid waveform description. This
> +		 * setting is only reachable when the hardware was programmed
> +		 * by the bootloader as .round_waveform_tohw() never produces
> +		 * it.
> +		 */
> +		wf->duty_length_ns = 0;
> +		wf->duty_offset_ns = 0;
> +	} else if (wfhw->inverted) {
> +		wf->duty_length_ns = meson_pwm_cnt_to_ns(wfhw->rate, wfhw->lo);
> +		wf->duty_offset_ns = meson_pwm_cnt_to_ns(wfhw->rate, wfhw->hi);

[Severity: Medium]
Since meson_pwm_cnt_to_ns() explicitly rounds up, can rounding duty_length_ns
and duty_offset_ns independently result in their sum exceeding
period_length_ns?

For example, if rate is 333333333 Hz and both hi and lo counts are 1,
period_length_ns rounds up to 7 ns, while duty_length_ns and duty_offset_ns
would each round up to 4 ns. Their sum (8 ns) would exceed the period length,
potentially leading to integer underflow in consumers attempting to calculate
the remaining period time.

> +	} else {
> +		wf->duty_length_ns = meson_pwm_cnt_to_ns(wfhw->rate, wfhw->hi);
> +		wf->duty_offset_ns = 0;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717-pwm_meson_convert_to_waveform_api-v1-1-d7dbdae1318d@baylibre.com?part=1

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic