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

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