* [PATCH v5 0/9] i2c: atr: allow usage of nested ATRs
@ 2025-05-07 12:19 Cosmin Tanislav
2025-05-07 12:19 ` [PATCH v5 1/9] i2c: atr: Fix lockdep for " Cosmin Tanislav
` (9 more replies)
0 siblings, 10 replies; 22+ messages in thread
From: Cosmin Tanislav @ 2025-05-07 12:19 UTC (permalink / raw)
Cc: Wolfram Sang, Tomi Valkeinen, Luca Ceresoli,
Mauro Carvalho Chehab, Romain Gantois, Arnd Bergmann,
Greg Kroah-Hartman, linux-i2c, linux-kernel, linux-media,
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.
The previous version is at:
https://lore.kernel.org/lkml/20250428102516.933571-1-demonsingur@gmail.com
V5:
* pick up Reviewed-by tags
* expand the I2C_ATR_F_STATIC description
* place i2c_atr_create_mapping_by_addr() before
i2c_atr_replace_mapping_by_addr()
V4:
* rebase on latest merged changes, and latest submitted fixes
V3:
* remove i2c_atr_new_flags(), add flags parameter to i2c_atr_new() in
a new patch
* remove "i2c: atr: unlock mutex after c2a access" patch as it will
be moved into the base series
* remove alias_pairs variable used only once
* remove err_del_c2a label used only once
* add lockdep_assert_held to i2c_atr_create_mapping_by_addr()
* I2C_ATR_STATIC -> I2C_ATR_F_STATIC
* I2C_ATR_PASSTHROUGH -> I2C_ATR_F_PASSTHROUGH
* add passthrough check to i2c_atr_smbus_xfer()
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: 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 flags parameter to i2c_atr_new()
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 | 197 ++++++++++++++++++++++------------
drivers/media/i2c/ds90ub960.c | 2 +-
drivers/misc/ti_fpc202.c | 2 +-
include/linux/i2c-atr.h | 19 +++-
4 files changed, 146 insertions(+), 74 deletions(-)
--
2.49.0
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v5 1/9] i2c: atr: Fix lockdep for nested ATRs
2025-05-07 12:19 [PATCH v5 0/9] i2c: atr: allow usage of nested ATRs Cosmin Tanislav
@ 2025-05-07 12:19 ` Cosmin Tanislav
2025-05-22 9:06 ` Wolfram Sang
2025-05-07 12:19 ` [PATCH v5 2/9] i2c: atr: find_mapping() -> get_mapping() Cosmin Tanislav
` (8 subsequent siblings)
9 siblings, 1 reply; 22+ messages in thread
From: Cosmin Tanislav @ 2025-05-07 12:19 UTC (permalink / raw)
Cc: Wolfram Sang, Tomi Valkeinen, Luca Ceresoli,
Mauro Carvalho Chehab, Romain Gantois, Arnd Bergmann,
Greg Kroah-Hartman, linux-i2c, linux-kernel, linux-media,
Tomi Valkeinen
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>
Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.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 1aeaecacc26c..a79ca87e8bbd 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;
@@ -683,7 +689,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;
@@ -711,6 +718,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);
@@ -727,6 +735,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");
@@ -761,8 +770,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);
@@ -839,6 +850,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;
}
@@ -876,6 +889,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.49.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v5 2/9] i2c: atr: find_mapping() -> get_mapping()
2025-05-07 12:19 [PATCH v5 0/9] i2c: atr: allow usage of nested ATRs Cosmin Tanislav
2025-05-07 12:19 ` [PATCH v5 1/9] i2c: atr: Fix lockdep for " Cosmin Tanislav
@ 2025-05-07 12:19 ` Cosmin Tanislav
2025-05-07 12:19 ` [PATCH v5 3/9] i2c: atr: split up i2c_atr_get_mapping_by_addr() Cosmin Tanislav
` (7 subsequent siblings)
9 siblings, 0 replies; 22+ messages in thread
From: Cosmin Tanislav @ 2025-05-07 12:19 UTC (permalink / raw)
Cc: Wolfram Sang, Tomi Valkeinen, Luca Ceresoli,
Mauro Carvalho Chehab, Romain Gantois, Arnd Bergmann,
Greg Kroah-Hartman, linux-i2c, linux-kernel, linux-media,
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>
Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.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 a79ca87e8bbd..939fb95fe781 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;
@@ -339,7 +339,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",
@@ -432,7 +432,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);
@@ -540,7 +540,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.49.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v5 3/9] i2c: atr: split up i2c_atr_get_mapping_by_addr()
2025-05-07 12:19 [PATCH v5 0/9] i2c: atr: allow usage of nested ATRs Cosmin Tanislav
2025-05-07 12:19 ` [PATCH v5 1/9] i2c: atr: Fix lockdep for " Cosmin Tanislav
2025-05-07 12:19 ` [PATCH v5 2/9] i2c: atr: find_mapping() -> get_mapping() Cosmin Tanislav
@ 2025-05-07 12:19 ` Cosmin Tanislav
2025-05-21 15:56 ` Luca Ceresoli
2025-05-07 12:19 ` [PATCH v5 4/9] i2c: atr: do not create mapping in detach_addr() Cosmin Tanislav
` (6 subsequent siblings)
9 siblings, 1 reply; 22+ messages in thread
From: Cosmin Tanislav @ 2025-05-07 12:19 UTC (permalink / raw)
Cc: Wolfram Sang, Tomi Valkeinen, Luca Ceresoli,
Mauro Carvalho Chehab, Romain Gantois, Arnd Bergmann,
Greg Kroah-Hartman, linux-i2c, linux-kernel, linux-media,
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 | 122 +++++++++++++++++++++++++++++-------------
1 file changed, 84 insertions(+), 38 deletions(-)
diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
index 939fb95fe781..215b6773fe06 100644
--- a/drivers/i2c/i2c-atr.c
+++ b/drivers/i2c/i2c-atr.c
@@ -239,56 +239,40 @@ 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_create_mapping_by_addr(struct i2c_atr_chan *chan, u16 addr)
{
struct i2c_atr *atr = chan->atr;
struct i2c_atr_alias_pair *c2a;
- struct list_head *alias_pairs;
- bool found = false;
u16 alias;
int ret;
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;
- }
-
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 +290,68 @@ i2c_atr_get_mapping_by_addr(struct i2c_atr_chan *chan, u16 addr)
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;
+ struct list_head *alias_pairs;
+ bool found = false;
+ u16 alias;
+ int ret;
+
+ lockdep_assert_held(&chan->alias_pairs_lock);
+
+ alias_pairs = &chan->alias_pairs;
+
+ 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_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.49.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v5 4/9] i2c: atr: do not create mapping in detach_addr()
2025-05-07 12:19 [PATCH v5 0/9] i2c: atr: allow usage of nested ATRs Cosmin Tanislav
` (2 preceding siblings ...)
2025-05-07 12:19 ` [PATCH v5 3/9] i2c: atr: split up i2c_atr_get_mapping_by_addr() Cosmin Tanislav
@ 2025-05-07 12:19 ` Cosmin Tanislav
2025-05-07 12:19 ` [PATCH v5 5/9] i2c: atr: deduplicate logic in attach_addr() Cosmin Tanislav
` (5 subsequent siblings)
9 siblings, 0 replies; 22+ messages in thread
From: Cosmin Tanislav @ 2025-05-07 12:19 UTC (permalink / raw)
Cc: Wolfram Sang, Tomi Valkeinen, Luca Ceresoli,
Mauro Carvalho Chehab, Romain Gantois, Arnd Bergmann,
Greg Kroah-Hartman, linux-i2c, linux-kernel, linux-media,
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>
Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.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 215b6773fe06..178b203c8777 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.49.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v5 5/9] i2c: atr: deduplicate logic in attach_addr()
2025-05-07 12:19 [PATCH v5 0/9] i2c: atr: allow usage of nested ATRs Cosmin Tanislav
` (3 preceding siblings ...)
2025-05-07 12:19 ` [PATCH v5 4/9] i2c: atr: do not create mapping in detach_addr() Cosmin Tanislav
@ 2025-05-07 12:19 ` Cosmin Tanislav
2025-05-07 12:19 ` [PATCH v5 6/9] i2c: atr: allow replacing mappings " Cosmin Tanislav
` (4 subsequent siblings)
9 siblings, 0 replies; 22+ messages in thread
From: Cosmin Tanislav @ 2025-05-07 12:19 UTC (permalink / raw)
Cc: Wolfram Sang, Tomi Valkeinen, Luca Ceresoli,
Mauro Carvalho Chehab, Romain Gantois, Arnd Bergmann,
Greg Kroah-Hartman, linux-i2c, linux-kernel, linux-media,
Cosmin Tanislav
This is the same logic as in i2c_atr_create_mapping_by_addr().
Signed-off-by: Cosmin Tanislav <demonsingur@gmail.com>
Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
Reviewed-by: Romain Gantois <romain.gantois@bootlin.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 178b203c8777..ae5c2ee629f0 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.49.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v5 6/9] i2c: atr: allow replacing mappings in attach_addr()
2025-05-07 12:19 [PATCH v5 0/9] i2c: atr: allow usage of nested ATRs Cosmin Tanislav
` (4 preceding siblings ...)
2025-05-07 12:19 ` [PATCH v5 5/9] i2c: atr: deduplicate logic in attach_addr() Cosmin Tanislav
@ 2025-05-07 12:19 ` Cosmin Tanislav
2025-05-21 15:56 ` Luca Ceresoli
2025-05-07 12:19 ` [PATCH v5 7/9] i2c: atr: add flags parameter to i2c_atr_new() Cosmin Tanislav
` (3 subsequent siblings)
9 siblings, 1 reply; 22+ messages in thread
From: Cosmin Tanislav @ 2025-05-07 12:19 UTC (permalink / raw)
Cc: Wolfram Sang, Tomi Valkeinen, Luca Ceresoli,
Mauro Carvalho Chehab, Romain Gantois, Arnd Bergmann,
Greg Kroah-Hartman, linux-i2c, linux-kernel, linux-media,
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 ae5c2ee629f0..91aabfb4379b 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.49.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v5 7/9] i2c: atr: add flags parameter to i2c_atr_new()
2025-05-07 12:19 [PATCH v5 0/9] i2c: atr: allow usage of nested ATRs Cosmin Tanislav
` (5 preceding siblings ...)
2025-05-07 12:19 ` [PATCH v5 6/9] i2c: atr: allow replacing mappings " Cosmin Tanislav
@ 2025-05-07 12:19 ` Cosmin Tanislav
2025-05-22 8:50 ` Wolfram Sang
2025-05-07 12:19 ` [PATCH v5 8/9] i2c: atr: add static flag Cosmin Tanislav
` (2 subsequent siblings)
9 siblings, 1 reply; 22+ messages in thread
From: Cosmin Tanislav @ 2025-05-07 12:19 UTC (permalink / raw)
Cc: Wolfram Sang, Tomi Valkeinen, Luca Ceresoli,
Mauro Carvalho Chehab, Romain Gantois, Arnd Bergmann,
Greg Kroah-Hartman, linux-i2c, linux-kernel, linux-media,
Cosmin Tanislav
In preparation for adding multiple flags that change the behavior,
add a flags parameter to i2c_atr_new() and an i2c_atr_flags enum.
Signed-off-by: Cosmin Tanislav <demonsingur@gmail.com>
Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
Reviewed-by: Romain Gantois <romain.gantois@bootlin.com>
---
drivers/i2c/i2c-atr.c | 6 +++++-
drivers/media/i2c/ds90ub960.c | 2 +-
drivers/misc/ti_fpc202.c | 2 +-
include/linux/i2c-atr.h | 10 +++++++++-
4 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
index 91aabfb4379b..121cabbdb85d 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;
@@ -703,7 +705,8 @@ static int i2c_atr_parse_alias_pool(struct i2c_atr *atr)
}
struct i2c_atr *i2c_atr_new(struct i2c_adapter *parent, struct device *dev,
- const struct i2c_atr_ops *ops, int max_adapters)
+ const struct i2c_atr_ops *ops, int max_adapters,
+ u32 flags)
{
struct i2c_atr *atr;
int ret;
@@ -725,6 +728,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;
diff --git a/drivers/media/i2c/ds90ub960.c b/drivers/media/i2c/ds90ub960.c
index 869e32bd07e8..6f475bae94b3 100644
--- a/drivers/media/i2c/ds90ub960.c
+++ b/drivers/media/i2c/ds90ub960.c
@@ -1122,7 +1122,7 @@ static int ub960_init_atr(struct ub960_data *priv)
struct i2c_adapter *parent_adap = priv->client->adapter;
priv->atr = i2c_atr_new(parent_adap, dev, &ub960_atr_ops,
- priv->hw_data->num_rxports);
+ priv->hw_data->num_rxports, 0);
if (IS_ERR(priv->atr))
return PTR_ERR(priv->atr);
diff --git a/drivers/misc/ti_fpc202.c b/drivers/misc/ti_fpc202.c
index b9c9ee4bfc4e..f7cde245ac95 100644
--- a/drivers/misc/ti_fpc202.c
+++ b/drivers/misc/ti_fpc202.c
@@ -349,7 +349,7 @@ static int fpc202_probe(struct i2c_client *client)
goto disable_gpio;
}
- priv->atr = i2c_atr_new(client->adapter, dev, &fpc202_atr_ops, 2);
+ priv->atr = i2c_atr_new(client->adapter, dev, &fpc202_atr_ops, 2, 0);
if (IS_ERR(priv->atr)) {
ret = PTR_ERR(priv->atr);
dev_err(dev, "failed to create i2c atr err %d\n", ret);
diff --git a/include/linux/i2c-atr.h b/include/linux/i2c-atr.h
index 1c3a5bcd939f..5082f4dd0e23 100644
--- a/include/linux/i2c-atr.h
+++ b/include/linux/i2c-atr.h
@@ -18,6 +18,12 @@ struct device;
struct fwnode_handle;
struct i2c_atr;
+/**
+ * enum i2c_atr_flags - Flags for an I2C ATR driver
+ */
+enum i2c_atr_flags {
+};
+
/**
* struct i2c_atr_ops - Callbacks from ATR to the device driver.
* @attach_addr: Notify the driver of a new device connected on a child
@@ -65,6 +71,7 @@ struct i2c_atr_adap_desc {
* @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.
@@ -74,7 +81,8 @@ 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);
+ const struct i2c_atr_ops *ops, int max_adapters,
+ u32 flags);
/**
* i2c_atr_delete - Delete an I2C ATR helper.
--
2.49.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v5 8/9] i2c: atr: add static flag
2025-05-07 12:19 [PATCH v5 0/9] i2c: atr: allow usage of nested ATRs Cosmin Tanislav
` (6 preceding siblings ...)
2025-05-07 12:19 ` [PATCH v5 7/9] i2c: atr: add flags parameter to i2c_atr_new() Cosmin Tanislav
@ 2025-05-07 12:19 ` Cosmin Tanislav
2025-05-21 15:56 ` Luca Ceresoli
2025-05-22 8:58 ` Wolfram Sang
2025-05-07 12:19 ` [PATCH v5 9/9] i2c: atr: add passthrough flag Cosmin Tanislav
2025-05-22 10:12 ` [PATCH v5 0/9] i2c: atr: allow usage of nested ATRs Wolfram Sang
9 siblings, 2 replies; 22+ messages in thread
From: Cosmin Tanislav @ 2025-05-07 12:19 UTC (permalink / raw)
Cc: Wolfram Sang, Tomi Valkeinen, Luca Ceresoli,
Mauro Carvalho Chehab, Romain Gantois, Arnd Bergmann,
Greg Kroah-Hartman, linux-i2c, linux-kernel, linux-media,
Cosmin Tanislav
Some I2C ATRs do not support dynamic remapping, only static mapping
of direct children.
Mappings will only be added or removed as a result of devices being
added or removed from a child bus.
The ATR pool will have to be big enough to accomodate all devices
expected to be added to the child buses.
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 | 6 +++++-
include/linux/i2c-atr.h | 7 +++++++
2 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
index 121cabbdb85d..76d70efdf190 100644
--- a/drivers/i2c/i2c-atr.c
+++ b/drivers/i2c/i2c-atr.c
@@ -341,12 +341,16 @@ i2c_atr_replace_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_F_STATIC)
+ return NULL;
+
c2a = i2c_atr_create_mapping_by_addr(chan, addr);
if (c2a)
return c2a;
@@ -545,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_F_STATIC))
c2a = i2c_atr_replace_mapping_by_addr(chan, addr);
if (!c2a) {
diff --git a/include/linux/i2c-atr.h b/include/linux/i2c-atr.h
index 5082f4dd0e23..5aaab1598084 100644
--- a/include/linux/i2c-atr.h
+++ b/include/linux/i2c-atr.h
@@ -20,8 +20,15 @@ struct i2c_atr;
/**
* enum i2c_atr_flags - Flags for an I2C ATR driver
+ *
+ * @I2C_ATR_F_STATIC: ATR does not support dynamic mapping, use static mapping.
+ * Mappings will only be added or removed as a result of
+ * devices being added or removed from a child bus.
+ * The ATR pool will have to be big enough to accomodate all
+ * devices expected to be added to the child buses.
*/
enum i2c_atr_flags {
+ I2C_ATR_F_STATIC = BIT(0),
};
/**
--
2.49.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v5 9/9] i2c: atr: add passthrough flag
2025-05-07 12:19 [PATCH v5 0/9] i2c: atr: allow usage of nested ATRs Cosmin Tanislav
` (7 preceding siblings ...)
2025-05-07 12:19 ` [PATCH v5 8/9] i2c: atr: add static flag Cosmin Tanislav
@ 2025-05-07 12:19 ` Cosmin Tanislav
2025-05-22 10:12 ` [PATCH v5 0/9] i2c: atr: allow usage of nested ATRs Wolfram Sang
9 siblings, 0 replies; 22+ messages in thread
From: Cosmin Tanislav @ 2025-05-07 12:19 UTC (permalink / raw)
Cc: Wolfram Sang, Tomi Valkeinen, Luca Ceresoli,
Mauro Carvalho Chehab, Romain Gantois, Arnd Bergmann,
Greg Kroah-Hartman, linux-i2c, linux-kernel, linux-media,
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>
Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
Reviewed-by: Romain Gantois <romain.gantois@bootlin.com>
---
drivers/i2c/i2c-atr.c | 7 +++++--
include/linux/i2c-atr.h | 2 ++
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
index 76d70efdf190..be7d6d41e0b2 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_F_PASSTHROUGH)
+ continue;
+
dev_err(atr->dev, "client 0x%02x not mapped!\n",
msgs[i].addr);
@@ -486,13 +489,13 @@ static int i2c_atr_smbus_xfer(struct i2c_adapter *adap, u16 addr,
c2a = i2c_atr_get_mapping_by_addr(chan, addr);
- if (!c2a) {
+ if (!c2a && !(atr->flags & I2C_ATR_F_PASSTHROUGH)) {
dev_err(atr->dev, "client 0x%02x not mapped!\n", addr);
mutex_unlock(&chan->alias_pairs_lock);
return -ENXIO;
}
- alias = c2a->alias;
+ alias = c2a ? c2a->alias : addr;
mutex_unlock(&chan->alias_pairs_lock);
diff --git a/include/linux/i2c-atr.h b/include/linux/i2c-atr.h
index 5aaab1598084..2bb54dc87c8e 100644
--- a/include/linux/i2c-atr.h
+++ b/include/linux/i2c-atr.h
@@ -26,9 +26,11 @@ struct i2c_atr;
* devices being added or removed from a child bus.
* The ATR pool will have to be big enough to accomodate all
* devices expected to be added to the child buses.
+ * @I2C_ATR_F_PASSTHROUGH: Allow unmapped incoming addresses to pass through
*/
enum i2c_atr_flags {
I2C_ATR_F_STATIC = BIT(0),
+ I2C_ATR_F_PASSTHROUGH = BIT(1),
};
/**
--
2.49.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v5 8/9] i2c: atr: add static flag
2025-05-07 12:19 ` [PATCH v5 8/9] i2c: atr: add static flag Cosmin Tanislav
@ 2025-05-21 15:56 ` Luca Ceresoli
2025-05-22 8:58 ` Wolfram Sang
1 sibling, 0 replies; 22+ messages in thread
From: Luca Ceresoli @ 2025-05-21 15:56 UTC (permalink / raw)
To: Cosmin Tanislav
Cc: Wolfram Sang, Tomi Valkeinen, Mauro Carvalho Chehab,
Romain Gantois, Arnd Bergmann, Greg Kroah-Hartman, linux-i2c,
linux-kernel, linux-media
On Wed, 7 May 2025 15:19:14 +0300
Cosmin Tanislav <demonsingur@gmail.com> wrote:
> Some I2C ATRs do not support dynamic remapping, only static mapping
> of direct children.
>
> Mappings will only be added or removed as a result of devices being
> added or removed from a child bus.
>
> The ATR pool will have to be big enough to accomodate all devices
> expected to be added to the child buses.
>
> 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>
Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v5 6/9] i2c: atr: allow replacing mappings in attach_addr()
2025-05-07 12:19 ` [PATCH v5 6/9] i2c: atr: allow replacing mappings " Cosmin Tanislav
@ 2025-05-21 15:56 ` Luca Ceresoli
0 siblings, 0 replies; 22+ messages in thread
From: Luca Ceresoli @ 2025-05-21 15:56 UTC (permalink / raw)
To: Cosmin Tanislav
Cc: Wolfram Sang, Tomi Valkeinen, Mauro Carvalho Chehab,
Romain Gantois, Arnd Bergmann, Greg Kroah-Hartman, linux-i2c,
linux-kernel, linux-media
On Wed, 7 May 2025 15:19:12 +0300
Cosmin Tanislav <demonsingur@gmail.com> wrote:
> 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>
Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v5 3/9] i2c: atr: split up i2c_atr_get_mapping_by_addr()
2025-05-07 12:19 ` [PATCH v5 3/9] i2c: atr: split up i2c_atr_get_mapping_by_addr() Cosmin Tanislav
@ 2025-05-21 15:56 ` Luca Ceresoli
0 siblings, 0 replies; 22+ messages in thread
From: Luca Ceresoli @ 2025-05-21 15:56 UTC (permalink / raw)
To: Cosmin Tanislav
Cc: Wolfram Sang, Tomi Valkeinen, Mauro Carvalho Chehab,
Romain Gantois, Arnd Bergmann, Greg Kroah-Hartman, linux-i2c,
linux-kernel, linux-media
On Wed, 7 May 2025 15:19:09 +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>
Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v5 7/9] i2c: atr: add flags parameter to i2c_atr_new()
2025-05-07 12:19 ` [PATCH v5 7/9] i2c: atr: add flags parameter to i2c_atr_new() Cosmin Tanislav
@ 2025-05-22 8:50 ` Wolfram Sang
2025-05-22 10:10 ` Luca Ceresoli
0 siblings, 1 reply; 22+ messages in thread
From: Wolfram Sang @ 2025-05-22 8:50 UTC (permalink / raw)
To: Cosmin Tanislav
Cc: Tomi Valkeinen, Luca Ceresoli, Mauro Carvalho Chehab,
Romain Gantois, Arnd Bergmann, Greg Kroah-Hartman, linux-i2c,
linux-kernel, linux-media
[-- Attachment #1: Type: text/plain, Size: 707 bytes --]
On Wed, May 07, 2025 at 03:19:13PM +0300, Cosmin Tanislav wrote:
> In preparation for adding multiple flags that change the behavior,
> add a flags parameter to i2c_atr_new() and an i2c_atr_flags enum.
>
> Signed-off-by: Cosmin Tanislav <demonsingur@gmail.com>
> Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> Reviewed-by: Romain Gantois <romain.gantois@bootlin.com>
How did you guys get it to build?
CC drivers/i2c/i2c-atr.o
In file included from drivers/i2c/i2c-atr.c:12:
./include/linux/i2c-atr.h:25:1: error: empty enum is invalid
25 | };
| ^
make[4]: *** [scripts/Makefile.build:203: drivers/i2c/i2c-atr.o] Error 1
I'll squash patches 7+8 to avoid it.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v5 8/9] i2c: atr: add static flag
2025-05-07 12:19 ` [PATCH v5 8/9] i2c: atr: add static flag Cosmin Tanislav
2025-05-21 15:56 ` Luca Ceresoli
@ 2025-05-22 8:58 ` Wolfram Sang
1 sibling, 0 replies; 22+ messages in thread
From: Wolfram Sang @ 2025-05-22 8:58 UTC (permalink / raw)
To: Cosmin Tanislav
Cc: Tomi Valkeinen, Luca Ceresoli, Mauro Carvalho Chehab,
Romain Gantois, Arnd Bergmann, Greg Kroah-Hartman, linux-i2c,
linux-kernel, linux-media
[-- Attachment #1: Type: text/plain, Size: 1007 bytes --]
On Wed, May 07, 2025 at 03:19:14PM +0300, Cosmin Tanislav wrote:
> Some I2C ATRs do not support dynamic remapping, only static mapping
> of direct children.
>
> Mappings will only be added or removed as a result of devices being
> added or removed from a child bus.
>
> The ATR pool will have to be big enough to accomodate all devices
CHECKPATCH
WARNING: 'accomodate' may be misspelled - perhaps 'accommodate'?
#12:
The ATR pool will have to be big enough to accomodate all devices
^^^^^^^^^^
WARNING: 'accomodate' may be misspelled - perhaps 'accommodate'?
#134: FILE: include/linux/i2c-atr.h:27:
+ * The ATR pool will have to be big enough to accomodate all
^^^^^^^^^^
total: 0 errors, 2 warnings, 0 checks, 105 lines checked
I fixed it for you this time.
Also, please make you sure you have a To: recipient in the mail header
next time.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v5 1/9] i2c: atr: Fix lockdep for nested ATRs
2025-05-07 12:19 ` [PATCH v5 1/9] i2c: atr: Fix lockdep for " Cosmin Tanislav
@ 2025-05-22 9:06 ` Wolfram Sang
2025-05-22 9:31 ` Cosmin Tanislav
2025-05-22 10:07 ` Luca Ceresoli
0 siblings, 2 replies; 22+ messages in thread
From: Wolfram Sang @ 2025-05-22 9:06 UTC (permalink / raw)
To: Cosmin Tanislav
Cc: Tomi Valkeinen, Luca Ceresoli, Mauro Carvalho Chehab,
Romain Gantois, Arnd Bergmann, Greg Kroah-Hartman, linux-i2c,
linux-kernel, linux-media, Tomi Valkeinen
[-- Attachment #1: Type: text/plain, Size: 700 bytes --]
On Wed, May 07, 2025 at 03:19:07PM +0300, Cosmin Tanislav wrote:
> 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>
> Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
Your SoB is missing. I will add it for you if you confirm here.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v5 1/9] i2c: atr: Fix lockdep for nested ATRs
2025-05-22 9:06 ` Wolfram Sang
@ 2025-05-22 9:31 ` Cosmin Tanislav
2025-05-22 10:07 ` Luca Ceresoli
1 sibling, 0 replies; 22+ messages in thread
From: Cosmin Tanislav @ 2025-05-22 9:31 UTC (permalink / raw)
To: Wolfram Sang, Tomi Valkeinen, Luca Ceresoli,
Mauro Carvalho Chehab, Romain Gantois, Arnd Bergmann,
Greg Kroah-Hartman, linux-i2c, linux-kernel, linux-media,
Tomi Valkeinen
On 5/22/25 12:06 PM, Wolfram Sang wrote:
> On Wed, May 07, 2025 at 03:19:07PM +0300, Cosmin Tanislav wrote:
>> 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>
>> Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
>
> Your SoB is missing. I will add it for you if you confirm here.
>
My bad.
Signed-off-by: Cosmin Tanislav <demonsingur@gmail.com>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v5 1/9] i2c: atr: Fix lockdep for nested ATRs
2025-05-22 9:06 ` Wolfram Sang
2025-05-22 9:31 ` Cosmin Tanislav
@ 2025-05-22 10:07 ` Luca Ceresoli
2025-05-22 10:08 ` Luca Ceresoli
2025-05-22 10:11 ` Wolfram Sang
1 sibling, 2 replies; 22+ messages in thread
From: Luca Ceresoli @ 2025-05-22 10:07 UTC (permalink / raw)
To: Wolfram Sang
Cc: Cosmin Tanislav, Tomi Valkeinen, Mauro Carvalho Chehab,
Romain Gantois, Arnd Bergmann, Greg Kroah-Hartman, linux-i2c,
linux-kernel, linux-media, Tomi Valkeinen
On Thu, 22 May 2025 11:06:06 +0200
Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:
> On Wed, May 07, 2025 at 03:19:07PM +0300, Cosmin Tanislav wrote:
> > 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>
> > Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
>
> Your SoB is missing. I will add it for you if you confirm here.
Mine? I didn't think it would be needed based on:
> The Signed-off-by: tag indicates that the signer was involved in the
> development of the patch, or that he/she was in the patch’s delivery
> path.
I'm not involved in the development, and "being in the delivery path" is probably not clear enought to me.
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v5 1/9] i2c: atr: Fix lockdep for nested ATRs
2025-05-22 10:07 ` Luca Ceresoli
@ 2025-05-22 10:08 ` Luca Ceresoli
2025-05-22 10:11 ` Wolfram Sang
1 sibling, 0 replies; 22+ messages in thread
From: Luca Ceresoli @ 2025-05-22 10:08 UTC (permalink / raw)
To: Wolfram Sang
Cc: Cosmin Tanislav, Tomi Valkeinen, Mauro Carvalho Chehab,
Romain Gantois, Arnd Bergmann, Greg Kroah-Hartman, linux-i2c,
linux-kernel, linux-media, Tomi Valkeinen
Hi,
bleurgh. Sent by mistake before I finished writing. :-/
On Thu, 22 May 2025 12:07:13 +0200
Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> On Thu, 22 May 2025 11:06:06 +0200
> Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:
>
> > On Wed, May 07, 2025 at 03:19:07PM +0300, Cosmin Tanislav wrote:
> > > 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>
> > > Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> >
> > Your SoB is missing. I will add it for you if you confirm here.
>
> Mine? I didn't think it would be needed based on:
>
> > The Signed-off-by: tag indicates that the signer was involved in the
> > development of the patch, or that he/she was in the patch’s delivery
> > path.
>
> I'm not involved in the development, and "being in the delivery path" is probably not clear enought to me.
However, if it is needed, I confirm you can add my:
Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
Luca
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v5 7/9] i2c: atr: add flags parameter to i2c_atr_new()
2025-05-22 8:50 ` Wolfram Sang
@ 2025-05-22 10:10 ` Luca Ceresoli
0 siblings, 0 replies; 22+ messages in thread
From: Luca Ceresoli @ 2025-05-22 10:10 UTC (permalink / raw)
To: Wolfram Sang
Cc: Cosmin Tanislav, Tomi Valkeinen, Mauro Carvalho Chehab,
Romain Gantois, Arnd Bergmann, Greg Kroah-Hartman, linux-i2c,
linux-kernel, linux-media
On Thu, 22 May 2025 10:50:24 +0200
Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:
> On Wed, May 07, 2025 at 03:19:13PM +0300, Cosmin Tanislav wrote:
> > In preparation for adding multiple flags that change the behavior,
> > add a flags parameter to i2c_atr_new() and an i2c_atr_flags enum.
> >
> > Signed-off-by: Cosmin Tanislav <demonsingur@gmail.com>
> > Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> > Reviewed-by: Romain Gantois <romain.gantois@bootlin.com>
>
> How did you guys get it to build?
I haven't :-)
I just reviewed the code, and apparently gcc's parser is stricter than
I am.
Luca
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v5 1/9] i2c: atr: Fix lockdep for nested ATRs
2025-05-22 10:07 ` Luca Ceresoli
2025-05-22 10:08 ` Luca Ceresoli
@ 2025-05-22 10:11 ` Wolfram Sang
1 sibling, 0 replies; 22+ messages in thread
From: Wolfram Sang @ 2025-05-22 10:11 UTC (permalink / raw)
To: Luca Ceresoli
Cc: Cosmin Tanislav, Tomi Valkeinen, Mauro Carvalho Chehab,
Romain Gantois, Arnd Bergmann, Greg Kroah-Hartman, linux-i2c,
linux-kernel, linux-media, Tomi Valkeinen
[-- Attachment #1: Type: text/plain, Size: 1043 bytes --]
On Thu, May 22, 2025 at 12:07:13PM +0200, Luca Ceresoli wrote:
> On Thu, 22 May 2025 11:06:06 +0200
> Wolfram Sang <wsa+renesas@sang-engineering.com> wrote:
>
> > On Wed, May 07, 2025 at 03:19:07PM +0300, Cosmin Tanislav wrote:
> > > 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>
> > > Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> >
> > Your SoB is missing. I will add it for you if you confirm here.
>
> Mine? I didn't think it would be needed based on:
Ah, I was talking to Cosmin. Sorry if that wasn't clear.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v5 0/9] i2c: atr: allow usage of nested ATRs
2025-05-07 12:19 [PATCH v5 0/9] i2c: atr: allow usage of nested ATRs Cosmin Tanislav
` (8 preceding siblings ...)
2025-05-07 12:19 ` [PATCH v5 9/9] i2c: atr: add passthrough flag Cosmin Tanislav
@ 2025-05-22 10:12 ` Wolfram Sang
9 siblings, 0 replies; 22+ messages in thread
From: Wolfram Sang @ 2025-05-22 10:12 UTC (permalink / raw)
To: Cosmin Tanislav
Cc: Tomi Valkeinen, Luca Ceresoli, Mauro Carvalho Chehab,
Romain Gantois, Arnd Bergmann, Greg Kroah-Hartman, linux-i2c,
linux-kernel, linux-media
[-- Attachment #1: Type: text/plain, Size: 2324 bytes --]
On Wed, May 07, 2025 at 03:19:06PM +0300, Cosmin Tanislav wrote:
> 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.
>
> The previous version is at:
> https://lore.kernel.org/lkml/20250428102516.933571-1-demonsingur@gmail.com
>
> V5:
> * pick up Reviewed-by tags
> * expand the I2C_ATR_F_STATIC description
> * place i2c_atr_create_mapping_by_addr() before
> i2c_atr_replace_mapping_by_addr()
>
> V4:
> * rebase on latest merged changes, and latest submitted fixes
>
> V3:
> * remove i2c_atr_new_flags(), add flags parameter to i2c_atr_new() in
> a new patch
> * remove "i2c: atr: unlock mutex after c2a access" patch as it will
> be moved into the base series
> * remove alias_pairs variable used only once
> * remove err_del_c2a label used only once
> * add lockdep_assert_held to i2c_atr_create_mapping_by_addr()
> * I2C_ATR_STATIC -> I2C_ATR_F_STATIC
> * I2C_ATR_PASSTHROUGH -> I2C_ATR_F_PASSTHROUGH
> * add passthrough check to i2c_atr_smbus_xfer()
>
> 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: 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 flags parameter to i2c_atr_new()
> i2c: atr: add static flag
> i2c: atr: add passthrough flag
>
> Tomi Valkeinen (1):
> i2c: atr: Fix lockdep for nested ATRs
>
Applied to for-next, thanks!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2025-05-22 10:12 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-07 12:19 [PATCH v5 0/9] i2c: atr: allow usage of nested ATRs Cosmin Tanislav
2025-05-07 12:19 ` [PATCH v5 1/9] i2c: atr: Fix lockdep for " Cosmin Tanislav
2025-05-22 9:06 ` Wolfram Sang
2025-05-22 9:31 ` Cosmin Tanislav
2025-05-22 10:07 ` Luca Ceresoli
2025-05-22 10:08 ` Luca Ceresoli
2025-05-22 10:11 ` Wolfram Sang
2025-05-07 12:19 ` [PATCH v5 2/9] i2c: atr: find_mapping() -> get_mapping() Cosmin Tanislav
2025-05-07 12:19 ` [PATCH v5 3/9] i2c: atr: split up i2c_atr_get_mapping_by_addr() Cosmin Tanislav
2025-05-21 15:56 ` Luca Ceresoli
2025-05-07 12:19 ` [PATCH v5 4/9] i2c: atr: do not create mapping in detach_addr() Cosmin Tanislav
2025-05-07 12:19 ` [PATCH v5 5/9] i2c: atr: deduplicate logic in attach_addr() Cosmin Tanislav
2025-05-07 12:19 ` [PATCH v5 6/9] i2c: atr: allow replacing mappings " Cosmin Tanislav
2025-05-21 15:56 ` Luca Ceresoli
2025-05-07 12:19 ` [PATCH v5 7/9] i2c: atr: add flags parameter to i2c_atr_new() Cosmin Tanislav
2025-05-22 8:50 ` Wolfram Sang
2025-05-22 10:10 ` Luca Ceresoli
2025-05-07 12:19 ` [PATCH v5 8/9] i2c: atr: add static flag Cosmin Tanislav
2025-05-21 15:56 ` Luca Ceresoli
2025-05-22 8:58 ` Wolfram Sang
2025-05-07 12:19 ` [PATCH v5 9/9] i2c: atr: add passthrough flag Cosmin Tanislav
2025-05-22 10:12 ` [PATCH v5 0/9] i2c: atr: allow usage of nested ATRs Wolfram Sang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox