| 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
| ||
|
Message-ID: <68507d77.050a0220.3cdb49.76dc@mx.google.com>
Date: Mon, 16 Jun 2025 22:24:22 +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>
Subject: Re: [PATCH v13] pwm: airoha: Add support for EN7581 SoC
On Fri, Jun 06, 2025 at 08:33:36AM +0200, Uwe Kleine-König wrote:
> Hello Christian,
>
> On Thu, Jun 05, 2025 at 11:45:52AM +0200, Christian Marangi wrote:
> > On Tue, Jun 03, 2025 at 06:57:36PM +0200, Uwe Kleine-König wrote:
> > > [....]
> > > It took me a while to come up with that, and it's completely untested.
> > >
> >
> > Mhhh it looks quite confusing. Also just to understand and looking at
> > the PWM core, we should search for the bucket that is both closer to the
> > requested duty and the requested period correct? In v12 I was with the
> > idea that only period had to be closer.
>
> The objective is as follows:
>
> Pick 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. (For the waveform callbacks the algorithm
> is slightly different.)
>
> > Anyway do you think this alternative version can work? I applied a more
> > simple logic.
> >
> > static int airoha_pwm_get_generator(struct airoha_pwm *pc, u64 duty_ns,
> > u64 period_ns)
> > {
> > unsigned long best_bucket_diff = ULONG_MAX;
> > int i, best = -ENOENT, unused = -ENOENT;
> >
> > for (i = 0; i < ARRAY_SIZE(pc->buckets); i++) {
> > struct airoha_pwm_bucket *bucket = &pc->buckets[i];
> > unsigned long duty_diff, period_diff, bucket_diff;
> > u64 bucket_period_ns = bucket->period_ns;
> > u64 bucket_duty_ns = bucket->duty_ns;
> > u32 duty_ticks, duty_ticks_bucket;
> >
> > /* If found, save an unused bucket to return it later */
> > if (!bucket->used && unused == -ENOENT) {
>
> You should drop the check for `unused == -ENOENT` here as with
> !bucket->used bucket_period_ns and bucket_duty_ns are not values that
> you should work with.
>
The unused == -ENOENT was there as without it we will always use the
last unused bucket instead of stopping at the first. Doesn't change
anything tho just taste. But you are probably right about the wrong
values.
> > unused = i;
> > continue;
> > }
> >
> > /* We found a matching bucket, exit early */
> > if (duty_ns == bucket_duty_ns &&
> > period_ns == bucket_period_ns)
> > return i;
> >
> > /*
> > * Unlike duty cycle zero, which can be handled by
> > * disabling PWM, a generator is needed for full duty
> > * cycle but it can be reused regardless of period
> > */
> > duty_ticks = airoha_pwm_get_duty_ticks_from_ns(period_ns, duty_ns);
> > duty_ticks_bucket = airoha_pwm_get_duty_ticks_from_ns(bucket_period_ns,
> > bucket_duty_ns);
> > if (duty_ticks == AIROHA_PWM_DUTY_FULL &&
> > duty_ticks_bucket == AIROHA_PWM_DUTY_FULL)
> > return i;
> >
> > /*
> > * With an unused bucket available, skip searching for
> > * a bucket to recycle (closer to the requested period/duty)
> > */
> > if (unused != -ENOENT)
> > continue;
> >
> > /* Ignore bucket with invalid configs */
> > if (bucket_period_ns > period_ns ||
> > bucket_duty_ns > duty_ns)
> > continue;
> >
> > /*
> > * Search for a bucket closer to the requested period/duty
> > * following this logic:
> > * 1. Calculate the bucket period diff from the requested period
> > * 2. Calculate the duty period diff from the requested duty
> > * 3. Sum the 2 diff
> > * 4. Search for the bucket that have the minor diff across all
> > * buckets.
> > */
> > period_diff = period_ns - bucket_period_ns;
> > duty_diff = duty_ns - bucket_duty_ns;
> > bucket_diff = period_diff + duty_diff;
> >
> > /* Save the best found bucket as we test each one */
> > if (bucket_diff < best_bucket_diff) {
> > best_bucket_diff = bucket_diff;
> > best = i;
> > }
> > }
>
> This is the wrong metric. If the request has period = 1000 and
> duty_cycle = 500 the possibility
>
> period = 999
> duty_cycle = 1
>
> is considered preferable to
>
> period = 998
> duty_cycle = 500
>
> (strange as this might seem). But the condition I used can indeed be
> simplified to
>
> if (bucket_period_ns > pc->buckets[best].period_ns ||
> (bucket_period_ns == pc->buckets[best].period_ns &&
> bucket_duty_ns > pc->buckets[best].duty_ns)
> best = i;
>
> as we already know that bucket_period_ns <= period_ns and
> bucket_duty_ns <= duty_ns.
>
I just sent v14 with a variant of this. I tested it and seems to work
correctly.
> > > > +static int airoha_pwm_consume_generator(struct airoha_pwm *pc,
> > > > + u64 duty_ns, u64 period_ns,
> > > > + unsigned int hwpwm)
> > > > +{
> > > > + int bucket;
> > > > +
> > > > + /*
> > > > + * Search for a bucket that already satisfy duty and period
> > > > + * or an unused one.
> > > > + * If not found, -ENOENT is returned.
> > > > + */
> > > > + bucket = airoha_pwm_get_generator(pc, duty_ns, period_ns);
> > > > + if (bucket < 0)
> > > > + return bucket;
> > > > +
> > > > + airoha_pwm_release_bucket_config(pc, hwpwm);
> > > > + pc->buckets[bucket].used |= BIT_ULL(hwpwm);
> > > > + pc->buckets[bucket].period_ns = period_ns;
> > > > + pc->buckets[bucket].duty_ns = duty_ns;
> > >
> > > pc->buckets[bucket].period_ns and pc->buckets[bucket].duty_ns should
> > > only get assigned if pc->buckets[bucket].used == 0.
> > >
> >
> > Yesp micro optimization but I changed the logic.
>
> No, that's not a micro optimisation. If you reuse a bucket you're not
> supposed to change the configuration of another output.
>
You are right. It all worked with the assumption that we always had
consistent duty and period but it's fragile. Better safe than sorry.
> > > > + /*
> > > > + * Period goes at 4ns step, normalize it to check if we can
> > > > + * share a generator.
> > > > + */
> > > > + period_ns = rounddown(period_ns, AIROHA_PWM_PERIOD_TICK_NS);
> > >
> > > Do the same for duty_ns?
> > >
> >
> > Duty is in % so we really can't round it. Am I wrong?
>
> Well, in airoha_pwm_config you do:
>
> if (duty_ns == bucket_duty_ns &&
> period_ns == bucket_period_ns)
> return i;
>
> so better make sure that duty_ns is a value that can be realized.
>
In v14 I applied a normalization also for duty. Wrong value for duty can
be used that would stop from reusing a bucket so yes it's required.
Hope v14 is THE ONE :D and thanks a lot for the feedback/review.
--
Ansuel
Powered by blists - more mailing lists