| Message ID | 20191231161400.1688-2-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:56PM +0100, Wolfram Sang wrote: > There is a pattern to check for existence of a client which is copied in > i2c_detect_address() and i2c_new_scanned_device(): > > 1) check if address is valid > 2) check if address is already registered > 3) send a message and check the reponse > > Because this pattern will be needed a third time soon, refactor it into > its own function. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > drivers/i2c/i2c-core-base.c | 57 ++++++++++++++++--------------------- > 1 file changed, 25 insertions(+), 32 deletions(-) > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > index a1eb28a3cc54..20a726dc78db 100644 > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > @@ -2108,29 +2108,39 @@ static int i2c_default_probe(struct i2c_adapter *adap, unsigned short addr) > return err >= 0; > } > > -static int i2c_detect_address(struct i2c_client *temp_client, > - struct i2c_driver *driver) > +static int i2c_scan_for_client(struct i2c_adapter *adap, unsigned short addr, > + int (*probe)(struct i2c_adapter *adap, unsigned short addr)) > { > - struct i2c_board_info info; > - struct i2c_adapter *adapter = temp_client->adapter; > - int addr = temp_client->addr; > int err; > > /* Make sure the address is valid */ > err = i2c_check_7bit_addr_validity_strict(addr); > - if (err) { > - dev_warn(&adapter->dev, "Invalid probe address 0x%02x\n", > - addr); > + if (WARN(err, "Invalid probe address 0x%02x\n", addr)) Does this deserve a full backtrace ? If so could you mention it in the commit message ? With this addressed, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > return err; > - } > > /* Skip if already in use (7 bit, no need to encode flags) */ > - if (i2c_check_addr_busy(adapter, addr)) > - return 0; > + if (i2c_check_addr_busy(adap, addr)) > + return -EBUSY; > > /* Make sure there is something at this address */ > - if (!i2c_default_probe(adapter, addr)) > - return 0; > + if (!probe(adap, addr)) > + return -ENODEV; > + > + return 0; > +} > + > +static int i2c_detect_address(struct i2c_client *temp_client, > + struct i2c_driver *driver) > +{ > + struct i2c_board_info info; > + struct i2c_adapter *adapter = temp_client->adapter; > + int addr = temp_client->addr; > + int err; > + > + /* Only report broken addresses, busy addresses are no error */ > + err = i2c_scan_for_client(adapter, addr, i2c_default_probe); > + if (err < 0) > + return err == -EINVAL ? -EINVAL : 0; > > /* Finally call the custom detection function */ > memset(&info, 0, sizeof(struct i2c_board_info)); > @@ -2232,26 +2242,9 @@ i2c_new_scanned_device(struct i2c_adapter *adap, > if (!probe) > probe = i2c_default_probe; > > - for (i = 0; addr_list[i] != I2C_CLIENT_END; i++) { > - /* Check address validity */ > - if (i2c_check_7bit_addr_validity_strict(addr_list[i]) < 0) { > - dev_warn(&adap->dev, "Invalid 7-bit address 0x%02x\n", > - addr_list[i]); > - continue; > - } > - > - /* Check address availability (7 bit, no need to encode flags) */ > - if (i2c_check_addr_busy(adap, addr_list[i])) { > - dev_dbg(&adap->dev, > - "Address 0x%02x already in use, not probing\n", > - addr_list[i]); > - continue; > - } > - > - /* Test address responsiveness */ > - if (probe(adap, addr_list[i])) > + for (i = 0; addr_list[i] != I2C_CLIENT_END; i++) > + if (i2c_scan_for_client(adap, addr_list[i], probe) == 0) > break; > - } > > if (addr_list[i] == I2C_CLIENT_END) { > dev_dbg(&adap->dev, "Probing failed, no device found\n");
Hi Wolfram, On 31/12/2019 16:13, Wolfram Sang wrote: > There is a pattern to check for existence of a client which is copied in > i2c_detect_address() and i2c_new_scanned_device(): > > 1) check if address is valid > 2) check if address is already registered > 3) send a message and check the reponse s/reponse/response/ (My email client highlights spelling issues, sorry :-D) > Because this pattern will be needed a third time soon, refactor it into > its own function. This looks reasonable to me, I see Laurent has a concern over the use of a WARN to present a backtrace, but I think in this instance it will be useful as it will facilitate identifying what code path provided the incorrect address. Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > drivers/i2c/i2c-core-base.c | 57 ++++++++++++++++--------------------- > 1 file changed, 25 insertions(+), 32 deletions(-) > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > index a1eb28a3cc54..20a726dc78db 100644 > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > @@ -2108,29 +2108,39 @@ static int i2c_default_probe(struct i2c_adapter *adap, unsigned short addr) > return err >= 0; > } > > -static int i2c_detect_address(struct i2c_client *temp_client, > - struct i2c_driver *driver) > +static int i2c_scan_for_client(struct i2c_adapter *adap, unsigned short addr, > + int (*probe)(struct i2c_adapter *adap, unsigned short addr)) > { > - struct i2c_board_info info; > - struct i2c_adapter *adapter = temp_client->adapter; > - int addr = temp_client->addr; > int err; > > /* Make sure the address is valid */ > err = i2c_check_7bit_addr_validity_strict(addr); > - if (err) { > - dev_warn(&adapter->dev, "Invalid probe address 0x%02x\n", > - addr); > + if (WARN(err, "Invalid probe address 0x%02x\n", addr)) > return err; > - } > > /* Skip if already in use (7 bit, no need to encode flags) */ > - if (i2c_check_addr_busy(adapter, addr)) > - return 0; > + if (i2c_check_addr_busy(adap, addr)) > + return -EBUSY; > > /* Make sure there is something at this address */ > - if (!i2c_default_probe(adapter, addr)) > - return 0; > + if (!probe(adap, addr)) > + return -ENODEV; > + > + return 0; > +} > + > +static int i2c_detect_address(struct i2c_client *temp_client, > + struct i2c_driver *driver) > +{ > + struct i2c_board_info info; > + struct i2c_adapter *adapter = temp_client->adapter; > + int addr = temp_client->addr; > + int err; > + > + /* Only report broken addresses, busy addresses are no error */ > + err = i2c_scan_for_client(adapter, addr, i2c_default_probe); > + if (err < 0) > + return err == -EINVAL ? -EINVAL : 0; > > /* Finally call the custom detection function */ > memset(&info, 0, sizeof(struct i2c_board_info)); > @@ -2232,26 +2242,9 @@ i2c_new_scanned_device(struct i2c_adapter *adap, > if (!probe) > probe = i2c_default_probe; > > - for (i = 0; addr_list[i] != I2C_CLIENT_END; i++) { > - /* Check address validity */ > - if (i2c_check_7bit_addr_validity_strict(addr_list[i]) < 0) { > - dev_warn(&adap->dev, "Invalid 7-bit address 0x%02x\n", > - addr_list[i]); > - continue; > - } > - > - /* Check address availability (7 bit, no need to encode flags) */ > - if (i2c_check_addr_busy(adap, addr_list[i])) { > - dev_dbg(&adap->dev, > - "Address 0x%02x already in use, not probing\n", > - addr_list[i]); > - continue; > - } > - > - /* Test address responsiveness */ > - if (probe(adap, addr_list[i])) > + for (i = 0; addr_list[i] != I2C_CLIENT_END; i++) > + if (i2c_scan_for_client(adap, addr_list[i], probe) == 0) > break; > - } > > if (addr_list[i] == I2C_CLIENT_END) { > dev_dbg(&adap->dev, "Probing failed, no device found\n"); >
Hi Kieran, On Tue, Jan 7, 2020 at 10:26 AM Kieran Bingham <kieran@ksquared.org.uk> wrote: > This looks reasonable to me, I see Laurent has a concern over the use of > a WARN to present a backtrace, but I think in this instance it will be > useful as it will facilitate identifying what code path provided the > incorrect address. Quoting GregKH: | We really do not want WARN_ON() anywhere, as that causes systems with | panic-on-warn to reboot. https://lore.kernel.org/lkml/20191121135743.GA552517@kroah.com/ Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, On 07/01/2020 09:53, Geert Uytterhoeven wrote: > Hi Kieran, > > On Tue, Jan 7, 2020 at 10:26 AM Kieran Bingham <kieran@ksquared.org.uk> wrote: >> This looks reasonable to me, I see Laurent has a concern over the use of >> a WARN to present a backtrace, but I think in this instance it will be >> useful as it will facilitate identifying what code path provided the >> incorrect address. > > Quoting GregKH: > | We really do not want WARN_ON() anywhere, as that causes systems with > | panic-on-warn to reboot. Ugh , why would anyone panic-on-warn :) Panic on Panic! (or at least at the disco...) > > https://lore.kernel.org/lkml/20191121135743.GA552517@kroah.com/ > Well there we go then ... Perhaps: s/WARN/dev_err/ (it was previously dev_warn()) -- Kieran > Gr{oetje,eeting}s, > > Geert > > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds >
> Quoting GregKH: > | We really do not want WARN_ON() anywhere, as that causes systems with > | panic-on-warn to reboot. > > https://lore.kernel.org/lkml/20191121135743.GA552517@kroah.com/ Huh? This renders WARN completely useless, or? If somebody wants panic on warn, this person should get it? If we don't add a stack trace, we only know that *someone* registered an invalid address. Finding out who can be annoying. Kieran spotted this correctly.
Hi Wolfram, On Tue, Jan 7, 2020 at 11:26 AM Wolfram Sang <wsa@the-dreams.de> wrote: > > Quoting GregKH: > > | We really do not want WARN_ON() anywhere, as that causes systems with > > | panic-on-warn to reboot. > > > > https://lore.kernel.org/lkml/20191121135743.GA552517@kroah.com/ > > Huh? This renders WARN completely useless, or? If somebody wants panic > on warn, this person should get it? I also have my doubts... > If we don't add a stack trace, we only know that *someone* registered an > invalid address. Finding out who can be annoying. Kieran spotted this > correctly. What other information will you have in the backtrace, that you don't have available inside the function? Would printing the i2c_driver name be sufficient? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
> > > https://lore.kernel.org/lkml/20191121135743.GA552517@kroah.com/ > > > > Huh? This renders WARN completely useless, or? If somebody wants panic > > on warn, this person should get it? > > I also have my doubts... Good to know :) > What other information will you have in the backtrace, that you don't have > available inside the function? > Would printing the i2c_driver name be sufficient? Yes, but we don't have the client struct, only the adapter and the address: +static int i2c_scan_for_client(struct i2c_adapter *adap, unsigned short addr, + int (*probe)(struct i2c_adapter *adap, unsigned short addr)) And even if we had the client struct, it would be empty because for most cases scanning happens before binding to the driver. We don't even have the name/type in case of i2c_detect().
Hi, On 07/01/20 12:23, Wolfram Sang wrote: > >>>> https://lore.kernel.org/lkml/20191121135743.GA552517@kroah.com/ >>> >>> Huh? This renders WARN completely useless, or? If somebody wants panic >>> on warn, this person should get it? >> >> I also have my doubts... > > Good to know :) > >> What other information will you have in the backtrace, that you don't have >> available inside the function? >> Would printing the i2c_driver name be sufficient? > > Yes, but we don't have the client struct, only the adapter and the > address: > > +static int i2c_scan_for_client(struct i2c_adapter *adap, unsigned short addr, > + int (*probe)(struct i2c_adapter *adap, unsigned short addr)) > > And even if we had the client struct, it would be empty because for most > cases scanning happens before binding to the driver. We don't even have > the name/type in case of i2c_detect(). > Maybe dev_warn() + dump_stack()? Just my 2c,
> Maybe dev_warn() + dump_stack()?
That would technically work, yet it feels like a bit like we now
open-code WARN because we shall not use it. I will ask gkh about his
statement...
On 07/01/2020 16:45, Wolfram Sang wrote: > >> Maybe dev_warn() + dump_stack()? > > That would technically work, yet it feels like a bit like we now > open-code WARN because we shall not use it. I will ask gkh about his > statement... Indeed, that would be my expected purpose for WARN() :-( Please let us know what GKH says!
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c index a1eb28a3cc54..20a726dc78db 100644 --- a/drivers/i2c/i2c-core-base.c +++ b/drivers/i2c/i2c-core-base.c @@ -2108,29 +2108,39 @@ static int i2c_default_probe(struct i2c_adapter *adap, unsigned short addr) return err >= 0; } -static int i2c_detect_address(struct i2c_client *temp_client, - struct i2c_driver *driver) +static int i2c_scan_for_client(struct i2c_adapter *adap, unsigned short addr, + int (*probe)(struct i2c_adapter *adap, unsigned short addr)) { - struct i2c_board_info info; - struct i2c_adapter *adapter = temp_client->adapter; - int addr = temp_client->addr; int err; /* Make sure the address is valid */ err = i2c_check_7bit_addr_validity_strict(addr); - if (err) { - dev_warn(&adapter->dev, "Invalid probe address 0x%02x\n", - addr); + if (WARN(err, "Invalid probe address 0x%02x\n", addr)) return err; - } /* Skip if already in use (7 bit, no need to encode flags) */ - if (i2c_check_addr_busy(adapter, addr)) - return 0; + if (i2c_check_addr_busy(adap, addr)) + return -EBUSY; /* Make sure there is something at this address */ - if (!i2c_default_probe(adapter, addr)) - return 0; + if (!probe(adap, addr)) + return -ENODEV; + + return 0; +} + +static int i2c_detect_address(struct i2c_client *temp_client, + struct i2c_driver *driver) +{ + struct i2c_board_info info; + struct i2c_adapter *adapter = temp_client->adapter; + int addr = temp_client->addr; + int err; + + /* Only report broken addresses, busy addresses are no error */ + err = i2c_scan_for_client(adapter, addr, i2c_default_probe); + if (err < 0) + return err == -EINVAL ? -EINVAL : 0; /* Finally call the custom detection function */ memset(&info, 0, sizeof(struct i2c_board_info)); @@ -2232,26 +2242,9 @@ i2c_new_scanned_device(struct i2c_adapter *adap, if (!probe) probe = i2c_default_probe; - for (i = 0; addr_list[i] != I2C_CLIENT_END; i++) { - /* Check address validity */ - if (i2c_check_7bit_addr_validity_strict(addr_list[i]) < 0) { - dev_warn(&adap->dev, "Invalid 7-bit address 0x%02x\n", - addr_list[i]); - continue; - } - - /* Check address availability (7 bit, no need to encode flags) */ - if (i2c_check_addr_busy(adap, addr_list[i])) { - dev_dbg(&adap->dev, - "Address 0x%02x already in use, not probing\n", - addr_list[i]); - continue; - } - - /* Test address responsiveness */ - if (probe(adap, addr_list[i])) + for (i = 0; addr_list[i] != I2C_CLIENT_END; i++) + if (i2c_scan_for_client(adap, addr_list[i], probe) == 0) break; - } if (addr_list[i] == I2C_CLIENT_END) { dev_dbg(&adap->dev, "Probing failed, no device found\n");
There is a pattern to check for existence of a client which is copied in i2c_detect_address() and i2c_new_scanned_device(): 1) check if address is valid 2) check if address is already registered 3) send a message and check the reponse Because this pattern will be needed a third time soon, refactor it into its own function. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- drivers/i2c/i2c-core-base.c | 57 ++++++++++++++++--------------------- 1 file changed, 25 insertions(+), 32 deletions(-)