Re: Patch "pwm: rockchip: Round period/duty down on apply, up on get" has been added to the 6.16-stable tree

Re: Patch "pwm: rockchip: Round period/duty down on apply, up on get" has been added to the 6.16-stable tree

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2536 bytes --]

Hello Sasha,

On Fri, Aug 08, 2025 at 06:30:33PM -0400, Sasha Levin wrote:
> This is a note to let you know that I've just added the patch titled
> 
>     pwm: rockchip: Round period/duty down on apply, up on get
> 
> to the 6.16-stable tree which can be found at:
>     http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
> 
> The filename of the patch is:
>      pwm-rockchip-round-period-duty-down-on-apply-up-on-g.patch
> and it can be found in the queue-6.16 subdirectory.
> 
> If you, or anyone else, feels it should not be added to the stable tree,
> please let <stable@vger.kernel.org> know about it.
> 
> 
> 
> commit 51144efa3159cd95ab37e786c982822a060d7d1a
> Author: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> Date:   Mon Jun 16 17:14:17 2025 +0200
> 
>     pwm: rockchip: Round period/duty down on apply, up on get
>     
>     [ Upstream commit 0b4d1abe5ca568c5b7f667345ec2b5ad0fb2e54b ]
>     
>     With CONFIG_PWM_DEBUG=y, the rockchip PWM driver produces warnings like
>     this:
>     
>       rockchip-pwm fd8b0010.pwm: .apply is supposed to round down
>       duty_cycle (requested: 23529/50000, applied: 23542/50000)
>     
>     This is because the driver chooses ROUND_CLOSEST for purported
>     idempotency reasons. However, it's possible to keep idempotency while
>     always rounding down in .apply().
>     
>     Do this by making .get_state() always round up, and making .apply()
>     always round down. This is done with u64 maths, and setting both period
>     and duty to U32_MAX (the biggest the hardware can support) if they would
>     exceed their 32 bits confines.
>     
>     Fixes: 12f9ce4a5198 ("pwm: rockchip: Fix period and duty cycle approximation")
>     Fixes: 1ebb74cf3537 ("pwm: rockchip: Add support for hardware readout")
>     Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
>     Link: https://lore.kernel.org/r/20250616-rockchip-pwm-rounding-fix-v2-1-a9c65acad7b6@collabora.com
>     Signed-off-by: Uwe Kleine-König <ukleinek@kernel.org>
>     Signed-off-by: Sasha Levin <sashal@kernel.org>

while the new code makes the driver match the PWM rules now, I'd be
conservative and not backport that patch because while I consider it a
(very minor) fix that's a change in behaviour and maybe people depend on
that old behaviour. So let's not break our user's workflows and reserve
that for a major release. Please drop this patch from your queue.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Patch "pwm: rockchip: Round period/duty down on apply, up on get" has been added to the 6.16-stable tree

[Greg KH]

On Sat, Aug 09, 2025 at 11:45:23AM +0200, Uwe Kleine-König wrote:
> Hello Sasha,
> 
> On Fri, Aug 08, 2025 at 06:30:33PM -0400, Sasha Levin wrote:
> > This is a note to let you know that I've just added the patch titled
> > 
> >     pwm: rockchip: Round period/duty down on apply, up on get
> > 
> > to the 6.16-stable tree which can be found at:
> >     http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
> > 
> > The filename of the patch is:
> >      pwm-rockchip-round-period-duty-down-on-apply-up-on-g.patch
> > and it can be found in the queue-6.16 subdirectory.
> > 
> > If you, or anyone else, feels it should not be added to the stable tree,
> > please let <stable@vger.kernel.org> know about it.
> > 
> > 
> > 
> > commit 51144efa3159cd95ab37e786c982822a060d7d1a
> > Author: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > Date:   Mon Jun 16 17:14:17 2025 +0200
> > 
> >     pwm: rockchip: Round period/duty down on apply, up on get
> >     
> >     [ Upstream commit 0b4d1abe5ca568c5b7f667345ec2b5ad0fb2e54b ]
> >     
> >     With CONFIG_PWM_DEBUG=y, the rockchip PWM driver produces warnings like
> >     this:
> >     
> >       rockchip-pwm fd8b0010.pwm: .apply is supposed to round down
> >       duty_cycle (requested: 23529/50000, applied: 23542/50000)
> >     
> >     This is because the driver chooses ROUND_CLOSEST for purported
> >     idempotency reasons. However, it's possible to keep idempotency while
> >     always rounding down in .apply().
> >     
> >     Do this by making .get_state() always round up, and making .apply()
> >     always round down. This is done with u64 maths, and setting both period
> >     and duty to U32_MAX (the biggest the hardware can support) if they would
> >     exceed their 32 bits confines.
> >     
> >     Fixes: 12f9ce4a5198 ("pwm: rockchip: Fix period and duty cycle approximation")
> >     Fixes: 1ebb74cf3537 ("pwm: rockchip: Add support for hardware readout")
> >     Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> >     Link: https://lore.kernel.org/r/20250616-rockchip-pwm-rounding-fix-v2-1-a9c65acad7b6@collabora.com
> >     Signed-off-by: Uwe Kleine-König <ukleinek@kernel.org>
> >     Signed-off-by: Sasha Levin <sashal@kernel.org>
> 
> while the new code makes the driver match the PWM rules now, I'd be
> conservative and not backport that patch because while I consider it a
> (very minor) fix that's a change in behaviour and maybe people depend on
> that old behaviour. So let's not break our user's workflows and reserve
> that for a major release. Please drop this patch from your queue.

Now dropped, but note, any behavior change is ok for ANY kernel version
as we guarantee they all work the same :)

So good luck with your users in the 6.17 release...

thanks

greg k-h

Re: Patch "pwm: rockchip: Round period/duty down on apply, up on get" has been added to the 6.16-stable tree

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1114 bytes --]

Hello Greg,

On Tue, Aug 12, 2025 at 10:53:11AM +0200, Greg KH wrote:
> On Sat, Aug 09, 2025 at 11:45:23AM +0200, Uwe Kleine-König wrote:
> > while the new code makes the driver match the PWM rules now, I'd be
> > conservative and not backport that patch because while I consider it a
> > (very minor) fix that's a change in behaviour and maybe people depend on
> > that old behaviour. So let's not break our user's workflows and reserve
> > that for a major release. Please drop this patch from your queue.
> 
> Now dropped, but note, any behavior change is ok for ANY kernel version
> as we guarantee they all work the same :)

Your statement makes no sense, I guess you missed to add a "don't".
Otherwise I'd like to know who "we" is :-)

> So good luck with your users in the 6.17 release...

Yeah, thanks. I still like reducing the changes in stable that have
little benefit and the potential to subtly break workflows. I don't like
that potential breakage for 6.17 either, but there it's better
justifiable and that's the only way to get the improvement in at all.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Patch "pwm: rockchip: Round period/duty down on apply, up on get" has been added to the 6.16-stable tree

[Greg KH]

On Tue, Aug 12, 2025 at 12:36:48PM +0200, Uwe Kleine-König wrote:
> Hello Greg,
> 
> On Tue, Aug 12, 2025 at 10:53:11AM +0200, Greg KH wrote:
> > On Sat, Aug 09, 2025 at 11:45:23AM +0200, Uwe Kleine-König wrote:
> > > while the new code makes the driver match the PWM rules now, I'd be
> > > conservative and not backport that patch because while I consider it a
> > > (very minor) fix that's a change in behaviour and maybe people depend on
> > > that old behaviour. So let's not break our user's workflows and reserve
> > > that for a major release. Please drop this patch from your queue.
> > 
> > Now dropped, but note, any behavior change is ok for ANY kernel version
> > as we guarantee they all work the same :)
> 
> Your statement makes no sense, I guess you missed to add a "don't".
> Otherwise I'd like to know who "we" is :-)

{sigh} yes, I mean "any behavior change is NOT ok..."

sorry,

greg k-h

Re: Patch "pwm: rockchip: Round period/duty down on apply, up on get" has been added to the 6.16-stable tree

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3386 bytes --]

Hello Greg,

On Tue, Aug 12, 2025 at 12:53:49PM +0200, Greg KH wrote:
> On Tue, Aug 12, 2025 at 12:36:48PM +0200, Uwe Kleine-König wrote:
> > On Tue, Aug 12, 2025 at 10:53:11AM +0200, Greg KH wrote:
> > > On Sat, Aug 09, 2025 at 11:45:23AM +0200, Uwe Kleine-König wrote:
> > > > while the new code makes the driver match the PWM rules now, I'd be
> > > > conservative and not backport that patch because while I consider it a
> > > > (very minor) fix that's a change in behaviour and maybe people depend on
> > > > that old behaviour. So let's not break our user's workflows and reserve
> > > > that for a major release. Please drop this patch from your queue.
> > > 
> > > Now dropped, but note, any behavior change is ok for ANY kernel version
> > > as we guarantee they all work the same :)
> > 
> > Your statement makes no sense, I guess you missed to add a "don't".
> > Otherwise I'd like to know who "we" is :-)
> 
> {sigh} yes, I mean "any behavior change is NOT ok..."

Ahh I though you wanted to say "any behavior change is ok for ANY kernel
version as we don't guarantee they all work the same". So let me explain
a bit:

A PWM consumer (think fan driver) gets an opaque handle to the used PWM
device and then requests something like: "Configure the PWM to have a
period length of 50 ms and a duty length of 20 ms." The typical PWM
cannot fulfill the request exactly and has to choose if it configures
(say) period=46 ms + duty=16 ms, or period=52 ms and duty=26 ms (or
possibly a mixture of the two, or an error code). Traditionally each
driver implemented its own policy here and so the generic fan driver
knows neither the possible hardware restrictions (another hardware might
be able to do period=51 ms and duty=19 ms) nor how the driver picked one
of the options. Making things harder, it depends on the use case which
policy is the best, which also explains that different drivers picked
different algorithms. And also it's unclear even what "nearest" means.
For example a hardware might be able to configure period=17 ms or
period=24 ms in reply to a request of period=20ms. You probably say that
17 ms is nearer. But if you think in frequencies the request of
period=20ms corresponds to 50 Hz and then 24 ms ~ 41.6667 Hz is better
than 17 ms ~ 58.8235 Hz.

To improve that situation a bit at least new PWM drivers are supposed to
round down both values. The commit under discussion modifies an existing
driver to align to that policy. So from a global point of view this is
an improvement, because it makes things a bit more deterministic for a
PWM consumer that doesn't know about the hardware details. The down side
is that a PWM consumer that does know these details might depend on the
actual implementation of the previously implemented policy.

So this is a change in behaviour, but it adds to the consistency of the
PWM framework. If you want to make an LED blink or drive a fan, that
(most likely) doesn't matter to you. If you drive a robot arm in a
construction facility, it might.

The mid term solution is a new PWM API that gives more control to the
consumer. The framework bits for that are already done and three drivers
are implementing that and two more are in the making. (And if you use
these with the legacy consumer function you also get the round down
behaviour.)

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Patch "pwm: rockchip: Round period/duty down on apply, up on get" has been added to the 6.16-stable tree

[Nicolas Frattaroli]

Hi Greg & Uwe,

On Tuesday, 12 August 2025 22:15:37 Central European Summer Time Uwe Kleine-König wrote:
> Hello Greg,
> 
> On Tue, Aug 12, 2025 at 12:53:49PM +0200, Greg KH wrote:
> > On Tue, Aug 12, 2025 at 12:36:48PM +0200, Uwe Kleine-König wrote:
> > > On Tue, Aug 12, 2025 at 10:53:11AM +0200, Greg KH wrote:
> > > > On Sat, Aug 09, 2025 at 11:45:23AM +0200, Uwe Kleine-König wrote:
> > > > > while the new code makes the driver match the PWM rules now, I'd be
> > > > > conservative and not backport that patch because while I consider it a
> > > > > (very minor) fix that's a change in behaviour and maybe people depend on
> > > > > that old behaviour. So let's not break our user's workflows and reserve
> > > > > that for a major release. Please drop this patch from your queue.
> > > > 
> > > > Now dropped, but note, any behavior change is ok for ANY kernel version
> > > > as we guarantee they all work the same :)
> > > 
> > > Your statement makes no sense, I guess you missed to add a "don't".
> > > Otherwise I'd like to know who "we" is :-)
> > 
> > {sigh} yes, I mean "any behavior change is NOT ok..."
> 
> Ahh I though you wanted to say "any behavior change is ok for ANY kernel
> version as we don't guarantee they all work the same". So let me explain
> a bit:
> 
> A PWM consumer (think fan driver) gets an opaque handle to the used PWM
> device and then requests something like: "Configure the PWM to have a
> period length of 50 ms and a duty length of 20 ms." The typical PWM
> cannot fulfill the request exactly and has to choose if it configures
> (say) period=46 ms + duty=16 ms, or period=52 ms and duty=26 ms (or
> possibly a mixture of the two, or an error code). Traditionally each
> driver implemented its own policy here and so the generic fan driver
> knows neither the possible hardware restrictions (another hardware might
> be able to do period=51 ms and duty=19 ms) nor how the driver picked one
> of the options. Making things harder, it depends on the use case which
> policy is the best, which also explains that different drivers picked
> different algorithms. And also it's unclear even what "nearest" means.
> For example a hardware might be able to configure period=17 ms or
> period=24 ms in reply to a request of period=20ms. You probably say that
> 17 ms is nearer. But if you think in frequencies the request of
> period=20ms corresponds to 50 Hz and then 24 ms ~ 41.6667 Hz is better
> than 17 ms ~ 58.8235 Hz.
> 
> To improve that situation a bit at least new PWM drivers are supposed to
> round down both values. The commit under discussion modifies an existing
> driver to align to that policy. So from a global point of view this is
> an improvement, because it makes things a bit more deterministic for a
> PWM consumer that doesn't know about the hardware details. The down side
> is that a PWM consumer that does know these details might depend on the
> actual implementation of the previously implemented policy.

As the author of the patch in question, I thought I should also chime
in to elaborate on what this means in real terms on the hardware this
patch is applicable to. In my testing, I've found the difference is
usually a few tens of Hertz at a scale of tens of thousands of Hertz at
best. However, out of an abundance of caution, I didn't want this to be
picked up by literally every stable kernel still supported, because if
this causes an actual observable functional change in the real world
then it's only accidental as a regression. This should not be the case,
but I'd rather not tempt fate here.

What it does do however, aside from improving consistency here, is that
it makes the PWM core no longer produce a warning on kernels built with
PWM_DEBUG whenever the fan on my RADXA ROCK 5B changes speed. As someone
who worked on both a new PWM driver where I wanted PWM_DEBUG on at the
time as well as running this existing driver on the aforementioned device
while working on other drivers of this SoC vendor, I did not want to
switch between kernel configs all the time, and this greatly improved
my willingness to keep on living.

So in conclusion:
1. this does not change behaviour for any real-world use case, as the
   difference is so tiny it drowns out in the usual tolerances of PWM
   consumers to account for clock shenanigans.
2. even if someone does rely on 0.01% differences in PWM output on a
   consumer device SoC, they would probably appreciate predictable
   behaviour going forward, instead of their requested rate having
   an error that is either positive or negative depending on how
   the math works out. But they'd probably appreciate this change as
   part of a new major release, not a stable patch release.
3. The real motivation is making me less sad while working on other
   things.

Kind regards,
Nicolas Frattaroli

> 
> So this is a change in behaviour, but it adds to the consistency of the
> PWM framework. If you want to make an LED blink or drive a fan, that
> (most likely) doesn't matter to you. If you drive a robot arm in a
> construction facility, it might.
> 
> The mid term solution is a new PWM API that gives more control to the
> consumer. The framework bits for that are already done and three drivers
> are implementing that and two more are in the making. (And if you use
> these with the legacy consumer function you also get the round down
> behaviour.)
> 
> Best regards
> Uwe
>