Re: [PATCH 05/10] i2c: i801: Improve is_dell_system_with_lis3lv02d

[Jean Delvare <jdelvare@suse.de>]

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@gmail.com>
> > >>> ---
> > >>>  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