Message-ID: <141bbac1-5289-4335-a566-387721439bef@ideasonboard.com>
Date: Fri, 29 Nov 2024 11:54:35 +0200
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Romain Gantois <romain.gantois@...tlin.com>
Cc: Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
 Kory Maincent <kory.maincent@...tlin.com>, linux-i2c@...r.kernel.org,
 linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
 linux-media@...r.kernel.org, linux-gpio@...r.kernel.org,
 Wolfram Sang <wsa+renesas@...g-engineering.com>,
 Luca Ceresoli <luca.ceresoli@...tlin.com>, Andi Shyti
 <andi.shyti@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Derek Kiernan <derek.kiernan@....com>,
 Dragan Cvetic <dragan.cvetic@....com>, Arnd Bergmann <arnd@...db.de>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Mauro Carvalho Chehab <mchehab@...nel.org>,
 Linus Walleij <linus.walleij@...aro.org>, Bartosz Golaszewski
 <brgl@...ev.pl>, Cosmin Tanislav <demonsingur@...il.com>
Subject: Re: [PATCH v3 8/9] i2c: Support dynamic address translation

Hi Romain,

On 25/11/2024 10:45, Romain Gantois wrote:
> The i2c-atr module keeps a list of associations between I2C client aliases
> and I2C addresses. This represents the address translation table which is
> programmed into an ATR channel at any given time. This list is only updated
> when a new client is bound to the channel.
> 
> However in some cases, an ATR channel can have more downstream clients than
> available aliases. One example of this is an SFP module that is bound to an
> FPC202 port. The FPC202 port can only access up to two logical I2C
> addresses. However, the SFP module may expose up to three logical I2C
> addresses: its EEPROM on 7-bit addresses 0x50 and 0x51, and a PHY
> transceiver on address 0x56.
> 
> In cases like these, it is necessary to reconfigure the channel's
> translation table on the fly, so that all three I2C addresses can be
> accessed when needed.
> 
> Add an optional "dynamic_c2a" flag to the i2c-atr module which allows it to
> provide on-the-fly address translation. This is achieved by modifying an
> ATR channel's translation table whenever an I2C transaction with unmapped
> clients is requested.
> 
> Add a mutex to protect alias_list. This prevents
> i2c_atr_dynamic_attach/detach_addr from racing with the bus notifier
> handler to modify alias_list.
> 
> Signed-off-by: Romain Gantois <romain.gantois@...tlin.com>
> ---
>   drivers/i2c/i2c-atr.c         | 244 ++++++++++++++++++++++++++++++++----------
>   drivers/media/i2c/ds90ub960.c |   2 +-
>   include/linux/i2c-atr.h       |  13 ++-
>   3 files changed, 202 insertions(+), 57 deletions(-)

This fails with:

WARNING: CPU: 1 PID: 360 at lib/list_debug.c:35 
__list_add_valid_or_report+0xe4/0x100

as the i2c_atr_create_c2a() calls list_add(), but i2c_atr_attach_addr(), 
which is changed to use i2c_atr_create_c2a(), also calls list_add().

Also, if you add i2c_atr_create_c2a() which hides the allocation and 
list_add, I think it makes sense to add a i2c_atr_destroy_c2a() to 
revert that.

There's also a memory error "BUG: KASAN: slab-use-after-free in 
__lock_acquire+0xc4/0x375c" (see below) when unloading the ub960 or 
ub953 driver. I haven't looked at that yet.

  Tomi

   316.204193] BUG: KASAN: slab-use-after-free in __lock_acquire+0xc4/0x375c
[  316.211059] Read of size 4 at addr cc44d558 by task rmmod/1505
[  316.216979]
[  316.218475] CPU: 1 UID: 0 PID: 1505 Comm: rmmod Not tainted 6.12.0+ #2
[  316.225097] Hardware name: Generic DRA74X (Flattened Device Tree)
[  316.231231] Call trace:
[  316.231262]  unwind_backtrace from show_stack+0x18/0x1c
[  316.239105]  show_stack from dump_stack_lvl+0x6c/0x8c
[  316.244232]  dump_stack_lvl from print_report+0x130/0x538
[  316.249694]  print_report from kasan_report+0x98/0xd8
[  316.254821]  kasan_report from __lock_acquire+0xc4/0x375c
[  316.260284]  __lock_acquire from lock_acquire.part.0+0x128/0x340
[  316.266357]  lock_acquire.part.0 from _raw_spin_lock+0x4c/0x5c
[  316.272247]  _raw_spin_lock from i2c_atr_release_alias+0x20/0x98 
[i2c_atr]
[  316.279235]  i2c_atr_release_alias [i2c_atr] from 
i2c_atr_bus_notifier_call+0x150/0x46c [i2c_atr]
[  316.288238]  i2c_atr_bus_notifier_call [i2c_atr] from 
notifier_call_chain+0x68/0x23c
[  316.296081]  notifier_call_chain from 
blocking_notifier_call_chain+0x5c/0x74
[  316.303192]  blocking_notifier_call_chain from bus_notify+0x3c/0x48
[  316.309539]  bus_notify from device_del+0xf0/0x514
[  316.314392]  device_del from device_unregister+0x28/0x80
[  316.319763]  device_unregister from __unregister_client+0x84/0x90
[  316.325927]  __unregister_client from device_for_each_child+0xc0/0x12c
[  316.332550]  device_for_each_child from i2c_del_adapter+0x28c/0x45c
[  316.338897]  i2c_del_adapter from i2c_atr_del_adapter+0x10c/0x250 
[i2c_atr]
[  316.345947]  i2c_atr_del_adapter [i2c_atr] from 
ub953_remove+0x48/0xcc [ds90ub953]
[  316.353637]  ub953_remove [ds90ub953] from i2c_device_remove+0x54/0xf8
[  316.360260]  i2c_device_remove from 
device_release_driver_internal+0x218/0x2a8
[  316.367553]  device_release_driver_internal from 
bus_remove_device+0x130/0x1cc
[  316.374847]  bus_remove_device from device_del+0x1f8/0x514
[  316.380401]  device_del from device_unregister+0x28/0x80
[  316.385772]  device_unregister from ub960_remove+0xb0/0x244 [ds90ub960]
[  316.392517]  ub960_remove [ds90ub960] from i2c_device_remove+0x54/0xf8
[  316.399139]  i2c_device_remove from 
device_release_driver_internal+0x218/0x2a8
[  316.406433]  device_release_driver_internal from driver_detach+0x6c/0xc4
[  316.413238]  driver_detach from bus_remove_driver+0xa0/0x134
[  316.418945]  bus_remove_driver from i2c_del_driver+0x48/0x84
[  316.424682]  i2c_del_driver from sys_delete_module+0x280/0x3cc
[  316.430572]  sys_delete_module from ret_fast_syscall+0x0/0x1c

Powered by blists - more mailing lists