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

[Luca Ceresoli]

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

> On 4/30/25 5:33 PM, Luca Ceresoli wrote:
> > On Mon, 28 Apr 2025 13:25:08 +0300
> > Cosmin Tanislav <demonsingur@xxxxxxxxx> 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@xxxxxxxxx>  
> > 
> > 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
https://bootlin.com