| Message ID | effeb0c0-36cb-afc4-4d9a-7ef348c928ae@gmail.com |
|---|---|
| State | Accepted |
| Commit | 2b3db4db660f9cc04878d81c2517f840763e6733 |
| Headers | show |
| Series | i2c: i801: Series with improvements | expand |
On Fri, Aug 06, 2021 at 11:15:15PM +0200, Heiner Kallweit wrote: > Replace the ugly cast of the return_value pointer with proper usage. > In addition use dmi_match() instead of open-coding it. ... > - acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, > - (void **)&found); > + acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, &err); > > - return found; > + return !IS_ERR(err); Shouldn't you also check the status of acpi_get_device()? -- With Best Regards, Andy Shevchenko
On 11.08.2021 17:45, Andy Shevchenko wrote: > On Fri, Aug 06, 2021 at 11:15:15PM +0200, Heiner Kallweit wrote: >> Replace the ugly cast of the return_value pointer with proper usage. >> In addition use dmi_match() instead of open-coding it. > > ... > >> - acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, >> - (void **)&found); >> + acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, &err); >> >> - return found; >> + return !IS_ERR(err); > > Shouldn't you also check the status of acpi_get_device()? > This shouldn't be needed because err isn't touched if function fails.
On Wed, Aug 11, 2021 at 10:28:25PM +0200, Heiner Kallweit wrote: > On 11.08.2021 17:45, Andy Shevchenko wrote: > > On Fri, Aug 06, 2021 at 11:15:15PM +0200, Heiner Kallweit wrote: > >> Replace the ugly cast of the return_value pointer with proper usage. > >> In addition use dmi_match() instead of open-coding it. > > > > ... > > > >> - acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, > >> - (void **)&found); > >> + acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, &err); > >> > >> - return found; > >> + return !IS_ERR(err); > > > > Shouldn't you also check the status of acpi_get_device()? > > > This shouldn't be needed because err isn't touched if function fails. For the sake of clearness of the code I would do it. But in any case what really hurt my eye is the last line here. To me sounds like if (IS_ERR(err)) return false; return true; is much better to read (and I bet the compiler will generate the very same code for it). -- With Best Regards, Andy Shevchenko
Hi Heiner, On Fri, 06 Aug 2021 23:15:15 +0200, Heiner Kallweit wrote: > Replace the ugly cast of the return_value pointer with proper usage. > In addition use dmi_match() instead of open-coding it. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > v2: > - avoid assigning potentially dangling pointer to *return_value > --- > drivers/i2c/busses/i2c-i801.c | 14 +++++--------- > 1 file changed, 5 insertions(+), 9 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c > index 89ae78ef1..f56060fcf 100644 > --- a/drivers/i2c/busses/i2c-i801.c > +++ b/drivers/i2c/busses/i2c-i801.c > @@ -1192,7 +1192,7 @@ static acpi_status check_acpi_smo88xx_device(acpi_handle obj_handle, > > kfree(info); > > - *((bool *)return_value) = true; > + *return_value = NULL; > return AE_CTRL_TERMINATE; > > smo88xx_not_found: > @@ -1202,11 +1202,9 @@ static acpi_status check_acpi_smo88xx_device(acpi_handle obj_handle, > > static bool is_dell_system_with_lis3lv02d(void) > { > - bool found; > - const char *vendor; > + void *err = ERR_PTR(-ENOENT); > > - vendor = dmi_get_system_info(DMI_SYS_VENDOR); > - if (!vendor || strcmp(vendor, "Dell Inc.")) > + if (!dmi_match(DMI_SYS_VENDOR, "Dell Inc.")) > return false; > > /* > @@ -1217,11 +1215,9 @@ static bool is_dell_system_with_lis3lv02d(void) > * accelerometer but unfortunately ACPI does not provide any other > * information (like I2C address). > */ > - found = false; > - acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, > - (void **)&found); > + acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, &err); > > - return found; > + return !IS_ERR(err); > } > > /* OK, it's a bit different from what I proposed but it still addresses the concerns that had been raised. So fine with me. Reviewed-by: Jean Delvare <jdelvare@suse.de> -- Jean Delvare SUSE L3 Support
On Thu, 12 Aug 2021 12:55:20 +0300, Andy Shevchenko wrote: > On Wed, Aug 11, 2021 at 10:28:25PM +0200, Heiner Kallweit wrote: > > On 11.08.2021 17:45, Andy Shevchenko wrote: > > > On Fri, Aug 06, 2021 at 11:15:15PM +0200, Heiner Kallweit wrote: > > >> Replace the ugly cast of the return_value pointer with proper usage. > > >> In addition use dmi_match() instead of open-coding it. > > > > > > ... > > > > > >> - acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, > > >> - (void **)&found); > > >> + acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, &err); > > >> > > >> - return found; > > >> + return !IS_ERR(err); > > > > > > Shouldn't you also check the status of acpi_get_device()? > > > > This shouldn't be needed because err isn't touched if function fails. > > For the sake of clearness of the code I would do it. This brings us back to how awkward the API is. Most callers don't bother checking the return value of acpi_get_devices() because it's useless in practice. But I agree that in theory it could return with an error and then it would be nicer to catch that. > (...) But in any case what > really hurt my eye is the last line here. To me sounds like > > if (IS_ERR(err)) > return false; > return true; > > is much better to read (and I bet the compiler will generate the very same > code for it). Somehow the assembly code differs, but I'm unable to see the relation between your proposed change and the assembly code changes. That's why I hate modern compilers. They pretend to be smart, but what they are essentially is unstable, and this ruins any attempt at such trivial comparisons. Sad. Personally I don't really care, Heiner's code did not strike me as being hard to read in the first place. I tend to avoid conditionals when possible. -- Jean Delvare SUSE L3 Support
On Fri, Aug 06, 2021 at 11:15:15PM +0200, Heiner Kallweit wrote: > Replace the ugly cast of the return_value pointer with proper usage. > In addition use dmi_match() instead of open-coding it. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> Applied to for-next, thanks!
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index 89ae78ef1..f56060fcf 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -1192,7 +1192,7 @@ static acpi_status check_acpi_smo88xx_device(acpi_handle obj_handle, kfree(info); - *((bool *)return_value) = true; + *return_value = NULL; return AE_CTRL_TERMINATE; smo88xx_not_found: @@ -1202,11 +1202,9 @@ static acpi_status check_acpi_smo88xx_device(acpi_handle obj_handle, static bool is_dell_system_with_lis3lv02d(void) { - bool found; - const char *vendor; + void *err = ERR_PTR(-ENOENT); - vendor = dmi_get_system_info(DMI_SYS_VENDOR); - if (!vendor || strcmp(vendor, "Dell Inc.")) + if (!dmi_match(DMI_SYS_VENDOR, "Dell Inc.")) return false; /* @@ -1217,11 +1215,9 @@ static bool is_dell_system_with_lis3lv02d(void) * accelerometer but unfortunately ACPI does not provide any other * information (like I2C address). */ - found = false; - acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, - (void **)&found); + acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, &err); - return found; + return !IS_ERR(err); } /*
Replace the ugly cast of the return_value pointer with proper usage. In addition use dmi_match() instead of open-coding it. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- v2: - avoid assigning potentially dangling pointer to *return_value --- drivers/i2c/busses/i2c-i801.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-)