| 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: <649643ac-525f-4a82-9591-021983b00b70@baylibre.com>
Date: Thu, 18 Sep 2025 08:16:23 -0500
From: David Lechner <dlechner@...libre.com>
To: Nuno Sá <noname.nuno@...il.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 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.
>> + /* 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.
>
>>
>> - if (cfg->cfg_slot == lru_cfg->cfg_slot)
>> - cfg->live = false;
>> + /* Find a free slot and write setup to ADC. */
>> + for (i = 0; i < AD7124_MAX_CONFIGS; i++) {
>> + if (st->cfg_slot_use_count[i] == 0) {
>> + st->cfg_slot_use_count[i]++;
>> + st->channels[channel].cfg.cfg_slot = i;
>> +
>> + dev_dbg(&st->sd.spi->dev,
>> + "Allocated config slot %u for channel %u, use
>> count now %u\n",
>> + i, channel, st->cfg_slot_use_count[i]);
>> +
>> + return ad7124_write_config(st, &st-
>>> channels[channel].cfg, i);
>> + }
>
> nit: I tend to prefer
>
> if (st->cfg_slot_use_count[i] != 0) // or omit the != 0 part
> continue;
>
Me too.
> ...
>
> - Nuno Sá
>
Powered by blists - more mailing lists