linux-kernel - Re: [PATCH v23] 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: <68c013fb.050a0220.702b3.6a13@mx.google.com>
Date: Tue, 9 Sep 2025 13:48:09 +0200
From: Christian Marangi <ansuelsmth@...il.com>
To: Uwe Kleine-König <ukleinek@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org,
	Benjamin Larsson <benjamin.larsson@...exis.eu>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	Lorenzo Bianconi <lorenzo@...nel.org>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH v23] pwm: airoha: Add support for EN7581 SoC

On Tue, Sep 09, 2025 at 12:26:48PM +0200, Uwe Kleine-König wrote:
> Hello Christian,
> 
> On Mon, Sep 08, 2025 at 08:48:38PM +0200, Christian Marangi wrote:
> > On Fri, Aug 01, 2025 at 11:15:41AM +0200, Uwe Kleine-König wrote:
> > > On Tue, Jul 08, 2025 at 04:50:52PM +0200, Christian Marangi wrote:
> > > > +	duty_ticks = airoha_pwm_get_duty_ticks_from_ns(period_ns, duty_ns);
> > > 
> > > As duty_ticks depends on the selected period_ticks, I think the bucket
> > > selection algorithm is still wrong.
> > > 
> > > Consider a request to implement
> > > 
> > > 	period_ns = 256 ms
> > > 	duty_ns = 128 ms
> > > 
> > > which at first correctly results in
> > > 
> > > 	period_ticks = 64
> > > 	duty_ticks = 127
> > > 
> > > If however all buckets are used and we only find one with say 62 period
> > > ticks we get period_ns = 248 and with that duty_ticks should better be
> > > 131 and not 127.
> >
> > sorry for checking this only now and maybe we need to catch this again.
> 
> no need to be sorry here. Taking time for replies is fine for me.
> 
> > Maybe we are getting confused here but itsn't this already handled by
> > the upper condition?
> > 
> > 		/* Ignore bucket with invalid configs */
> > 		if (bucket_period_ticks > period_ticks ||
> > 		    bucket_duty_ticks > duty_ticks)
> > 			continue;
> > 
> > 		/*
> > 		 * Search for a bucket closer to the requested period/duty
> > 		 * that has the maximal possible period that isn't bigger
> > 		 * than the requested period. For that period pick the maximal
> > 		 * duty cycle that isn't bigger than the requested duty_cycle.
> > 		 */
> > 		if (bucket_period_ticks > best_period_ticks ||
> > 		    (bucket_period_ticks == best_period_ticks &&
> > 		     bucket_duty_ticks > best_duty_ticks)) {
> > 			best_period_ticks = bucket_period_ticks;
> > 			best_duty_ticks = bucket_duty_ticks;
> > 			best = i;
> > 		}
> > 
> > We first limit for a bucket that doesn't got over both period and duty
> > and then we search for period and best duty. This should account for
> > never exceeding a duty since both period and duty are precalculated for
> > the current bucket and even if duty depends on period, again it's
> > precalculated. Am I missing something?
> 
> Let me describe the issue in more detail:
> 
> The period length is configured in the AIROHA_PWM_WAVE_GEN_CYCLE
> register in multiples of 4 ms. The duty length is configured in the
> AIROHA_PWM_GPIO_FLASH_PRD_HIGH register in multiples of
> $period_length/255.
> 
> So if you calcultate the number of multiples you need for duty_ns = 128
> ms based on the assumption that period_ns = 256 ms the result becomes
> wrong when you are forced to switch to period_ns = 248.
> 
> So to implement a request for period = 256 ms (64 ticks) and duty_cycle
> = 128 ms (127.5 duty ticks) having the choice between the two buckets:
> 
>  a) period_ticks = 62; duty_ticks = 127
>     (period = 248 ms, duty_cycle = 123.51372549019608 ms)
>  b) period_ticks = 62; duty_ticks = 131
>     (period = 248 ms, duty_cycle = 127.40392156862744 ms)
> 
> b) is the better one despite 127 duty_ticks would be an exact match for
> period_ticks = 64. So the issue is that the "Ignore bucket with invalid
> configs" kicks out b). That's wrong because
> 
> 	bucket_duty_ticks > duty_ticks
> 
> doesn't imply
> 
> 	bucket_duty > duty
> 
> .
> 

Thanks for the quick feedback hope we can takle this quick so we can
have this finally merged.

I changed the logic to this. What do you think? (I introduced an helper
to calculate the ns from raw ticks)

	duty_ns = airoha_pwm_get_duty_ns_from_ticks(period_ticks, duty_ticks);

...

		/* Ignore bucket with invalid period */
		if (bucket_period_ticks > period_ticks)
			continue;

		/*
		 * Search for a bucket closer to the requested period
		 * that has the maximal possible period that isn't bigger
		 * than the requested period. For that period pick the maximal
		 * duty cycle that isn't bigger than the requested duty_cycle.
		 */
		if (bucket_period_ticks >= best_period_ticks) {
			bucket_duty_ns = airoha_pwm_get_duty_ns_from_ticks(bucket_period_ticks,
									   bucket_duty_ticks);

			/* Skip bucket that goes over the requested duty */
			if (bucket_duty_ns > duty_ns)
				continue;

			if (bucket_duty_ns > best_duty_ns) {
				best_period_ticks = bucket_period_ticks;
				best_duty_ns = bucket_duty_ns;
				best = i;
			}
		}

We first search the period and then we calculate the duty in NS and
calculate the duty for each bucket. Should comply with the fact that
duty depends on period right?

-- 
	Ansuel