linux-kernel - Re: [PATCH] pwm: rockchip: round period/duty down on apply, up on get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <nf5bpi3sv5rrlvjg7q2ehyck6brmm3bzmzw4zclirwtiy7vrjp@yzsiao7krnt7>
Date: Mon, 16 Jun 2025 15:03:36 +0200
From: Uwe Kleine-König <ukleinek@...nel.org>
To: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
Cc: Heiko Stuebner <heiko@...ech.de>, 
	Brian Norris <briannorris@...omium.org>, Boris Brezillon <bbrezillon@...nel.org>, 
	Thierry Reding <thierry.reding@...il.com>, kernel@...labora.com, linux-pwm@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] pwm: rockchip: round period/duty down on apply, up on get

Hello Nico,

On Thu, May 22, 2025 at 09:26:44PM +0200, Nicolas Frattaroli wrote:
> 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 idempotency
> reasons. However, it's possible to keep idempotency while always
> rounding down in .apply.

Note that rounding nearest is difficult for idempotency. Consider a PWM
can implement the following period lengths:

	20.61, 21.4, 22.19

First if you use round-nearest you cannot set 21.4 because if you
request 21 you get 20.61 and if you request 22 you get 22.19. So if the
hardware still runs at 21.4, obviously apply ⚬ get_state cannot be
idempotent. If you round down in apply and up in get_state (or
vice-versa) you get idempotency. So using ROUND_CLOSEST for idempotency
is based on a wrong intuition.

> 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@...labora.com>
> ---
> This fix may need some careful testing from others before definitely
> being applied and backported. While I did test it myself of course,
> making sure to try a combination of periods and duty cycles, I really
> don't want to accidentally undo someone else's fix.

I like it apart from a small nitpick, see below. I think the danger of
breaking something is small and I tend to apply your patch.

> Some of the u64 math is a bit overkill, but I don't want to assume
> prescalers will never get larger than 4, which is where we start needing
> the 64-bit prescaled NSECS_PER_SEC value. clk_rate could also
> comfortably fit within u32 for any expected clock rate, but unsigned
> long can fit more depending on architecture, even if nobody is running
> the PWM hardware at 4.294967296 GHz.
> ---
> [...]
> @@ -103,8 +106,9 @@ static void rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  			       const struct pwm_state *state)
>  {
>  	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
> -	unsigned long period, duty;
> -	u64 clk_rate, div;
> +	u64 prescaled_ns = (u64)pc->data->prescaler * NSEC_PER_SEC;
> +	u64 clk_rate, tmp;
> +	u32 period, duty;

These hold the period and duty in ticks, so I'd call them period_ticks
and duty_ticks respectively.

>  	u32 ctrl;
>  
>  	clk_rate = clk_get_rate(pc->clk);
> @@ -114,12 +118,15 @@ static void rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)