Date: Mon, 24 Jun 2024 07:54:32 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Krzysztof Olędzki <ole@....pl>,
 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 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.

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

Powered by blists - more mailing lists