[PATCH v2 0/3] i2c: atr: A few i2c-atr fixes

[PATCH v2 0/3] i2c: atr: A few i2c-atr fixes

[Tomi Valkeinen]

The last two are perhaps not strictly fixes, as they essentially add
support for nested ATRs. The first one is a fix, thus stable is in cc.

 Tomi

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
Changes in v2:
- Use mutext_init_with_key()
- Rearrange the series so that the fix is the first patch
- Link to v1: https://lore.kernel.org/r/20241122-i2c-atr-fixes-v1-0-62c51ce790be@ideasonboard.com

---
Cosmin Tanislav (1):
      i2c: atr: Allow unmapped addresses from nested ATRs

Tomi Valkeinen (2):
      i2c: atr: Fix client detach
      i2c: atr: Fix lockdep for nested ATRs

 drivers/i2c/i2c-atr.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)
---
base-commit: adc218676eef25575469234709c2d87185ca223a
change-id: 20241121-i2c-atr-fixes-6d52b9f5f0c1

Best regards,
-- 
Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

[PATCH v2 1/3] i2c: atr: Fix client detach

[Tomi Valkeinen]

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

i2c-atr catches the BUS_NOTIFY_DEL_DEVICE event on the bus and removes
the translation by calling i2c_atr_detach_client().

However, BUS_NOTIFY_DEL_DEVICE happens when the device is about to be
removed from this bus, i.e. before removal, and thus before calling
.remove() on the driver. If the driver happens to do any i2c
transactions in its remove(), they will fail.

Fix this by catching BUS_NOTIFY_REMOVED_DEVICE instead, thus removing
the translation only after the device is actually removed.

Fixes: a076a860acae ("media: i2c: add I2C Address Translator (ATR) support")
Cc: stable@vger.kernel.org
Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
---
 drivers/i2c/i2c-atr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
index f21475ae5921..0d54d0b5e327 100644
--- a/drivers/i2c/i2c-atr.c
+++ b/drivers/i2c/i2c-atr.c
@@ -412,7 +412,7 @@ static int i2c_atr_bus_notifier_call(struct notifier_block *nb,
 				dev_name(dev), ret);
 		break;
 
-	case BUS_NOTIFY_DEL_DEVICE:
+	case BUS_NOTIFY_REMOVED_DEVICE:
 		i2c_atr_detach_client(client->adapter, client);
 		break;
 

-- 
2.43.0

Re: [PATCH v2 1/3] i2c: atr: Fix client detach

[Andy Shevchenko]

On Fri, Nov 22, 2024 at 02:26:18PM +0200, Tomi Valkeinen wrote:
> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>

We (used to?) have a check in Linux Next against missing SoB of the committer,
wouldn't this trap into it?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/3] i2c: atr: Fix client detach

[Tomi Valkeinen]

Hi,

On 22/11/2024 16:07, Andy Shevchenko wrote:
> On Fri, Nov 22, 2024 at 02:26:18PM +0200, Tomi Valkeinen wrote:
>> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> 
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> 
> We (used to?) have a check in Linux Next against missing SoB of the committer,
> wouldn't this trap into it?

I don't think linux-next can check that. Or rather, it can, but the 
committer there (which is the subsystem maintainer, probably) is not 
related to the sender of this mail, so I don't think linux-next can 
complain about this particular issue.

I didn't right away figure out how to easily change the From address for 
the email with the standard tooling. As this is such a clear case of the 
sender and the author being the same person, I'll just ignore this point 
for now (although strictly speaking I think you're right).

  Tomi

Re: [PATCH v2 1/3] i2c: atr: Fix client detach

[Luca Ceresoli]

Hello Tomi,

+Cc: Romain who is doing a different kind of sorcery on i2c-atr.c, so
he is aware of this series.

On Fri, 22 Nov 2024 14:26:18 +0200
Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote:

> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> 
> i2c-atr catches the BUS_NOTIFY_DEL_DEVICE event on the bus and removes
> the translation by calling i2c_atr_detach_client().
> 
> However, BUS_NOTIFY_DEL_DEVICE happens when the device is about to be
> removed from this bus, i.e. before removal, and thus before calling
> .remove() on the driver. If the driver happens to do any i2c
> transactions in its remove(), they will fail.
> 
> Fix this by catching BUS_NOTIFY_REMOVED_DEVICE instead, thus removing
> the translation only after the device is actually removed.
> 
> Fixes: a076a860acae ("media: i2c: add I2C Address Translator (ATR) support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>

Looks good:
Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 1/3] i2c: atr: Fix client detach

[Tomi Valkeinen]

Hi Romain,

On 26/11/2024 10:14, Luca Ceresoli wrote:
> Hello Tomi,
> 
> +Cc: Romain who is doing a different kind of sorcery on i2c-atr.c, so
> he is aware of this series.
> 
> On Fri, 22 Nov 2024 14:26:18 +0200
> Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote:
> 
>> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>>
>> i2c-atr catches the BUS_NOTIFY_DEL_DEVICE event on the bus and removes
>> the translation by calling i2c_atr_detach_client().
>>
>> However, BUS_NOTIFY_DEL_DEVICE happens when the device is about to be
>> removed from this bus, i.e. before removal, and thus before calling
>> .remove() on the driver. If the driver happens to do any i2c
>> transactions in its remove(), they will fail.
>>
>> Fix this by catching BUS_NOTIFY_REMOVED_DEVICE instead, thus removing
>> the translation only after the device is actually removed.
>>
>> Fixes: a076a860acae ("media: i2c: add I2C Address Translator (ATR) support")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> 
> Looks good:
> Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 

Can you test this one with your setup, and give your Rb/Tb?

I think it's an obvious fix, and could be merged separately from the 
rest, which still need discussion.

  Tomi

Re: [PATCH v2 1/3] i2c: atr: Fix client detach

[Romain Gantois]

On vendredi 22 novembre 2024 13:26:18 heure normale d’Europe centrale Tomi 
Valkeinen wrote:
> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> 
> i2c-atr catches the BUS_NOTIFY_DEL_DEVICE event on the bus and removes
> the translation by calling i2c_atr_detach_client().
> 
> However, BUS_NOTIFY_DEL_DEVICE happens when the device is about to be
> removed from this bus, i.e. before removal, and thus before calling
> .remove() on the driver. If the driver happens to do any i2c
> transactions in its remove(), they will fail.
> 
> Fix this by catching BUS_NOTIFY_REMOVED_DEVICE instead, thus removing
> the translation only after the device is actually removed.
> 
> Fixes: a076a860acae ("media: i2c: add I2C Address Translator (ATR) support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> ---
>  drivers/i2c/i2c-atr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
> index f21475ae5921..0d54d0b5e327 100644
> --- a/drivers/i2c/i2c-atr.c
> +++ b/drivers/i2c/i2c-atr.c
> @@ -412,7 +412,7 @@ static int i2c_atr_bus_notifier_call(struct
> notifier_block *nb, dev_name(dev), ret);
>  		break;
> 
> -	case BUS_NOTIFY_DEL_DEVICE:
> +	case BUS_NOTIFY_REMOVED_DEVICE:
>  		i2c_atr_detach_client(client->adapter, client);
>  		break;

LGTM, tested on a TI FPC202 ATR.

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

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

Re: [PATCH v2 1/3] i2c: atr: Fix client detach

[Tomi Valkeinen]

Hi Wolfram,

On 22/11/2024 14:26, Tomi Valkeinen wrote:
> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> 
> i2c-atr catches the BUS_NOTIFY_DEL_DEVICE event on the bus and removes
> the translation by calling i2c_atr_detach_client().
> 
> However, BUS_NOTIFY_DEL_DEVICE happens when the device is about to be
> removed from this bus, i.e. before removal, and thus before calling
> .remove() on the driver. If the driver happens to do any i2c
> transactions in its remove(), they will fail.
> 
> Fix this by catching BUS_NOTIFY_REMOVED_DEVICE instead, thus removing
> the translation only after the device is actually removed.
> 
> Fixes: a076a860acae ("media: i2c: add I2C Address Translator (ATR) support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> ---
>   drivers/i2c/i2c-atr.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
> index f21475ae5921..0d54d0b5e327 100644
> --- a/drivers/i2c/i2c-atr.c
> +++ b/drivers/i2c/i2c-atr.c
> @@ -412,7 +412,7 @@ static int i2c_atr_bus_notifier_call(struct notifier_block *nb,
>   				dev_name(dev), ret);
>   		break;
>   
> -	case BUS_NOTIFY_DEL_DEVICE:
> +	case BUS_NOTIFY_REMOVED_DEVICE:
>   		i2c_atr_detach_client(client->adapter, client);
>   		break;
>   
> 

Can you pick this one up (ignore the two other patches in this series), 
or should I send it as a separate patch?

  Tomi

Re: [PATCH v2 1/3] i2c: atr: Fix client detach

[Wolfram Sang]

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

On Fri, Nov 22, 2024 at 02:26:18PM +0200, Tomi Valkeinen wrote:
> From: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> 
> i2c-atr catches the BUS_NOTIFY_DEL_DEVICE event on the bus and removes
> the translation by calling i2c_atr_detach_client().
> 
> However, BUS_NOTIFY_DEL_DEVICE happens when the device is about to be
> removed from this bus, i.e. before removal, and thus before calling
> .remove() on the driver. If the driver happens to do any i2c
> transactions in its remove(), they will fail.
> 
> Fix this by catching BUS_NOTIFY_REMOVED_DEVICE instead, thus removing
> the translation only after the device is actually removed.
> 
> Fixes: a076a860acae ("media: i2c: add I2C Address Translator (ATR) support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>

Applied to for-current, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 2/3] i2c: atr: Allow unmapped addresses from nested ATRs

[Tomi Valkeinen]

From: Cosmin Tanislav <demonsingur@gmail.com>

i2c-atr translates the i2c transactions and forwards them to its parent
i2c bus. Any transaction to an i2c address that has not been mapped on
the i2c-atr will be rejected with an error.

However, if the parent i2c bus is another i2c-atr, the parent i2c-atr
gets a transaction to an i2c address that is not mapped in the parent
i2c-atr, and thus fails.

Relax the checks, and allow non-mapped transactions to fix this issue.

Signed-off-by: Cosmin Tanislav <demonsingur@gmail.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
---
 drivers/i2c/i2c-atr.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
index 0d54d0b5e327..5adb720af84e 100644
--- a/drivers/i2c/i2c-atr.c
+++ b/drivers/i2c/i2c-atr.c
@@ -134,7 +134,6 @@ i2c_atr_find_mapping_by_addr(const struct list_head *list, u16 phys_addr)
 static int i2c_atr_map_msgs(struct i2c_atr_chan *chan, struct i2c_msg *msgs,
 			    int num)
 {
-	struct i2c_atr *atr = chan->atr;
 	static struct i2c_atr_alias_pair *c2a;
 	int i;
 
@@ -157,15 +156,8 @@ static int i2c_atr_map_msgs(struct i2c_atr_chan *chan, struct i2c_msg *msgs,
 
 		c2a = i2c_atr_find_mapping_by_addr(&chan->alias_list,
 						   msgs[i].addr);
-		if (!c2a) {
-			dev_err(atr->dev, "client 0x%02x not mapped!\n",
-				msgs[i].addr);
-
-			while (i--)
-				msgs[i].addr = chan->orig_addrs[i];
-
-			return -ENXIO;
-		}
+		if (!c2a)
+			continue;
 
 		msgs[i].addr = c2a->alias;
 	}

-- 
2.43.0

Re: [PATCH v2 2/3] i2c: atr: Allow unmapped addresses from nested ATRs

[Andy Shevchenko]

On Fri, Nov 22, 2024 at 02:26:19PM +0200, Tomi Valkeinen wrote:

...

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>

SoB != committer.

(Yes, I know what + means in the email there, but I don't know
 if it's a problem for all those checks or not)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/3] i2c: atr: Allow unmapped addresses from nested ATRs

[Luca Ceresoli]

Hello Tomi,

On Fri, 22 Nov 2024 14:26:19 +0200
Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote:

> From: Cosmin Tanislav <demonsingur@gmail.com>
> 
> i2c-atr translates the i2c transactions and forwards them to its parent
> i2c bus. Any transaction to an i2c address that has not been mapped on
> the i2c-atr will be rejected with an error.
> 
> However, if the parent i2c bus is another i2c-atr, the parent i2c-atr
> gets a transaction to an i2c address that is not mapped in the parent
> i2c-atr, and thus fails.

Nested ATRs are "interesting", to say the least! :-)

I must say I don't understand the problem here. If this is the picture:

  adapter ---->     ATR1     ---->     ATR2     ----> leaf device
                    map:               map:              addr:
                 alias addr         alias addr           0x10
                 0x30  0x20         0x20  0x10

Then I'd expect this:

 1. the leaf device asks ATR2 for a transaction to 0x10
 2. 0x10 is in ATR2 map, ATR2 translates address 0x10 to 0x20
 3. ATR2 asks ATR1 for a transaction to 0x20
 4. 0x20 is in ATR1 map, ATR1 translates address 0x20 to 0x30
 5. ATR1 asks adapter for transaction on 0x30

So ATR1 is never asked for 0x10.

However I'm very likely missing something. Can you elaborate with a
practical example?

Best regards,
Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 2/3] i2c: atr: Allow unmapped addresses from nested ATRs

[Tomi Valkeinen]

Hi Luca,

On 26/11/2024 10:16, Luca Ceresoli wrote:
> Hello Tomi,
> 
> On Fri, 22 Nov 2024 14:26:19 +0200
> Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote:
> 
>> From: Cosmin Tanislav <demonsingur@gmail.com>
>>
>> i2c-atr translates the i2c transactions and forwards them to its parent
>> i2c bus. Any transaction to an i2c address that has not been mapped on
>> the i2c-atr will be rejected with an error.
>>
>> However, if the parent i2c bus is another i2c-atr, the parent i2c-atr
>> gets a transaction to an i2c address that is not mapped in the parent
>> i2c-atr, and thus fails.
> 
> Nested ATRs are "interesting", to say the least! :-)
> 
> I must say I don't understand the problem here. If this is the picture:
> 
>    adapter ---->     ATR1     ---->     ATR2     ----> leaf device
>                      map:               map:              addr:
>                   alias addr         alias addr           0x10
>                   0x30  0x20         0x20  0x10
> 
> Then I'd expect this:
> 
>   1. the leaf device asks ATR2 for a transaction to 0x10
>   2. 0x10 is in ATR2 map, ATR2 translates address 0x10 to 0x20
>   3. ATR2 asks ATR1 for a transaction to 0x20
>   4. 0x20 is in ATR1 map, ATR1 translates address 0x20 to 0x30
>   5. ATR1 asks adapter for transaction on 0x30
> 
> So ATR1 is never asked for 0x10.

Yes, that case would work. But in your example the ATR1 somehow has 
created a mapping for ATR2's alias.

Generally speaking, ATR1 has aliases only for devices in its master bus 
(i.e. the i2c bus where the ATR1 is the master, not slave), and 
similarly for ATR2. Thus I think a more realistic example is:

     adapter ---->     ATR1     ---->     ATR2     ----> leaf device
                    addr: 0x50         addr: 0x30
                       map:               map:              addr:
                    alias addr         alias addr           0x10
                    0x40  0x30         0x20  0x10

So, both ATRs create the alias mapping based on the i2c-aliases given to 
them in the DT, for the slave devices in their i2c bus. Assumption is, 
of course, that the aliases are not otherwise used, and not overlapping.

Thus the aliases on ATR2 are not present in the alias table of ATR1.

  Tomi

Re: [PATCH v2 2/3] i2c: atr: Allow unmapped addresses from nested ATRs

[Luca Ceresoli]

Hello Tomi,

On Tue, 26 Nov 2024 10:35:46 +0200
Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote:

> Hi Luca,
> 
> On 26/11/2024 10:16, Luca Ceresoli wrote:
> > Hello Tomi,
> > 
> > On Fri, 22 Nov 2024 14:26:19 +0200
> > Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote:
> >   
> >> From: Cosmin Tanislav <demonsingur@gmail.com>
> >>
> >> i2c-atr translates the i2c transactions and forwards them to its parent
> >> i2c bus. Any transaction to an i2c address that has not been mapped on
> >> the i2c-atr will be rejected with an error.
> >>
> >> However, if the parent i2c bus is another i2c-atr, the parent i2c-atr
> >> gets a transaction to an i2c address that is not mapped in the parent
> >> i2c-atr, and thus fails.  
> > 
> > Nested ATRs are "interesting", to say the least! :-)
> > 
> > I must say I don't understand the problem here. If this is the picture:
> > 
> >    adapter ---->     ATR1     ---->     ATR2     ----> leaf device
> >                      map:               map:              addr:
> >                   alias addr         alias addr           0x10
> >                   0x30  0x20         0x20  0x10
> > 
> > Then I'd expect this:
> > 
> >   1. the leaf device asks ATR2 for a transaction to 0x10
> >   2. 0x10 is in ATR2 map, ATR2 translates address 0x10 to 0x20
> >   3. ATR2 asks ATR1 for a transaction to 0x20
> >   4. 0x20 is in ATR1 map, ATR1 translates address 0x20 to 0x30
> >   5. ATR1 asks adapter for transaction on 0x30
> > 
> > So ATR1 is never asked for 0x10.  
> 
> Yes, that case would work. But in your example the ATR1 somehow has 
> created a mapping for ATR2's alias.

You're of course right. I had kind of assumed ATR1 is somehow
configured to map 0x30 on 0x20, but this is not going to happen
magically and there is no code AFAIK to do that. So of course my
comment is bogus, thanks for taking time to explain.

> Generally speaking, ATR1 has aliases only for devices in its master bus 
> (i.e. the i2c bus where the ATR1 is the master, not slave), and 
> similarly for ATR2. Thus I think a more realistic example is:
> 
>      adapter ---->     ATR1     ---->     ATR2     ----> leaf device
>                     addr: 0x50         addr: 0x30
>                        map:               map:              addr:
>                     alias addr         alias addr           0x10
>                     0x40  0x30         0x20  0x10
> 
> So, both ATRs create the alias mapping based on the i2c-aliases given to 
> them in the DT, for the slave devices in their i2c bus. Assumption is, 
> of course, that the aliases are not otherwise used, and not overlapping.
> 
> Thus the aliases on ATR2 are not present in the alias table of ATR1.

OK, so the above is what now I'd expect to be configured in the ATR
alias tables.

I still fail to understand how that would work. This is the actions I'd
expect:

  1. the leaf device asks ATR2 for a transaction to 0x10
  2. 0x10 is in ATR2 map, ATR2 translates address 0x10 to 0x20
  3. ATR2 asks ATR1 for a transaction to 0x20
  4. 0x20 is *not* in ATR1 map, *but* this patch is applied
      => i2c-atr lets the transaction through, unmodified
  5. ATR1 asks adapter for transaction on 0x20
  6. adapter sends transaction for 0x20 on wires
  7. ATR1 chip receives transaction for 0x20
      => 0x20 not in its tables, ignores it

Note steps 1-5 are in software (kernel). Step 7 may work if ATR1 were
configured to let all transactions for unknown addresses go through
unmodified, but I don't remember having seen patches to allow that in
i2c-atr.c and I'm not even sure the hardware allows that, the DS90UB9xx
at least.

And even in case that were possible, that would seems a bit fragile.
What if two child ATRs attached to two different ports of the parent
ATR use the same alias, and the parent ATR let transactions for such
alias go through both ports unmodified? Sure, the alias pools can be
carefully crafted to avoid such duplicated aliases, but pools have long
been considered a non-optimal solution, and they make no sense at all
in cases like the FPC202 that Romain is working to support.

Again, I'm pretty sure I'm missing something here. If you could
elaborate with a complete example, including the case of two child ATRs
attached to two ports of the same parent ATR, I'm sure that would be
very helpful.

At my current level of understanding, it looks like the only correct
way to manage nested ATRs is to add a "recursive alias mapping", i.e.
to add for each alias another alias to all parent ATRs, up to the top
one, like in my initial picture. I realize that would imply several
complications, though.

Best regards,
Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 2/3] i2c: atr: Allow unmapped addresses from nested ATRs

[Tomi Valkeinen]

Hi,

On 27/11/2024 14:19, Luca Ceresoli wrote:
> Hello Tomi,
> 
> On Tue, 26 Nov 2024 10:35:46 +0200
> Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote:
> 
>> Hi Luca,
>>
>> On 26/11/2024 10:16, Luca Ceresoli wrote:
>>> Hello Tomi,
>>>
>>> On Fri, 22 Nov 2024 14:26:19 +0200
>>> Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote:
>>>    
>>>> From: Cosmin Tanislav <demonsingur@gmail.com>
>>>>
>>>> i2c-atr translates the i2c transactions and forwards them to its parent
>>>> i2c bus. Any transaction to an i2c address that has not been mapped on
>>>> the i2c-atr will be rejected with an error.
>>>>
>>>> However, if the parent i2c bus is another i2c-atr, the parent i2c-atr
>>>> gets a transaction to an i2c address that is not mapped in the parent
>>>> i2c-atr, and thus fails.
>>>
>>> Nested ATRs are "interesting", to say the least! :-)
>>>
>>> I must say I don't understand the problem here. If this is the picture:
>>>
>>>     adapter ---->     ATR1     ---->     ATR2     ----> leaf device
>>>                       map:               map:              addr:
>>>                    alias addr         alias addr           0x10
>>>                    0x30  0x20         0x20  0x10
>>>
>>> Then I'd expect this:
>>>
>>>    1. the leaf device asks ATR2 for a transaction to 0x10
>>>    2. 0x10 is in ATR2 map, ATR2 translates address 0x10 to 0x20
>>>    3. ATR2 asks ATR1 for a transaction to 0x20
>>>    4. 0x20 is in ATR1 map, ATR1 translates address 0x20 to 0x30
>>>    5. ATR1 asks adapter for transaction on 0x30
>>>
>>> So ATR1 is never asked for 0x10.
>>
>> Yes, that case would work. But in your example the ATR1 somehow has
>> created a mapping for ATR2's alias.
> 
> You're of course right. I had kind of assumed ATR1 is somehow
> configured to map 0x30 on 0x20, but this is not going to happen
> magically and there is no code AFAIK to do that. So of course my
> comment is bogus, thanks for taking time to explain.
> 
>> Generally speaking, ATR1 has aliases only for devices in its master bus
>> (i.e. the i2c bus where the ATR1 is the master, not slave), and
>> similarly for ATR2. Thus I think a more realistic example is:
>>
>>       adapter ---->     ATR1     ---->     ATR2     ----> leaf device
>>                      addr: 0x50         addr: 0x30
>>                         map:               map:              addr:
>>                      alias addr         alias addr           0x10
>>                      0x40  0x30         0x20  0x10
>>
>> So, both ATRs create the alias mapping based on the i2c-aliases given to
>> them in the DT, for the slave devices in their i2c bus. Assumption is,
>> of course, that the aliases are not otherwise used, and not overlapping.
>>
>> Thus the aliases on ATR2 are not present in the alias table of ATR1.
> 
> OK, so the above is what now I'd expect to be configured in the ATR
> alias tables.
> 
> I still fail to understand how that would work. This is the actions I'd
> expect:
> 
>    1. the leaf device asks ATR2 for a transaction to 0x10
>    2. 0x10 is in ATR2 map, ATR2 translates address 0x10 to 0x20
>    3. ATR2 asks ATR1 for a transaction to 0x20
>    4. 0x20 is *not* in ATR1 map, *but* this patch is applied
>        => i2c-atr lets the transaction through, unmodified
>    5. ATR1 asks adapter for transaction on 0x20
>    6. adapter sends transaction for 0x20 on wires
>    7. ATR1 chip receives transaction for 0x20
>        => 0x20 not in its tables, ignores it
> 
> Note steps 1-5 are in software (kernel). Step 7 may work if ATR1 were
> configured to let all transactions for unknown addresses go through
> unmodified, but I don't remember having seen patches to allow that in
> i2c-atr.c and I'm not even sure the hardware allows that, the DS90UB9xx
> at least.

DS90UB9xx has I2C_PASS_THROUGH_ALL. However, our particular use case is 
with Maxim GMSL desers and sers. They're not as nice as the FPD-Link 
devices in this particular area.

Cosmin, feel free to elaborate or fix my mistakes, but here's a summary:

The deserializers don't have ATRs, whereas the serializers do (so vice 
versa compared to FPD-Link). The deserializers forward everything to all 
enabled GMSL ports. At probe/setup time we can enable a single link at a 
time, so that we can direct transactions to a specific serializer (or 
devices behind it), but after the setup, we need to keep all the ports 
enabled, as otherwise the video streams would stop for all the other 
ports except the one we want to send an i2c transaction to.

The serializers have their own i2c address, but transactions to anything 
else go through the ser's ATR. The ATR does the translation, if an entry 
exists in the table, but all transactions are forwarded, whether they 
are translated or not.

Where's the nested ATR, you ask? That's a detail which is a bit 
"interesting": all the serializers have a default i2c address. So we can 
have 4 serializers all replying to the same address. But we don't have 
an ATR at the deser. However, we can change the address of the 
serializer by writing to a serializer register. This must be done at the 
deser probe time (and before the ser driver probes) where we can enable 
just a single link at a time. So at probe time we change the addresses 
of the serializers to be distinct values.

Still no ATR, right? Well, the i2c-atr accomplishes the above quite 
nicely: there's an address pool (for the new ser addresses), 
.attach_client() where we can set the new address for the serializer, 
and .detach_client() where we can (optionally) restore the original 
address. This way the serializer driver will operate using the original 
address, but when it does an i2c transaction, the i2c-atr changes it to 
the new address.

So strictly speaking it's not an ATR, but this achieves the same.

> And even in case that were possible, that would seems a bit fragile.
> What if two child ATRs attached to two different ports of the parent
> ATR use the same alias, and the parent ATR let transactions for such
> alias go through both ports unmodified? Sure, the alias pools can be
> carefully crafted to avoid such duplicated aliases, but pools have long

Yes, the pools have to be non-overlapping and no overlap with anything 
on the main i2c bus.

I feel the GMSL HW requires quite strict design rules, and preferably 
the deser would be on an i2c bus alone. I think an eeprom at 0x10 and a 
remote sensor at 0x10 would cause trouble, without any way to deal with 
it in the SW.

> been considered a non-optimal solution, and they make no sense at all
> in cases like the FPC202 that Romain is working to support.
> 
> Again, I'm pretty sure I'm missing something here. If you could
> elaborate with a complete example, including the case of two child ATRs
> attached to two ports of the same parent ATR, I'm sure that would be
> very helpful.

I hope my text above covered this.

> At my current level of understanding, it looks like the only correct
> way to manage nested ATRs is to add a "recursive alias mapping", i.e.
> to add for each alias another alias to all parent ATRs, up to the top
> one, like in my initial picture. I realize that would imply several
> complications, though.

Yes, that has complications too. Say, if we have a device that has an 
ATR but also passes everything through (like the GMSL ser), then the 
driver has to manage two different kinds of aliases: one set for the 
actual ATR, and one that are just pass-through ones.

Or what if we have i2c-atr - i2c-mux - i2c-atr chain. The system would 
need to skip the i2c-mux, and continue to the first i2c-atr.

Probably both are solvable, though, but it doesn't sound simple.

  Tomi

Re: [PATCH v2 2/3] i2c: atr: Allow unmapped addresses from nested ATRs

[Luca Ceresoli]

Hi Tomi,

+Cc Matti

On Thu, 28 Nov 2024 19:50:46 +0200
Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote:

> Hi,
> 
> On 27/11/2024 14:19, Luca Ceresoli wrote:
> > Hello Tomi,
> > 
> > On Tue, 26 Nov 2024 10:35:46 +0200
> > Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote:
> >   
> >> Hi Luca,
> >>
> >> On 26/11/2024 10:16, Luca Ceresoli wrote:  
> >>> Hello Tomi,
> >>>
> >>> On Fri, 22 Nov 2024 14:26:19 +0200
> >>> Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote:
> >>>      
> >>>> From: Cosmin Tanislav <demonsingur@gmail.com>
> >>>>
> >>>> i2c-atr translates the i2c transactions and forwards them to its parent
> >>>> i2c bus. Any transaction to an i2c address that has not been mapped on
> >>>> the i2c-atr will be rejected with an error.
> >>>>
> >>>> However, if the parent i2c bus is another i2c-atr, the parent i2c-atr
> >>>> gets a transaction to an i2c address that is not mapped in the parent
> >>>> i2c-atr, and thus fails.  
> >>>
> >>> Nested ATRs are "interesting", to say the least! :-)
> >>>
> >>> I must say I don't understand the problem here. If this is the picture:
> >>>
> >>>     adapter ---->     ATR1     ---->     ATR2     ----> leaf device
> >>>                       map:               map:              addr:
> >>>                    alias addr         alias addr           0x10
> >>>                    0x30  0x20         0x20  0x10
> >>>
> >>> Then I'd expect this:
> >>>
> >>>    1. the leaf device asks ATR2 for a transaction to 0x10
> >>>    2. 0x10 is in ATR2 map, ATR2 translates address 0x10 to 0x20
> >>>    3. ATR2 asks ATR1 for a transaction to 0x20
> >>>    4. 0x20 is in ATR1 map, ATR1 translates address 0x20 to 0x30
> >>>    5. ATR1 asks adapter for transaction on 0x30
> >>>
> >>> So ATR1 is never asked for 0x10.  
> >>
> >> Yes, that case would work. But in your example the ATR1 somehow has
> >> created a mapping for ATR2's alias.  
> > 
> > You're of course right. I had kind of assumed ATR1 is somehow
> > configured to map 0x30 on 0x20, but this is not going to happen
> > magically and there is no code AFAIK to do that. So of course my
> > comment is bogus, thanks for taking time to explain.
> >   
> >> Generally speaking, ATR1 has aliases only for devices in its master bus
> >> (i.e. the i2c bus where the ATR1 is the master, not slave), and
> >> similarly for ATR2. Thus I think a more realistic example is:
> >>
> >>       adapter ---->     ATR1     ---->     ATR2     ----> leaf device
> >>                      addr: 0x50         addr: 0x30
> >>                         map:               map:              addr:
> >>                      alias addr         alias addr           0x10
> >>                      0x40  0x30         0x20  0x10
> >>
> >> So, both ATRs create the alias mapping based on the i2c-aliases given to
> >> them in the DT, for the slave devices in their i2c bus. Assumption is,
> >> of course, that the aliases are not otherwise used, and not overlapping.
> >>
> >> Thus the aliases on ATR2 are not present in the alias table of ATR1.  
> > 
> > OK, so the above is what now I'd expect to be configured in the ATR
> > alias tables.
> > 
> > I still fail to understand how that would work. This is the actions I'd
> > expect:
> > 
> >    1. the leaf device asks ATR2 for a transaction to 0x10
> >    2. 0x10 is in ATR2 map, ATR2 translates address 0x10 to 0x20
> >    3. ATR2 asks ATR1 for a transaction to 0x20
> >    4. 0x20 is *not* in ATR1 map, *but* this patch is applied  
> >        => i2c-atr lets the transaction through, unmodified  
> >    5. ATR1 asks adapter for transaction on 0x20
> >    6. adapter sends transaction for 0x20 on wires
> >    7. ATR1 chip receives transaction for 0x20  
> >        => 0x20 not in its tables, ignores it  
> > 
> > Note steps 1-5 are in software (kernel). Step 7 may work if ATR1 were
> > configured to let all transactions for unknown addresses go through
> > unmodified, but I don't remember having seen patches to allow that in
> > i2c-atr.c and I'm not even sure the hardware allows that, the DS90UB9xx
> > at least.  
> 
> DS90UB9xx has I2C_PASS_THROUGH_ALL. However, our particular use case is 
> with Maxim GMSL desers and sers. They're not as nice as the FPD-Link 
> devices in this particular area.
> 
> Cosmin, feel free to elaborate or fix my mistakes, but here's a summary:
> 
> The deserializers don't have ATRs, whereas the serializers do (so vice 
> versa compared to FPD-Link). The deserializers forward everything to all 
> enabled GMSL ports. At probe/setup time we can enable a single link at a 
> time, so that we can direct transactions to a specific serializer (or 
> devices behind it), but after the setup, we need to keep all the ports 
> enabled, as otherwise the video streams would stop for all the other 
> ports except the one we want to send an i2c transaction to.
> 
> The serializers have their own i2c address, but transactions to anything 
> else go through the ser's ATR. The ATR does the translation, if an entry 
> exists in the table, but all transactions are forwarded, whether they 
> are translated or not.
> 
> Where's the nested ATR, you ask? That's a detail which is a bit 
> "interesting": all the serializers have a default i2c address. So we can 
> have 4 serializers all replying to the same address. But we don't have 
> an ATR at the deser. However, we can change the address of the 
> serializer by writing to a serializer register. This must be done at the 
> deser probe time (and before the ser driver probes) where we can enable 
> just a single link at a time. So at probe time we change the addresses 
> of the serializers to be distinct values.
> 
> Still no ATR, right? Well, the i2c-atr accomplishes the above quite 
> nicely: there's an address pool (for the new ser addresses), 
> .attach_client() where we can set the new address for the serializer, 
> and .detach_client() where we can (optionally) restore the original 
> address. This way the serializer driver will operate using the original 
> address, but when it does an i2c transaction, the i2c-atr changes it to 
> the new address.
> 
> So strictly speaking it's not an ATR, but this achieves the same.

Thanks for the extensive and very useful explanation. I had completely
missed the GMSL serder and their different I2C handling, apologies.

So, the "parent ATR" is the GMSL deser, which is not an ATR but
implementing it using i2c-atr makes the implementation cleaner. That
makes sense.

However removing the checks in i2c_atr_map_msgs() is dangerous in the
general case with "proper" ATRs (the TI ones and AFAIK the Rohm ones)
and it conflicts with the FPC202 case as Romain pointed out.

So I think we need those checks to be disabled only when in the the
"nested ATR" scenario, which leads to Romain's suggestion of adding a
flag when instantiating the ATR, so I'll switch to that branch of this
discussion.

> > And even in case that were possible, that would seems a bit fragile.
> > What if two child ATRs attached to two different ports of the parent
> > ATR use the same alias, and the parent ATR let transactions for such
> > alias go through both ports unmodified? Sure, the alias pools can be
> > carefully crafted to avoid such duplicated aliases, but pools have long  
> 
> Yes, the pools have to be non-overlapping and no overlap with anything 
> on the main i2c bus.
> 
> I feel the GMSL HW requires quite strict design rules, and preferably 
> the deser would be on an i2c bus alone. I think an eeprom at 0x10 and a 
> remote sensor at 0x10 would cause trouble, without any way to deal with 
> it in the SW.
> 
> > been considered a non-optimal solution, and they make no sense at all
> > in cases like the FPC202 that Romain is working to support.
> > 
> > Again, I'm pretty sure I'm missing something here. If you could
> > elaborate with a complete example, including the case of two child ATRs
> > attached to two ports of the same parent ATR, I'm sure that would be
> > very helpful.  
> 
> I hope my text above covered this.
> 
> > At my current level of understanding, it looks like the only correct
> > way to manage nested ATRs is to add a "recursive alias mapping", i.e.
> > to add for each alias another alias to all parent ATRs, up to the top
> > one, like in my initial picture. I realize that would imply several
> > complications, though.  
> 
> Yes, that has complications too. Say, if we have a device that has an 
> ATR but also passes everything through (like the GMSL ser), then the 
> driver has to manage two different kinds of aliases: one set for the 
> actual ATR, and one that are just pass-through ones.

I agree this won't make sense for the GMSL case you described.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 2/3] i2c: atr: Allow unmapped addresses from nested ATRs

[Tomi Valkeinen]

On 29/11/2024 13:53, Luca Ceresoli wrote:

>> So strictly speaking it's not an ATR, but this achieves the same.
> 
> Thanks for the extensive and very useful explanation. I had completely
> missed the GMSL serder and their different I2C handling, apologies.
> 
> So, the "parent ATR" is the GMSL deser, which is not an ATR but
> implementing it using i2c-atr makes the implementation cleaner. That
> makes sense.

Right.

But, honestly, I can't make my mind if I like the use of ATR here or not =).

So it's not an ATR, but I'm not quite sure what it is. It's not just 
that we need to change the addresses of the serializers, we need to do 
that in particular way, enabling one port at a time to do the change.

If we forget about the init time hurdles, and consider the situation 
after the serializers are been set up and all ports have been enabled, 
we have:

There's the main i2c bus, on which we have the deserializer. The 
deserializer acts as a i2c repeater (for any transaction that's not 
directed to the deser), sending the messages to all serializers. The 
serializers catch transactions directed at the ser, and everything else 
goes through ATR and to the remote bus.

Do we have something that represents such a "i2c repeater"? I guess we 
could just have an i2c bus, created by the deser, and all the sers would 
be on that bus. So we'd somehow do the initial address change first, 
then set up the i2c bus, and the serializer i2c clients would be added 
to that bus.

  Tomi

Re: [PATCH v2 2/3] i2c: atr: Allow unmapped addresses from nested ATRs

[Luca Ceresoli]

Hello Tomi,

On Fri, 29 Nov 2024 15:31:45 +0200
Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote:

> On 29/11/2024 13:53, Luca Ceresoli wrote:
> 
> >> So strictly speaking it's not an ATR, but this achieves the same.  
> > 
> > Thanks for the extensive and very useful explanation. I had completely
> > missed the GMSL serder and their different I2C handling, apologies.
> > 
> > So, the "parent ATR" is the GMSL deser, which is not an ATR but
> > implementing it using i2c-atr makes the implementation cleaner. That
> > makes sense.  
> 
> Right.
> 
> But, honestly, I can't make my mind if I like the use of ATR here or not =).

Hehe, indeed, hardware designers use a lot of fantasy in stretching the
I2C standard to its limits, perhaps more than actually needed.

> So it's not an ATR, but I'm not quite sure what it is. It's not just 
> that we need to change the addresses of the serializers, we need to do 
> that in particular way, enabling one port at a time to do the change.
> 
> If we forget about the init time hurdles, and consider the situation 
> after the serializers are been set up and all ports have been enabled, 
> we have:
> 
> There's the main i2c bus, on which we have the deserializer. The 
> deserializer acts as a i2c repeater (for any transaction that's not 
> directed to the deser), sending the messages to all serializers. The 
> serializers catch transactions directed at the ser, and everything else 
> goes through ATR and to the remote bus.
> 
> Do we have something that represents such a "i2c repeater"? I guess we 
> could just have an i2c bus, created by the deser, and all the sers would 
> be on that bus. So we'd somehow do the initial address change first, 
> then set up the i2c bus, and the serializer i2c clients would be added 
> to that bus.

So you think about another thing, like i2c-repeater, in addition to
i2c-mux and i2c-atr?

Well, I think it would make sense, as it would generalize a feature
that might be used by other chips. However at the moment we do have a
working driver for the GMSL deser, and so I don't see the benefit of
extracting the i2c-repeater functionality to a separate file, unless
there are drivers for other chips being implemented: this would motivate
extracting common features to a shared file. IOW, I'd not generalize
something with a single user.

[Interesting side note: the i2c-atr has been implemented with a single
user, violating the above principle O:-) but I think that was due to the
similarity with i2c-mux or something like that. Out of luck, another
ATR user appeared after some time.]

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 2/3] i2c: atr: Allow unmapped addresses from nested ATRs

[Cosmin Tanislav]

On 12/3/24 11:39 AM, Luca Ceresoli wrote:
> Hello Tomi,
> 
> On Fri, 29 Nov 2024 15:31:45 +0200
> Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote:
> 
>> On 29/11/2024 13:53, Luca Ceresoli wrote:
>>
>>>> So strictly speaking it's not an ATR, but this achieves the same.
>>>
>>> Thanks for the extensive and very useful explanation. I had completely
>>> missed the GMSL serder and their different I2C handling, apologies.
>>>
>>> So, the "parent ATR" is the GMSL deser, which is not an ATR but
>>> implementing it using i2c-atr makes the implementation cleaner. That
>>> makes sense.
>>
>> Right.
>>
>> But, honestly, I can't make my mind if I like the use of ATR here or not =).
> 
> Hehe, indeed, hardware designers use a lot of fantasy in stretching the
> I2C standard to its limits, perhaps more than actually needed.
> 
>> So it's not an ATR, but I'm not quite sure what it is. It's not just
>> that we need to change the addresses of the serializers, we need to do
>> that in particular way, enabling one port at a time to do the change.
>>
>> If we forget about the init time hurdles, and consider the situation
>> after the serializers are been set up and all ports have been enabled,
>> we have:
>>
>> There's the main i2c bus, on which we have the deserializer. The
>> deserializer acts as a i2c repeater (for any transaction that's not
>> directed to the deser), sending the messages to all serializers. The
>> serializers catch transactions directed at the ser, and everything else
>> goes through ATR and to the remote bus.
>>
>> Do we have something that represents such a "i2c repeater"? I guess we
>> could just have an i2c bus, created by the deser, and all the sers would
>> be on that bus. So we'd somehow do the initial address change first,
>> then set up the i2c bus, and the serializer i2c clients would be added
>> to that bus.
> 
> So you think about another thing, like i2c-repeater, in addition to
> i2c-mux and i2c-atr?
> 

Since most of the functionality needed (besides allowing pass-through
transfers for unmapped I2C addresses) can be achieved already using I2C
ATR, I think we should make use of it.

> Well, I think it would make sense, as it would generalize a feature
> that might be used by other chips. However at the moment we do have a
> working driver for the GMSL deser, and so I don't see the benefit of
> extracting the i2c-repeater functionality to a separate file, unless
> there are drivers for other chips being implemented: this would motivate
> extracting common features to a shared file. IOW, I'd not generalize
> something with a single user.
> 

We have GMSL drivers for 6 new chips that make use of the I2C ATR, and
we want to send these upstream. Adding pass-through support for the I2C
ATR is one of the only things keeping us back, and it's the solution
that makes the most sense to me.

Semantically, indeed, our GMSL deserializers don't have an ATR hardware
block. The serializers do, with the particularity that they pass through
all traffic, even if it is for unmapped addresses.

The current GMSL2 deserializer driver doesn't make use of ATR
functionality because it's a single link deserializer, so it doesn't
need to handle multiple serializers with the same I2C address.

The GMSL deserializers we want to add have more than one link (either
2 or 4), and they need the ATR to reassign the serializer I2C addresses
to ones that can be addresses without conflicts. The address changing
is done in the ATR attach_client() callback.

The ATR driver already exists and allows us to implement this, even if
semantically there's no translation block.

> [Interesting side note: the i2c-atr has been implemented with a single
> user, violating the above principle O:-) but I think that was due to the
> similarity with i2c-mux or something like that. Out of luck, another
> ATR user appeared after some time.]
> 
> Luca
> 

Re: [PATCH v2 2/3] i2c: atr: Allow unmapped addresses from nested ATRs

[Luca Ceresoli]

Hello Cosmin, Tomi,

On Tue, 3 Dec 2024 12:35:29 +0200
Cosmin Tanislav <demonsingur@gmail.com> wrote:

> On 12/3/24 11:39 AM, Luca Ceresoli wrote:
> > Hello Tomi,
> > 
> > On Fri, 29 Nov 2024 15:31:45 +0200
> > Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote:
> >   
> >> On 29/11/2024 13:53, Luca Ceresoli wrote:
> >>  
> >>>> So strictly speaking it's not an ATR, but this achieves the same.  
> >>>
> >>> Thanks for the extensive and very useful explanation. I had completely
> >>> missed the GMSL serder and their different I2C handling, apologies.
> >>>
> >>> So, the "parent ATR" is the GMSL deser, which is not an ATR but
> >>> implementing it using i2c-atr makes the implementation cleaner. That
> >>> makes sense.  
> >>
> >> Right.
> >>
> >> But, honestly, I can't make my mind if I like the use of ATR here or not =).  
> > 
> > Hehe, indeed, hardware designers use a lot of fantasy in stretching the
> > I2C standard to its limits, perhaps more than actually needed.
> >   
> >> So it's not an ATR, but I'm not quite sure what it is. It's not just
> >> that we need to change the addresses of the serializers, we need to do
> >> that in particular way, enabling one port at a time to do the change.
> >>
> >> If we forget about the init time hurdles, and consider the situation
> >> after the serializers are been set up and all ports have been enabled,
> >> we have:
> >>
> >> There's the main i2c bus, on which we have the deserializer. The
> >> deserializer acts as a i2c repeater (for any transaction that's not
> >> directed to the deser), sending the messages to all serializers. The
> >> serializers catch transactions directed at the ser, and everything else
> >> goes through ATR and to the remote bus.
> >>
> >> Do we have something that represents such a "i2c repeater"? I guess we
> >> could just have an i2c bus, created by the deser, and all the sers would
> >> be on that bus. So we'd somehow do the initial address change first,
> >> then set up the i2c bus, and the serializer i2c clients would be added
> >> to that bus.  
> > 
> > So you think about another thing, like i2c-repeater, in addition to
> > i2c-mux and i2c-atr?
> >   
> 
> Since most of the functionality needed (besides allowing pass-through
> transfers for unmapped I2C addresses) can be achieved already using I2C
> ATR, I think we should make use of it.

If it allows code reuse, then it makes sense.

> > Well, I think it would make sense, as it would generalize a feature
> > that might be used by other chips. However at the moment we do have a
> > working driver for the GMSL deser, and so I don't see the benefit of
> > extracting the i2c-repeater functionality to a separate file, unless
> > there are drivers for other chips being implemented: this would motivate
> > extracting common features to a shared file. IOW, I'd not generalize
> > something with a single user.
> >   
> 
> We have GMSL drivers for 6 new chips that make use of the I2C ATR, and
> we want to send these upstream. Adding pass-through support for the I2C
> ATR is one of the only things keeping us back, and it's the solution
> that makes the most sense to me.

I see, so I understand the aim of this patch. As it was presented
initially, in a "fixes" series and without any mention to new GMSL
drivers, it didn't make sense to me. I think it would be ideal to send
it in the same series as the GMSL driver(s) using it, otherwise there
is no reason to want this change. Also it should not be presented as a
fix, because it is not: it is changing the ATR functionality in order
to extend it to new chips. Finally, documenting the oddities (the deser
being implemented using i2c-atr even though it is not an ATR, in the
first place) wouldbe very useful in the commit message and/or cover
letter.

Best regards,
Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 2/3] i2c: atr: Allow unmapped addresses from nested ATRs

[Romain Gantois]

Hello Tomi,

On vendredi 22 novembre 2024 13:26:19 heure normale d’Europe centrale Tomi Valkeinen wrote:
> From: Cosmin Tanislav <demonsingur@gmail.com>
> 
> i2c-atr translates the i2c transactions and forwards them to its parent
> i2c bus. Any transaction to an i2c address that has not been mapped on
> the i2c-atr will be rejected with an error.
> 
> However, if the parent i2c bus is another i2c-atr, the parent i2c-atr
> gets a transaction to an i2c address that is not mapped in the parent
> i2c-atr, and thus fails.
> 
> Relax the checks, and allow non-mapped transactions to fix this issue.

I have a series in the review pipeline which adds optional support for dynamic 
I2C ATR address translation. If this option is enabled, then transactions on any
unmapped address are assigned an alias on-the-fly. This feature is required to
handle alias shortages on some hardware setups:

https://lore.kernel.org/all/20241125-fpc202-v3-0-34e86bcb5b56@bootlin.com/

Letting all non-mapped transactions through would conflict with my series, since
these two scenarios would be indistinguishable:

case 1:
transaction with 3 messages is requested, msg1 -> 0x50; msg2 -> 0x51; msg3 -> 0x56
alias pool can only hold 2 mappings at a time, so transaction cannot go through

case 2:
transaction with 3 messages is requested, msg1 -> 0x50; msg2 -> 0x51; msg3 -> 0x50
alias pool can hold 2 mappings at a time, so transaction can go through

Could you perhaps introduce an ATR flag which would enable/disable letting all unmapped
messages through? I have something similar in patch 8 of my FPC202 series:

https://lore.kernel.org/all/20241125-fpc202-v3-8-34e86bcb5b56@bootlin.com/

There could be a flag named "I2C_ATR_FLAG_NESTED_ATR", which would be enabled in i2c_atr_new():

```
@@ i2c_atr_new()

if (is_an_atr(parent))
	atr->flags |= I2C_ATR_FLAG_NESTED_ATR;
```

Please let me know what you think.

Best Regards,

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

Re: [PATCH v2 2/3] i2c: atr: Allow unmapped addresses from nested ATRs

[Luca Ceresoli]

Hello Tomi, Romain,

+Cc Matti

On Tue, 26 Nov 2024 09:34:56 +0100
Romain Gantois <romain.gantois@bootlin.com> wrote:

> Hello Tomi,
> 
> On vendredi 22 novembre 2024 13:26:19 heure normale d’Europe centrale Tomi Valkeinen wrote:
> > From: Cosmin Tanislav <demonsingur@gmail.com>
> > 
> > i2c-atr translates the i2c transactions and forwards them to its parent
> > i2c bus. Any transaction to an i2c address that has not been mapped on
> > the i2c-atr will be rejected with an error.
> > 
> > However, if the parent i2c bus is another i2c-atr, the parent i2c-atr
> > gets a transaction to an i2c address that is not mapped in the parent
> > i2c-atr, and thus fails.
> > 
> > Relax the checks, and allow non-mapped transactions to fix this issue.  
> 
> I have a series in the review pipeline which adds optional support for dynamic 
> I2C ATR address translation. If this option is enabled, then transactions on any
> unmapped address are assigned an alias on-the-fly. This feature is required to
> handle alias shortages on some hardware setups:
> 
> https://lore.kernel.org/all/20241125-fpc202-v3-0-34e86bcb5b56@bootlin.com/
> 
> Letting all non-mapped transactions through would conflict with my series, since
> these two scenarios would be indistinguishable:
> 
> case 1:
> transaction with 3 messages is requested, msg1 -> 0x50; msg2 -> 0x51; msg3 -> 0x56
> alias pool can only hold 2 mappings at a time, so transaction cannot go through
> 
> case 2:
> transaction with 3 messages is requested, msg1 -> 0x50; msg2 -> 0x51; msg3 -> 0x50
> alias pool can hold 2 mappings at a time, so transaction can go through
> 
> Could you perhaps introduce an ATR flag which would enable/disable letting all unmapped
> messages through? I have something similar in patch 8 of my FPC202 series:
> 
> https://lore.kernel.org/all/20241125-fpc202-v3-8-34e86bcb5b56@bootlin.com/
> 
> There could be a flag named "I2C_ATR_FLAG_NESTED_ATR", which would be enabled in i2c_atr_new():
> 
> ```
> @@ i2c_atr_new()
> 
> if (is_an_atr(parent))
> 	atr->flags |= I2C_ATR_FLAG_NESTED_ATR;
> ```

As I discussed on another branch of this thread, after Tomi's
explanations I think the flag is the correct solution because disabling
the checks in i2c_atr_map_msgs() is useful for a specific
implementation of the GMSL deserializer, but armful is other cases.

About the implementation, I think we should not use something like
is_an_atr(parent). First, it would potentially need to recursively
climb the parent tree, and handling all combinations of
ats/mux/whatever would be very complex. But even more because it is not
needed. Tomi explained the use case is for the GMSL deser being the
"parent ATR" (even though it is not physically an ATR) and the GMSL
serializer the "child ATR", and the change in this patch is only needed
for the parent ATR AFAICU. So the GMSL deser driver could
unconditionally set the flag, and no other driver should ever set it.
Tomi, do you think this is correct?

Based on the above,  i2c_atr_new() or other parts of the i2c-atr code
are unable to self-detect whether the flag should be set or not. That
would mean we have a new user for the 'flags' field of i2c_atr_new()
that Romain proposed [0].

Finally, I think the name should not mention "nested ATR" but rather
tell what it does, like I2C_ATR_FLAG_PASS_THROUGH.

So, that's my random thoughts, what do you think about this?

[0] https://lore.kernel.org/linux-i2c/20241125-fpc202-v3-8-34e86bcb5b56@bootlin.com/

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

[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>
---
 drivers/i2c/i2c-atr.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/i2c-atr.c b/drivers/i2c/i2c-atr.c
index 5adb720af84e..5cc18606bca9 100644
--- a/drivers/i2c/i2c-atr.c
+++ b/drivers/i2c/i2c-atr.c
@@ -54,6 +54,7 @@ struct i2c_atr_chan {
 
 	/* 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;
 };
@@ -84,6 +85,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;
 
 	size_t num_aliases;
@@ -505,7 +507,9 @@ 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);
+
 	spin_lock_init(&atr->alias_mask_lock);
 
 	atr->parent = parent;
@@ -535,6 +539,7 @@ struct i2c_atr *i2c_atr_new(struct i2c_adapter *parent, struct device *dev,
 	kfree(atr->aliases);
 err_destroy_mutex:
 	mutex_destroy(&atr->lock);
+	lockdep_unregister_key(&atr->lock_key);
 	kfree(atr);
 
 	return ERR_PTR(ret);
@@ -552,6 +557,7 @@ void i2c_atr_delete(struct i2c_atr *atr)
 	bitmap_free(atr->alias_use_mask);
 	kfree(atr->aliases);
 	mutex_destroy(&atr->lock);
+	lockdep_unregister_key(&atr->lock_key);
 	kfree(atr);
 }
 EXPORT_SYMBOL_NS_GPL(i2c_atr_delete, I2C_ATR);
@@ -586,7 +592,8 @@ int i2c_atr_add_adapter(struct i2c_atr *atr, u32 chan_id,
 	chan->atr = atr;
 	chan->chan_id = chan_id;
 	INIT_LIST_HEAD(&chan->alias_list);
-	mutex_init(&chan->orig_addrs_lock);
+	lockdep_register_key(&chan->orig_addrs_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);
@@ -646,6 +653,7 @@ int i2c_atr_add_adapter(struct i2c_atr *atr, u32 chan_id,
 err_fwnode_put:
 	fwnode_handle_put(dev_fwnode(&chan->adap.dev));
 	mutex_destroy(&chan->orig_addrs_lock);
+	lockdep_unregister_key(&chan->orig_addrs_lock_key);
 	kfree(chan);
 	return ret;
 }
@@ -679,6 +687,7 @@ void i2c_atr_del_adapter(struct i2c_atr *atr, u32 chan_id)
 
 	fwnode_handle_put(fwnode);
 	mutex_destroy(&chan->orig_addrs_lock);
+	lockdep_unregister_key(&chan->orig_addrs_lock_key);
 	kfree(chan->orig_addrs);
 	kfree(chan);
 }

-- 
2.43.0

Re: [PATCH v2 0/3] i2c: atr: A few i2c-atr fixes

[Andy Shevchenko]

On Fri, Nov 22, 2024 at 02:26:17PM +0200, Tomi Valkeinen wrote:
> The last two are perhaps not strictly fixes, as they essentially add
> support for nested ATRs. The first one is a fix, thus stable is in cc.

Other than SoB chain issues, code wise LGTM,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/3] i2c: atr: A few i2c-atr fixes

[Tomi Valkeinen]

Hi Andy,

On 22/11/2024 16:13, Andy Shevchenko wrote:
> On Fri, Nov 22, 2024 at 02:26:17PM +0200, Tomi Valkeinen wrote:
>> The last two are perhaps not strictly fixes, as they essentially add
>> support for nested ATRs. The first one is a fix, thus stable is in cc.
> 
> Other than SoB chain issues, code wise LGTM,
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks. I'll try to figure that out. Looks like even if the committer in 
my git tree is +renesas, b4 send still sends them with my normal email 
address.

It's not hard to add both SoBs, but... it feels a bit silly.

  Tomi