Re: [PATCH v5 2/4] Fix SW_TABLET_MODE detection method

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Thu, 8 Aug 2024, Stefan Sichler wrote:

> this patch (which is now committed to the kernel as commit
> 520ee4ea1cc60251a6e3c911cf0336278aa52634 since v5.18-rc1) unfortunately
> introduced a regression on my HP EliteBook 2760p Convertible:
> 
> Tablet mode is no longer detected.
> 
> It worked flawlessly before (when enable_tablet_mode_sw module param was
> set to 1).
> 
> Debugging showed that on this device, two problems prevent the table
> mode detection from working:
> 
>   - Chassis Type is reported as 0x10 (= Lunch Box)
> 
>   - the query of HPWMI_SYSTEM_DEVICE_MODE does not report tablet state
> at all
> 
> Note that the chassis type of this device (switch to tablet mode by
> screen *rotation*) actually differs from the newer HP models (switch to
> tablet mode by screen *flipping*).
> 
> 
> I suggest fixing this by re-adding the removed module parameter
> "enable_tablet_mode_sw", but change its behavior to work in the
> following way:
> 
>   - when left at default -1 (auto): no change to current (new)
> implementation
> 
>   - when set to 0: unconditionally disable table mode reporting at all
> 
>   - when set to 1: ignore Chassis type and use old-skool
> hp_wmi_hw_state(HPWMI_TABLET_MASK) query method to determine tablet mode
> in addition to new hp_wmi_perform_query(HPWMI_SYSTEM_DEVICE_MODE...) method
> 
> 
> I prepared a patch based on commit
> 520ee4ea1cc60251a6e3c911cf0336278aa52634, and tested it successfully on
> my device.
> See below.
> 
> Regards,
> Stefan
> 
> --- hp-wmi.c.orig	2024-03-10 21:38:09.000000000 +0100
> +++ hp-wmi.c	2024-08-08 09:23:29.509113900 +0200

This submission does not follow the normal patch formatting guidelines,
please see Documentation/process/submitting-patches.rst.

> @@ -35,6 +35,10 @@
>  MODULE_ALIAS("wmi:95F24279-4D7B-4334-9387-ACCDC67EF61C");
>  MODULE_ALIAS("wmi:5FB7F034-2C63-45e9-BE91-3D44E2C707E4");
> 
> +static int enable_tablet_mode_sw = -1;
> +module_param(enable_tablet_mode_sw, int, 0444);
> +MODULE_PARM_DESC(enable_tablet_mode_sw, "Enable SW_TABLET_MODE
> reporting (-1=auto, 0=no, 1=yes)");
> +
>  #define HPWMI_EVENT_GUID "95F24279-4D7B-4334-9387-ACCDC67EF61C"
>  #define HPWMI_BIOS_GUID "5FB7F034-2C63-45e9-BE91-3D44E2C707E4"
>  #define HP_OMEN_EC_THERMAL_PROFILE_OFFSET 0x95
> @@ -428,6 +432,9 @@
>  	bool tablet_found;
>  	int ret;
> 
> +	if (!enable_tablet_mode_sw)
> +		return -ENODEV;
> +
>  	chassis_type = dmi_get_system_info(DMI_CHASSIS_TYPE);
>  	if (!chassis_type)
>  		return -ENODEV;
> @@ -435,16 +442,24 @@
>  	tablet_found = match_string(tablet_chassis_types,
>  				    ARRAY_SIZE(tablet_chassis_types),
>  				    chassis_type) >= 0;
> -	if (!tablet_found)
> +	if (!tablet_found && enable_tablet_mode_sw < 0 /*auto*/)

Having to add a comment like that is a very strong indication you'd want 
to have a named define instead, e.g., HPWMI_TABLET_MODE_AUTO.

>  		return -ENODEV;
> 
>  	ret = hp_wmi_perform_query(HPWMI_SYSTEM_DEVICE_MODE, HPWMI_READ,
>  				   system_device_mode,
> zero_if_sup(system_device_mode),
>  				   sizeof(system_device_mode));
> -	if (ret < 0)
> -		return ret;
> +	if (ret >= 0)
> +		ret = (system_device_mode[0] == DEVICE_MODE_TABLET);
> +
> +	/* workaround for older convertibles */
> +	if (ret <= 0 && enable_tablet_mode_sw > 0)
> +	{
> +		ret = hp_wmi_read_int(HPWMI_HARDWARE_QUERY);
> +		if (!(ret < 0))
> +			ret = !!(ret & HPWMI_TABLET_MASK);
> +	}
> 
> -	return system_device_mode[0] == DEVICE_MODE_TABLET;
> +	return ret;

The logic is quite hard to follow. It would be better to return 
early.

	if (ret < 0 && enable_tablet_mode_sw == HPWMI_TABLET_MODE_AUTO)
		return ret;

	if (ret >= 0 && system_device_mode[0] == DEVICE_MODE_TABLET)
		return 1;

	ret = hp_wmi_read_int(HPWMI_HARDWARE_QUERY);
	if (ret < 0)
		return ret;

	return !!(ret & HPWMI_TABLET_MASK);


However, automatically detecting this condition over adding the module 
parameter would be the preferred solution.

-- 
 i.

>  }
> 
>  static int omen_thermal_profile_set(int mode)
> 
>