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

On Fri, Jul 17, 2026 at 9:27 PM <[email protected]> wrote:
>
> 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.
> --
...
> > +     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?
>

At a sub-count boundary floor(duty) + floor(offset) can be cnt - 1 instead of
cnt, so an inverted request can indeed fall back to a non-inverted setting with
duty_offset rounded down to 0. That is a valid result, not a failure:
.round_waveform_tohw() must round every quantity down and only has to return a
waveform not exceeding the request (see the contract above
pwm_set_waveform_might_sleep() and pwm_check_rounding() in drivers/pwm/core.c).
duty_offset 0 <= requested duty_offset, so the result stays within the contract
and CONFIG_PWM_DEBUG accepts it.

The same count-based test is used by pwm-stm32.c (duty_ticks + offset_ticks >=
arr + 1). So the conservative count-based check is intentional. No change.

> > +     /*
> > +      * 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.

duty_offset_ns + duty_length_ns > period_length_ns is not an invalid waveform.
The waveform API requires only duty_length_ns <= period_length_ns and
duty_offset_ns < period_length_ns (pwm_wf_valid() in drivers/pwm/core.c); the
sum is deliberately allowed to exceed the period and represents the high phase
wrapping past the period end, i.e. inverted polarity (see the kerneldoc on
struct pwm_waveform in include/linux/pwm.h). For the example {period 7, duty 4,
offset 4} both invariants hold (4 <= 7, 4 < 7), so it is valid.

The core handles it explicitly: pwm_wf2state() maps
duty_length + duty_offset >= period to PWM_POLARITY_INVERSED with
duty_cycle = period - duty_length = 3 ns, so there is no underflow. Rounding
each reported value up independently is what pwm-stm32.c, pwm_th1520.rs and
pwm-axi-pwmgen.c do as well; it is required so that feeding the reported
waveform back through .round_waveform_tohw() reproduces the same counters.
No change.

Alexandre Mergnat

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic