[PATCH v2] i2c: core: Lock address during client device instantiation

[PATCH v2] i2c: core: Lock address during client device instantiation

[Heiner Kallweit]

Krzysztof reported an issue [0] which is caused by parallel attempts to
instantiate the same I2C client device. This can happen if driver
supports auto-detection, but certain devices are also instantiated
explicitly.
The original change isn't actually wrong, it just revealed that I2C core
isn't prepared yet to handle this scenario.
Calls to i2c_new_client_device() can be nested, therefore we can't use a
simple mutex here. Parallel instantiation of devices at different addresses
is ok, so we just have to prevent parallel instantiation at the same address.
We can use a bitmap with one bit per 7-bit I2C client address, and atomic
bit operations to set/check/clear bits.
Now a parallel attempt to instantiate a device at the same address will
result in -EBUSY being returned, avoiding the "sysfs: cannot create duplicate
filename" splash.

Note: This patch version includes small cosmetic changes to the Tested-by
      version, only functional change is that address locking is supported
      for slave addresses too.

[0] https://lore.kernel.org/linux-i2c/9479fe4e-eb0c-407e-84c0-bd60c15baf74@ans.pl/T/#m12706546e8e2414d8f1a0dc61c53393f731685cc

Fixes: caba40ec3531 ("eeprom: at24: Probe for DDR3 thermal sensor in the SPD case")
Cc: stable@vger.kernel.org
Tested-by: Krzysztof Piotr Oledzki <ole@ans.pl>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- fixed comment style
- use address locking for slave addresses too
---
 drivers/i2c/i2c-core-base.c | 28 ++++++++++++++++++++++++++++
 include/linux/i2c.h         |  3 +++
 2 files changed, 31 insertions(+)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index b63f75e44..e39e8d792 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -915,6 +915,27 @@ int i2c_dev_irq_from_resources(const struct resource *resources,
 	return 0;
 }
 
+/*
+ * Serialize device instantiation in case it can be instantiated explicitly
+ * and by auto-detection
+ */
+static int i2c_lock_addr(struct i2c_adapter *adap, unsigned short addr,
+			 unsigned short flags)
+{
+	if (!(flags & I2C_CLIENT_TEN) &&
+	    test_and_set_bit(addr, adap->addrs_in_instantiation))
+		return -EBUSY;
+
+	return 0;
+}
+
+static void i2c_unlock_addr(struct i2c_adapter *adap, unsigned short addr,
+			    unsigned short flags)
+{
+	if (!(flags & I2C_CLIENT_TEN))
+		clear_bit(addr, adap->addrs_in_instantiation);
+}
+
 /**
  * i2c_new_client_device - instantiate an i2c device
  * @adap: the adapter managing the device
@@ -962,6 +983,10 @@ i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *inf
 		goto out_err_silent;
 	}
 
+	status = i2c_lock_addr(adap, client->addr, client->flags);
+	if (status)
+		goto out_err_silent;
+
 	/* Check for address business */
 	status = i2c_check_addr_busy(adap, i2c_encode_flags_to_addr(client));
 	if (status)
@@ -993,6 +1018,8 @@ i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *inf
 	dev_dbg(&adap->dev, "client [%s] registered with bus id %s\n",
 		client->name, dev_name(&client->dev));
 
+	i2c_unlock_addr(adap, client->addr, client->flags);
+
 	return client;
 
 out_remove_swnode:
@@ -1004,6 +1031,7 @@ i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *inf
 	dev_err(&adap->dev,
 		"Failed to register i2c client %s at 0x%02x (%d)\n",
 		client->name, client->addr, status);
+	i2c_unlock_addr(adap, client->addr, client->flags);
 out_err_silent:
 	if (need_put)
 		put_device(&client->dev);
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 07e33bbc9..e555d0cf8 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -761,6 +761,9 @@ struct i2c_adapter {
 	struct regulator *bus_regulator;
 
 	struct dentry *debugfs;
+
+	/* 7bit address space */
+	DECLARE_BITMAP(addrs_in_instantiation, 1 << 7);
 };
 #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)
 
-- 
2.46.0

Re: [PATCH v2] i2c: core: Lock address during client device instantiation

[Dmitry Torokhov]

Hi Heiner,

On Thu, Aug 15, 2024 at 09:44:50PM +0200, Heiner Kallweit wrote:
>  
> +/*
> + * Serialize device instantiation in case it can be instantiated explicitly
> + * and by auto-detection
> + */
> +static int i2c_lock_addr(struct i2c_adapter *adap, unsigned short addr,
> +			 unsigned short flags)
> +{
> +	if (!(flags & I2C_CLIENT_TEN) &&
> +	    test_and_set_bit(addr, adap->addrs_in_instantiation))
> +		return -EBUSY;

Why don't you add a list of client devices to the adapter structure
instead of using bitmap? Then you could handle cases with 10 bit
addresses as well.

I know that there is already a list of children in the driver core, but
it is populated too late for what we need.

Thanks.

-- 
Dmitry

Re: [PATCH v2] i2c: core: Lock address during client device instantiation

[Heiner Kallweit]

On 16.08.2024 01:10, Dmitry Torokhov wrote:
> Hi Heiner,
> 
> On Thu, Aug 15, 2024 at 09:44:50PM +0200, Heiner Kallweit wrote:
>>  
>> +/*
>> + * Serialize device instantiation in case it can be instantiated explicitly
>> + * and by auto-detection
>> + */
>> +static int i2c_lock_addr(struct i2c_adapter *adap, unsigned short addr,
>> +			 unsigned short flags)
>> +{
>> +	if (!(flags & I2C_CLIENT_TEN) &&
>> +	    test_and_set_bit(addr, adap->addrs_in_instantiation))
>> +		return -EBUSY;
> 
> Why don't you add a list of client devices to the adapter structure
> instead of using bitmap? Then you could handle cases with 10 bit
> addresses as well.
> 
I think this question in the same as asked by Wolfram: whether a linked list
would be better suited. It would require more complexity to deal with it than
the bitmap. And we could use the bitmap also with 10bit addresses, then the
bitmap would have 128 bytes. It's an acceptable tradeoff to exclude (very rare)
10 bit addresses from the check.

> I know that there is already a list of children in the driver core, but
> it is populated too late for what we need.
> 
> Thanks.
> 

Re: [PATCH v2] i2c: core: Lock address during client device instantiation

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1541 bytes --]

On Thu, Aug 15, 2024 at 09:44:50PM +0200, Heiner Kallweit wrote:
> Krzysztof reported an issue [0] which is caused by parallel attempts to
> instantiate the same I2C client device. This can happen if driver
> supports auto-detection, but certain devices are also instantiated
> explicitly.
> The original change isn't actually wrong, it just revealed that I2C core
> isn't prepared yet to handle this scenario.
> Calls to i2c_new_client_device() can be nested, therefore we can't use a
> simple mutex here. Parallel instantiation of devices at different addresses
> is ok, so we just have to prevent parallel instantiation at the same address.
> We can use a bitmap with one bit per 7-bit I2C client address, and atomic
> bit operations to set/check/clear bits.
> Now a parallel attempt to instantiate a device at the same address will
> result in -EBUSY being returned, avoiding the "sysfs: cannot create duplicate
> filename" splash.
> 
> Note: This patch version includes small cosmetic changes to the Tested-by
>       version, only functional change is that address locking is supported
>       for slave addresses too.
> 
> [0] https://lore.kernel.org/linux-i2c/9479fe4e-eb0c-407e-84c0-bd60c15baf74@ans.pl/T/#m12706546e8e2414d8f1a0dc61c53393f731685cc
> 
> Fixes: caba40ec3531 ("eeprom: at24: Probe for DDR3 thermal sensor in the SPD case")
> Cc: stable@vger.kernel.org
> Tested-by: Krzysztof Piotr Oledzki <ole@ans.pl>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]