| Message ID | 5d8e72e2-085b-32ea-0a86-eeecfe1e94f3@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: i801: Series with improvements | expand |
On Sun, 01 Aug 2021 16:23:34 +0200, Heiner Kallweit wrote: > - Use an initializer for struct i2c_board_info info > - Use dmi_match() > - Simplify loop logic > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/i2c/busses/i2c-i801.c | 28 +++++++++------------------- > 1 file changed, 9 insertions(+), 19 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c > index 958b2e14b..1ca92a1e0 100644 > --- a/drivers/i2c/busses/i2c-i801.c > +++ b/drivers/i2c/busses/i2c-i801.c > @@ -1245,28 +1245,18 @@ static const struct { > > static void register_dell_lis3lv02d_i2c_device(struct i801_priv *priv) > { > - struct i2c_board_info info; > - const char *dmi_product_name; > + struct i2c_board_info info = { .type = "lis3lv02d" }; Can it be moved to the inner loop where it is used, so that initialization only takes place when needed? I don't know how the compiler handles that, to be honest. > int i; > > - dmi_product_name = dmi_get_system_info(DMI_PRODUCT_NAME); > - for (i = 0; i < ARRAY_SIZE(dell_lis3lv02d_devices); ++i) { > - if (strcmp(dmi_product_name, > - dell_lis3lv02d_devices[i].dmi_product_name) == 0) > - break; > - } > - > - if (i == ARRAY_SIZE(dell_lis3lv02d_devices)) { > - dev_warn(&priv->pci_dev->dev, > - "Accelerometer lis3lv02d is present on SMBus but its" > - " address is unknown, skipping registration\n"); > - return; > - } > + for (i = 0; i < ARRAY_SIZE(dell_lis3lv02d_devices); ++i) Outer block without curly braces is discouraged for readability and maintenance reasons (see Documentation/process/coding-style.rst section 3). > + if (dmi_match(DMI_PRODUCT_NAME, dell_lis3lv02d_devices[i].dmi_product_name)) { This causes dmi_get_system_info(DMI_PRODUCT_NAME) to be called for every iteration of the loop, slowing down the lookup. It's an exported function so it can't be inlined by the compiler. I know this happens only once, but we try to keep boot times as short as possible. > + info.addr = dell_lis3lv02d_devices[i].i2c_addr; > + i2c_new_client_device(&priv->adapter, &info); > + return; > + } > > - memset(&info, 0, sizeof(struct i2c_board_info)); > - info.addr = dell_lis3lv02d_devices[i].i2c_addr; > - strlcpy(info.type, "lis3lv02d", I2C_NAME_SIZE); > - i2c_new_client_device(&priv->adapter, &info); > + pci_warn(priv->pci_dev, > + "Accelerometer lis3lv02d is present on SMBus but its address is unknown, skipping registration\n"); > } > > /* Register optional slaves */
On 05.08.2021 16:23, Jean Delvare wrote: > On Sun, 01 Aug 2021 16:23:34 +0200, Heiner Kallweit wrote: >> - Use an initializer for struct i2c_board_info info >> - Use dmi_match() >> - Simplify loop logic >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> drivers/i2c/busses/i2c-i801.c | 28 +++++++++------------------- >> 1 file changed, 9 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c >> index 958b2e14b..1ca92a1e0 100644 >> --- a/drivers/i2c/busses/i2c-i801.c >> +++ b/drivers/i2c/busses/i2c-i801.c >> @@ -1245,28 +1245,18 @@ static const struct { >> >> static void register_dell_lis3lv02d_i2c_device(struct i801_priv *priv) >> { >> - struct i2c_board_info info; >> - const char *dmi_product_name; >> + struct i2c_board_info info = { .type = "lis3lv02d" }; > > Can it be moved to the inner loop where it is used, so that > initialization only takes place when needed? I don't know how the > compiler handles that, to be honest. > >> int i; >> >> - dmi_product_name = dmi_get_system_info(DMI_PRODUCT_NAME); >> - for (i = 0; i < ARRAY_SIZE(dell_lis3lv02d_devices); ++i) { >> - if (strcmp(dmi_product_name, >> - dell_lis3lv02d_devices[i].dmi_product_name) == 0) >> - break; >> - } >> - >> - if (i == ARRAY_SIZE(dell_lis3lv02d_devices)) { >> - dev_warn(&priv->pci_dev->dev, >> - "Accelerometer lis3lv02d is present on SMBus but its" >> - " address is unknown, skipping registration\n"); >> - return; >> - } >> + for (i = 0; i < ARRAY_SIZE(dell_lis3lv02d_devices); ++i) > > Outer block without curly braces is discouraged for readability and > maintenance reasons (see Documentation/process/coding-style.rst section > 3). > >> + if (dmi_match(DMI_PRODUCT_NAME, dell_lis3lv02d_devices[i].dmi_product_name)) { > > This causes dmi_get_system_info(DMI_PRODUCT_NAME) to be called for > every iteration of the loop, slowing down the lookup. It's an exported > function so it can't be inlined by the compiler. I know this happens > only once, but we try to keep boot times as short as possible. > I'm aware of this. However we just talk about a small in-memory operation and the performance impact should be neglectable. dmi_get_system_info() is just the following: const char *dmi_get_system_info(int field) { return dmi_ident[field]; } EXPORT_SYMBOL(dmi_get_system_info); I'd rate the simpler and better maintainable code higher. But that's just a personal opinion and mileage may vary. >> + info.addr = dell_lis3lv02d_devices[i].i2c_addr; >> + i2c_new_client_device(&priv->adapter, &info); >> + return; >> + } >> >> - memset(&info, 0, sizeof(struct i2c_board_info)); >> - info.addr = dell_lis3lv02d_devices[i].i2c_addr; >> - strlcpy(info.type, "lis3lv02d", I2C_NAME_SIZE); >> - i2c_new_client_device(&priv->adapter, &info); >> + pci_warn(priv->pci_dev, >> + "Accelerometer lis3lv02d is present on SMBus but its address is unknown, skipping registration\n"); >> } >> >> /* Register optional slaves */ > >
Hi Heiner, On Fri, 6 Aug 2021 22:49:00 +0200, Heiner Kallweit wrote: > On 05.08.2021 16:23, Jean Delvare wrote: > > On Sun, 01 Aug 2021 16:23:34 +0200, Heiner Kallweit wrote: > >> + if (dmi_match(DMI_PRODUCT_NAME, dell_lis3lv02d_devices[i].dmi_product_name)) { > > > > This causes dmi_get_system_info(DMI_PRODUCT_NAME) to be called for > > every iteration of the loop, slowing down the lookup. It's an exported > > function so it can't be inlined by the compiler. I know this happens > > only once, but we try to keep boot times as short as possible. > > > I'm aware of this. However we just talk about a small in-memory operation and > the performance impact should be neglectable. dmi_get_system_info() is just > the following: > > const char *dmi_get_system_info(int field) > { > return dmi_ident[field]; > } > EXPORT_SYMBOL(dmi_get_system_info); > > I'd rate the simpler and better maintainable code higher. > But that's just a personal opinion and mileage may vary. I'm not worried about multiple calls to dmi_get_system_info(), which is indeed simple and inlined anyway, but about multiple calls to dmi_match (which can't be inlined). Function calls have a high cost (which is the very reason why the compiler will try to inline functions whenever possible). I wouldn't mind if you were replacing several lines of code, but here you are only removing one local variable and one simple line of code, or 15 bytes of binary code total. But you add up to 8 function calls, and that number could grow in the future as we add support for more devices. That's why I say the benefit of the change is questionable. If it was new code, I probably wouldn't mind. But when changing existing code, I need to be convinced that the new code is unquestionably better than what we had before. That's not the case here. (And don't get me wrong, I would love to live in a world where you don't have do choose between best performance and and systematic use of existing APIs. Alas, we often have to make choices in either direction.)
On 09.08.2021 15:33, Jean Delvare wrote: > Hi Heiner, > > On Fri, 6 Aug 2021 22:49:00 +0200, Heiner Kallweit wrote: >> On 05.08.2021 16:23, Jean Delvare wrote: >>> On Sun, 01 Aug 2021 16:23:34 +0200, Heiner Kallweit wrote: >>>> + if (dmi_match(DMI_PRODUCT_NAME, dell_lis3lv02d_devices[i].dmi_product_name)) { >>> >>> This causes dmi_get_system_info(DMI_PRODUCT_NAME) to be called for >>> every iteration of the loop, slowing down the lookup. It's an exported >>> function so it can't be inlined by the compiler. I know this happens >>> only once, but we try to keep boot times as short as possible. >>> >> I'm aware of this. However we just talk about a small in-memory operation and >> the performance impact should be neglectable. dmi_get_system_info() is just >> the following: >> >> const char *dmi_get_system_info(int field) >> { >> return dmi_ident[field]; >> } >> EXPORT_SYMBOL(dmi_get_system_info); >> >> I'd rate the simpler and better maintainable code higher. >> But that's just a personal opinion and mileage may vary. > > I'm not worried about multiple calls to dmi_get_system_info(), which is > indeed simple and inlined anyway, but about multiple calls to dmi_match > (which can't be inlined). Function calls have a high cost (which is the > very reason why the compiler will try to inline functions whenever > possible). > > I wouldn't mind if you were replacing several lines of code, > but here you are only removing one local variable and one simple line > of code, or 15 bytes of binary code total. But you add up to 8 function > calls, and that number could grow in the future as we add support for > more devices. That's why I say the benefit of the change is > questionable. > This code is called only if is_dell_system_with_lis3lv02d() returns true, what further reduces the impact of what you mention. But sure, the preference is a question of personal taste. Let me know whether you have any other review comments regarding v2 of the series, then I'll drop this change in v3. > If it was new code, I probably wouldn't mind. But when changing > existing code, I need to be convinced that the new code is > unquestionably better than what we had before. That's not the case here. > > (And don't get me wrong, I would love to live in a world where you don't > have do choose between best performance and and systematic use of > existing APIs. Alas, we often have to make choices in either direction.) >
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index 958b2e14b..1ca92a1e0 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -1245,28 +1245,18 @@ static const struct { static void register_dell_lis3lv02d_i2c_device(struct i801_priv *priv) { - struct i2c_board_info info; - const char *dmi_product_name; + struct i2c_board_info info = { .type = "lis3lv02d" }; int i; - dmi_product_name = dmi_get_system_info(DMI_PRODUCT_NAME); - for (i = 0; i < ARRAY_SIZE(dell_lis3lv02d_devices); ++i) { - if (strcmp(dmi_product_name, - dell_lis3lv02d_devices[i].dmi_product_name) == 0) - break; - } - - if (i == ARRAY_SIZE(dell_lis3lv02d_devices)) { - dev_warn(&priv->pci_dev->dev, - "Accelerometer lis3lv02d is present on SMBus but its" - " address is unknown, skipping registration\n"); - return; - } + for (i = 0; i < ARRAY_SIZE(dell_lis3lv02d_devices); ++i) + if (dmi_match(DMI_PRODUCT_NAME, dell_lis3lv02d_devices[i].dmi_product_name)) { + info.addr = dell_lis3lv02d_devices[i].i2c_addr; + i2c_new_client_device(&priv->adapter, &info); + return; + } - memset(&info, 0, sizeof(struct i2c_board_info)); - info.addr = dell_lis3lv02d_devices[i].i2c_addr; - strlcpy(info.type, "lis3lv02d", I2C_NAME_SIZE); - i2c_new_client_device(&priv->adapter, &info); + pci_warn(priv->pci_dev, + "Accelerometer lis3lv02d is present on SMBus but its address is unknown, skipping registration\n"); } /* Register optional slaves */
- Use an initializer for struct i2c_board_info info - Use dmi_match() - Simplify loop logic Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/i2c/busses/i2c-i801.c | 28 +++++++++------------------- 1 file changed, 9 insertions(+), 19 deletions(-)