| Message ID | 20191231161400.1688-4-wsa+renesas@sang-engineering.com |
|---|---|
| State | RFC |
| Headers | show |
| Series | i2c: implement mechanism to retrieve an alias device | expand |
Hi Wolfram, Thank you for the patch. On Tue, Dec 31, 2019 at 05:13:58PM +0100, Wolfram Sang wrote: > Some devices are able to reprogram their I2C address at runtime. This > can prevent address collisions when one is able to activate and > reprogram these devices one by one. For that to work, they need to be > assigned an unused address. This new functions allows drivers to request > for such an address. It assumes all non-occupied addresses are free. It > will then send a message to such a free address to make sure there is > really nothing listening there. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > drivers/i2c/i2c-core-base.c | 22 ++++++++++++++++++++++ > include/linux/i2c.h | 2 ++ > 2 files changed, 24 insertions(+) > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > index 51bd953ddfb2..5a010e7e698f 100644 > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > @@ -2241,6 +2241,28 @@ static int i2c_detect(struct i2c_adapter *adapter, struct i2c_driver *driver) > return err; > } > Missing kerneldoc, but you already know about this. > +struct i2c_client *i2c_new_alias_device(struct i2c_adapter *adap) > +{ > + struct i2c_client *alias = ERR_PTR(-EBUSY); > + int ret; > + u16 addr; > + > + i2c_lock_bus(adap, I2C_LOCK_SEGMENT); > + > + for (addr = 0x08; addr < 0x78; addr++) { > + ret = i2c_scan_for_client(adap, addr, i2c_unlocked_read_byte_probe); > + if (ret == -ENODEV) { > + alias = i2c_new_dummy_device(adap, addr); > + dev_dbg(&adap->dev, "Found alias: 0x%x\n", addr); > + break; > + } > + } This looks quite inefficient, especially if the beginning of the range is populated with devices. Furthermore, I think there's a high risk of false negatives, as acquiring a free address and reprogramming the client to make use of it are separate operations. Another call to i2c_new_alias_device() could occur in-between. There's also the issue that I2C hasn't been designed for scanning, so some devices may not appreciate this. What happened to the idea of reporting busy address ranges in the firmware (DT, ACPI, ...) ? > + > + i2c_unlock_bus(adap, I2C_LOCK_SEGMENT); > + return alias; > +} > +EXPORT_SYMBOL_GPL(i2c_new_alias_device); > + > int i2c_probe_func_quick_read(struct i2c_adapter *adap, unsigned short addr) > { > return i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0, > diff --git a/include/linux/i2c.h b/include/linux/i2c.h > index f834687989f7..583ca2aec022 100644 > --- a/include/linux/i2c.h > +++ b/include/linux/i2c.h > @@ -441,6 +441,8 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info); > struct i2c_client * > i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *info); > > +struct i2c_client *i2c_new_alias_device(struct i2c_adapter *adap); > + > /* If you don't know the exact address of an I2C device, use this variant > * instead, which can probe for device presence in a list of possible > * addresses. The "probe" callback function is optional. If it is provided,
Hi Wolfram, thanks for having started working on this! On 01/01/20 17:55, Laurent Pinchart wrote: > Hi Wolfram, > > Thank you for the patch. > > On Tue, Dec 31, 2019 at 05:13:58PM +0100, Wolfram Sang wrote: >> Some devices are able to reprogram their I2C address at runtime. This >> can prevent address collisions when one is able to activate and >> reprogram these devices one by one. For that to work, they need to be >> assigned an unused address. This new functions allows drivers to request >> for such an address. It assumes all non-occupied addresses are free. It >> will then send a message to such a free address to make sure there is >> really nothing listening there. >> >> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> >> --- >> drivers/i2c/i2c-core-base.c | 22 ++++++++++++++++++++++ >> include/linux/i2c.h | 2 ++ >> 2 files changed, 24 insertions(+) >> >> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c >> index 51bd953ddfb2..5a010e7e698f 100644 >> --- a/drivers/i2c/i2c-core-base.c >> +++ b/drivers/i2c/i2c-core-base.c >> @@ -2241,6 +2241,28 @@ static int i2c_detect(struct i2c_adapter *adapter, struct i2c_driver *driver) >> return err; >> } >> > > Missing kerneldoc, but you already know about this. > >> +struct i2c_client *i2c_new_alias_device(struct i2c_adapter *adap) >> +{ >> + struct i2c_client *alias = ERR_PTR(-EBUSY); >> + int ret; >> + u16 addr; >> + >> + i2c_lock_bus(adap, I2C_LOCK_SEGMENT); >> + >> + for (addr = 0x08; addr < 0x78; addr++) { >> + ret = i2c_scan_for_client(adap, addr, i2c_unlocked_read_byte_probe); >> + if (ret == -ENODEV) { >> + alias = i2c_new_dummy_device(adap, addr); >> + dev_dbg(&adap->dev, "Found alias: 0x%x\n", addr); >> + break; >> + } >> + } > > This looks quite inefficient, especially if the beginning of the range > is populated with devices. Furthermore, I think there's a high risk of > false negatives, as acquiring a free address and reprogramming the > client to make use of it are separate operations. Right. Applying the alias could raise other errors, thus one would need i2c_new_alias_device() to keep the alias locked until programming it has either failed or has been successfully programmed. > Another call to > i2c_new_alias_device() could occur in-between. There's also the issue > that I2C hasn't been designed for scanning, so some devices may not > appreciate this. > > What happened to the idea of reporting busy address ranges in the > firmware (DT, ACPI, ...) ? Indeed that's how I remember it as well, and I'm a bit suspicious about sending out probe messages that might have side effects (even if the false negative issue mentioned by Laurent were solved). You know, I've been taught to "expect the worse" :) so I'd like to better understand what are the strong reasons in favor of probing, as well as the potential side effects.
Hi Laurent, > > + i2c_lock_bus(adap, I2C_LOCK_SEGMENT); > > + > > + for (addr = 0x08; addr < 0x78; addr++) { > > + ret = i2c_scan_for_client(adap, addr, i2c_unlocked_read_byte_probe); > > + if (ret == -ENODEV) { > > + alias = i2c_new_dummy_device(adap, addr); > > + dev_dbg(&adap->dev, "Found alias: 0x%x\n", addr); > > + break; > > + } > > + } > > This looks quite inefficient, especially if the beginning of the range > is populated with devices. Well, we have to start somewhere? And actually, the range from 0x08 onwards is the least used I encountered so far. What would you suggest? > Furthermore, I think there's a high risk of > false negatives, as acquiring a free address and reprogramming the > client to make use of it are separate operations. Another call to > i2c_new_alias_device() could occur in-between. This is why the whole function is protected using i2c_lock_bus. No other device can scan simultaneously. And once we have the dummy device registered, it is blocked like any other registered device. As we register the dummy device with the lock being held, I don't see how the above race can happen. > There's also the issue > that I2C hasn't been designed for scanning, so some devices may not > appreciate this. This is inevitable. What GMSL and FPD-Link do is very non-I2Cish. I don't see a "perfect" solution. We could skip this transaction and rely only on that all devices are pre-registered. Yet, I think the requirement that all busses *must* be fully described is more dangerous. I'd rather risk that some rare device doesn't like the transaction. I think a byte_read is the best we can do. Much better than a quick command, for sure. > What happened to the idea of reporting busy address ranges in the > firmware (DT, ACPI, ...) ? Fully in place. All pre-registered devices won't be considered because i2c_scan_for_client() uses i2c_check_addr_busy(). Did you think the new helper only relies on the transfer sent out? This is not the case, the transfer is only the final safety measure for so far unclaimed addresses. All the best for 2020, Wolfram
Hi Luca, > > This looks quite inefficient, especially if the beginning of the range > > is populated with devices. Furthermore, I think there's a high risk of > > false negatives, as acquiring a free address and reprogramming the > > client to make use of it are separate operations. > > Right. Applying the alias could raise other errors, thus one would need > i2c_new_alias_device() to keep the alias locked until programming it has > either failed or has been successfully programmed. Please see my reply to Laurent, I don't think it is racy. But please elaborate if you think I am wrong. > > What happened to the idea of reporting busy address ranges in the > > firmware (DT, ACPI, ...) ? > > Indeed that's how I remember it as well, and I'm a bit suspicious about > sending out probe messages that might have side effects (even if the > false negative issue mentioned by Laurent were solved). You know, I've > been taught to "expect the worse" :) so I'd like to better understand > what are the strong reasons in favor of probing, as well as the > potential side effects. As I said to Laurent, too, I think the risk that a bus is not fully described is higher than a device which does not respond to a read_byte. In both cases, we would wrongly use an address in use. Also, all the best for you in 2020! Wolfram
Hi Wolfram, On 02/01/20 22:13, Wolfram Sang wrote: > Hi Luca, > >>> This looks quite inefficient, especially if the beginning of the range >>> is populated with devices. Furthermore, I think there's a high risk of >>> false negatives, as acquiring a free address and reprogramming the >>> client to make use of it are separate operations. >> >> Right. Applying the alias could raise other errors, thus one would need >> i2c_new_alias_device() to keep the alias locked until programming it has >> either failed or has been successfully programmed. > > Please see my reply to Laurent, I don't think it is racy. But please > elaborate if you think I am wrong. Uhm, you are right here, it's not racy. Sorry, I had read the code quickly and didn't notice the i2c_new_dummy_device() call. So this means if i2c_new_alias_device() succeeds but the caller later fails while applying the alias, then it has to call i2c_unregister_device() to free the alias. Correct? >>> What happened to the idea of reporting busy address ranges in the >>> firmware (DT, ACPI, ...) ? >> >> Indeed that's how I remember it as well, and I'm a bit suspicious about >> sending out probe messages that might have side effects (even if the >> false negative issue mentioned by Laurent were solved). You know, I've >> been taught to "expect the worse" :) so I'd like to better understand >> what are the strong reasons in favor of probing, as well as the >> potential side effects. > > As I said to Laurent, too, I think the risk that a bus is not fully > described is higher than a device which does not respond to a read_byte. > In both cases, we would wrongly use an address in use. OK, I'm still uncomfortable with sending unexpected transactions to the dark outer space, but this is more a feeling than based on facts, and you know more than me, so I guess I can live with that. > Also, all the best for you in 2020! Thanks. Best wishes to you too for the new year!
On Thu, Jan 02, 2020 at 11:27:57PM +0100, Luca Ceresoli wrote: > Hi Wolfram, > > On 02/01/20 22:13, Wolfram Sang wrote: > > Hi Luca, > > > >>> This looks quite inefficient, especially if the beginning of the range > >>> is populated with devices. Furthermore, I think there's a high risk of > >>> false negatives, as acquiring a free address and reprogramming the > >>> client to make use of it are separate operations. > >> > >> Right. Applying the alias could raise other errors, thus one would need > >> i2c_new_alias_device() to keep the alias locked until programming it has > >> either failed or has been successfully programmed. > > > > Please see my reply to Laurent, I don't think it is racy. But please > > elaborate if you think I am wrong. > > Uhm, you are right here, it's not racy. Sorry, I had read the code > quickly and didn't notice the i2c_new_dummy_device() call. > > So this means if i2c_new_alias_device() succeeds but the caller later > fails while applying the alias, then it has to call > i2c_unregister_device() to free the alias. Correct? I was wrong as well, sorry about that. > >>> What happened to the idea of reporting busy address ranges in the > >>> firmware (DT, ACPI, ...) ? > >> > >> Indeed that's how I remember it as well, and I'm a bit suspicious about > >> sending out probe messages that might have side effects (even if the > >> false negative issue mentioned by Laurent were solved). You know, I've > >> been taught to "expect the worse" :) so I'd like to better understand > >> what are the strong reasons in favor of probing, as well as the > >> potential side effects. > > > > As I said to Laurent, too, I think the risk that a bus is not fully > > described is higher than a device which does not respond to a read_byte. > > In both cases, we would wrongly use an address in use. I don't fully agree with this, I think we shouldn't impose a penalty on every user because some device trees don't fully describe the hardware. I think we should, at the very least, skip the probe and rely on DT if DT explicitly states that all used addresses are listed. We discussed a property to report addresses used by devices not described in DT, if that property is listed I would prefer trusting DT. > OK, I'm still uncomfortable with sending unexpected transactions to the > dark outer space, but this is more a feeling than based on facts, and > you know more than me, so I guess I can live with that. > > > Also, all the best for you in 2020! > > Thanks. Best wishes to you too for the new year! Likewise. Let's start with a simple wish of getting this issue resolved :-)
Hi Wolfram, On 31/12/2019 16:13, Wolfram Sang wrote: > Some devices are able to reprogram their I2C address at runtime. This > can prevent address collisions when one is able to activate and > reprogram these devices one by one. For that to work, they need to be > assigned an unused address. This new functions allows drivers to request > for such an address. It assumes all non-occupied addresses are free. It > will then send a message to such a free address to make sure there is > really nothing listening there. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > drivers/i2c/i2c-core-base.c | 22 ++++++++++++++++++++++ > include/linux/i2c.h | 2 ++ > 2 files changed, 24 insertions(+) > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > index 51bd953ddfb2..5a010e7e698f 100644 > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > @@ -2241,6 +2241,28 @@ static int i2c_detect(struct i2c_adapter *adapter, struct i2c_driver *driver) > return err; > } > > +struct i2c_client *i2c_new_alias_device(struct i2c_adapter *adap) > +{ > + struct i2c_client *alias = ERR_PTR(-EBUSY); > + int ret; > + u16 addr; > + > + i2c_lock_bus(adap, I2C_LOCK_SEGMENT); > + > + for (addr = 0x08; addr < 0x78; addr++) { > + ret = i2c_scan_for_client(adap, addr, i2c_unlocked_read_byte_probe); Are all 'known' devices on a bus (all the ones declared in DT etc) marked as 'busy' or taken by the time this call is made? (edit, I don't think they are) Perhaps this is a constructed corner case, but I'm just trying to follow it through: I.e. if say the adv748x had in DT defined aliases at 0x08, 0x09, 0x0A..., but not yet probed (thus no device is listening at these addresses) ... and then a max9286 came along and asked for 'any' spare address with this call, would it be given 0x08 first? If so (which I think is what the case would be currently, until I'm pointed otherwise) do we need to mark all addresses on the bus as reserved against this some how? I'm not sure how that would occur, as it would be up to the adv748x in that instance to parse it's extended register list to identify the extra aliases it will create, *and* that would only happen if the device driver was enabled in the first place. So this seems a bit 'racy' in a different context; not the i2c_lock_bus, but rather the probe order of devices on the bus could affect the allocations. Perhaps that is unavoidable though... -- Kieran > + if (ret == -ENODEV) { > + alias = i2c_new_dummy_device(adap, addr); > + dev_dbg(&adap->dev, "Found alias: 0x%x\n", addr); > + break; > + } > + } > + > + i2c_unlock_bus(adap, I2C_LOCK_SEGMENT); > + return alias; > +} > +EXPORT_SYMBOL_GPL(i2c_new_alias_device); > + > int i2c_probe_func_quick_read(struct i2c_adapter *adap, unsigned short addr) > { > return i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0, > diff --git a/include/linux/i2c.h b/include/linux/i2c.h > index f834687989f7..583ca2aec022 100644 > --- a/include/linux/i2c.h > +++ b/include/linux/i2c.h > @@ -441,6 +441,8 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info); > struct i2c_client * > i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *info); > > +struct i2c_client *i2c_new_alias_device(struct i2c_adapter *adap); > + > /* If you don't know the exact address of an I2C device, use this variant > * instead, which can probe for device presence in a list of possible > * addresses. The "probe" callback function is optional. If it is provided, >
Hi Laurent, On 03/01/20 01:10, Laurent Pinchart wrote: > On Thu, Jan 02, 2020 at 11:27:57PM +0100, Luca Ceresoli wrote: >> Hi Wolfram, >> >> On 02/01/20 22:13, Wolfram Sang wrote: >>> Hi Luca, >>> >>>>> This looks quite inefficient, especially if the beginning of the range >>>>> is populated with devices. Furthermore, I think there's a high risk of >>>>> false negatives, as acquiring a free address and reprogramming the >>>>> client to make use of it are separate operations. >>>> >>>> Right. Applying the alias could raise other errors, thus one would need >>>> i2c_new_alias_device() to keep the alias locked until programming it has >>>> either failed or has been successfully programmed. >>> >>> Please see my reply to Laurent, I don't think it is racy. But please >>> elaborate if you think I am wrong. >> >> Uhm, you are right here, it's not racy. Sorry, I had read the code >> quickly and didn't notice the i2c_new_dummy_device() call. >> >> So this means if i2c_new_alias_device() succeeds but the caller later >> fails while applying the alias, then it has to call >> i2c_unregister_device() to free the alias. Correct? > > I was wrong as well, sorry about that. > >>>>> What happened to the idea of reporting busy address ranges in the >>>>> firmware (DT, ACPI, ...) ? >>>> >>>> Indeed that's how I remember it as well, and I'm a bit suspicious about >>>> sending out probe messages that might have side effects (even if the >>>> false negative issue mentioned by Laurent were solved). You know, I've >>>> been taught to "expect the worse" :) so I'd like to better understand >>>> what are the strong reasons in favor of probing, as well as the >>>> potential side effects. >>> >>> As I said to Laurent, too, I think the risk that a bus is not fully >>> described is higher than a device which does not respond to a read_byte. >>> In both cases, we would wrongly use an address in use. > > I don't fully agree with this, I think we shouldn't impose a penalty on > every user because some device trees don't fully describe the hardware. > I think we should, at the very least, skip the probe and rely on DT if > DT explicitly states that all used addresses are listed. We discussed a > property to report addresses used by devices not described in DT, if > that property is listed I would prefer trusting DT. It would be nice, but I'm not sure this is really doable. Say the DT for board X lists all the used slave addresses. Then the kernel would assume all the other addresses are available. But then somebody includes the DT of board X in the DT for product Z, based on board X + add-on board Y. Add-on board Y has 2 I2C chips, but only one is described in DT. Now the kernel still thinks it knows all the used address, but this is wrong. At my current pondering status, I think only two approaches are doable: either assuming all DTs fully describe the hardware (which is still a good goal to pursue, generally speaking) or use Wolfram's proposal. The difference between the two is the call to i2c_unlocked_read_byte_probe(). However a hybrid approach is to speak out loud if we get a response from an address that is not marked as busy, to invite the developers to fix their DT. In other words: ret = i2c_scan_for_client(adap, addr, i2c_unlocked_read_byte_probe); if (ret == -ENODEV) { alias = i2c_new_dummy_device(adap, addr); dev_dbg(&adap->dev, "Found alias: 0x%x\n", addr); break; +} else if (ret == 0) { + dev_err(&adap->dev, + "alien found at %02x, please add it to your DT!!!\n", + addr); } Wolfram, do think this could work? Do we have all the addresses listed in DT marked as busy early enough?
Hi Kieran, On Tue, Jan 07, 2020 at 09:40:35AM +0000, Kieran Bingham wrote: > On 31/12/2019 16:13, Wolfram Sang wrote: > > Some devices are able to reprogram their I2C address at runtime. This > > can prevent address collisions when one is able to activate and > > reprogram these devices one by one. For that to work, they need to be > > assigned an unused address. This new functions allows drivers to request > > for such an address. It assumes all non-occupied addresses are free. It > > will then send a message to such a free address to make sure there is > > really nothing listening there. > > > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > > --- > > drivers/i2c/i2c-core-base.c | 22 ++++++++++++++++++++++ > > include/linux/i2c.h | 2 ++ > > 2 files changed, 24 insertions(+) > > > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > > index 51bd953ddfb2..5a010e7e698f 100644 > > --- a/drivers/i2c/i2c-core-base.c > > +++ b/drivers/i2c/i2c-core-base.c > > @@ -2241,6 +2241,28 @@ static int i2c_detect(struct i2c_adapter *adapter, struct i2c_driver *driver) > > return err; > > } > > > > +struct i2c_client *i2c_new_alias_device(struct i2c_adapter *adap) > > +{ > > + struct i2c_client *alias = ERR_PTR(-EBUSY); > > + int ret; > > + u16 addr; > > + > > + i2c_lock_bus(adap, I2C_LOCK_SEGMENT); > > + > > + for (addr = 0x08; addr < 0x78; addr++) { > > + ret = i2c_scan_for_client(adap, addr, i2c_unlocked_read_byte_probe); > > Are all 'known' devices on a bus (all the ones declared in DT etc) > marked as 'busy' or taken by the time this call is made? (edit, I don't > think they are) > > Perhaps this is a constructed corner case, but I'm just trying to follow > it through: > > I.e. if say the adv748x had in DT defined aliases at 0x08, 0x09, > 0x0A..., but not yet probed (thus no device is listening at these > addresses) ... and then a max9286 came along and asked for 'any' spare > address with this call, would it be given 0x08 first? > > If so (which I think is what the case would be currently, until I'm > pointed otherwise) do we need to mark all addresses on the bus as > reserved against this some how? > > I'm not sure how that would occur, as it would be up to the adv748x in > that instance to parse it's extended register list to identify the extra > aliases it will create, *and* that would only happen if the device > driver was enabled in the first place. > > So this seems a bit 'racy' in a different context; not the i2c_lock_bus, > but rather the probe order of devices on the bus could affect the > allocations. > > Perhaps that is unavoidable though... But it's a real problem... Could the I2C core parse all the addresses on the bus before probing drivers ? > > + if (ret == -ENODEV) { > > + alias = i2c_new_dummy_device(adap, addr); > > + dev_dbg(&adap->dev, "Found alias: 0x%x\n", addr); > > + break; > > + } > > + } > > + > > + i2c_unlock_bus(adap, I2C_LOCK_SEGMENT); > > + return alias; > > +} > > +EXPORT_SYMBOL_GPL(i2c_new_alias_device); > > + > > int i2c_probe_func_quick_read(struct i2c_adapter *adap, unsigned short addr) > > { > > return i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0, > > diff --git a/include/linux/i2c.h b/include/linux/i2c.h > > index f834687989f7..583ca2aec022 100644 > > --- a/include/linux/i2c.h > > +++ b/include/linux/i2c.h > > @@ -441,6 +441,8 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info); > > struct i2c_client * > > i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *info); > > > > +struct i2c_client *i2c_new_alias_device(struct i2c_adapter *adap); > > + > > /* If you don't know the exact address of an I2C device, use this variant > > * instead, which can probe for device presence in a list of possible > > * addresses. The "probe" callback function is optional. If it is provided,
Hi Luca, On Tue, Jan 07, 2020 at 04:03:29PM +0100, Luca Ceresoli wrote: > On 03/01/20 01:10, Laurent Pinchart wrote: > > On Thu, Jan 02, 2020 at 11:27:57PM +0100, Luca Ceresoli wrote: > >> On 02/01/20 22:13, Wolfram Sang wrote: > >>>>> This looks quite inefficient, especially if the beginning of the range > >>>>> is populated with devices. Furthermore, I think there's a high risk of > >>>>> false negatives, as acquiring a free address and reprogramming the > >>>>> client to make use of it are separate operations. > >>>> > >>>> Right. Applying the alias could raise other errors, thus one would need > >>>> i2c_new_alias_device() to keep the alias locked until programming it has > >>>> either failed or has been successfully programmed. > >>> > >>> Please see my reply to Laurent, I don't think it is racy. But please > >>> elaborate if you think I am wrong. > >> > >> Uhm, you are right here, it's not racy. Sorry, I had read the code > >> quickly and didn't notice the i2c_new_dummy_device() call. > >> > >> So this means if i2c_new_alias_device() succeeds but the caller later > >> fails while applying the alias, then it has to call > >> i2c_unregister_device() to free the alias. Correct? > > > > I was wrong as well, sorry about that. > > > >>>>> What happened to the idea of reporting busy address ranges in the > >>>>> firmware (DT, ACPI, ...) ? > >>>> > >>>> Indeed that's how I remember it as well, and I'm a bit suspicious about > >>>> sending out probe messages that might have side effects (even if the > >>>> false negative issue mentioned by Laurent were solved). You know, I've > >>>> been taught to "expect the worse" :) so I'd like to better understand > >>>> what are the strong reasons in favor of probing, as well as the > >>>> potential side effects. > >>> > >>> As I said to Laurent, too, I think the risk that a bus is not fully > >>> described is higher than a device which does not respond to a read_byte. > >>> In both cases, we would wrongly use an address in use. > > > > I don't fully agree with this, I think we shouldn't impose a penalty on > > every user because some device trees don't fully describe the hardware. > > I think we should, at the very least, skip the probe and rely on DT if > > DT explicitly states that all used addresses are listed. We discussed a > > property to report addresses used by devices not described in DT, if > > that property is listed I would prefer trusting DT. > > It would be nice, but I'm not sure this is really doable. Say the DT for > board X lists all the used slave addresses. Then the kernel would assume > all the other addresses are available. But then somebody includes the DT > of board X in the DT for product Z, based on board X + add-on board Y. > Add-on board Y has 2 I2C chips, but only one is described in DT. Now the > kernel still thinks it knows all the used address, but this is wrong. That's the fault of the system integrator though. We can't prevent people from making incorrect DT, and we shouldn't go to great length to still support them. > At my current pondering status, I think only two approaches are doable: > either assuming all DTs fully describe the hardware (which is still a > good goal to pursue, generally speaking) or use Wolfram's proposal. The > difference between the two is the call to i2c_unlocked_read_byte_probe(). > > However a hybrid approach is to speak out loud if we get a response from > an address that is not marked as busy, to invite the developers to fix > their DT. In other words: > > ret = i2c_scan_for_client(adap, addr, i2c_unlocked_read_byte_probe); > if (ret == -ENODEV) { > alias = i2c_new_dummy_device(adap, addr); > dev_dbg(&adap->dev, "Found alias: 0x%x\n", addr); > break; > +} else if (ret == 0) { > + dev_err(&adap->dev, > + "alien found at %02x, please add it to your DT!!!\n", > + addr); > } > > Wolfram, do think this could work? Do we have all the addresses listed > in DT marked as busy early enough?
Hi Laurent, On 07/01/2020 17:11, Laurent Pinchart wrote: > Hi Kieran, > > On Tue, Jan 07, 2020 at 09:40:35AM +0000, Kieran Bingham wrote: >> On 31/12/2019 16:13, Wolfram Sang wrote: >>> Some devices are able to reprogram their I2C address at runtime. This >>> can prevent address collisions when one is able to activate and >>> reprogram these devices one by one. For that to work, they need to be >>> assigned an unused address. This new functions allows drivers to request >>> for such an address. It assumes all non-occupied addresses are free. It >>> will then send a message to such a free address to make sure there is >>> really nothing listening there. >>> >>> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> >>> --- >>> drivers/i2c/i2c-core-base.c | 22 ++++++++++++++++++++++ >>> include/linux/i2c.h | 2 ++ >>> 2 files changed, 24 insertions(+) >>> >>> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c >>> index 51bd953ddfb2..5a010e7e698f 100644 >>> --- a/drivers/i2c/i2c-core-base.c >>> +++ b/drivers/i2c/i2c-core-base.c >>> @@ -2241,6 +2241,28 @@ static int i2c_detect(struct i2c_adapter *adapter, struct i2c_driver *driver) >>> return err; >>> } >>> >>> +struct i2c_client *i2c_new_alias_device(struct i2c_adapter *adap) >>> +{ >>> + struct i2c_client *alias = ERR_PTR(-EBUSY); >>> + int ret; >>> + u16 addr; >>> + >>> + i2c_lock_bus(adap, I2C_LOCK_SEGMENT); >>> + >>> + for (addr = 0x08; addr < 0x78; addr++) { >>> + ret = i2c_scan_for_client(adap, addr, i2c_unlocked_read_byte_probe); >> >> Are all 'known' devices on a bus (all the ones declared in DT etc) >> marked as 'busy' or taken by the time this call is made? (edit, I don't >> think they are) >> >> Perhaps this is a constructed corner case, but I'm just trying to follow >> it through: >> >> I.e. if say the adv748x had in DT defined aliases at 0x08, 0x09, >> 0x0A..., but not yet probed (thus no device is listening at these >> addresses) ... and then a max9286 came along and asked for 'any' spare >> address with this call, would it be given 0x08 first? >> >> If so (which I think is what the case would be currently, until I'm >> pointed otherwise) do we need to mark all addresses on the bus as >> reserved against this some how? >> >> I'm not sure how that would occur, as it would be up to the adv748x in >> that instance to parse it's extended register list to identify the extra >> aliases it will create, *and* that would only happen if the device >> driver was enabled in the first place. >> >> So this seems a bit 'racy' in a different context; not the i2c_lock_bus, >> but rather the probe order of devices on the bus could affect the >> allocations. >> >> Perhaps that is unavoidable though... > > But it's a real problem... Could the I2C core parse all the addresses on > the bus before probing drivers ? That's my point :-D The core 'could' parse all reg entries, and conclude that any extended entries within a device node are aliases as well, which should be reserved, but I don't think it could know if the device is actually going to be enabled by a driver (well, it could look it up). I think if core-i2c parses all device tree nodes for register addresses first, it would have to consider all addresses it came across as potentially in use. But it would also have to traverse any i2c-muxes too! -- Kieran > >>> + if (ret == -ENODEV) { >>> + alias = i2c_new_dummy_device(adap, addr); >>> + dev_dbg(&adap->dev, "Found alias: 0x%x\n", addr); >>> + break; >>> + } >>> + } >>> + >>> + i2c_unlock_bus(adap, I2C_LOCK_SEGMENT); >>> + return alias; >>> +} >>> +EXPORT_SYMBOL_GPL(i2c_new_alias_device); >>> + >>> int i2c_probe_func_quick_read(struct i2c_adapter *adap, unsigned short addr) >>> { >>> return i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0, >>> diff --git a/include/linux/i2c.h b/include/linux/i2c.h >>> index f834687989f7..583ca2aec022 100644 >>> --- a/include/linux/i2c.h >>> +++ b/include/linux/i2c.h >>> @@ -441,6 +441,8 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info); >>> struct i2c_client * >>> i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *info); >>> >>> +struct i2c_client *i2c_new_alias_device(struct i2c_adapter *adap); >>> + >>> /* If you don't know the exact address of an I2C device, use this variant >>> * instead, which can probe for device presence in a list of possible >>> * addresses. The "probe" callback function is optional. If it is provided, >
> > > As I said to Laurent, too, I think the risk that a bus is not fully > > > described is higher than a device which does not respond to a read_byte. > > > In both cases, we would wrongly use an address in use. > > I don't fully agree with this, I think we shouldn't impose a penalty on > every user because some device trees don't fully describe the hardware. I haven't decided yet. However, my general preference is that for a generic OS like Linux, saftey comes first, then performance. If you have a fully described DT, then the overhead will be 1 read_byte transaction per requested alias at probe time. We could talk about using quick_read to half the overhead. You could even patch it away, if it is too much for $customer. > I think we should, at the very least, skip the probe and rely on DT if > DT explicitly states that all used addresses are listed. We discussed a > property to report addresses used by devices not described in DT, if > that property is listed I would prefer trusting DT. Yeah, we discussed this property and I have no intentions of dropping it. I haven't though of including it into this series, but it probably makes sense. We don't have to define much anyhow, just state what already exists, I guess. From Documentation/devicetree/bindings/i2c/i2c-ocores.txt: dummy@60 { compatible = "dummy"; reg = <0x60>; }; I think "dummy" is generic enough to be described in i2c.txt.
> +} else if (ret == 0) { > + dev_err(&adap->dev, > + "alien found at %02x, please add it to your DT!!!\n", > + addr); > } > > Wolfram, do think this could work? Do we have all the addresses listed > in DT marked as busy early enough? Not sure. We could do that, but we will still have the transaction you guys worry about.
> > It would be nice, but I'm not sure this is really doable. Say the DT for > > board X lists all the used slave addresses. Then the kernel would assume > > all the other addresses are available. But then somebody includes the DT > > of board X in the DT for product Z, based on board X + add-on board Y. > > Add-on board Y has 2 I2C chips, but only one is described in DT. Now the > > kernel still thinks it knows all the used address, but this is wrong. > > That's the fault of the system integrator though. We can't prevent > people from making incorrect DT, and we shouldn't go to great length to > still support them. Currently, there is no paradigm that all I2C busses must be fully described. Enforcing it now all of a sudden is not too user-friendly, or? Especially since calling read_byte once is not necessarily "great length" in my book. If you have 8 cameras on a 400kHz bus, the 8 * 18 bits should take 360us if I am not mistaken?
Hi Wolfram, On Wed, Jan 8, 2020 at 2:20 PM Wolfram Sang <wsa@the-dreams.de> wrote: > > > > As I said to Laurent, too, I think the risk that a bus is not fully > > > > described is higher than a device which does not respond to a read_byte. > > > > In both cases, we would wrongly use an address in use. > > > > I don't fully agree with this, I think we shouldn't impose a penalty on > > every user because some device trees don't fully describe the hardware. > > I haven't decided yet. However, my general preference is that for a > generic OS like Linux, saftey comes first, then performance. If you have > a fully described DT, then the overhead will be 1 read_byte transaction > per requested alias at probe time. We could talk about using quick_read > to half the overhead. You could even patch it away, if it is too much > for $customer. > > > I think we should, at the very least, skip the probe and rely on DT if > > DT explicitly states that all used addresses are listed. We discussed a > > property to report addresses used by devices not described in DT, if > > that property is listed I would prefer trusting DT. > > Yeah, we discussed this property and I have no intentions of dropping > it. I haven't though of including it into this series, but it probably > makes sense. We don't have to define much anyhow, just state what > already exists, I guess. > > From Documentation/devicetree/bindings/i2c/i2c-ocores.txt: > > dummy@60 { > compatible = "dummy"; > reg = <0x60>; > }; > > I think "dummy" is generic enough to be described in i2c.txt. Dummy-the-node or dummy-the-compatible value? Probably dummy nodes should have no compatible value at all? Gr{oetje,eeting}s, Geert
On Wed, Jan 08, 2020 at 02:27:08PM +0100, Wolfram Sang wrote: > > > > It would be nice, but I'm not sure this is really doable. Say the DT for > > > board X lists all the used slave addresses. Then the kernel would assume > > > all the other addresses are available. But then somebody includes the DT > > > of board X in the DT for product Z, based on board X + add-on board Y. > > > Add-on board Y has 2 I2C chips, but only one is described in DT. Now the > > > kernel still thinks it knows all the used address, but this is wrong. > > > > That's the fault of the system integrator though. We can't prevent > > people from making incorrect DT, and we shouldn't go to great length to > > still support them. > > Currently, there is no paradigm that all I2C busses must be fully > described. Enforcing it now all of a sudden is not too user-friendly, > or? We're only enforcing it for systems that want to make use of this new API, so it's not breaking backward compatibility. > Especially since calling read_byte once is not necessarily "great > length" in my book. If you have 8 cameras on a 400kHz bus, the 8 * 18 > bits should take 360us if I am not mistaken? That's assuming the first scanned address is free. There could also be I2C-controller I2C muxes or gates in front of the bus. Things can quickly get more expensive.
On Wed, Jan 08, 2020 at 02:19:29PM +0100, Wolfram Sang wrote: > > > > > As I said to Laurent, too, I think the risk that a bus is not fully > > > > described is higher than a device which does not respond to a read_byte. > > > > In both cases, we would wrongly use an address in use. > > > > I don't fully agree with this, I think we shouldn't impose a penalty on > > every user because some device trees don't fully describe the hardware. > > I haven't decided yet. However, my general preference is that for a > generic OS like Linux, saftey comes first, then performance. If you have > a fully described DT, then the overhead will be 1 read_byte transaction > per requested alias at probe time. We could talk about using quick_read > to half the overhead. You could even patch it away, if it is too much > for $customer. > > > I think we should, at the very least, skip the probe and rely on DT if > > DT explicitly states that all used addresses are listed. We discussed a > > property to report addresses used by devices not described in DT, if > > that property is listed I would prefer trusting DT. > > Yeah, we discussed this property and I have no intentions of dropping > it. I haven't though of including it into this series, but it probably > makes sense. We don't have to define much anyhow, just state what > already exists, I guess. > > From Documentation/devicetree/bindings/i2c/i2c-ocores.txt: > > dummy@60 { > compatible = "dummy"; > reg = <0x60>; > }; > > I think "dummy" is generic enough to be described in i2c.txt. We may want a compatible value that guarantees noone will ever match it :-) I was imagining a single property at the bus level with multiple ranges instead, but dummy nodes could be OK too.
> >> I.e. if say the adv748x had in DT defined aliases at 0x08, 0x09, > >> 0x0A..., but not yet probed (thus no device is listening at these > >> addresses) ... and then a max9286 came along and asked for 'any' spare > >> address with this call, would it be given 0x08 first? You have a point here. Ancillary addresses are not blocked until the driver probes, this is true. I wonder now if we should handle multiple addresses in i2c-core-of.c somehow, too? It does block the first <reg> entry, but not all. > The core 'could' parse all reg entries, and conclude that any extended > entries within a device node are aliases as well, which should be > reserved, but I don't think it could know if the device is actually > going to be enabled by a driver (well, it could look it up). We could argue that if it is described in DT, it should be blocked in any case, or? > But it would also have to traverse any i2c-muxes too! I probably need a second thought about muxes as well.
On Wed, Jan 08, 2020 at 02:35:33PM +0100, Wolfram Sang wrote: > > > >> I.e. if say the adv748x had in DT defined aliases at 0x08, 0x09, > > >> 0x0A..., but not yet probed (thus no device is listening at these > > >> addresses) ... and then a max9286 came along and asked for 'any' spare > > >> address with this call, would it be given 0x08 first? > > You have a point here. Ancillary addresses are not blocked until the > driver probes, this is true. I wonder now if we should handle multiple > addresses in i2c-core-of.c somehow, too? It does block the first <reg> > entry, but not all. > > > The core 'could' parse all reg entries, and conclude that any extended > > entries within a device node are aliases as well, which should be > > reserved, but I don't think it could know if the device is actually > > going to be enabled by a driver (well, it could look it up). > > We could argue that if it is described in DT, it should be blocked in > any case, or? That seems fair to me. > > But it would also have to traverse any i2c-muxes too! > > I probably need a second thought about muxes as well.
> > Currently, there is no paradigm that all I2C busses must be fully > > described. Enforcing it now all of a sudden is not too user-friendly, > > or? > > We're only enforcing it for systems that want to make use of this new > API, so it's not breaking backward compatibility. Well, even new systems might need to update old DTSIs which they include. > > Especially since calling read_byte once is not necessarily "great > > length" in my book. If you have 8 cameras on a 400kHz bus, the 8 * 18 > > bits should take 360us if I am not mistaken? > > That's assuming the first scanned address is free. There could also be > I2C-controller I2C muxes or gates in front of the bus. Things can > quickly get more expensive. Not on a fully described bus, or? The first address will always be free.
On 2020-01-01 17:55, Laurent Pinchart wrote: > Hi Wolfram, > > Thank you for the patch. > > On Tue, Dec 31, 2019 at 05:13:58PM +0100, Wolfram Sang wrote: >> Some devices are able to reprogram their I2C address at runtime. This >> can prevent address collisions when one is able to activate and >> reprogram these devices one by one. For that to work, they need to be >> assigned an unused address. This new functions allows drivers to request >> for such an address. It assumes all non-occupied addresses are free. It >> will then send a message to such a free address to make sure there is >> really nothing listening there. >> >> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> >> --- >> drivers/i2c/i2c-core-base.c | 22 ++++++++++++++++++++++ >> include/linux/i2c.h | 2 ++ >> 2 files changed, 24 insertions(+) >> >> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c >> index 51bd953ddfb2..5a010e7e698f 100644 >> --- a/drivers/i2c/i2c-core-base.c >> +++ b/drivers/i2c/i2c-core-base.c >> @@ -2241,6 +2241,28 @@ static int i2c_detect(struct i2c_adapter *adapter, struct i2c_driver *driver) >> return err; >> } >> > > Missing kerneldoc, but you already know about this. > >> +struct i2c_client *i2c_new_alias_device(struct i2c_adapter *adap) >> +{ >> + struct i2c_client *alias = ERR_PTR(-EBUSY); >> + int ret; >> + u16 addr; >> + >> + i2c_lock_bus(adap, I2C_LOCK_SEGMENT); >> + >> + for (addr = 0x08; addr < 0x78; addr++) { >> + ret = i2c_scan_for_client(adap, addr, i2c_unlocked_read_byte_probe); >> + if (ret == -ENODEV) { >> + alias = i2c_new_dummy_device(adap, addr); >> + dev_dbg(&adap->dev, "Found alias: 0x%x\n", addr); >> + break; >> + } >> + } > > This looks quite inefficient, especially if the beginning of the range > is populated with devices. Furthermore, I think there's a high risk of > false negatives, as acquiring a free address and reprogramming the > client to make use of it are separate operations. Another call to > i2c_new_alias_device() could occur in-between. There's also the issue > that I2C hasn't been designed for scanning, so some devices may not > appreciate this. > > What happened to the idea of reporting busy address ranges in the > firmware (DT, ACPI, ...) ? Another argument against probing (perhaps weak, but still) is that the probed address might already be programmed in (one of) the reprogrammable chip(s) making it respond to the probe and thus preventing the use of a perfectly good alias address. In /extreme/ situations this /might/ prevent finding a needed alias at all. Cheers, Peter
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index 51bd953ddfb2..5a010e7e698f 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -2241,6 +2241,28 @@ static int i2c_detect(struct i2c_adapter *adapter, struct i2c_driver *driver) return err; } +struct i2c_client *i2c_new_alias_device(struct i2c_adapter *adap) +{ + struct i2c_client *alias = ERR_PTR(-EBUSY); + int ret; + u16 addr; + + i2c_lock_bus(adap, I2C_LOCK_SEGMENT); + + for (addr = 0x08; addr < 0x78; addr++) { + ret = i2c_scan_for_client(adap, addr, i2c_unlocked_read_byte_probe); + if (ret == -ENODEV) { + alias = i2c_new_dummy_device(adap, addr); + dev_dbg(&adap->dev, "Found alias: 0x%x\n", addr); + break; + } + } + + i2c_unlock_bus(adap, I2C_LOCK_SEGMENT); + return alias; +} +EXPORT_SYMBOL_GPL(i2c_new_alias_device); + int i2c_probe_func_quick_read(struct i2c_adapter *adap, unsigned short addr) { return i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0, diff --git a/include/linux/i2c.h b/include/linux/i2c.h index f834687989f7..583ca2aec022 100644 --- a/include/linux/i2c.h +++ b/include/linux/i2c.h @@ -441,6 +441,8 @@ i2c_new_device(struct i2c_adapter *adap, struct i2c_board_info const *info); struct i2c_client * i2c_new_client_device(struct i2c_adapter *adap, struct i2c_board_info const *info); +struct i2c_client *i2c_new_alias_device(struct i2c_adapter *adap); + /* If you don't know the exact address of an I2C device, use this variant * instead, which can probe for device presence in a list of possible * addresses. The "probe" callback function is optional. If it is provided,
Some devices are able to reprogram their I2C address at runtime. This can prevent address collisions when one is able to activate and reprogram these devices one by one. For that to work, they need to be assigned an unused address. This new functions allows drivers to request for such an address. It assumes all non-occupied addresses are free. It will then send a message to such a free address to make sure there is really nothing listening there. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- drivers/i2c/i2c-core-base.c | 22 ++++++++++++++++++++++ include/linux/i2c.h | 2 ++ 2 files changed, 24 insertions(+)