| Message ID | 20250428102516.933571-4-demonsingur@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: atr: allow usage of nested ATRs | expand |
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. As a nice side effect, this might make the diff more readable. Luca
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(-) 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. > As a nice side effect, this might make the diff more readable. > > Luca >
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
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) +{ + struct i2c_atr *atr = chan->atr; + struct i2c_atr_alias_pair *c2a; + u16 alias; + int ret; + + lockdep_assert_held(&chan->alias_pairs_lock); + ret = i2c_atr_reserve_alias(chan->alias_pool); - if (ret < 0) { - // If no free aliases are left, replace an existing one - if (unlikely(list_empty(alias_pairs))) - return NULL; + if (ret < 0) + return NULL; - list_for_each_entry_reverse(c2a, alias_pairs, node) { - if (!c2a->fixed) { - found = true; - break; - } - } + alias = ret; - if (!found) - return NULL; - - atr->ops->detach_addr(atr, chan->chan_id, c2a->addr); - c2a->addr = addr; - - // Move updated entry to beginning of list - list_move(&c2a->node, alias_pairs); - - alias = c2a->alias; - } else { - alias = ret; - - c2a = i2c_atr_create_c2a(chan, alias, addr); - if (!c2a) - goto err_release_alias; - } + c2a = i2c_atr_create_c2a(chan, alias, addr); + if (!c2a) + goto err_release_alias; ret = atr->ops->attach_addr(atr, chan->chan_id, c2a->addr, c2a->alias); if (ret) { @@ -306,6 +336,22 @@ i2c_atr_get_mapping_by_addr(struct i2c_atr_chan *chan, u16 addr) return NULL; } +static struct i2c_atr_alias_pair * +i2c_atr_get_mapping_by_addr(struct i2c_atr_chan *chan, u16 addr) +{ + struct i2c_atr_alias_pair *c2a; + + c2a = i2c_atr_find_mapping_by_addr(chan, addr); + if (c2a) + return c2a; + + c2a = i2c_atr_create_mapping_by_addr(chan, addr); + if (c2a) + return c2a; + + return i2c_atr_replace_mapping_by_addr(chan, addr); +} + /* * Replace all message addresses with their aliases, saving the original * addresses.
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> --- drivers/i2c/i2c-atr.c | 110 ++++++++++++++++++++++++++++++------------ 1 file changed, 78 insertions(+), 32 deletions(-)