Re: Regression caused by "eeprom: at24: Probe for DDR3 thermal sensor in the SPD case" - "sysfs: cannot create duplicate filename"

[Heiner Kallweit <hkallweit1@gmail.com>]

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
>