| 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: <8f412c82cfd511b95d4b19a70d86d151ff666d1b.camel@gmail.com>
Date: Thu, 18 Sep 2025 15:07:48 +0100
From: Nuno Sá <noname.nuno@...il.com>
To: David Lechner <dlechner@...libre.com>, Michael Hennerich
<Michael.Hennerich@...log.com>, Jonathan Cameron <jic23@...nel.org>, Nuno
Sá
<nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>
Cc: linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: adc: ad7124: change setup reg allocation strategy
On Thu, 2025-09-18 at 08:16 -0500, David Lechner wrote:
> On 9/18/25 7:24 AM, Nuno Sá wrote:
> > On Wed, 2025-09-17 at 17:05 -0500, David Lechner wrote:
> > > Change the allocation strategy of the 8 SETUP registers from a least-
> > > recently-used (LRU) to a first-come-first-served basis.
> > >
>
> ...
>
> > > - /*
> > > - * This is just to make sure that the comparison is adapted after
> > > - * struct ad7124_channel_config was changed.
> > > - */
> > > - static_assert(sizeof_field(struct ad7124_channel_config,
> > > config_props) ==
> > > - sizeof(struct {
> > > - enum ad7124_ref_sel refsel;
> > > - bool bipolar;
> > > - bool buf_positive;
> > > - bool buf_negative;
> > > - unsigned int vref_mv;
> > > - unsigned int pga_bits;
> > > - unsigned int odr_sel_bits;
> > > - enum ad7124_filter_type filter_type;
> > > - unsigned int calibration_offset;
> > > - unsigned int calibration_gain;
> > > - }));
> > > -
> > > - for (i = 0; i < st->num_channels; i++) {
> > > - cfg_aux = &st->channels[i].cfg;
> > > -
> > > - if (cfg_aux->live &&
> > > - cfg->refsel == cfg_aux->refsel &&
> > > - cfg->bipolar == cfg_aux->bipolar &&
> > > - cfg->buf_positive == cfg_aux->buf_positive &&
> > > - cfg->buf_negative == cfg_aux->buf_negative &&
> > > - cfg->vref_mv == cfg_aux->vref_mv &&
> > > - cfg->pga_bits == cfg_aux->pga_bits &&
> > > - cfg->odr_sel_bits == cfg_aux->odr_sel_bits &&
> > > - cfg->filter_type == cfg_aux->filter_type &&
> > > - cfg->calibration_offset == cfg_aux-
> > > >calibration_offset &&
> > > - cfg->calibration_gain == cfg_aux->calibration_gain)
> > > - return cfg_aux;
> > > - }
> > > -
> > > - return NULL;
> > > -}
> > > -
> > > -static int ad7124_find_free_config_slot(struct ad7124_state *st)
> > > -{
> > > - unsigned int free_cfg_slot;
> > > -
> > > - free_cfg_slot = find_first_zero_bit(&st->cfg_slots_status,
> > > AD7124_MAX_CONFIGS);
> > > - if (free_cfg_slot == AD7124_MAX_CONFIGS)
> > > - return -1;
> > > -
> > > - return free_cfg_slot;
> > > -}
> > > -
> > > /* Only called during probe, so dev_err_probe() can be used */
> > > static int ad7124_init_config_vref(struct ad7124_state *st, struct
> > > ad7124_channel_config *cfg)
> > > {
> > > @@ -485,6 +427,21 @@ static int ad7124_init_config_vref(struct
> > > ad7124_state
> > > *st, struct ad7124_channe
> > > }
> > > }
> > >
> > > +static bool ad7124_config_equal(struct ad7124_channel_config *a,
> > > + struct ad7124_channel_config *b)
> > > +{
> > > + return a->refsel == b->refsel &&
> > > + a->bipolar == b->bipolar &&
> > > + a->buf_positive == b->buf_positive &&
> > > + a->buf_negative == b->buf_negative &&
> > > + a->vref_mv == b->vref_mv &&
> > > + a->pga_bits == b->pga_bits &&
> > > + a->odr_sel_bits == b->odr_sel_bits &&
> > > + a->filter_type == b->filter_type &&
> > > + a->calibration_offset == b->calibration_offset &&
> > > + a->calibration_gain == b->calibration_gain;
> > > +}
> >
> > Why not keeping the static_assert()? IIRC, Uwe felt fairly strong about
> > having
> > it.
>
> I thought by now that we had implemented all of the possible
> values so nothing else would be added so we didn't really need
> the check anymore. But I guess there are a few bits left in the
> CONFIG register that aren't accounted for.
>
> TBH, when making the other recent changes it just felt like a
> chore keeping it up to date and not particularly helpful. There
> is already a comment where the fields are declared that was
> enough to remind me to update this as well.
I brought it up because I remember Uwe kind of really wanted it (or maybe it was
requested by someone reviewing). Not sure now :).
I'm fine with removing it anyways.
>
> > > + /* Find the first channel using this slot, if any. */
> > > + for (j = 0; j < st->num_channels; j++) {
> > > + if (st->channels[j].cfg.cfg_slot != i)
> > > + continue;
> > >
> > > - /* mark slot as free */
> > > - assign_bit(lru_cfg->cfg_slot, &st->cfg_slots_status, 0);
> > > + /*
> > > + * If there is a match, increase the use count
> > > and
> > > share
> > > + * the slot with the requesting channel.
> > > + */
> > > + if (ad7124_config_equal(&st->channels[j].cfg,
> > > + &st-
> > > >channels[channel].cfg))
> > > {
> > > + st->cfg_slot_use_count[i]++;
> > > + st->channels[channel].cfg.cfg_slot = i;
> > >
> > > - /* invalidate all other configs that pointed to this one */
> > > - for (i = 0; i < st->num_channels; i++) {
> > > - cfg = &st->channels[i].cfg;
> > > + dev_dbg(&st->sd.spi->dev,
> > > + "Re-using config slot %u for
> > > channel
> > > %u, use count now %u\n",
> > > + i, channel, st-
> > > > cfg_slot_use_count[i]);
> > > +
> > > + return 0;
> > > + }
> > > + }
> > > + }
> >
> > I think we could have the above a bit simpler. Something like:
> >
> > for (j = 0; j < st->num_channels; j++) {
> > if (st->channels[j].cfg.cfg_slot == AD7124_CFG_SLOT_UNASSIGNED)
> > continue;
> > if (!ad7124_config_equal(&st->channels[j].cfg,
> > &st->channels[channel].cfg))
> > continue;
> >
> > i = st->channels[j].cfg.cfg_slot;
> > st->cfg_slot_use_count[i]++;
> > st->channels[channel].cfg.cfg_slot = i;
> > }
> >
> > Am I missing something?
>
> I like it. I should also rename the i and j variables to slot and channel
> to make it a bit more clear.
>
Sure...
>
- Nuno Sá
Powered by blists - more mailing lists