Message-ID: <ee1996fa-4cdd-4897-b7fa-800cc9863599@ans.pl>
Date: Thu, 27 Jun 2024 05:12:17 -0700
From: Krzysztof Olędzki <ole@....pl>
To: Guenter Roeck <linux@...ck-us.net>,
        Heiner Kallweit
 <hkallweit1@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Bartosz Golaszewski <brgl@...ev.pl>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Wolfram Sang <wsa@...-dreams.de>
Cc: stable@...r.kernel.org, linux-i2c@...r.kernel.org,
        linux-hwmon@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: Regression caused by "eeprom: at24: Probe for DDR3 thermal sensor
 in the SPD case" - "sysfs: cannot create duplicate filename"

On 27.06.2024 at 04:29, Krzysztof Olędzki wrote:
> On 24.06.2024 at 20:45, Guenter Roeck wrote:
>> On 6/24/24 13:58, Heiner Kallweit wrote:
>> [ ... ]
>>>
>>> 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.
>>
>> It isn't just a race, though. Try to unload the at24 (or ee1004 driver
>> for DDR4) and load it again, and you'll see the -EBUSY errors. Problem
>> is that instantiating those drivers _always_ triggers the call to
>> i2c_new_client_device() even if the jc42 device is already instantiated.
>> Unloading the spd/eeprom driver doesn't unload the jc42 driver,
>> so -EBUSY will be seen if the spd/eeprom driver is loaded again.
>>
>> I have not been able to reproduce the backtrace with my systems, but those
>> are all with AMD CPUs using the piix4 driver, so timing is likely different.
>> Another difference is that my systems (with DDR4) use the ee1004 driver.
>> That driver instantiates the jc42 devices under a driver lock, so it is
>> guaranteed that a single instantiation doesn't interfere with other
>> instantiations running in parallel.
> 
> Right, sorry for not mentioning this in the original report:
> 
> [    0.269013] pci 0000:00:1f.3: [8086:1c22] type 00 class 0x0c0500
> [    0.269098] pci 0000:00:1f.3: reg 0x10: [mem 0xc3a02000-0xc3a020ff 64bit]
> [    0.269186] pci 0000:00:1f.3: reg 0x20: [io  0x3000-0x301f]
> [    0.334962] pci 0000:00:1f.3: Adding to iommu group 7
> [    7.874736] i801_smbus 0000:00:1f.3: SMBus using PCI interrupt
> 
> $ lspci -s 0000:00:1f.3 -vvnn
> 00:1f.3 SMBus [0c05]: Intel Corporation 6 Series/C200 Series Chipset Family SMBus Controller [8086:1c22] (rev 04)
>         Subsystem: Dell Device [1028:04de]
>         Control: I/O+ Mem+ BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx-
>         Status: Cap- 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
>         Interrupt: pin C routed to IRQ 19
>         IOMMU group: 7
>         Region 0: Memory at c3a02000 (64-bit, non-prefetchable) [size=256]
>         Region 4: I/O ports at 3000 [size=32]
>         Kernel driver in use: i801_smbus

Also, here is a different trace showing a different code path, which even more suggest a race:

[    7.871973] i2c_dev: i2c /dev entries driver
[    7.878215] i2c i2c-12: 4/4 memory slots populated (from DMI)
[    7.881116] at24 12-0050: 256 byte spd EEPROM, read-only
[    7.881887] i2c i2c-12: Successfully instantiated SPD at 0x50
[    7.894183] at24 12-0051: 256 byte spd EEPROM, read-only
[    7.895910] i2c i2c-12: Failed to register i2c client jc42 at 0x19 (-16)
[    7.896039] i2c i2c-12: Successfully instantiated SPD at 0x51
[    7.896850] i2c i2c-12: Failed creating jc42 at 0x19
[    7.903444] sysfs: cannot create duplicate filename '/devices/pci0000:00/0000:00:1f.3/i2c-12/12-001a'
[    7.904085] at24 12-0052: 256 byte spd EEPROM, read-only
[    7.905284] Hardware name: Dell Inc. PowerEdge T110 II/0PM2CW, BIOS 2.10.0 05/24/2018
[    7.905284] Call Trace:
[    7.905284]  <TASK>
[    7.909238]  dump_stack_lvl+0x37/0x4a
[    7.910488] at24 12-0053: 256 byte spd EEPROM, read-only
[    7.909855]  sysfs_warn_dup+0x55/0x61
[    7.911456] i2c i2c-12: Successfully instantiated SPD at 0x53
[    7.911597]  sysfs_create_dir_ns+0xa6/0xd2
[    7.911597]  kobject_add_internal+0xc3/0x1c0
[    7.914606]  kobject_add+0xba/0xe4
[    7.915595]  ? device_add+0x53/0x726
[    7.915595]  device_add+0x132/0x726
[    7.916622]  i2c_new_client_device+0x1ee/0x246
[    7.916622]  i2c_detect.isra.0+0x17c/0x223
[    7.918603]  ? __pfx___process_new_driver+0x10/0x10
[    7.919603]  __process_new_driver+0x17/0x1e
[    7.919603]  bus_for_each_dev+0x8b/0xcf
[    7.920595]  ? __pfx___process_new_driver+0x10/0x10
[    7.920595]  i2c_for_each_dev+0x2d/0x49
[    7.922608]  i2c_register_driver+0x51/0x63
[    7.922608]  ? __pfx_jc42_driver_init+0x10/0x10
[    7.923595]  do_one_initcall+0x93/0x182
[    7.924601]  kernel_init_freeable+0x1be/0x204
[    7.924601]  ? __pfx_kernel_init+0x10/0x10
[    7.924601]  kernel_init+0x15/0x110
[    7.926609]  ret_from_fork+0x23/0x35
[    7.927602]  ? __pfx_kernel_init+0x10/0x10
[    7.927602]  ret_from_fork_asm+0x1b/0x30
[    7.927602]  </TASK>
[    7.929937] kobject: kobject_add_internal failed for 12-001a with -EEXIST, don't try to register things with the same name in the same directory.
[    7.932129] i2c i2c-12: Failed to register i2c client jc42 at 0x1a (-17)
[    7.933257] i2c i2c-12: Failed creating jc42 at 0x1a

Note there is no warning for 0x18 and 0x1b.

# sensors|grep jc42-i2c|sort
jc42-i2c-12-18
jc42-i2c-12-19
jc42-i2c-12-1a
jc42-i2c-12-1b


Krzysztof

Powered by blists - more mailing lists