Hi Pali, Heiner,
On Fri, 6 Aug 2021 01:08:18 +0200, Pali Rohár wrote:
> On Thursday 05 August 2021 21:42:23 Heiner Kallweit wrote:
> > On 05.08.2021 21:11, Pali Rohár wrote:
> > > On Thursday 05 August 2021 11:51:56 Jean Delvare wrote:
> > >> On Sun, 01 Aug 2021 16:20:19 +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.
> > >>
> > >> Pali, would you be able to test this patch?
> > >
> > > Tested now on Latitude E6440 and patch is working fine (no difference).
Thank you for joining the discussion and testing.
> > >>> Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx>
> > >>> ---
> > >>> drivers/i2c/busses/i2c-i801.c | 13 ++++---------
> > >>> 1 file changed, 4 insertions(+), 9 deletions(-)
> > >>>
> > >>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> > >>> index d971ee20c..a6287c520 100644
> > >>> --- a/drivers/i2c/busses/i2c-i801.c
> > >>> +++ b/drivers/i2c/busses/i2c-i801.c
> > >>> @@ -1191,7 +1191,7 @@ static acpi_status check_acpi_smo88xx_device(acpi_handle obj_handle,
> > >>>
> > >>> kfree(info);
> > >>>
> > >>> - *((bool *)return_value) = true;
> > >>> + *return_value = obj_handle;
> > >
> > > You are missing a cast here. "obj_handle" is of unknown typedef type
> > > acpi_handle and *return_value is of type void*. So this can generate a
> > > compile warning (either now or in future).
> >
> > acpi_handle is defined as: typedef void *acpi_handle;
> > Therefore compiler is happy (as long as acpi_handle is any pointer type).
>
> But point of this typedefing is to hide real type and let user to use
> this "unknown" type without excepting any specific type.
>
> "Therefore compiler is happy" here is there just a "hack" which
> currently mute casting warning. But I think it is not something which
> should be used as it is against how API / code of specific function was
> designed.
But you can't respect that "unknown type" and still code cleanly. The
definition of acpi_handle hides the fact that this is a pointer type.
And you can't code cleanly if you need to manipulate an "object" but
have no idea whether it's a pointer, a scalar value or a structure.
(Well, you *could* with an API which does all the manipulation for you.
But that's not what we have here.)
In my opinion, the only value of "acpi_handle" as it is currently
defined is to let people know what type of data is found behind that
pointer. But I would much prefer real structure and an explicit pointer
to it.
> For me this situation looks like: Somebody created API and specified how
> to use it. It was realized that specified usage is not ideal for some
> operations. And then people started "hacking" this API to look better
> in these special cases.
>
> But solution for this issue is to fix API (or create a new API which
> better for this purpose), not hacking or workarounding it to looks
> better by hiding / workarounding other important details.
The practical issue here is that we are talking about ACPICA, which is
partly developed outside / independently of the rest of the kernel. If
you can convince the ACPICA developers to implement a better
alternative to acpi_get_devices(), and/or make acpi_handle a better
type, I'll be more than happy to use that in the i2c-i801 driver. But I
don't see that happening any time soon, if ever, and for the time
being, we have to live with the poor API we are given.
> > > So you need to write it something like this:
> > >
> > > *((acpi_handle *)return_value) = obj_handle;
> > >
> > > But what is benefit of this change? Is not usage of explicit true and
> > > false values better than some acpi_handle type of undefined value stored
> > > in obj_handle?
> >
> > From a logical perspective I agree. My motivation is that I see explicit
> > casts as a last resort and try to avoid them as far as possible.
I tend to agree with that, FWIW.
> But in this case you really should not avoid casting. It is different
> pointer type of unknown (or rather hidden) type. Currently it does not
> throw warning (maybe because compiler is not smart enough). But it does
> not mean that code is really semantically correct or that in future
> compiler (or its new version) does not throw warning.
It's not about the smartness of the compiler. acpi_handle is equal to
void *, and Heiner's code is perfectly valid for a void *. No cast
needed with any compiler or on any platform.
Of course, things would break if the definition of acpi_handle ever
changes. Which would be great new actually, as I wrote above. And we
can revisit the code then.
> Syntactically code looks better, but only until reader start studding
> what code is really doing.
I have to admit I got pretty confused when reading it at first. On the
other hand, I was equally confused by the code that it attempts to
replace ^^
> > The current code abuses a void* variable to store a bool. This makes the
> > implicit assumption that a pointer variable is always big enough to
> > store a bool.
>
> I understand your concerns and also your motivation. API is not ideal
> for usage. But both current and your proposed solution is just a hack to
> workaround this API usage.
>
> I think that according to C standard it is possible to cast between
> pointer and non-pointer (integer-like) types only via uintptr_t (or
> intptr_t) type...
>
> So compliant C code could look like this?
>
> void func(void **ret) {
> *ret = (void *)(uintptr_t)1;
> }
>
> bool test(void) {
> void *found = (uintptr_t)0;
> func(&found);
> return (uintptr_t)found;
> }
>
> or test() function may be simplified:
>
> bool test(void) {
> void *found = NULL;
> func(&found);
> return found;
> }
>
> (but for me it looks strange if I'm reading _word_ NULL when used as a
> false value in 2-state logic variable)
I don't like this and I'm not even sure if that is allowed in the kernel.
> > With regard to "acpi_handle of undefined value": I'm just interested
> > in the information whether handle is NULL or not. That's the normal
> > implicit cast to bool like in every if(pointer) clause.
>
> Yes, of course, this is fully valid.
> (...)
> > > Anyway, it looks strange to use name "found" for object handle
> > > variable. I would expect that something named "found" is storing
> > > something which refers to 2-state logic and not some handle value.
It's actually a rather common pattern for lookup functions, returning
NULL when the expected item wasn't found, or a pointer to the item if
found. What makes things a bit weird here is that we don't actually
care about acpi_handle. All we need is a pointer to pretty much
anything, to differentiate between the found and not found cases.
Therefore I would propose the following alternative:
--- linux-5.13.orig/drivers/i2c/busses/i2c-i801.c 2021-08-06 11:11:44.275200299 +0200
+++ linux-5.13/drivers/i2c/busses/i2c-i801.c 2021-08-06 11:18:19.847469822 +0200
@@ -1194,7 +1194,7 @@ static acpi_status check_acpi_smo88xx_de
kfree(info);
- *((bool *)return_value) = true;
+ *return_value = hid; /* Could be any address, used as true value */
return AE_CTRL_TERMINATE;
smo88xx_not_found:
@@ -1204,13 +1204,10 @@ static acpi_status check_acpi_smo88xx_de
static bool is_dell_system_with_lis3lv02d(void)
{
- bool found;
- const char *vendor;
+ acpi_handle found = NULL;
- vendor = dmi_get_system_info(DMI_SYS_VENDOR);
- if (!vendor || strcmp(vendor, "Dell Inc."))
+ if (!dmi_match(DMI_SYS_VENDOR, "Dell Inc."))
return false;
-
/*
* Check that ACPI device SMO88xx is present and is functioning.
* Function acpi_get_devices() already filters all ACPI devices
@@ -1219,9 +1216,7 @@ static bool is_dell_system_with_lis3lv02
* 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, &found);
return found;
}
Basically, it's Heiner's patch except for one line, the idea is to
return the HID string pointer (which has type char *) instead of the
acpi_handle. That way we don't depend on an opaque ACPI type. (I first
tried with the matching acpi_smo8800_ids entry but compiler complained
about incompatible pointer types due to the const).
Actually, I think we could use pretty much ANY pointer. Heck, that
would work too:
*return_value = &disable_features; /* Could be any address, used as true value */
Would be kinda confusing, but the comment is supposed to address that.
In fact we could even do:
*return_value = &i; /* Could be any address, used as true value */
That's the address of a local variable on the stack, which will no
longer exist by the time we check it, but that's still a non-NULL
pointer so it would work :-D Seriously, let's not do that, simply
because static code analyzers would possibly complain.
--
Jean Delvare
SUSE L3 Support
