| Message ID | 32a2d535-d7c8-47da-a42f-b41d3fae972f@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] i2c: core: Lock address during client device instantiation | expand |
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.
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. >
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!
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)
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(+)