| Message ID | 3b1964fa-56fd-464f-93d3-98d46c70b872@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: core: Lock address during client device instantiation | expand |
Hi Heiner, thanks for tackling this! > +static int i2c_lock_addr(struct i2c_adapter *adap, unsigned short addr, > + unsigned short flags) What about just using 'struct i2c_client *client' here as an argument. It has all we need and it currently seems unlikely that we need to call it from somewhere else where we need this seperation. > + if (!(flags & I2C_CLIENT_TEN) && !(flags & I2C_CLIENT_SLAVE) && From a pedantic point of view, I don't see a reason for not handling those two cases above. I hate to be pedantic because 10-bit mode is practically unused (and I am tempted to remove support for it once in a while because it makes other solutions clumsy). And the other one is super unlikely to happen because the backends do not autoload. However, it is theoretically possible if someone loads a devicetree overlay and initiates via sysfs at the same time. I liked the solution with the bitfield and atomic access, but maybe a linked list is better? Happy hacking, Wolfram
On 14.08.2024 13:14, Wolfram Sang wrote: > Hi Heiner, > > thanks for tackling this! > >> +static int i2c_lock_addr(struct i2c_adapter *adap, unsigned short addr, >> + unsigned short flags) > > What about just using 'struct i2c_client *client' here as an argument. > It has all we need and it currently seems unlikely that we need to call > it from somewhere else where we need this seperation. > I did it this way in the RFC version. Passing addr and flags as separate arguments is intentional because the current patch is good enough to prevent the reported issue, but there's still a small chance of races. Not necessarily as a fix, but as an improvement we may call i2c_lock_addr() in i2c_detect_address() and i2c_new_scanned_device(), before the call to i2c_check_addr_busy(). Apart from requiring an unlocked version of i2c_new_client_device(), this would include calls where separate arguments are better. >> + if (!(flags & I2C_CLIENT_TEN) && !(flags & I2C_CLIENT_SLAVE) && > > From a pedantic point of view, I don't see a reason for not handling > those two cases above. I hate to be pedantic because 10-bit mode is I considered just the case of parallel auto-detection. But right, we can also have the case that explicit kernel space instantiation and instantiation from user space race. > practically unused (and I am tempted to remove support for it once in a > while because it makes other solutions clumsy). And the other one is > super unlikely to happen because the backends do not autoload. However, > it is theoretically possible if someone loads a devicetree overlay and > initiates via sysfs at the same time. I liked the solution with the > bitfield and atomic access, but maybe a linked list is better? > Well, the answer may depend on the definition of better. With a linked list we may be able to save a few byte in the i2c_adapter struct, but the code would become more complex. We need a mutex to protect list operations, and we have to dynamically allocate list elements consisting of at least the address and a list_head. Last but not least the check whether an address is locked would become more complex and expensive. Even for 10bit addresses the bitmap would have just 128 bytes. A single list_head has 16bytes (on 64 bit systems), for a mutex it depends on the kernel config, etc. So we don't gain much with a linked list. Not being a politician, but being open for compromises: We could include slave addresses in the address locking, but still exclude 10bit addresses. Then the bitmap has only 16 bytes. > Happy hacking, > > Wolfram > Heiner
Hello Heiner, > i2c_check_addr_busy(). Apart from requiring an unlocked version of > i2c_new_client_device(), this would include calls where separate arguments > are better. OK. If that is already on the horizon, then we can leave it. > We could include slave addresses in the address locking, but still > exclude 10bit addresses. Then the bitmap has only 16 bytes. That sounds feasible to me! Let's do it this way. Thanks, Wolfram
Hi Heiner, kernel test robot noticed the following build warnings: [auto build test WARNING on wsa/i2c/for-next] [also build test WARNING on linus/master v6.11-rc3 next-20240814] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Heiner-Kallweit/i2c-core-Lock-address-during-client-device-instantiation/20240814-223311 base: https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next patch link: https://lore.kernel.org/r/3b1964fa-56fd-464f-93d3-98d46c70b872%40gmail.com patch subject: [PATCH] i2c: core: Lock address during client device instantiation config: x86_64-buildonly-randconfig-001-20240815 (https://download.01.org/0day-ci/archive/20240815/202408151708.WYTNiDnN-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240815/202408151708.WYTNiDnN-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202408151708.WYTNiDnN-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/i2c/i2c-core-base.c:919: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst * Serialize device instantiation in case it can be instantiated explicitly vim +919 drivers/i2c/i2c-core-base.c 917 918 /** > 919 * Serialize device instantiation in case it can be instantiated explicitly 920 * and by auto-detection 921 */ 922 static int i2c_lock_addr(struct i2c_adapter *adap, unsigned short addr, 923 unsigned short flags) 924 { 925 if (!(flags & I2C_CLIENT_TEN) && !(flags & I2C_CLIENT_SLAVE) && 926 test_and_set_bit(addr, adap->addrs_in_instantiation)) 927 return -EBUSY; 928 929 return 0; 930 } 931
On 14.08.2024 13:14, Wolfram Sang wrote: > Hi Heiner, > > thanks for tackling this! > >> +static int i2c_lock_addr(struct i2c_adapter *adap, unsigned short addr, >> + unsigned short flags) > > What about just using 'struct i2c_client *client' here as an argument. > It has all we need and it currently seems unlikely that we need to call > it from somewhere else where we need this seperation. > >> + if (!(flags & I2C_CLIENT_TEN) && !(flags & I2C_CLIENT_SLAVE) && > > From a pedantic point of view, I don't see a reason for not handling > those two cases above. I hate to be pedantic because 10-bit mode is > practically unused (and I am tempted to remove support for it once in a > while because it makes other solutions clumsy). And the other one is > super unlikely to happen because the backends do not autoload. However, > it is theoretically possible if someone loads a devicetree overlay and > initiates via sysfs at the same time. I liked the solution with the > bitfield and atomic access, but maybe a linked list is better? > Wrt 10 bit addresses: I didn't find a single dts(i) with a 10bit i2c device. Because you said that you're tempted to remove 10 bit support: Could the device tree part be a starting point? > Happy hacking, > > Wolfram > Heiner
Hi Heiner, > I didn't find a single dts(i) with a 10bit i2c device. Because you said In all those years, I didn't even find a device supporting 10 bit addresses. And I really looked especially for them. Some controllers offer 10-bit support in target mode, but that's all I found. Also, there is only one user of I2C_CLIENT_TEN, and this is only a hack to allow instantiating the device at address 0x00. No actual 10-bit usage involved. I will remove this as a first step, because this hack is terrible anyhow. > that you're tempted to remove 10 bit support: Could the device tree part > be a starting point? I don't really have a roadmap how to deprecate 10 bit support. Because it is exported to userspace, the first question is if we can deprecate it, after all. But not much bandwidth even for that, currently. Happy hacking, Wolfram
On 16.08.2024 11:23, Wolfram Sang wrote: > Hi Heiner, > >> I didn't find a single dts(i) with a 10bit i2c device. Because you said > > In all those years, I didn't even find a device supporting 10 bit > addresses. And I really looked especially for them. Some controllers > offer 10-bit support in target mode, but that's all I found. > I found LM8330 which supports 10 bit addressing. However the upper three bits of supported addresses are always zero, so there's no benefit in using 10 bit addressing. > Also, there is only one user of I2C_CLIENT_TEN, and this is only a hack > to allow instantiating the device at address 0x00. No actual 10-bit > usage involved. I will remove this as a first step, because this hack is > terrible anyhow. > >> that you're tempted to remove 10 bit support: Could the device tree part >> be a starting point? > > I don't really have a roadmap how to deprecate 10 bit support. Because > it is exported to userspace, the first question is if we can deprecate > it, after all. But not much bandwidth even for that, currently. > Yes, removing UAPI functionality may be tricky. What I meant was that as a starting point we could replace the following in of_i2c_get_board_info() with an error message stating that 10 bit mode support has been removed. if (addr & I2C_TEN_BIT_ADDRESS) { addr &= ~I2C_TEN_BIT_ADDRESS; info->flags |= I2C_CLIENT_TEN; } > Happy hacking, > > Wolfram > Heiner
> > In all those years, I didn't even find a device supporting 10 bit > > addresses. And I really looked especially for them. Some controllers > > offer 10-bit support in target mode, but that's all I found. > > > I found LM8330 which supports 10 bit addressing. However the upper three > bits of supported addresses are always zero, so there's no benefit in > using 10 bit addressing. Interesting. Never saw this. No Linux driver as well. And for some funny coincidence, I wrote the LM8333 (no 10 bit there) driver 12 years ago, and Dmitry asked me just yesterday if we can remove the driver because there is no user and nobody updated it for DT. But still, nice find! > > I don't really have a roadmap how to deprecate 10 bit support. Because > > it is exported to userspace, the first question is if we can deprecate > > it, after all. But not much bandwidth even for that, currently. > > > Yes, removing UAPI functionality may be tricky. > What I meant was that as a starting point we could replace the following in > of_i2c_get_board_info() with an error message stating that 10 bit mode > support has been removed. Sorry, same answer. I'd need a big picture which I don't have yet.
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index b63f75e44..39892b6f8 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) && !(flags & I2C_CLIENT_SLAVE) && + 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) && !(flags & I2C_CLIENT_SLAVE)) + 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, but no functional change. [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> --- drivers/i2c/i2c-core-base.c | 28 ++++++++++++++++++++++++++++ include/linux/i2c.h | 3 +++ 2 files changed, 31 insertions(+)