On 24.06.2024 16:54, Guenter Roeck wrote: > On 6/24/24 01:38, Krzysztof Olędzki wrote: >> On 23.06.2024 at 22:33, Guenter Roeck wrote: >>> On 6/23/24 11:47, Krzysztof Olędzki wrote: >>>> Hi, >>>> >>>> After upgrading kernel to Linux 6.6.34 on one of my systems, I noticed "sysfs: cannot create duplicate filename" and i2c registration errors in dmesg, please see below. >>>> >>>> This seems to be related to https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=linux-6.6.y&id=4d5ace787273cb159bfdcf1c523df957938b3e42 - reverting the change fixes the problem. >>>> >>>> Note that jc42 devices are registered correctly and work with and without the change. >>>> >>> >>> My guess is that the devices are fist instantiated through the jc42 >>> driver's _detect function and then again from the at24 driver. >>> The at24 driver should possibly call i2c_new_scanned_device() instead >>> of i2c_new_client_device() to only instantiate the device if it wasn't >>> already instantiated. >> >> i2c_new_scanned_device() also calls i2c_default_probe() at the end (unless >> different probe is provided) which seems risky given the comment that explains >> that it would use quick write for that address. However, maybe it is safe in this case? >> I wish we had a way to just tell "no probing is needed". >> > > Sorry, I don't understand why it would be less risky to just probe the device > without such a test. > >> We also know the exact address so no scanning is needed. >> >> Perhaps it would be better to just call i2c_check_addr_busy() in >> at24_probe_temp_sensor()? >> >> Something like this: >> --- a/drivers/misc/eeprom/at24.c 2024-06-24 09:16:11.251855130 +0200 >> +++ b/drivers/misc/eeprom/at24.c 2024-06-24 09:27:01.158170725 +0200 >> @@ -603,6 +603,10 @@ >> info.addr = 0x18 | (client->addr & 7); >> + /* The device may be already instantiated through the jc42 driver */ >> + if (i2c_check_addr_busy(client->adapter, info.addr)) >> + return; >> + >> i2c_new_client_device(client->adapter, &info); >> } >> >> Unfortunately, i2c_check_addr_busy is not exported and declared as static, > > That is why I did not suggest that. > >> I assume intentionally? Unless this can be changed, we are back to the original >> recommendation: >> >> --- a/drivers/misc/eeprom/at24.c 2024-06-24 09:16:11.251855130 +0200 >> +++ b/drivers/misc/eeprom/at24.c 2024-06-24 10:25:39.142567472 +0200 >> @@ -585,6 +585,7 @@ >> { >> struct at24_data *at24 = i2c_get_clientdata(client); >> struct i2c_board_info info = { .type = "jc42" }; >> + unsigned short addr_list[] = { 0, I2C_CLIENT_END }; >> int ret; >> u8 val; >> @@ -601,9 +602,10 @@ >> if (ret || !(val & BIT(7))) >> return; >> - info.addr = 0x18 | (client->addr & 7); >> + addr_list[0] = 0x18 | (client->addr & 7); >> - i2c_new_client_device(client->adapter, &info); >> + /* The device may be already instantiated through the jc42 driver */ >> + i2c_new_scanned_device(client->adapter, &info, addr_list, NULL); >> } >> static int at24_probe(struct i2c_client *client) >> >> For now compile-tested only given the write-test concern above. >> > > The device detect code in the i2c core does that same write-test that you > are concerned about. > >> That said, I have some follow-up questions: >> >> 1. if the jc42 driver handles this already, I wonder what's the point of adding >> at24_probe_temp_sensor()? Is there a situation where it would not do it properly? >> Or do we expect to remove the probing functionally from jc42.c? >> > > The jc42 driver is not auto-loaded. When suggesting to remove the "probing > functionally", I assume you mean to remove its detect function. That would only > work if SPD EEPROMs were only connected to I2C adapters calling i2c_register_spd(), > and if the systems with those adapters would support DMI. > > In v6.9, i2c_register_spd() is only called from the i801 driver (Intel systems). > In v6.11, piix4 (AMD) will be added. Even after that, all non-Intel / non-AMD systems > would no longer be able to support jc42 compatible chips by just loading the jc42 > driver. That would not be acceptable. > >> 2. I don't understand why we are also getting the "Failed creating jc42" and >> "sysfs: cannot create duplicate filename" errors since i2c_new_client_device() calls >> i2c_check_addr_busy() on its own and should abort after the first error message? >> > > The "Failed creating" message is from the i2c core's detect function which > is only called if a new i2c adapter is added. This is actually the case here, > since the call sequence of the backtrace includes i801_probe(). It looks like > i2c_detect() runs asynchronously and doesn't protect itself against having > devices added to a bus while it is running on that same bus. That is just > a guess, though - I have not tried to verify it. > Too me the issue also looks like a race. According to the OP's logs: - jc42 at 0x18 is instantiated successfully - jc42 at 0x19 returns -EBUSY. This is what is expected if the device has been instantiated otherwise already. - jc42 at 0x1a returns -EEXIST. Here two instantiations of the the same device seem to collide. - jc42 at 0x1b returns -EBUSY, like at 0x19. So it looks like referenced change isn't wrong, but reveals an underlying issue with device instantiation races. I'll have a look how this could be fixed. > That does suggest, though, that even your suggested code above might not > completely fix the problem. It may be necessary to call i2c_lock_bus() > or similar from i2c_new_scanned_device() and i2c_detect(), but I don't know > if that is save, sufficient, or even possible. > >> 3. (unrelated but found while looking at the code) The comment for >> delete_device_store() seems to be outdated as it mentions i2c_sysfs_new_device >> which does not exist any longer, as it was renamed in >> "i2c: core: Use DEVICE_ATTR_*() helper macros" back in 2019: >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/i2c/i2c-core-base.c?id=54a19fd4a6402ef47fce5c3a5374c71f52373c40 - >> >> For the Greg's question if it is also in 6.9: I have not tested that kernel yet, >> but unless there have been some recent changes in the i2c code I would expect >> it should behave the same way. If required, I should be able to do this next week. >> > Agreed. > > Guenter >
