Re: [PATCH v4 3/9] i2c: atr: split up i2c_atr_get_mapping_by_addr()

Luca Ceresoli
2025-05-05T11:22:52Z

On Mon, 5 May 2025 13:26:54 +0300
Cosmin Tanislav <demonsingur@gmail.com> wrote:

> On 4/30/25 5:33 PM, Luca Ceresoli wrote:
> > On Mon, 28 Apr 2025 13:25:08 +0300
> > Cosmin Tanislav <demonsingur@gmail.com> wrote:
> >
> >> The i2c_atr_get_mapping_by_addr() function handles three separate
> >> usecases: finding an existing mapping, creating a new mapping, or
> >> replacing an existing mapping if a new mapping cannot be created
> >> because there aren't enough aliases available.
> >>
> >> Split up the function into three different functions handling its
> >> individual usecases to prepare for better usage of each one.
> >>
> >> Signed-off-by: Cosmin Tanislav <demonsingur@gmail.com>
> >
> > This function has become quite complex over time, so this looks like a
> > good cleanup by itself even not counting the advantages coming with the
> > following patches.
> >
> > I have only one small remark, see below.
> >
> >> diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
> >> index 939fb95fe781..184c57c31e60 100644
> >> --- a/drivers/i2c/i2c-atr.c
> >> +++ b/drivers/i2c/i2c-atr.c
> >> @@ -239,9 +239,23 @@ static void i2c_atr_release_alias(struct i2c_atr_alias_pool *alias_pool, u16 ali
> >> spin_unlock(&alias_pool->lock);
> >> }
> >>
> >> -/* Must be called with alias_pairs_lock held */
> >> static struct i2c_atr_alias_pair *
> >> -i2c_atr_get_mapping_by_addr(struct i2c_atr_chan *chan, u16 addr)
> >> +i2c_atr_find_mapping_by_addr(struct i2c_atr_chan *chan, u16 addr)
> >> +{
> >> + struct i2c_atr_alias_pair *c2a;
> >> +
> >> + lockdep_assert_held(&chan->alias_pairs_lock);
> >> +
> >> + list_for_each_entry(c2a, &chan->alias_pairs, node) {
> >> + if (c2a->addr == addr)
> >> + return c2a;
> >> + }
> >> +
> >> + return NULL;
> >> +}
> >> +
> >> +static struct i2c_atr_alias_pair *
> >> +i2c_atr_replace_mapping_by_addr(struct i2c_atr_chan *chan, u16 addr)
> >> {
> >> struct i2c_atr *atr = chan->atr;
> >> struct i2c_atr_alias_pair *c2a;
> >> @@ -254,41 +268,57 @@ i2c_atr_get_mapping_by_addr(struct i2c_atr_chan *chan, u16 addr)
> >>
> >> alias_pairs = &chan->alias_pairs;
> >>
> >> - list_for_each_entry(c2a, alias_pairs, node) {
> >> - if (c2a->addr == addr)
> >> - return c2a;
> >> + if (unlikely(list_empty(alias_pairs)))
> >> + return NULL;
> >> +
> >> + list_for_each_entry_reverse(c2a, alias_pairs, node) {
> >> + if (!c2a->fixed) {
> >> + found = true;
> >> + break;
> >> + }
> >> }
> >>
> >> + if (!found)
> >> + return NULL;
> >> +
> >> + atr->ops->detach_addr(atr, chan->chan_id, c2a->addr);
> >> + c2a->addr = addr;
> >> +
> >> + list_move(&c2a->node, alias_pairs);
> >> +
> >> + alias = c2a->alias;
> >> +
> >> + ret = atr->ops->attach_addr(atr, chan->chan_id, c2a->addr, c2a->alias);
> >> + if (ret) {
> >> + dev_err(atr->dev, "failed to attach 0x%02x on channel %d: err %d\n",
> >> + addr, chan->chan_id, ret);
> >> + i2c_atr_destroy_c2a(&c2a);
> >> + i2c_atr_release_alias(chan->alias_pool, alias);
> >> + return NULL;
> >> + }
> >> +
> >> + return c2a;
> >> +}
> >> +
> >> +static struct i2c_atr_alias_pair *
> >> +i2c_atr_create_mapping_by_addr(struct i2c_atr_chan *chan, u16 addr)
> >
> > I would move the _create function before the _replace one, because
> > that's the logical order in which they are called.
> >
>
> Sadly the diff actually becomes bigger by doing this.
> before: 78 insertions(+), 32 deletions(-)
> after: 84 insertions(+), 38 deletions(-)

The diff size is not at all the primary goal. I just epected it would
reduce, but OK, it does not matter.

> If we were to put things in a logical order then should we put _find()
> after create(), or after replace()? There's no specific order in that
> case. I think we should keep things as-is as it matches the previous
> branches of the code, just split into separate functions.

Definitely find, create, replace. It's the order in which they are
executed, as clearly visible i2c_atr_get_mapping_by_addr(). It's also
the logical order in the old code, even though it is visually looking
reverse:

[old] i2c_atr_find_mapping_by_addr():
- list_for_each_entry() # then new _find
- i2c_atr_reserve_alias() # this is the 1st half of the new _create
- if (success)
- i2c_atr_create_c2a() # 2nd half of the new _create
- else
- list_for_each_entry_reverse... atr->ops->detach_addr...
list_move... # the new _replace

This has of course no impact on the actual executed code, it's just a
matter of code organization which I believe should be intuitive when
doable with a small effort.

Luca

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering