[PATCH v2 0/9] i2c: atr: allow usage of nested ATRs

[PATCH v2 0/9] i2c: atr: allow usage of nested ATRs

[Cosmin Tanislav]

For upcoming GMSL drivers, we need to be able to use nested ATRs.
The deserializer changes the address of the serializers, and can only
do that for the serializers, while the serializers have proper address
translation hardware, and can translate the address of its children.

To achieve this, add a static flag and a passthrough flag.
The static flag prevents usage of dynamic remapping by disallowing
creation of new mappings outside of the attach_addr() function.
The passthrough flag prevents messages coming from non-direct children
(which don't have a local mapping) to be treated as erroneous.

This series also contains various fixes to the logic observed during
development.

This series depends on:
https://lore.kernel.org/lkml/20250204-fpc202-v7-0-78b4b8a35cf1@bootlin.com

The previous version is at:
https://lore.kernel.org/all/20250203121629.2027871-1-demonsingur@gmail.com

V2:
 * rename and split up i2c_atr_find_mapping_by_addr() to allow for
   usage of parts of its logic where applicable

Cosmin Tanislav (8):
  i2c: atr: unlock mutex after c2a access
  i2c: atr: find_mapping() -> get_mapping()
  i2c: atr: split up i2c_atr_get_mapping_by_addr()
  i2c: atr: do not create mapping in detach_addr()
  i2c: atr: deduplicate logic in attach_addr()
  i2c: atr: allow replacing mappings in attach_addr()
  i2c: atr: add static flag
  i2c: atr: add passthrough flag

Tomi Valkeinen (1):
  i2c: atr: Fix lockdep for nested ATRs

 drivers/i2c/i2c-atr.c   | 187 ++++++++++++++++++++++++++--------------
 include/linux/i2c-atr.h |  22 ++++-
 2 files changed, 142 insertions(+), 67 deletions(-)

-- 
2.48.1

[PATCH v2 1/9] i2c: atr: Fix lockdep for nested ATRs

[Cosmin Tanislav]

From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>

When we have an ATR, and another ATR as a subdevice of the first ATR,
we get lockdep warnings for the i2c_atr.lock and
i2c_atr_chan.orig_addrs_lock. This is because lockdep uses a static key
for the locks, and doesn't see the locks of the separate ATR instances
as separate.

Fix this by generating a dynamic lock key per lock instance.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
---
 drivers/i2c/i2c-atr.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
index 1a6ff47b4200..39b3b95c6842 100644
--- a/drivers/i2c/i2c-atr.c
+++ b/drivers/i2c/i2c-atr.c
@@ -68,11 +68,13 @@ struct i2c_atr_alias_pool {
  * @atr:             The parent I2C ATR
  * @chan_id:         The ID of this channel
  * @alias_pairs_lock: Mutex protecting @alias_pairs
+ * @alias_pairs_lock_key: Lock key for @alias_pairs_lock
  * @alias_pairs:     List of @struct i2c_atr_alias_pair containing the
  *                   assigned aliases
  * @alias_pool:      Pool of available client aliases
  *
  * @orig_addrs_lock: Mutex protecting @orig_addrs
+ * @orig_addrs_lock_key: Lock key for @orig_addrs_lock
  * @orig_addrs:      Buffer used to store the original addresses during transmit
  * @orig_addrs_size: Size of @orig_addrs
  */
@@ -83,11 +85,13 @@ struct i2c_atr_chan {
 
 	/* Lock alias_pairs during attach/detach */
 	struct mutex alias_pairs_lock;
+	struct lock_class_key alias_pairs_lock_key;
 	struct list_head alias_pairs;
 	struct i2c_atr_alias_pool *alias_pool;
 
 	/* Lock orig_addrs during xfer */
 	struct mutex orig_addrs_lock;
+	struct lock_class_key orig_addrs_lock_key;
 	u16 *orig_addrs;
 	unsigned int orig_addrs_size;
 };
@@ -100,6 +104,7 @@ struct i2c_atr_chan {
  * @priv:      Private driver data, set with i2c_atr_set_driver_data()
  * @algo:      The &struct i2c_algorithm for adapters
  * @lock:      Lock for the I2C bus segment (see &struct i2c_lock_operations)
+ * @lock_key:  Lock key for @lock
  * @max_adapters: Maximum number of adapters this I2C ATR can have
  * @alias_pool: Optional common pool of available client aliases
  * @i2c_nb:    Notifier for remote client add & del events
@@ -115,6 +120,7 @@ struct i2c_atr {
 	struct i2c_algorithm algo;
 	/* lock for the I2C bus segment (see struct i2c_lock_operations) */
 	struct mutex lock;
+	struct lock_class_key lock_key;
 	int max_adapters;
 
 	struct i2c_atr_alias_pool *alias_pool;
@@ -679,7 +685,8 @@ struct i2c_atr *i2c_atr_new(struct i2c_adapter *parent, struct device *dev,
 	if (!atr)
 		return ERR_PTR(-ENOMEM);
 
-	mutex_init(&atr->lock);
+	lockdep_register_key(&atr->lock_key);
+	mutex_init_with_key(&atr->lock, &atr->lock_key);
 
 	atr->parent = parent;
 	atr->dev = dev;
@@ -707,6 +714,7 @@ struct i2c_atr *i2c_atr_new(struct i2c_adapter *parent, struct device *dev,
 	i2c_atr_free_alias_pool(atr->alias_pool);
 err_destroy_mutex:
 	mutex_destroy(&atr->lock);
+	lockdep_unregister_key(&atr->lock_key);
 	kfree(atr);
 
 	return ERR_PTR(ret);
@@ -723,6 +731,7 @@ void i2c_atr_delete(struct i2c_atr *atr)
 	bus_unregister_notifier(&i2c_bus_type, &atr->i2c_nb);
 	i2c_atr_free_alias_pool(atr->alias_pool);
 	mutex_destroy(&atr->lock);
+	lockdep_unregister_key(&atr->lock_key);
 	kfree(atr);
 }
 EXPORT_SYMBOL_NS_GPL(i2c_atr_delete, "I2C_ATR");
@@ -757,8 +766,10 @@ int i2c_atr_add_adapter(struct i2c_atr *atr, struct i2c_atr_adap_desc *desc)
 	chan->atr = atr;
 	chan->chan_id = chan_id;
 	INIT_LIST_HEAD(&chan->alias_pairs);
-	mutex_init(&chan->alias_pairs_lock);
-	mutex_init(&chan->orig_addrs_lock);
+	lockdep_register_key(&chan->alias_pairs_lock_key);
+	lockdep_register_key(&chan->orig_addrs_lock_key);
+	mutex_init_with_key(&chan->alias_pairs_lock, &chan->alias_pairs_lock_key);
+	mutex_init_with_key(&chan->orig_addrs_lock, &chan->orig_addrs_lock_key);
 
 	snprintf(chan->adap.name, sizeof(chan->adap.name), "i2c-%d-atr-%d",
 		 i2c_adapter_id(parent), chan_id);
@@ -835,6 +846,8 @@ int i2c_atr_add_adapter(struct i2c_atr *atr, struct i2c_atr_adap_desc *desc)
 	fwnode_handle_put(dev_fwnode(&chan->adap.dev));
 	mutex_destroy(&chan->orig_addrs_lock);
 	mutex_destroy(&chan->alias_pairs_lock);
+	lockdep_unregister_key(&chan->orig_addrs_lock_key);
+	lockdep_unregister_key(&chan->alias_pairs_lock_key);
 	kfree(chan);
 	return ret;
 }
@@ -872,6 +885,8 @@ void i2c_atr_del_adapter(struct i2c_atr *atr, u32 chan_id)
 	fwnode_handle_put(fwnode);
 	mutex_destroy(&chan->orig_addrs_lock);
 	mutex_destroy(&chan->alias_pairs_lock);
+	lockdep_unregister_key(&chan->orig_addrs_lock_key);
+	lockdep_unregister_key(&chan->alias_pairs_lock_key);
 	kfree(chan->orig_addrs);
 	kfree(chan);
 }
-- 
2.48.1

[PATCH v2 2/9] i2c: atr: unlock mutex after c2a access

[Cosmin Tanislav]

i2c_atr_release_alias(), i2c_atr_destroy_c2a(), and c2a access, are
protected everywhere with alias_pairs_lock, use it here too.

i2c_atr_destroy_c2a() accesses the elements inside alias_pairs, which
needs to be mutex protected.

Signed-off-by: Cosmin Tanislav <demonsingur@gmail.com>
---
 drivers/i2c/i2c-atr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
index 39b3b95c6842..f6033c99f474 100644
--- a/drivers/i2c/i2c-atr.c
+++ b/drivers/i2c/i2c-atr.c
@@ -544,8 +544,6 @@ static void i2c_atr_detach_addr(struct i2c_adapter *adapter,
 		return;
 	}
 
-	mutex_unlock(&chan->alias_pairs_lock);
-
 	i2c_atr_release_alias(chan->alias_pool, c2a->alias);
 
 	dev_dbg(atr->dev,
@@ -553,6 +551,8 @@ static void i2c_atr_detach_addr(struct i2c_adapter *adapter,
 		chan->chan_id, c2a->alias, addr);
 
 	i2c_atr_destroy_c2a(&c2a);
+
+	mutex_unlock(&chan->alias_pairs_lock);
 }
 
 static int i2c_atr_bus_notifier_call(struct notifier_block *nb,
-- 
2.48.1

Re: [PATCH v2 2/9] i2c: atr: unlock mutex after c2a access

[Romain Gantois]

[-- Attachment #1: Type: text/plain, Size: 755 bytes --]

Hello Cosmin,

On mardi 25 février 2025 12:39:30 heure normale d’Europe centrale Cosmin 
Tanislav wrote:
> i2c_atr_release_alias(), i2c_atr_destroy_c2a(), and c2a access, are
> protected everywhere with alias_pairs_lock, use it here too.
> 
> i2c_atr_destroy_c2a() accesses the elements inside alias_pairs, which
> needs to be mutex protected.

This looks like something that should be fixed in my FPC202 series. I'll fix it 
in v9 so that you don't have to do it in your series. FYI here's the link to 
v8 of my FPC202 series: 

https://lore.kernel.org/all/20250227-fpc202-v8-0-b7994117fbe2@bootlin.com/

I'll put you in Cc of v9.

Thanks,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 3/9] i2c: atr: find_mapping() -> get_mapping()

[Cosmin Tanislav]

A find operation implies that a null result is not an error.

Use get naming to clarify things and to prepare for splitting up the
logic inside this function.

Signed-off-by: Cosmin Tanislav <demonsingur@gmail.com>
---
 drivers/i2c/i2c-atr.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
index f6033c99f474..f2485d1670a2 100644
--- a/drivers/i2c/i2c-atr.c
+++ b/drivers/i2c/i2c-atr.c
@@ -241,7 +241,7 @@ static void i2c_atr_release_alias(struct i2c_atr_alias_pool *alias_pool, u16 ali
 
 /* Must be called with alias_pairs_lock held */
 static struct i2c_atr_alias_pair *
-i2c_atr_find_mapping_by_addr(struct i2c_atr_chan *chan, u16 addr)
+i2c_atr_get_mapping_by_addr(struct i2c_atr_chan *chan, u16 addr)
 {
 	struct i2c_atr *atr = chan->atr;
 	struct i2c_atr_alias_pair *c2a;
@@ -335,7 +335,7 @@ static int i2c_atr_map_msgs(struct i2c_atr_chan *chan, struct i2c_msg *msgs,
 	for (i = 0; i < num; i++) {
 		chan->orig_addrs[i] = msgs[i].addr;
 
-		c2a = i2c_atr_find_mapping_by_addr(chan, msgs[i].addr);
+		c2a = i2c_atr_get_mapping_by_addr(chan, msgs[i].addr);
 
 		if (!c2a) {
 			dev_err(atr->dev, "client 0x%02x not mapped!\n",
@@ -428,7 +428,7 @@ static int i2c_atr_smbus_xfer(struct i2c_adapter *adap, u16 addr,
 
 	mutex_lock(&chan->alias_pairs_lock);
 
-	c2a = i2c_atr_find_mapping_by_addr(chan, addr);
+	c2a = i2c_atr_get_mapping_by_addr(chan, addr);
 
 	if (!c2a) {
 		dev_err(atr->dev, "client 0x%02x not mapped!\n", addr);
@@ -536,7 +536,7 @@ static void i2c_atr_detach_addr(struct i2c_adapter *adapter,
 
 	mutex_lock(&chan->alias_pairs_lock);
 
-	c2a = i2c_atr_find_mapping_by_addr(chan, addr);
+	c2a = i2c_atr_get_mapping_by_addr(chan, addr);
 	if (!c2a) {
 		 /* This should never happen */
 		dev_warn(atr->dev, "Unable to find address mapping\n");
-- 
2.48.1

Re: [PATCH v2 3/9] i2c: atr: find_mapping() -> get_mapping()

[Romain Gantois]

[-- Attachment #1: Type: text/plain, Size: 2286 bytes --]

On mardi 25 février 2025 12:39:31 heure normale d’Europe centrale Cosmin 
Tanislav wrote:
> A find operation implies that a null result is not an error.
> 
> Use get naming to clarify things and to prepare for splitting up the
> logic inside this function.
> 
> Signed-off-by: Cosmin Tanislav <demonsingur@gmail.com>
> ---
>  drivers/i2c/i2c-atr.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
> index f6033c99f474..f2485d1670a2 100644
> --- a/drivers/i2c/i2c-atr.c
> +++ b/drivers/i2c/i2c-atr.c
> @@ -241,7 +241,7 @@ static void i2c_atr_release_alias(struct
> i2c_atr_alias_pool *alias_pool, u16 ali
> 
>  /* Must be called with alias_pairs_lock held */
>  static struct i2c_atr_alias_pair *
> -i2c_atr_find_mapping_by_addr(struct i2c_atr_chan *chan, u16 addr)
> +i2c_atr_get_mapping_by_addr(struct i2c_atr_chan *chan, u16 addr)
>  {
>  	struct i2c_atr *atr = chan->atr;
>  	struct i2c_atr_alias_pair *c2a;
> @@ -335,7 +335,7 @@ static int i2c_atr_map_msgs(struct i2c_atr_chan *chan,
> struct i2c_msg *msgs, for (i = 0; i < num; i++) {
>  		chan->orig_addrs[i] = msgs[i].addr;
> 
> -		c2a = i2c_atr_find_mapping_by_addr(chan, msgs[i].addr);
> +		c2a = i2c_atr_get_mapping_by_addr(chan, msgs[i].addr);
> 
>  		if (!c2a) {
>  			dev_err(atr->dev, "client 0x%02x not mapped!\n",
> @@ -428,7 +428,7 @@ static int i2c_atr_smbus_xfer(struct i2c_adapter *adap,
> u16 addr,
> 
>  	mutex_lock(&chan->alias_pairs_lock);
> 
> -	c2a = i2c_atr_find_mapping_by_addr(chan, addr);
> +	c2a = i2c_atr_get_mapping_by_addr(chan, addr);
> 
>  	if (!c2a) {
>  		dev_err(atr->dev, "client 0x%02x not mapped!\n", addr);
> @@ -536,7 +536,7 @@ static void i2c_atr_detach_addr(struct i2c_adapter
> *adapter,
> 
>  	mutex_lock(&chan->alias_pairs_lock);
> 
> -	c2a = i2c_atr_find_mapping_by_addr(chan, addr);
> +	c2a = i2c_atr_get_mapping_by_addr(chan, addr);
>  	if (!c2a) {
>  		 /* This should never happen */
>  		dev_warn(atr->dev, "Unable to find address mapping\n");

Looks good to me. This is more of a preparation/improvement patch rather than 
a fix so I think it's okay to leave it in your series.

Reviewed-by: Romain Gantois <romain.gantois@bootlin.com>


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 4/9] i2c: atr: split up i2c_atr_get_mapping_by_addr()

[Cosmin Tanislav]

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 | 108 ++++++++++++++++++++++++++++++------------
 1 file changed, 79 insertions(+), 29 deletions(-)

diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
index f2485d1670a2..9c4e9e8ec802 100644
--- a/drivers/i2c/i2c-atr.c
+++ b/drivers/i2c/i2c-atr.c
@@ -239,9 +239,26 @@ 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;
+	struct list_head *alias_pairs;
+
+	lockdep_assert_held(&chan->alias_pairs_lock);
+
+	alias_pairs = &chan->alias_pairs;
+
+	list_for_each_entry(c2a, 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;
@@ -253,38 +270,55 @@ 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)
+			break;
+
+	if (c2a->fixed)
+		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);
+		goto err_del_c2a;
 	}
 
+	return c2a;
+
+err_del_c2a:
+	i2c_atr_destroy_c2a(&c2a);
+	i2c_atr_release_alias(chan->alias_pool, alias);
+	return NULL;
+}
+
+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;
+
 	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)
-				break;
+	alias = ret;
 
-		if (c2a->fixed)
-			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) {
@@ -302,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.
-- 
2.48.1

Re: [PATCH v2 4/9] i2c: atr: split up i2c_atr_get_mapping_by_addr()

[Romain Gantois]

[-- Attachment #1: Type: text/plain, Size: 4846 bytes --]

On mardi 25 février 2025 12:39:32 heure normale d’Europe centrale Cosmin 
Tanislav 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>
> ---
>  drivers/i2c/i2c-atr.c | 108 ++++++++++++++++++++++++++++++------------
>  1 file changed, 79 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
> index f2485d1670a2..9c4e9e8ec802 100644
> --- a/drivers/i2c/i2c-atr.c
> +++ b/drivers/i2c/i2c-atr.c
> @@ -239,9 +239,26 @@ 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;
> +	struct list_head *alias_pairs;
> +
> +	lockdep_assert_held(&chan->alias_pairs_lock);
> +
> +	alias_pairs = &chan->alias_pairs;

This variable isn't really needed since it's only being used once.

> +
> +	list_for_each_entry(c2a, 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;
> @@ -253,38 +270,55 @@ 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)
> +			break;
> +
> +	if (c2a->fixed)
> +		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);
> +		goto err_del_c2a;

I think this goto should be removed, the corresponding label code is only used 
here anyway.

>  	}
> 
> +	return c2a;
> +
> +err_del_c2a:
> +	i2c_atr_destroy_c2a(&c2a);
> +	i2c_atr_release_alias(chan->alias_pool, alias);
> +	return NULL;
> +}
> +
> +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;
> +
>  	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)
> -				break;
> +	alias = ret;
> 
> -		if (c2a->fixed)
> -			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) {
> @@ -302,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.

Best Regards,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 4/9] i2c: atr: split up i2c_atr_get_mapping_by_addr()

[Romain Gantois]

[-- Attachment #1: Type: text/plain, Size: 1440 bytes --]

On mardi 25 février 2025 12:39:32 heure normale d’Europe centrale Cosmin 
Tanislav 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>
> ---
>  drivers/i2c/i2c-atr.c | 108 ++++++++++++++++++++++++++++++------------
>  1 file changed, 79 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
> index f2485d1670a2..9c4e9e8ec802 100644
> --- a/drivers/i2c/i2c-atr.c
> +++ b/drivers/i2c/i2c-atr.c
> @@ -239,9 +239,26 @@ static void i2c_atr_release_alias(struct
...
> +	i2c_atr_destroy_c2a(&c2a);
> +	i2c_atr_release_alias(chan->alias_pool, alias);
> +	return NULL;
> +}
> +
> +static struct i2c_atr_alias_pair *
> +i2c_atr_create_mapping_by_addr(struct i2c_atr_chan *chan, u16 addr)
> +{

I've missed this on my first review of this patch, but shouldn't 
i2c_atr_create_mapping_by_addr() also get a lockdep annotation, just like the 
other two mapping functions?

Thanks,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 5/9] i2c: atr: do not create mapping in detach_addr()

[Cosmin Tanislav]

It is useless to create a new mapping just to detach it immediately.

Use the newly added i2c_atr_find_mapping_by_addr() function to avoid it,
and exit without logging an error if not found.

Signed-off-by: Cosmin Tanislav <demonsingur@gmail.com>
---
 drivers/i2c/i2c-atr.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
index 9c4e9e8ec802..b62aa6ae452e 100644
--- a/drivers/i2c/i2c-atr.c
+++ b/drivers/i2c/i2c-atr.c
@@ -586,10 +586,8 @@ static void i2c_atr_detach_addr(struct i2c_adapter *adapter,
 
 	mutex_lock(&chan->alias_pairs_lock);
 
-	c2a = i2c_atr_get_mapping_by_addr(chan, addr);
+	c2a = i2c_atr_find_mapping_by_addr(chan, addr);
 	if (!c2a) {
-		 /* This should never happen */
-		dev_warn(atr->dev, "Unable to find address mapping\n");
 		mutex_unlock(&chan->alias_pairs_lock);
 		return;
 	}
-- 
2.48.1

Re: [PATCH v2 5/9] i2c: atr: do not create mapping in detach_addr()

[Romain Gantois]

[-- Attachment #1: Type: text/plain, Size: 1110 bytes --]

On mardi 25 février 2025 12:39:33 heure normale d’Europe centrale Cosmin 
Tanislav wrote:
> It is useless to create a new mapping just to detach it immediately.
> 
> Use the newly added i2c_atr_find_mapping_by_addr() function to avoid it,
> and exit without logging an error if not found.
> 
> Signed-off-by: Cosmin Tanislav <demonsingur@gmail.com>
> ---
>  drivers/i2c/i2c-atr.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
> index 9c4e9e8ec802..b62aa6ae452e 100644
> --- a/drivers/i2c/i2c-atr.c
> +++ b/drivers/i2c/i2c-atr.c
> @@ -586,10 +586,8 @@ static void i2c_atr_detach_addr(struct i2c_adapter
> *adapter,
> 
>  	mutex_lock(&chan->alias_pairs_lock);
> 
> -	c2a = i2c_atr_get_mapping_by_addr(chan, addr);
> +	c2a = i2c_atr_find_mapping_by_addr(chan, addr);
>  	if (!c2a) {
> -		 /* This should never happen */
> -		dev_warn(atr->dev, "Unable to find address mapping\n");
>  		mutex_unlock(&chan->alias_pairs_lock);
>  		return;
>  	}

Reviewed-by: Romain Gantois <romain.gantois@bootlin.com>

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 6/9] i2c: atr: deduplicate logic in attach_addr()

[Cosmin Tanislav]

This is the same logic as in i2c_atr_create_mapping_by_addr().

Signed-off-by: Cosmin Tanislav <demonsingur@gmail.com>
---
 drivers/i2c/i2c-atr.c | 30 ++++++------------------------
 1 file changed, 6 insertions(+), 24 deletions(-)

diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
index b62aa6ae452e..5b53eaee0408 100644
--- a/drivers/i2c/i2c-atr.c
+++ b/drivers/i2c/i2c-atr.c
@@ -538,38 +538,20 @@ static int i2c_atr_attach_addr(struct i2c_adapter *adapter,
 	struct i2c_atr_chan *chan = adapter->algo_data;
 	struct i2c_atr *atr = chan->atr;
 	struct i2c_atr_alias_pair *c2a;
-	u16 alias;
-	int ret;
-
-	ret = i2c_atr_reserve_alias(chan->alias_pool);
-	if (ret < 0) {
-		dev_err(atr->dev, "failed to find a free alias\n");
-		return ret;
-	}
-
-	alias = ret;
+	int ret = 0;
 
 	mutex_lock(&chan->alias_pairs_lock);
 
-	c2a = i2c_atr_create_c2a(chan, alias, addr);
+	c2a = i2c_atr_create_mapping_by_addr(chan, addr);
 	if (!c2a) {
-		ret = -ENOMEM;
-		goto err_release_alias;
+		dev_err(atr->dev, "failed to find a free alias\n");
+		ret = -EBUSY;
+		goto out_unlock;
 	}
 
-	ret = atr->ops->attach_addr(atr, chan->chan_id, addr, alias);
-	if (ret)
-		goto err_del_c2a;
-
 	dev_dbg(atr->dev, "chan%u: using alias 0x%02x for addr 0x%02x\n",
-		chan->chan_id, alias, addr);
+		chan->chan_id, c2a->alias, addr);
 
-	goto out_unlock;
-
-err_del_c2a:
-	i2c_atr_destroy_c2a(&c2a);
-err_release_alias:
-	i2c_atr_release_alias(chan->alias_pool, alias);
 out_unlock:
 	mutex_unlock(&chan->alias_pairs_lock);
 	return ret;
-- 
2.48.1

Re: [PATCH v2 6/9] i2c: atr: deduplicate logic in attach_addr()

[Romain Gantois]

[-- Attachment #1: Type: text/plain, Size: 1930 bytes --]

On mardi 25 février 2025 12:39:34 heure normale d’Europe centrale Cosmin 
Tanislav wrote:
> This is the same logic as in i2c_atr_create_mapping_by_addr().
> 
> Signed-off-by: Cosmin Tanislav <demonsingur@gmail.com>
> ---
>  drivers/i2c/i2c-atr.c | 30 ++++++------------------------
>  1 file changed, 6 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
> index b62aa6ae452e..5b53eaee0408 100644
> --- a/drivers/i2c/i2c-atr.c
> +++ b/drivers/i2c/i2c-atr.c
> @@ -538,38 +538,20 @@ static int i2c_atr_attach_addr(struct i2c_adapter
> *adapter, struct i2c_atr_chan *chan = adapter->algo_data;
>  	struct i2c_atr *atr = chan->atr;
>  	struct i2c_atr_alias_pair *c2a;
> -	u16 alias;
> -	int ret;
> -
> -	ret = i2c_atr_reserve_alias(chan->alias_pool);
> -	if (ret < 0) {
> -		dev_err(atr->dev, "failed to find a free alias\n");
> -		return ret;
> -	}
> -
> -	alias = ret;
> +	int ret = 0;
> 
>  	mutex_lock(&chan->alias_pairs_lock);

A mutex guard could be used here.

> 
> -	c2a = i2c_atr_create_c2a(chan, alias, addr);
> +	c2a = i2c_atr_create_mapping_by_addr(chan, addr);
>  	if (!c2a) {
> -		ret = -ENOMEM;
> -		goto err_release_alias;
> +		dev_err(atr->dev, "failed to find a free alias\n");
> +		ret = -EBUSY;
> +		goto out_unlock;
>  	}
> 
> -	ret = atr->ops->attach_addr(atr, chan->chan_id, addr, alias);
> -	if (ret)
> -		goto err_del_c2a;
> -
>  	dev_dbg(atr->dev, "chan%u: using alias 0x%02x for addr 0x%02x\n",
> -		chan->chan_id, alias, addr);
> +		chan->chan_id, c2a->alias, addr);
> 
> -	goto out_unlock;
> -
> -err_del_c2a:
> -	i2c_atr_destroy_c2a(&c2a);
> -err_release_alias:
> -	i2c_atr_release_alias(chan->alias_pool, alias);
>  out_unlock:
>  	mutex_unlock(&chan->alias_pairs_lock);
>  	return ret;

Best Regards,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 6/9] i2c: atr: deduplicate logic in attach_addr()

[Cosmin Tanislav]

On 2/27/25 3:36 PM, Romain Gantois wrote:
> On mardi 25 février 2025 12:39:34 heure normale d’Europe centrale Cosmin
> Tanislav wrote:
>> This is the same logic as in i2c_atr_create_mapping_by_addr().
>>
>> Signed-off-by: Cosmin Tanislav <demonsingur@gmail.com>
>> ---
>>   drivers/i2c/i2c-atr.c | 30 ++++++------------------------
>>   1 file changed, 6 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
>> index b62aa6ae452e..5b53eaee0408 100644
>> --- a/drivers/i2c/i2c-atr.c
>> +++ b/drivers/i2c/i2c-atr.c
>> @@ -538,38 +538,20 @@ static int i2c_atr_attach_addr(struct i2c_adapter
>> *adapter, struct i2c_atr_chan *chan = adapter->algo_data;
>>   	struct i2c_atr *atr = chan->atr;
>>   	struct i2c_atr_alias_pair *c2a;
>> -	u16 alias;
>> -	int ret;
>> -
>> -	ret = i2c_atr_reserve_alias(chan->alias_pool);
>> -	if (ret < 0) {
>> -		dev_err(atr->dev, "failed to find a free alias\n");
>> -		return ret;
>> -	}
>> -
>> -	alias = ret;
>> +	int ret = 0;
>>
>>   	mutex_lock(&chan->alias_pairs_lock);
> 
> A mutex guard could be used here.
> 

Should we be using it in other places in the driver too then?

We could leave that conversion for a separate commit.

>>
>> -	c2a = i2c_atr_create_c2a(chan, alias, addr);
>> +	c2a = i2c_atr_create_mapping_by_addr(chan, addr);
>>   	if (!c2a) {
>> -		ret = -ENOMEM;
>> -		goto err_release_alias;
>> +		dev_err(atr->dev, "failed to find a free alias\n");
>> +		ret = -EBUSY;
>> +		goto out_unlock;
>>   	}
>>
>> -	ret = atr->ops->attach_addr(atr, chan->chan_id, addr, alias);
>> -	if (ret)
>> -		goto err_del_c2a;
>> -
>>   	dev_dbg(atr->dev, "chan%u: using alias 0x%02x for addr 0x%02x\n",
>> -		chan->chan_id, alias, addr);
>> +		chan->chan_id, c2a->alias, addr);
>>
>> -	goto out_unlock;
>> -
>> -err_del_c2a:
>> -	i2c_atr_destroy_c2a(&c2a);
>> -err_release_alias:
>> -	i2c_atr_release_alias(chan->alias_pool, alias);
>>   out_unlock:
>>   	mutex_unlock(&chan->alias_pairs_lock);
>>   	return ret;
> 
> Best Regards,
> 

[PATCH v2 7/9] i2c: atr: allow replacing mappings in attach_addr()

[Cosmin Tanislav]

It is possible for aliases to be exhausted while we are still attaching
children.

Allow replacing mapping on attach by calling
i2c_atr_replace_mapping_by_addr() if i2c_atr_create_mapping_by_addr()
fails.

Signed-off-by: Cosmin Tanislav <demonsingur@gmail.com>
---
 drivers/i2c/i2c-atr.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
index 5b53eaee0408..d8748d71ae15 100644
--- a/drivers/i2c/i2c-atr.c
+++ b/drivers/i2c/i2c-atr.c
@@ -543,6 +543,9 @@ static int i2c_atr_attach_addr(struct i2c_adapter *adapter,
 	mutex_lock(&chan->alias_pairs_lock);
 
 	c2a = i2c_atr_create_mapping_by_addr(chan, addr);
+	if (!c2a)
+		c2a = i2c_atr_replace_mapping_by_addr(chan, addr);
+
 	if (!c2a) {
 		dev_err(atr->dev, "failed to find a free alias\n");
 		ret = -EBUSY;
-- 
2.48.1

[PATCH v2 8/9] i2c: atr: add static flag

[Cosmin Tanislav]

Some I2C ATRs do not support dynamic remapping, only static mapping
of direct children.

Add a new flag that prevents old mappings to be replaced or new mappings
to be created in the alias finding code paths.

Signed-off-by: Cosmin Tanislav <demonsingur@gmail.com>
---
 drivers/i2c/i2c-atr.c   | 16 ++++++++++++----
 include/linux/i2c-atr.h | 20 +++++++++++++++++---
 2 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
index d8748d71ae15..f7b853f55630 100644
--- a/drivers/i2c/i2c-atr.c
+++ b/drivers/i2c/i2c-atr.c
@@ -106,6 +106,7 @@ struct i2c_atr_chan {
  * @lock:      Lock for the I2C bus segment (see &struct i2c_lock_operations)
  * @lock_key:  Lock key for @lock
  * @max_adapters: Maximum number of adapters this I2C ATR can have
+ * @flags:     Flags for ATR
  * @alias_pool: Optional common pool of available client aliases
  * @i2c_nb:    Notifier for remote client add & del events
  * @adapter:   Array of adapters
@@ -122,6 +123,7 @@ struct i2c_atr {
 	struct mutex lock;
 	struct lock_class_key lock_key;
 	int max_adapters;
+	u32 flags;
 
 	struct i2c_atr_alias_pool *alias_pool;
 
@@ -339,12 +341,16 @@ i2c_atr_create_mapping_by_addr(struct i2c_atr_chan *chan, u16 addr)
 static struct i2c_atr_alias_pair *
 i2c_atr_get_mapping_by_addr(struct i2c_atr_chan *chan, u16 addr)
 {
+	struct i2c_atr *atr = chan->atr;
 	struct i2c_atr_alias_pair *c2a;
 
 	c2a = i2c_atr_find_mapping_by_addr(chan, addr);
 	if (c2a)
 		return c2a;
 
+	if (atr->flags & I2C_ATR_STATIC)
+		return NULL;
+
 	c2a = i2c_atr_create_mapping_by_addr(chan, addr);
 	if (c2a)
 		return c2a;
@@ -543,7 +549,7 @@ static int i2c_atr_attach_addr(struct i2c_adapter *adapter,
 	mutex_lock(&chan->alias_pairs_lock);
 
 	c2a = i2c_atr_create_mapping_by_addr(chan, addr);
-	if (!c2a)
+	if (!c2a && !(atr->flags & I2C_ATR_STATIC))
 		c2a = i2c_atr_replace_mapping_by_addr(chan, addr);
 
 	if (!c2a) {
@@ -702,8 +708,9 @@ static int i2c_atr_parse_alias_pool(struct i2c_atr *atr)
 	return ret;
 }
 
-struct i2c_atr *i2c_atr_new(struct i2c_adapter *parent, struct device *dev,
-			    const struct i2c_atr_ops *ops, int max_adapters)
+struct i2c_atr *i2c_atr_new_flags(struct i2c_adapter *parent, struct device *dev,
+				  const struct i2c_atr_ops *ops, int max_adapters,
+				  u32 flags)
 {
 	struct i2c_atr *atr;
 	int ret;
@@ -725,6 +732,7 @@ struct i2c_atr *i2c_atr_new(struct i2c_adapter *parent, struct device *dev,
 	atr->dev = dev;
 	atr->ops = ops;
 	atr->max_adapters = max_adapters;
+	atr->flags = flags;
 
 	if (parent->algo->master_xfer)
 		atr->algo.master_xfer = i2c_atr_master_xfer;
@@ -752,7 +760,7 @@ struct i2c_atr *i2c_atr_new(struct i2c_adapter *parent, struct device *dev,
 
 	return ERR_PTR(ret);
 }
-EXPORT_SYMBOL_NS_GPL(i2c_atr_new, "I2C_ATR");
+EXPORT_SYMBOL_NS_GPL(i2c_atr_new_flags, "I2C_ATR");
 
 void i2c_atr_delete(struct i2c_atr *atr)
 {
diff --git a/include/linux/i2c-atr.h b/include/linux/i2c-atr.h
index 1c3a5bcd939f..2f79d0d9140f 100644
--- a/include/linux/i2c-atr.h
+++ b/include/linux/i2c-atr.h
@@ -18,6 +18,15 @@ struct device;
 struct fwnode_handle;
 struct i2c_atr;
 
+/**
+ * enum i2c_atr_flags - Flags for an I2C ATR driver
+ *
+ * @I2C_ATR_STATIC: ATR does not support dynamic mapping, use static mapping
+ */
+enum i2c_atr_flags {
+	I2C_ATR_STATIC = BIT(0),
+};
+
 /**
  * struct i2c_atr_ops - Callbacks from ATR to the device driver.
  * @attach_addr: Notify the driver of a new device connected on a child
@@ -60,11 +69,12 @@ struct i2c_atr_adap_desc {
 };
 
 /**
- * i2c_atr_new() - Allocate and initialize an I2C ATR helper.
+ * i2c_atr_new_flags() - Allocate and initialize an I2C ATR helper.
  * @parent:       The parent (upstream) adapter
  * @dev:          The device acting as an ATR
  * @ops:          Driver-specific callbacks
  * @max_adapters: Maximum number of child adapters
+ * @flags:        Flags for ATR
  *
  * The new ATR helper is connected to the parent adapter but has no child
  * adapters. Call i2c_atr_add_adapter() to add some.
@@ -73,8 +83,12 @@ struct i2c_atr_adap_desc {
  *
  * Return: pointer to the new ATR helper object, or ERR_PTR
  */
-struct i2c_atr *i2c_atr_new(struct i2c_adapter *parent, struct device *dev,
-			    const struct i2c_atr_ops *ops, int max_adapters);
+struct i2c_atr *i2c_atr_new_flags(struct i2c_adapter *parent, struct device *dev,
+				  const struct i2c_atr_ops *ops, int max_adapters,
+				  u32 flags);
+
+#define i2c_atr_new(parent, dev, ops, max_adapters) \
+	i2c_atr_new_flags(parent, dev, ops, max_adapters, 0)
 
 /**
  * i2c_atr_delete - Delete an I2C ATR helper.
-- 
2.48.1

Re: [PATCH v2 8/9] i2c: atr: add static flag

[Romain Gantois]

[-- Attachment #1: Type: text/plain, Size: 5479 bytes --]

On mardi 25 février 2025 12:39:36 heure normale d’Europe centrale Cosmin 
Tanislav wrote:
> Some I2C ATRs do not support dynamic remapping, only static mapping
> of direct children.
> 
> Add a new flag that prevents old mappings to be replaced or new mappings
> to be created in the alias finding code paths.
> 
> Signed-off-by: Cosmin Tanislav <demonsingur@gmail.com>
> ---
>  drivers/i2c/i2c-atr.c   | 16 ++++++++++++----
>  include/linux/i2c-atr.h | 20 +++++++++++++++++---
>  2 files changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
> index d8748d71ae15..f7b853f55630 100644
> --- a/drivers/i2c/i2c-atr.c
> +++ b/drivers/i2c/i2c-atr.c
> @@ -106,6 +106,7 @@ struct i2c_atr_chan {
>   * @lock:      Lock for the I2C bus segment (see &struct
> i2c_lock_operations) * @lock_key:  Lock key for @lock
>   * @max_adapters: Maximum number of adapters this I2C ATR can have
> + * @flags:     Flags for ATR
>   * @alias_pool: Optional common pool of available client aliases
>   * @i2c_nb:    Notifier for remote client add & del events
>   * @adapter:   Array of adapters
> @@ -122,6 +123,7 @@ struct i2c_atr {
>  	struct mutex lock;
>  	struct lock_class_key lock_key;
>  	int max_adapters;
> +	u32 flags;
> 
>  	struct i2c_atr_alias_pool *alias_pool;
> 
> @@ -339,12 +341,16 @@ i2c_atr_create_mapping_by_addr(struct i2c_atr_chan
> *chan, u16 addr) static struct i2c_atr_alias_pair *
>  i2c_atr_get_mapping_by_addr(struct i2c_atr_chan *chan, u16 addr)
>  {
> +	struct i2c_atr *atr = chan->atr;
>  	struct i2c_atr_alias_pair *c2a;
> 
>  	c2a = i2c_atr_find_mapping_by_addr(chan, addr);
>  	if (c2a)
>  		return c2a;
> 
> +	if (atr->flags & I2C_ATR_STATIC)
> +		return NULL;
> +
>  	c2a = i2c_atr_create_mapping_by_addr(chan, addr);
>  	if (c2a)
>  		return c2a;
> @@ -543,7 +549,7 @@ static int i2c_atr_attach_addr(struct i2c_adapter
> *adapter, mutex_lock(&chan->alias_pairs_lock);
> 
>  	c2a = i2c_atr_create_mapping_by_addr(chan, addr);
> -	if (!c2a)
> +	if (!c2a && !(atr->flags & I2C_ATR_STATIC))
>  		c2a = i2c_atr_replace_mapping_by_addr(chan, addr);
> 
>  	if (!c2a) {
> @@ -702,8 +708,9 @@ static int i2c_atr_parse_alias_pool(struct i2c_atr *atr)
> return ret;
>  }
> 
> -struct i2c_atr *i2c_atr_new(struct i2c_adapter *parent, struct device *dev,
> -			    const struct i2c_atr_ops *ops, int max_adapters)
> +struct i2c_atr *i2c_atr_new_flags(struct i2c_adapter *parent, struct device
> *dev, +				  const struct i2c_atr_ops *ops, int 
max_adapters,
> +				  u32 flags)
>  {
>  	struct i2c_atr *atr;
>  	int ret;
> @@ -725,6 +732,7 @@ struct i2c_atr *i2c_atr_new(struct i2c_adapter *parent,
> struct device *dev, atr->dev = dev;
>  	atr->ops = ops;
>  	atr->max_adapters = max_adapters;
> +	atr->flags = flags;
> 
>  	if (parent->algo->master_xfer)
>  		atr->algo.master_xfer = i2c_atr_master_xfer;
> @@ -752,7 +760,7 @@ struct i2c_atr *i2c_atr_new(struct i2c_adapter *parent,
> struct device *dev,
> 
>  	return ERR_PTR(ret);
>  }
> -EXPORT_SYMBOL_NS_GPL(i2c_atr_new, "I2C_ATR");
> +EXPORT_SYMBOL_NS_GPL(i2c_atr_new_flags, "I2C_ATR");
> 
>  void i2c_atr_delete(struct i2c_atr *atr)
>  {
> diff --git a/include/linux/i2c-atr.h b/include/linux/i2c-atr.h
> index 1c3a5bcd939f..2f79d0d9140f 100644
> --- a/include/linux/i2c-atr.h
> +++ b/include/linux/i2c-atr.h
> @@ -18,6 +18,15 @@ struct device;
>  struct fwnode_handle;
>  struct i2c_atr;
> 
> +/**
> + * enum i2c_atr_flags - Flags for an I2C ATR driver
> + *
> + * @I2C_ATR_STATIC: ATR does not support dynamic mapping, use static
> mapping + */
> +enum i2c_atr_flags {
> +	I2C_ATR_STATIC = BIT(0),

I'd personally prefer the name I2C_ATR_F_STATIC, which would make it explicit 
that this is a flag.

> +};
> +
>  /**
>   * struct i2c_atr_ops - Callbacks from ATR to the device driver.
>   * @attach_addr: Notify the driver of a new device connected on a child
> @@ -60,11 +69,12 @@ struct i2c_atr_adap_desc {
>  };
> 
>  /**
> - * i2c_atr_new() - Allocate and initialize an I2C ATR helper.
> + * i2c_atr_new_flags() - Allocate and initialize an I2C ATR helper.
>   * @parent:       The parent (upstream) adapter
>   * @dev:          The device acting as an ATR
>   * @ops:          Driver-specific callbacks
>   * @max_adapters: Maximum number of child adapters
> + * @flags:        Flags for ATR
>   *
>   * The new ATR helper is connected to the parent adapter but has no child
>   * adapters. Call i2c_atr_add_adapter() to add some.
> @@ -73,8 +83,12 @@ struct i2c_atr_adap_desc {
>   *
>   * Return: pointer to the new ATR helper object, or ERR_PTR
>   */
> -struct i2c_atr *i2c_atr_new(struct i2c_adapter *parent, struct device *dev,
> -			    const struct i2c_atr_ops *ops, int max_adapters);
> +struct i2c_atr *i2c_atr_new_flags(struct i2c_adapter *parent, struct device
> *dev, +				  const struct i2c_atr_ops *ops, int 
max_adapters,
> +				  u32 flags);
> +
> +#define i2c_atr_new(parent, dev, ops, max_adapters) \
> +	i2c_atr_new_flags(parent, dev, ops, max_adapters, 0)

There aren't that many users of i2c_atr_new() yet, so I think it would be 
preferable to just add the "0" parameter to existing callers (including 
FPC202 which this series depends on) rather than adding a layer of 
indirection.

Thanks,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 9/9] i2c: atr: add passthrough flag

[Cosmin Tanislav]

Some I2C ATRs can have other I2C ATRs as children. The I2C messages of
the child ATRs need to be forwarded as-is if the parent I2C ATR can
only do static mapping.

In the case of GMSL, the deserializer I2C ATR actually doesn't have I2C
address remapping hardware capabilities, but it is able to select which
GMSL link to talk to, allowing it to change the address of the
serializer.

The child ATRs need to have their alias pools defined in such a way to
prevent overlapping addresses between them, but there's no way around
this without orchestration between multiple ATR instances.

To allow for this use-case, add a flag that allows unmapped addresses
to be passed through, since they are already remapped by the child ATRs.

There's no case where an address that has not been remapped by the child
ATR will hit the parent ATR.

Signed-off-by: Cosmin Tanislav <demonsingur@gmail.com>
---
 drivers/i2c/i2c-atr.c   | 3 +++
 include/linux/i2c-atr.h | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
index f7b853f55630..1986fa055f20 100644
--- a/drivers/i2c/i2c-atr.c
+++ b/drivers/i2c/i2c-atr.c
@@ -394,6 +394,9 @@ static int i2c_atr_map_msgs(struct i2c_atr_chan *chan, struct i2c_msg *msgs,
 		c2a = i2c_atr_get_mapping_by_addr(chan, msgs[i].addr);
 
 		if (!c2a) {
+			if (atr->flags & I2C_ATR_PASSTHROUGH)
+				continue;
+
 			dev_err(atr->dev, "client 0x%02x not mapped!\n",
 				msgs[i].addr);
 
diff --git a/include/linux/i2c-atr.h b/include/linux/i2c-atr.h
index 2f79d0d9140f..b3797a930a7a 100644
--- a/include/linux/i2c-atr.h
+++ b/include/linux/i2c-atr.h
@@ -22,9 +22,11 @@ struct i2c_atr;
  * enum i2c_atr_flags - Flags for an I2C ATR driver
  *
  * @I2C_ATR_STATIC: ATR does not support dynamic mapping, use static mapping
+ * @I2C_ATR_PASSTHROUGH: Allow unmapped incoming addresses to pass through
  */
 enum i2c_atr_flags {
 	I2C_ATR_STATIC = BIT(0),
+	I2C_ATR_PASSTHROUGH = BIT(1),
 };
 
 /**
-- 
2.48.1

Re: [PATCH v2 9/9] i2c: atr: add passthrough flag

[Romain Gantois]

[-- Attachment #1: Type: text/plain, Size: 2447 bytes --]

On mardi 25 février 2025 12:39:37 heure normale d’Europe centrale Cosmin 
Tanislav wrote:
> Some I2C ATRs can have other I2C ATRs as children. The I2C messages of
> the child ATRs need to be forwarded as-is if the parent I2C ATR can
> only do static mapping.
> 
> In the case of GMSL, the deserializer I2C ATR actually doesn't have I2C
> address remapping hardware capabilities, but it is able to select which
> GMSL link to talk to, allowing it to change the address of the
> serializer.
> 
> The child ATRs need to have their alias pools defined in such a way to
> prevent overlapping addresses between them, but there's no way around
> this without orchestration between multiple ATR instances.
> 
> To allow for this use-case, add a flag that allows unmapped addresses
> to be passed through, since they are already remapped by the child ATRs.
> 
> There's no case where an address that has not been remapped by the child
> ATR will hit the parent ATR.
> 
> Signed-off-by: Cosmin Tanislav <demonsingur@gmail.com>
> ---
>  drivers/i2c/i2c-atr.c   | 3 +++
>  include/linux/i2c-atr.h | 2 ++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
> index f7b853f55630..1986fa055f20 100644
> --- a/drivers/i2c/i2c-atr.c
> +++ b/drivers/i2c/i2c-atr.c
> @@ -394,6 +394,9 @@ static int i2c_atr_map_msgs(struct i2c_atr_chan *chan,
> struct i2c_msg *msgs, c2a = i2c_atr_get_mapping_by_addr(chan,
> msgs[i].addr);
> 
>  		if (!c2a) {
> +			if (atr->flags & I2C_ATR_PASSTHROUGH)
> +				continue;

Shouldn't this check also be added to i2c_atr_smbus_xfer?

> +
>  			dev_err(atr->dev, "client 0x%02x not mapped!\n",
>  				msgs[i].addr);
> 
> diff --git a/include/linux/i2c-atr.h b/include/linux/i2c-atr.h
> index 2f79d0d9140f..b3797a930a7a 100644
> --- a/include/linux/i2c-atr.h
> +++ b/include/linux/i2c-atr.h
> @@ -22,9 +22,11 @@ struct i2c_atr;
>   * enum i2c_atr_flags - Flags for an I2C ATR driver
>   *
>   * @I2C_ATR_STATIC: ATR does not support dynamic mapping, use static
> mapping + * @I2C_ATR_PASSTHROUGH: Allow unmapped incoming addresses to pass
> through */
>  enum i2c_atr_flags {
>  	I2C_ATR_STATIC = BIT(0),
> +	I2C_ATR_PASSTHROUGH = BIT(1),

As stated for the previous patch, I'd prefer the "I2C_ATR_F_*" naming 
convention.

Thanks,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]