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

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 3/10/22 22:08, Jorge Lopez wrote:
> The purpose of this patch is to introduce a fix and removal of the
> current hack when determining tablet mode status.
> 
> Determining the tablet mode status requires reading Byte 0 bit 2 as
> reported by HPWMI_HARDWARE_QUERY.  The investigation identified the
> failure was rooted in two areas: HPWMI_HARDWARE_QUERY failure (0x05)
> and reading Byte 0, bit 2 only to determine the table mode status.
> HPWMI_HARDWARE_QUERY WMI failure also rendered the dock state value
> invalid.
> 
> The latest changes use SMBIOS Type 3 (chassis type) and WMI Command
> 0x40 (device_mode_status) information to determine if the device is
> in tablet mode or not.
> 
> hp_wmi_hw_state function was split into two functions;
> hp_wmi_get_dock_state and hp_wmi_get_tablet_mode.  The new functions
> separate how dock_state and tablet_mode is handled in a cleaner
> manner.
> 
> All changes were validated on a HP ZBook Workstation notebook,
> HP EliteBook x360, and HP EliteBook 850 G8.
> 
> Signed-off-by: Jorge Lopez <jorge.lopez2@hp.com>
> 
> ---
> Based on the latest platform-drivers-x86.git/for-next
> 
> This patch requires  patch "Fix hp_wmi_read_int() reporting
> error (0x05)" in order to work correctly.
> ---
>  drivers/platform/x86/hp-wmi.c | 71 +++++++++++++++++++++++++----------
>  1 file changed, 52 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
> index 103f56399ed0..e9aa05c26a40 100644
> --- a/drivers/platform/x86/hp-wmi.c
> +++ b/drivers/platform/x86/hp-wmi.c
> @@ -35,10 +35,6 @@ MODULE_LICENSE("GPL");
>  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
> @@ -107,6 +103,7 @@ enum hp_wmi_commandtype {
>  	HPWMI_FEATURE2_QUERY		= 0x0d,
>  	HPWMI_WIRELESS2_QUERY		= 0x1b,
>  	HPWMI_POSTCODEERROR_QUERY	= 0x2a,
> +	HPWMI_SYSTEM_DEVICE_MODE	= 0x40,
>  	HPWMI_THERMAL_PROFILE_QUERY	= 0x4c,
>  };
>  
> @@ -217,6 +214,18 @@ struct rfkill2_device {
>  static int rfkill2_count;
>  static struct rfkill2_device rfkill2[HPWMI_MAX_RFKILL2_DEVICES];
>  
> +/* Chassis Types values were obtained from SMBIOS reference
> + * specification version 3.00. A complete list of system enclosures
> + * and chassis types is available on Table 17. 
> + */
> +static const char * const tablet_chassis_types[] = {
> +	"30", /* Tablet*/
> +	"31", /* Convertible */
> +	"32"  /* Detachable */
> +};
> +
> +#define DEVICE_MODE_TABLET	0x06
> +
>  /* map output size to the corresponding WMI method id */
>  static inline int encode_outsize_for_pvsz(int outsize)
>  {
> @@ -345,14 +354,40 @@ static int hp_wmi_read_int(int query)
>  	return val;
>  }
>  
> -static int hp_wmi_hw_state(int mask)
> +static int hp_wmi_get_dock_state(void)
>  {
>  	int state = hp_wmi_read_int(HPWMI_HARDWARE_QUERY);
>  
>  	if (state < 0)
>  		return state;
>  
> -	return !!(state & mask);
> +	return !!(state & HPWMI_DOCK_MASK);
> +}
> +
> +static int hp_wmi_get_tablet_mode(void)
> +{
> +	char system_device_mode[4] = { 0 };
> +	int ret;
> +	bool tablet_found = false;
> +
> +	const char *chassis_type = dmi_get_system_info(DMI_CHASSIS_TYPE);

This new-line in the mids of the variable declaration block looks 
weird. Also we usually put variable declarations at the top of
a function in reverse-christmas tree order (longest variable
declarations first).

I've fixed this up to look like this:

	char system_device_mode[4] = { 0 };
	const char *chassis_type;
	bool tablet_found;
	int ret;

	chassis_type = dmi_get_system_info(DMI_CHASSIS_TYPE);
	if (!chassis_type)
		return -ENODEV;

(no functional changes).

> +	tablet_found = match_string(tablet_chassis_types,
> +			    ARRAY_SIZE(tablet_chassis_types),
> +			    chassis_type) >= 0;
> +	if (!tablet_found)
> +		return -ENODEV;
> +
> +	ret = hp_wmi_perform_query(HPWMI_SYSTEM_DEVICE_MODE, HPWMI_READ,
> +				       system_device_mode, 0, sizeof(system_device_mode));
> +

The newline here between the call + check looks weird, I've dropped this while
merging the patch:

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans


> +	if (ret < 0)
> +		return ret;
> +
> +	return system_device_mode[0] == DEVICE_MODE_TABLET;
>  }
>  
>  static int omen_thermal_profile_set(int mode)
> @@ -568,7 +603,7 @@ static ssize_t als_show(struct device *dev, struct device_attribute *attr,
>  static ssize_t dock_show(struct device *dev, struct device_attribute *attr,
>  			 char *buf)
>  {
> -	int value = hp_wmi_hw_state(HPWMI_DOCK_MASK);
> +	int value = hp_wmi_get_dock_state();
>  	if (value < 0)
>  		return value;
>  	return sprintf(buf, "%d\n", value);
> @@ -577,7 +612,7 @@ static ssize_t dock_show(struct device *dev, struct device_attribute *attr,
>  static ssize_t tablet_show(struct device *dev, struct device_attribute *attr,
>  			   char *buf)
>  {
> -	int value = hp_wmi_hw_state(HPWMI_TABLET_MASK);
> +	int value = hp_wmi_get_tablet_mode();
>  	if (value < 0)
>  		return value;
>  	return sprintf(buf, "%d\n", value);
> @@ -699,10 +734,10 @@ static void hp_wmi_notify(u32 value, void *context)
>  	case HPWMI_DOCK_EVENT:
>  		if (test_bit(SW_DOCK, hp_wmi_input_dev->swbit))
>  			input_report_switch(hp_wmi_input_dev, SW_DOCK,
> -					    hp_wmi_hw_state(HPWMI_DOCK_MASK));
> +					    hp_wmi_get_dock_state());
>  		if (test_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit))
>  			input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE,
> -					    hp_wmi_hw_state(HPWMI_TABLET_MASK));
> +					    hp_wmi_get_tablet_mode());
>  		input_sync(hp_wmi_input_dev);
>  		break;
>  	case HPWMI_PARK_HDD:
> @@ -780,19 +815,17 @@ static int __init hp_wmi_input_setup(void)
>  	__set_bit(EV_SW, hp_wmi_input_dev->evbit);
>  
>  	/* Dock */
> -	val = hp_wmi_hw_state(HPWMI_DOCK_MASK);
> +	val = hp_wmi_get_dock_state();
>  	if (!(val < 0)) {
>  		__set_bit(SW_DOCK, hp_wmi_input_dev->swbit);
>  		input_report_switch(hp_wmi_input_dev, SW_DOCK, val);
>  	}
>  
>  	/* Tablet mode */
> -	if (enable_tablet_mode_sw > 0) {
> -		val = hp_wmi_hw_state(HPWMI_TABLET_MASK);
> -		if (val >= 0) {
> -			__set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit);
> -			input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, val);
> -		}
> +	val = hp_wmi_get_tablet_mode();
> +	if (!(val < 0)) {
> +		__set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit);
> +		input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, val);
>  	}
>  
>  	err = sparse_keymap_setup(hp_wmi_input_dev, hp_wmi_keymap, NULL);
> @@ -1227,10 +1260,10 @@ static int hp_wmi_resume_handler(struct device *device)
>  	if (hp_wmi_input_dev) {
>  		if (test_bit(SW_DOCK, hp_wmi_input_dev->swbit))
>  			input_report_switch(hp_wmi_input_dev, SW_DOCK,
> -					    hp_wmi_hw_state(HPWMI_DOCK_MASK));
> +					    hp_wmi_get_dock_state());
>  		if (test_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit))
>  			input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE,
> -					    hp_wmi_hw_state(HPWMI_TABLET_MASK));
> +					    hp_wmi_get_tablet_mode());
>  		input_sync(hp_wmi_input_dev);
>  	}
>