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
