mbox series

[v4,0/9] i2c: atr: allow usage of nested ATRs

Message ID 20250428102516.933571-1-demonsingur@gmail.com
Headers show
Series i2c: atr: allow usage of nested ATRs | expand

Message

Cosmin Tanislav April 28, 2025, 10:25 a.m. UTC 
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/aAii_iawJdptQyCt@stanley.mountain

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

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

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         | 185 ++++++++++++++++++++++------------
 drivers/media/i2c/ds90ub960.c |   2 +-
 drivers/misc/ti_fpc202.c      |   2 +-
 include/linux/i2c-atr.h       |  15 ++-
 4 files changed, 136 insertions(+), 68 deletions(-)

Comments

Luca Ceresoli April 30, 2025, 2:36 p.m. UTC | #1 
On Mon, 28 Apr 2025 13:25:14 +0300
Cosmin Tanislav <demonsingur@gmail.com> 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>

Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
Romain Gantois May 5, 2025, 3:40 p.m. UTC | #2 
On Monday, 28 April 2025 12:25:10 CEST 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 42f433846f63..bf7b2ac5e9cf 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;

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