Re: Patch "pwm: rockchip: Round period/duty down on apply, up on get" has been added to the 6.16-stable tree
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 getto the 6.16-stable tree which can be found at: http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git%3Ba=su...
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-a9c65acad7... 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
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 getto the 6.16-stable tree which can be found at: http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git%3Ba=su...
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-a9c65acad7... 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
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
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
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
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
linux-stable-mirror@lists.linaro.org
-
Greg KH -
Nicolas Frattaroli -
Uwe Kleine-König