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

[PATCH 0/3] 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.

This series depends on:
https://lore.kernel.org/lkml/20250115-fpc202-v6-0-d47a34820753@bootlin.com

Cosmin Tanislav (2):
  i2c: atr: deduplicate logic in attach_addr()
  i2c: atr: add passthrough flag

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

 drivers/i2c/i2c-atr.c   | 78 ++++++++++++++++++++++-------------------
 include/linux/i2c-atr.h | 20 +++++++++--
 2 files changed, 59 insertions(+), 39 deletions(-)

-- 
2.48.1

[PATCH 1/3] 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 1a6ff47b42002..39b3b95c6842a 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 2/3] i2c: atr: deduplicate logic in attach_addr()

[Cosmin Tanislav]

This is mainly the same logic as in i2c_atr_find_mapping_by_addr(),
except for the missing dynamic detach / attach.

There's actually no reason for that logic to not be run, since it is
possible for a single channel to exceed alias pooling capabilities and
to need to remove some aliases before even finishing adding the
channels.

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

diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
index 39b3b95c6842a..13f7e07fd8e87 100644
--- a/drivers/i2c/i2c-atr.c
+++ b/drivers/i2c/i2c-atr.c
@@ -488,41 +488,22 @@ 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;
 
 	mutex_lock(&chan->alias_pairs_lock);
 
-	c2a = i2c_atr_create_c2a(chan, alias, addr);
+	c2a = i2c_atr_find_mapping_by_addr(chan, addr);
 	if (!c2a) {
-		ret = -ENOMEM;
-		goto err_release_alias;
+		dev_err(atr->dev, "failed to find a free alias\n");
+		mutex_unlock(&chan->alias_pairs_lock);
+		return -EBUSY;
 	}
 
-	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;
+
+	return 0;
 }
 
 static void i2c_atr_detach_addr(struct i2c_adapter *adapter,
-- 
2.48.1

[PATCH 3/3] 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 since the parent I2C ATR can
only do address remapping for the direct children.

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,
and disables dynamic remapping, since devices that need passthrough
messages to be forwarded as-is, can only handle remapping for their
direct children.

There's no case where a non-remapped address will hit the parent ATR.

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

diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
index 13f7e07fd8e87..5f0e8f1cf69f7 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;
 
@@ -241,7 +243,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_find_mapping_by_addr(struct i2c_atr_chan *chan, u16 addr, bool new_addr)
 {
 	struct i2c_atr *atr = chan->atr;
 	struct i2c_atr_alias_pair *c2a;
@@ -260,6 +262,9 @@ i2c_atr_find_mapping_by_addr(struct i2c_atr_chan *chan, u16 addr)
 
 	ret = i2c_atr_reserve_alias(chan->alias_pool);
 	if (ret < 0) {
+		if (!new_addr && (atr->flags & I2C_ATR_PASSTHROUGH))
+			return NULL;
+
 		// If no free aliases are left, replace an existing one
 		if (unlikely(list_empty(alias_pairs)))
 			return NULL;
@@ -335,9 +340,12 @@ 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_find_mapping_by_addr(chan, msgs[i].addr, false);
 
 		if (!c2a) {
+			if (atr->flags & I2C_ATR_PASSTHROUGH)
+				continue;
+
 			dev_err(atr->dev, "client 0x%02x not mapped!\n",
 				msgs[i].addr);
 
@@ -428,7 +436,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_find_mapping_by_addr(chan, addr, false);
 
 	if (!c2a) {
 		dev_err(atr->dev, "client 0x%02x not mapped!\n", addr);
@@ -491,7 +499,7 @@ static int i2c_atr_attach_addr(struct i2c_adapter *adapter,
 
 	mutex_lock(&chan->alias_pairs_lock);
 
-	c2a = i2c_atr_find_mapping_by_addr(chan, addr);
+	c2a = i2c_atr_find_mapping_by_addr(chan, addr, true);
 	if (!c2a) {
 		dev_err(atr->dev, "failed to find a free alias\n");
 		mutex_unlock(&chan->alias_pairs_lock);
@@ -517,7 +525,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_find_mapping_by_addr(chan, addr, false);
 	if (!c2a) {
 		 /* This should never happen */
 		dev_warn(atr->dev, "Unable to find address mapping\n");
@@ -650,8 +658,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;
@@ -673,6 +682,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;
@@ -700,7 +710,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 1c3a5bcd939fc..116067b5b9ba6 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_PASSTHROUGH: Allow unmapped incoming addresses to pass through
+ */
+enum i2c_atr_flags {
+	I2C_ATR_PASSTHROUGH = 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 3/3] i2c: atr: add passthrough flag

[Romain Gantois]

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

Hello Cosmin,

On lundi 3 février 2025 13:15:17 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 since the parent I2C ATR can
> only do address remapping for the direct children.
> 
> 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,
> and disables dynamic remapping, since devices that need passthrough
> messages to be forwarded as-is, can only handle remapping for their
> direct children.
> 
> There's no case where a non-remapped address will hit the parent ATR.

I'm having trouble understanding this, because it seems like there's a 
contradiction with your previous statement:

> add a flag that allows unmapped addresses to be passed through

Unmapped addresses are "non-remapped" by definition right? And they can hit the 
parent ATR since we're adding a flag to allow them to pass through...

> 
> Signed-off-by: Cosmin Tanislav <demonsingur@gmail.com>
> ---
>  drivers/i2c/i2c-atr.c   | 26 ++++++++++++++++++--------
>  include/linux/i2c-atr.h | 20 +++++++++++++++++---
>  2 files changed, 35 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
> index 13f7e07fd8e87..5f0e8f1cf69f7 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;
> 
> @@ -241,7 +243,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_find_mapping_by_addr(struct i2c_atr_chan *chan, u16 addr, bool
> new_addr) {

IMO the "new_addr" naming is quite confusing.

After this patch is applied, the expected behavior is:

i2c_atr_find_mapping_by_addr() called from i2c_atr_attach_addr():
  1. find existing mapping, return it
  2. OR find free alias, create mapping and return it
  3. OR remap used alias, return mapping
  4. OR fail

i2c_atr_find_mapping_by_addr(), called from anywhere else:
   1. find existing mapping, return it
   2. OR find free alias, create mapping and return it
   3. OR if the ATR has PASSTHROUGH set, fail
   4. OR remap used alias, return mapping
   5. OR fail

To me, the proposed code doesn't make it immediately obvious why the 
PASSTHROUGH flag should have anything to do with not attempting alias 
remapping.

Moreover, if we truly want to ignore *all* unmapped addresses, then shouldn't 
we also give up on step 2.? (the one that tries to map a free alias to the 
requested address).

In that case, I think something like this would be clearer:

in  i2c_atr_smbus_xfer() and i2c_atr_map_msgs():

```
#never attempts to create a new mapping, only to find an existing one
c2a = i2c_atr_find_mapping_by_addr(chan, msgs[i].addr);
if (!c2a) {
	if (PASSTHROUGH)
		# Since passthrough is set, we ignore unmapped addresses
		goto success or whatever;

	c2a = i2c_atr_create_mapping(chan, msgs[i].addr);
	if (!c2a)
		fail;
}
```

in i2c_atr_attach_addr():

```
c2a = i2c_atr_find_mapping_by_addr(chan, msgs[i].addr);
if (!c2a) {
	c2a = i2c_atr_create_mapping(chan, msgs[i].addr);
	if (!c2a)
		fail;
}
```

So what I'm suggesting is to remove all c2a mapping creation logic from
find_mapping_by_addr() entirely, and to move it to a separate function.

Please let me know what you think.

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 --]

Re: [PATCH 3/3] i2c: atr: add passthrough flag

[Cosmin Tanislav]

On 2/19/25 11:52 AM, Romain Gantois wrote:
> Hello Cosmin,
> 
> On lundi 3 février 2025 13:15:17 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 since the parent I2C ATR can
>> only do address remapping for the direct children.
>>
>> 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,
>> and disables dynamic remapping, since devices that need passthrough
>> messages to be forwarded as-is, can only handle remapping for their
>> direct children.
>>
>> There's no case where a non-remapped address will hit the parent ATR.
> 
> I'm having trouble understanding this, because it seems like there's a
> contradiction with your previous statement:
> 
>> add a flag that allows unmapped addresses to be passed through
> 
> Unmapped addresses are "non-remapped" by definition right? And they can hit the
> parent ATR since we're adding a flag to allow them to pass through...
> 

Non-remapped address means addresses that have never been remapped, on
any ATR instance, not on the parent one.

It's impossible for non-remapped addresses to reach the parent ATR
since the direct children are remapped by the parent ATR and the
children of the child ATRs are remapped by the child ATRs.

Unampped address means addresses that are not remapped on the current
ATR instance (the parent one, in this case, since that's where the flag
is supposed to be used).

I agree that my explanation was a bit confusing.

>>
>> Signed-off-by: Cosmin Tanislav <demonsingur@gmail.com>
>> ---
>>   drivers/i2c/i2c-atr.c   | 26 ++++++++++++++++++--------
>>   include/linux/i2c-atr.h | 20 +++++++++++++++++---
>>   2 files changed, 35 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
>> index 13f7e07fd8e87..5f0e8f1cf69f7 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;
>>
>> @@ -241,7 +243,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_find_mapping_by_addr(struct i2c_atr_chan *chan, u16 addr, bool
>> new_addr) {
> 
> IMO the "new_addr" naming is quite confusing.
> 

Could you suggest a better name? I picked new_addr since when that flag
is true, we are mapping a new address, the call is coming from
i2c_atr_attach_addr().
When we're mapping a new address, and we are in passthrough mode, we're
free to reserve a new alias.
I've since then modified the conditions slightly to cover an edge case.

if (!new_addr && (atr->flags & I2C_ATR_PASSTHROUGH))
	return NULL;

ret = i2c_atr_reserve_alias(chan->alias_pool);
if (ret < 0) {
	if (atr->flags & I2C_ATR_PASSTHROUGH)
		return NULL;

	...
}

With this change, if we've made it past the section where we look for
an existing mapping, and we're not adding a new address, and we're in
passthrough mode, we return NULL. Passthrough mode shouldn't allow
aliases to be reserved dynamically, since the hardware can only map
direct children.

Also, if we are adding a new address, and we failed to reserve a free
alias, and we are in passthrough mode, we don't allow replacing an
existing mapping.

> After this patch is applied, the expected behavior is:
> 
> i2c_atr_find_mapping_by_addr() called from i2c_atr_attach_addr():
>    1. find existing mapping, return it
>    2. OR find free alias, create mapping and return it
>    3. OR remap used alias, return mapping
>    4. OR fail
> 
> i2c_atr_find_mapping_by_addr(), called from anywhere else:
>     1. find existing mapping, return it
>     2. OR find free alias, create mapping and return it
>     3. OR if the ATR has PASSTHROUGH set, fail
>     4. OR remap used alias, return mapping
>     5. OR fail
> 
> To me, the proposed code doesn't make it immediately obvious why the
> PASSTHROUGH flag should have anything to do with not attempting alias
> remapping.
> 
> Moreover, if we truly want to ignore *all* unmapped addresses, then shouldn't
> we also give up on step 2.? (the one that tries to map a free alias to the
> requested address).
> 
> In that case, I think something like this would be clearer:
> 
> in  i2c_atr_smbus_xfer() and i2c_atr_map_msgs():
> 
> ```
> #never attempts to create a new mapping, only to find an existing one
> c2a = i2c_atr_find_mapping_by_addr(chan, msgs[i].addr);
> if (!c2a) {
> 	if (PASSTHROUGH)
> 		# Since passthrough is set, we ignore unmapped addresses
> 		goto success or whatever;
> 
> 	c2a = i2c_atr_create_mapping(chan, msgs[i].addr);
> 	if (!c2a)
> 		fail;
> }
> ```
> 
> in i2c_atr_attach_addr():
> 
> ```
> c2a = i2c_atr_find_mapping_by_addr(chan, msgs[i].addr);
> if (!c2a) {
> 	c2a = i2c_atr_create_mapping(chan, msgs[i].addr);
> 	if (!c2a)
> 		fail;
> }
> ```
> 
> So what I'm suggesting is to remove all c2a mapping creation logic from
> find_mapping_by_addr() entirely, and to move it to a separate function.
> 
> Please let me know what you think.
> 

In the case of passthrough ATR, mapping creation should only be allowed
when direct devices are attached, ie: in the call to
i2c_atr_find_mapping_by_addr() from i2c_atr_attach_addr().
If i2c_atr_find_mapping_by_addr() cannot find a free alias in that case,
it should fail.

Other calls to i2c_atr_find_mapping_by_addr() should either return an
existing alias or NULL, and not attempt to create a new one or to
replace an existing one.

Let me know if my explanations made it clearer and what the you think
about going forward with this patch. In the meantime, I'll try to see
how the code looks with splitting creating from finding.

> Thanks,
> 

Re: [PATCH 3/3] i2c: atr: add passthrough flag

[Romain Gantois]

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

On mercredi 19 février 2025 11:22:12 heure normale d’Europe centrale Cosmin 
Tanislav wrote:
> On 2/19/25 11:52 AM, Romain Gantois wrote:
> > Hello Cosmin,
> > 
> > On lundi 3 février 2025 13:15:17 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 since the parent I2C ATR can
> >> only do address remapping for the direct children.
> >> 
> >> 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,
> >> and disables dynamic remapping, since devices that need passthrough
> >> messages to be forwarded as-is, can only handle remapping for their
> >> direct children.
> >> 
> >> There's no case where a non-remapped address will hit the parent ATR.
> > 
> > I'm having trouble understanding this, because it seems like there's a
> > 
> > contradiction with your previous statement:
> >> add a flag that allows unmapped addresses to be passed through
> > 
> > Unmapped addresses are "non-remapped" by definition right? And they can
> > hit the parent ATR since we're adding a flag to allow them to pass
> > through...
> Non-remapped address means addresses that have never been remapped, on
> any ATR instance, not on the parent one.
> 

Ah I see, that makes more sense.

> It's impossible for non-remapped addresses to reach the parent ATR
> since the direct children are remapped by the parent ATR and the
> children of the child ATRs are remapped by the child ATRs.
> 
> Unampped address means addresses that are not remapped on the current
> ATR instance (the parent one, in this case, since that's where the flag
> is supposed to be used).
> 
> I agree that my explanation was a bit confusing.
> 

Well ATR's aren't the most straightforward kind of component anyway, but in 
any case I definitely think that these explanations of the "unmapped" and "non-
remapped" terminology should be in the commit log.
 
> >> Signed-off-by: Cosmin Tanislav <demonsingur@gmail.com>
> >> ---
> >> 
> >>   drivers/i2c/i2c-atr.c   | 26 ++++++++++++++++++--------
> >>   include/linux/i2c-atr.h | 20 +++++++++++++++++---
> >>   2 files changed, 35 insertions(+), 11 deletions(-)
> >> 
> >> diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
> >> index 13f7e07fd8e87..5f0e8f1cf69f7 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;
> >> 
> >> @@ -241,7 +243,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_find_mapping_by_addr(struct i2c_atr_chan *chan, u16 addr, bool
> >> new_addr) {
> > 
> > IMO the "new_addr" naming is quite confusing.
> 
> Could you suggest a better name? I picked new_addr since when that flag
> is true, we are mapping a new address, the call is coming from
> i2c_atr_attach_addr().

My issue with this terminology is that "new address" could mean a number of 
things, including "an address that the ATR hasn't encountered before". IMO the 
effects of the flag on the function body should be understandable without having 
to go through all of the function's call sites.

Rather than renaming the flag however, I'd rather we not handle this logic in 
find_mapping_by_addr() at all. If we just search for mappings in 
find_mapping_by_addr() and separate out mapping creation into another function, 
we can avoid the need for such a flag altogether.

> When we're mapping a new address, and we are in passthrough mode, we're
> free to reserve a new alias.
> I've since then modified the conditions slightly to cover an edge case.
> 
> if (!new_addr && (atr->flags & I2C_ATR_PASSTHROUGH))
> 	return NULL;
> 
> ret = i2c_atr_reserve_alias(chan->alias_pool);
> if (ret < 0) {
> 	if (atr->flags & I2C_ATR_PASSTHROUGH)
> 		return NULL;
> 
> 	...
> }
> 
> With this change, if we've made it past the section where we look for
> an existing mapping, and we're not adding a new address, and we're in
> passthrough mode, we return NULL. Passthrough mode shouldn't allow
> aliases to be reserved dynamically, since the hardware can only map
> direct children.
> 
> Also, if we are adding a new address, and we failed to reserve a free
> alias, and we are in passthrough mode, we don't allow replacing an
> existing mapping.
> 
> > After this patch is applied, the expected behavior is:
> > 
> > i2c_atr_find_mapping_by_addr() called from i2c_atr_attach_addr():
> >    1. find existing mapping, return it
> >    2. OR find free alias, create mapping and return it
> >    3. OR remap used alias, return mapping
> >    4. OR fail
> > 
> > i2c_atr_find_mapping_by_addr(), called from anywhere else:
> >     1. find existing mapping, return it
> >     2. OR find free alias, create mapping and return it
> >     3. OR if the ATR has PASSTHROUGH set, fail
> >     4. OR remap used alias, return mapping
> >     5. OR fail
> > 
> > To me, the proposed code doesn't make it immediately obvious why the
> > PASSTHROUGH flag should have anything to do with not attempting alias
> > remapping.
> > 
> > Moreover, if we truly want to ignore *all* unmapped addresses, then
> > shouldn't we also give up on step 2.? (the one that tries to map a free
> > alias to the requested address).
> > 
> > In that case, I think something like this would be clearer:
> > 
> > in  i2c_atr_smbus_xfer() and i2c_atr_map_msgs():
> > 
> > ```
> > #never attempts to create a new mapping, only to find an existing one
> > c2a = i2c_atr_find_mapping_by_addr(chan, msgs[i].addr);
> > if (!c2a) {
> > 
> > 	if (PASSTHROUGH)
> > 	
> > 		# Since passthrough is set, we ignore unmapped addresses
> > 		goto success or whatever;
> > 	
> > 	c2a = i2c_atr_create_mapping(chan, msgs[i].addr);
> > 	if (!c2a)
> > 	
> > 		fail;
> > 
> > }
> > ```
> > 
> > in i2c_atr_attach_addr():
> > 
> > ```
> > c2a = i2c_atr_find_mapping_by_addr(chan, msgs[i].addr);
> > if (!c2a) {
> > 
> > 	c2a = i2c_atr_create_mapping(chan, msgs[i].addr);
> > 	if (!c2a)
> > 	
> > 		fail;
> > 
> > }
> > ```
> > 
> > So what I'm suggesting is to remove all c2a mapping creation logic from
> > find_mapping_by_addr() entirely, and to move it to a separate function.
> > 
> > Please let me know what you think.
> 
> In the case of passthrough ATR, mapping creation should only be allowed
> when direct devices are attached, ie: in the call to
> i2c_atr_find_mapping_by_addr() from i2c_atr_attach_addr().
> If i2c_atr_find_mapping_by_addr() cannot find a free alias in that case,
> it should fail.
> 

I think it would be better if i2c_atr_find_mapping_by_addr() never tried to 
create a new mapping, not even if an alias is available. This would eliminate 
the need for a special flag passed to the function and let 
i2c_atr_attach_addr() handle it's own logic instead.

> Other calls to i2c_atr_find_mapping_by_addr() should either return an
> existing alias or NULL, and not attempt to create a new one or to
> replace an existing one.
> 
> Let me know if my explanations made it clearer and what the you think
> about going forward with this patch. In the meantime, I'll try to see
> how the code looks with splitting creating from finding.
> 

The patch itself looks like a valid idea to me, but indeed I'd really prefer 
to see a solution where mapping creation and finding are completely separated, 
including for cases where a free alias is used to create a mapping.

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 --]