linux-kernel - Re: [PATCH v19] pwm: airoha: Add support for EN7581 SoC

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <wntjec4p7nepuauucwqwgwcresphjikln7cqchep3vjocpuo6u@6hjpkwcbvx7d>
Date: Wed, 2 Jul 2025 08:01:05 +0200
From: Uwe Kleine-König <ukleinek@...nel.org>
To: Christian Marangi <ansuelsmth@...il.com>
Cc: linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org, 
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, Benjamin Larsson <benjamin.larsson@...exis.eu>, 
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>, Lorenzo Bianconi <lorenzo@...nel.org>
Subject: Re: [PATCH v19] pwm: airoha: Add support for EN7581 SoC

On Tue, Jul 01, 2025 at 09:20:24PM +0200, Christian Marangi wrote:
> On Tue, Jul 01, 2025 at 09:40:03AM +0200, Uwe Kleine-König wrote:
> > > +	shift = AIROHA_PWM_REG_CYCLE_CFG_SHIFT(shift);
> > > +
> > > +	/* Configure frequency divisor */
> > > +	mask = AIROHA_PWM_WAVE_GEN_CYCLE << shift;
> > > +	val = FIELD_PREP(AIROHA_PWM_WAVE_GEN_CYCLE, period_ticks) << shift;
> > > +	ret = regmap_update_bits(pc->regmap, AIROHA_PWM_REG_CYCLE_CFG_VALUE(offset),
> > > +				 mask, val);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	offset = bucket;
> > > +	shift = do_div(offset, AIROHA_PWM_BUCKET_PER_FLASH_PROD);
> > > +	shift = AIROHA_PWM_REG_GPIO_FLASH_PRD_SHIFT(shift);
> > > +
> > > +	/* Configure duty cycle */
> > > +	mask = AIROHA_PWM_GPIO_FLASH_PRD_HIGH << shift;
> > > +	val = FIELD_PREP(AIROHA_PWM_GPIO_FLASH_PRD_HIGH, duty_ticks) << shift;
> > > +	ret = regmap_update_bits(pc->regmap, AIROHA_PWM_REG_GPIO_FLASH_PRD_SET(offset),
> > > +				 mask, val);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	mask = AIROHA_PWM_GPIO_FLASH_PRD_LOW << shift;
> > > +	val = FIELD_PREP(AIROHA_PWM_GPIO_FLASH_PRD_LOW,
> > > +			 AIROHA_PWM_DUTY_FULL - duty_ticks) << shift;
> > > +	return regmap_update_bits(pc->regmap, AIROHA_PWM_REG_GPIO_FLASH_PRD_SET(offset),
> > > +				  mask, val);
> > 
> > Strange hardware, why do you have to configure both the high and the low
> > relative duty? What happens if AIROHA_PWM_GPIO_FLASH_PRD_LOW +
> > AIROHA_PWM_GPIO_FLASH_PRD_HIGH != AIROHA_PWM_DUTY_FULL?
> 
> From documentation it gets rejected and configured bucket doesn't work.

ok.

> > > [...]
> > > +	/*
> > > +	 * Duty is divided in 255 segment, normalize it to check if we
> > > +	 * can share a generator.
> > > +	 */
> > > +	duty_ns = DIV_U64_ROUND_UP(duty_ns * AIROHA_PWM_DUTY_FULL,
> > > +				   AIROHA_PWM_DUTY_FULL);
> > 
> > This looks bogus. This is just duty_ns = duty_ns, or what do I miss?
> > Also duty_ns is an u32 and AIROHA_PWM_DUTY_FULL an int, so there is no
> > need for a 64 bit division.
> 
> duty_ns * 255 goes beyond max u32.

In that case duty_ns * AIROHA_PWM_DUTY_FULL overflows to a smaller
value. Just because the value then is used by DIV_U64_ROUND_UP doesn't
fix the overflow. You need (u64)duty_ns * AIROHA_PWM_DUTY_FULL then.

> 225000000000.
> 
> Some revision ago it was asked to round also the duty_ns. And this is
> really to round_up duty in 255 segment.

Yes, and I identified this as the code that intends to do that. Please
double check this really works. I would claim you need:

	duty_ns = DIV_ROUND_UP(duty_ns, AIROHA_PWM_DUTY_FULL) * AIROHA_PWM_DUTY_FULL;

here because no matter if you round up or down, dividing
n * AIROHA_PWM_DUTY_FULL by AIROHA_PWM_DUTY_FULL yields n.

Best regards
Uwe

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