[PATCH v2] platform/x86: intel-hid: Add support for Device Specific Methods

[PATCH v2] platform/x86: intel-hid: Add support for Device Specific Methods

[Srinivas Pandruvada]

In some of the recent platforms, it is possible that stand alone methods
for HEBC() or other methods used in this driver may not exist. In this
case intel-hid driver will fail to load and power button will not be
functional.

It is also possible that some quirks in this driver added for some
platforms may have same issue in loading intel-hid driver.

There is an update to the ACPI details for the HID event filter driver.
In the updated specification a _DSM is added, which has separate function
indexes for each of the previous stand alone methods.

This change brings in support for the _DSM and allows usage of function
index for corresponding stand alone methods.

Details of Device Specific Method:

Intel HID Event Filter Driver _DSM UUID:
eeec56b3-4442-408f-a792-4edd4d758054

• Function index 0: Returns a buffer with a bit-field representing the
supported function IDs.

Function Index	ASL Object
--------------------------------
1		BTNL
2		HDMM
3		HDSM
4		HDEM
5		BTNS
6		BTNE
7		HEBC
8		VGBS
9		HEBC

One significant change is to query the supported methods implemented on
the platform. So the previous HEBC() has two variants. HEBC v1 and
HEBC v2. The v2 version allowed further define which of the 5-button
are actually defined by the platform. HEBC v2 support is only available
via new DSM.

v1 Button details:
Bits [0] - Rotation Lock, Num Lock, Home, End, Page Up,
Page Down
Bits [1] - Wireless Radio Control
Bits [2] - System Power Down
Bits [3] - System Hibernate
Bits [4] - System Sleep/ System Wake
Bits [5] - Scan Next Track
Bits [6] - Scan Previous Track
Bits [7] - Stop
Bits [8] - Play/Pause
Bits [9] - Mute
Bits [10] - Volume Increment
Bits [11] - Volume Decrement
Bits [12] - Display Brightness Increment
Bits [13] - Display Brightness Decrement
Bits [14] - Lock Tablet
Bits [15] - Release Tablet
Bits [16] - Toggle Bezel
Bits [17] - 5 button array
Bits [18-31] - reserved

v2 Buttom details:
Bits [0-16] - Same as v1 version
Bits [17] - 5 button array
Bits [18] – Power Button
Bits [19] - W Home Button
Bits [20] - Volume Up Button
Bits [21] - Volume Down Button
Bits [22] – Rotation Lock Button
Bits [23-31] – reserved

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
Accidently sent the patch without change of version, so please ignore
the previous one sent today.
v2:
	Minor changes suggested by Andy

 drivers/platform/x86/intel-hid.c | 178 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 157 insertions(+), 21 deletions(-)

diff --git a/drivers/platform/x86/intel-hid.c b/drivers/platform/x86/intel-hid.c
index b5adba227783..6cf9b7fa5bf0 100644
--- a/drivers/platform/x86/intel-hid.c
+++ b/drivers/platform/x86/intel-hid.c
@@ -96,13 +96,140 @@ struct intel_hid_priv {
 	bool wakeup_mode;
 };
 
-static int intel_hid_set_enable(struct device *device, bool enable)
+#define HID_EVENT_FILTER_UUID	"eeec56b3-4442-408f-a792-4edd4d758054"
+
+enum intel_hid_dsm_fn_codes {
+	INTEL_HID_DSM_FN_INVALID,
+	INTEL_HID_DSM_BTNL_FN,
+	INTEL_HID_DSM_HDMM_FN,
+	INTEL_HID_DSM_HDSM_FN,
+	INTEL_HID_DSM_HDEM_FN,
+	INTEL_HID_DSM_BTNS_FN,
+	INTEL_HID_DSM_BTNE_FN,
+	INTEL_HID_DSM_HEBC_V1_FN,
+	INTEL_HID_DSM_VGBS_FN,
+	INTEL_HID_DSM_HEBC_V2_FN,
+	INTEL_HID_DSM_FN_MAX
+};
+
+static const char *intel_hid_dsm_fn_to_method[INTEL_HID_DSM_FN_MAX] = {
+	NULL,
+	"BTNL",
+	"HDMM",
+	"HDSM",
+	"HDEM",
+	"BTNS",
+	"BTNE",
+	"HEBC",
+	"VGBS",
+	"HEBC"
+};
+
+static unsigned long long intel_hid_dsm_fn_mask;
+static guid_t intel_dsm_guid;
+
+static bool intel_hid_execute_method(acpi_handle handle,
+				     enum intel_hid_dsm_fn_codes fn_index,
+				     unsigned long long arg)
 {
+	union acpi_object *obj, argv4, req;
 	acpi_status status;
+	char *method_name;
 
-	status = acpi_execute_simple_method(ACPI_HANDLE(device), "HDSM",
-					    enable);
-	if (ACPI_FAILURE(status)) {
+	if (fn_index <= INTEL_HID_DSM_FN_INVALID ||
+	    fn_index >= INTEL_HID_DSM_FN_MAX)
+		return false;
+
+	method_name = (char *)intel_hid_dsm_fn_to_method[fn_index];
+
+	if (!(intel_hid_dsm_fn_mask & fn_index))
+		goto skip_dsm_exec;
+
+	/* All methods expects a package with one integer element */
+	req.type = ACPI_TYPE_INTEGER;
+	req.integer.value = arg;
+
+	argv4.type = ACPI_TYPE_PACKAGE;
+	argv4.package.count = 1;
+	argv4.package.elements = &req;
+
+	obj = acpi_evaluate_dsm(handle, &intel_dsm_guid, 1, fn_index, &argv4);
+	if (obj) {
+		acpi_handle_debug(handle, "Exec DSM Fn code: %d[%s] success\n",
+				  fn_index, method_name);
+		ACPI_FREE(obj);
+		return true;
+	}
+
+skip_dsm_exec:
+	status = acpi_execute_simple_method(handle, method_name, arg);
+	if (ACPI_SUCCESS(status))
+		return true;
+
+	return false;
+}
+
+static bool intel_hid_evaluate_method(acpi_handle handle,
+				      enum intel_hid_dsm_fn_codes fn_index,
+				      unsigned long long *result)
+{
+	union acpi_object *obj;
+	acpi_status status;
+	char *method_name;
+
+	if (fn_index <= INTEL_HID_DSM_FN_INVALID ||
+	    fn_index >= INTEL_HID_DSM_FN_MAX)
+		return false;
+
+	method_name = (char *)intel_hid_dsm_fn_to_method[fn_index];
+
+	if (!(intel_hid_dsm_fn_mask & fn_index))
+		goto skip_dsm_eval;
+
+	obj = acpi_evaluate_dsm_typed(handle, &intel_dsm_guid,
+				      1, fn_index,
+				      NULL,  ACPI_TYPE_INTEGER);
+	if (obj) {
+		*result = obj->integer.value;
+		acpi_handle_debug(handle,
+				  "Eval DSM Fn code: %d[%s] results: 0x%llx\n",
+				  fn_index, method_name, *result);
+		ACPI_FREE(obj);
+		return true;
+	}
+
+skip_dsm_eval:
+	status = acpi_evaluate_integer(handle, method_name, NULL, result);
+	if (ACPI_SUCCESS(status))
+		return true;
+
+	return false;
+}
+
+static void intel_hid_init_dsm(acpi_handle handle)
+{
+	union acpi_object *obj;
+
+	guid_parse(HID_EVENT_FILTER_UUID, &intel_dsm_guid);
+
+	obj = acpi_evaluate_dsm_typed(handle, &intel_dsm_guid, 1, 0, NULL,
+				      ACPI_TYPE_BUFFER);
+	if (obj) {
+		intel_hid_dsm_fn_mask = *obj->buffer.pointer;
+		ACPI_FREE(obj);
+	}
+
+	acpi_handle_debug(handle, "intel_hid_dsm_fn_mask = %llx\n",
+			  intel_hid_dsm_fn_mask);
+}
+
+static int intel_hid_set_enable(struct device *device, bool enable)
+{
+	acpi_handle handle = ACPI_HANDLE(device);
+
+	/* Enable|disable features - power button is always enabled */
+	if (!intel_hid_execute_method(handle, INTEL_HID_DSM_HDSM_FN,
+				      enable)) {
 		dev_warn(device, "failed to %sable hotkeys\n",
 			 enable ? "en" : "dis");
 		return -EIO;
@@ -129,9 +256,8 @@ static void intel_button_array_enable(struct device *device, bool enable)
 	}
 
 	/* Enable|disable features - power button is always enabled */
-	status = acpi_execute_simple_method(handle, "BTNE",
-					    enable ? button_cap : 1);
-	if (ACPI_FAILURE(status))
+	if (!intel_hid_execute_method(handle, INTEL_HID_DSM_BTNE_FN,
+				      enable ? button_cap : 1))
 		dev_warn(device, "failed to set button capability\n");
 }
 
@@ -217,7 +343,6 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
 	struct platform_device *device = context;
 	struct intel_hid_priv *priv = dev_get_drvdata(&device->dev);
 	unsigned long long ev_index;
-	acpi_status status;
 
 	if (priv->wakeup_mode) {
 		/*
@@ -269,8 +394,8 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
 		return;
 	}
 
-	status = acpi_evaluate_integer(handle, "HDEM", NULL, &ev_index);
-	if (ACPI_FAILURE(status)) {
+	if (!intel_hid_evaluate_method(handle, INTEL_HID_DSM_HDEM_FN,
+				       &ev_index)) {
 		dev_warn(&device->dev, "failed to get event index\n");
 		return;
 	}
@@ -284,17 +409,24 @@ static bool button_array_present(struct platform_device *device)
 {
 	acpi_handle handle = ACPI_HANDLE(&device->dev);
 	unsigned long long event_cap;
-	acpi_status status;
-	bool supported = false;
 
-	status = acpi_evaluate_integer(handle, "HEBC", NULL, &event_cap);
-	if (ACPI_SUCCESS(status) && (event_cap & 0x20000))
-		supported = true;
+	if (intel_hid_evaluate_method(handle, INTEL_HID_DSM_HEBC_V2_FN,
+				      &event_cap)) {
+		/* Check presence of 5 button array or v2 power button */
+		if (event_cap & 0x60000)
+			return true;
+	}
+
+	if (intel_hid_evaluate_method(handle, INTEL_HID_DSM_HEBC_V1_FN,
+				      &event_cap)) {
+		if (event_cap & 0x20000)
+			return true;
+	}
 
 	if (dmi_check_system(button_array_table))
-		supported = true;
+		return true;
 
-	return supported;
+	return false;
 }
 
 static int intel_hid_probe(struct platform_device *device)
@@ -305,8 +437,9 @@ static int intel_hid_probe(struct platform_device *device)
 	acpi_status status;
 	int err;
 
-	status = acpi_evaluate_integer(handle, "HDMM", NULL, &mode);
-	if (ACPI_FAILURE(status)) {
+	intel_hid_init_dsm(handle);
+
+	if (!intel_hid_evaluate_method(handle, INTEL_HID_DSM_HDMM_FN, &mode)) {
 		dev_warn(&device->dev, "failed to read mode\n");
 		return -ENODEV;
 	}
@@ -352,13 +485,16 @@ static int intel_hid_probe(struct platform_device *device)
 		goto err_remove_notify;
 
 	if (priv->array) {
+		unsigned long long dummy;
+
 		intel_button_array_enable(&device->dev, true);
 
 		/* Call button load method to enable HID power button */
-		status = acpi_evaluate_object(handle, "BTNL", NULL, NULL);
-		if (ACPI_FAILURE(status))
+		if (!intel_hid_evaluate_method(handle, INTEL_HID_DSM_BTNL_FN,
+					       &dummy)) {
 			dev_warn(&device->dev,
 				 "failed to enable HID power button\n");
+		}
 	}
 
 	device_init_wakeup(&device->dev, true);
-- 
2.14.1

RE: [PATCH v2] platform/x86: intel-hid: Add support for Device Specific Methods

[Mario.Limonciello]

> -----Original Message-----
> From: Srinivas Pandruvada [mailto:srinivas.pandruvada@linux.intel.com]
> Sent: Thursday, June 28, 2018 1:19 PM
> To: alex.hung@canonical.com; dvhart@infradead.org; andy@infradead.org
> Cc: platform-driver-x86@vger.kernel.org; linux-kernel@vger.kernel.org;
> rjw@rjwysocki.net; Limonciello, Mario; Srinivas Pandruvada
> Subject: [PATCH v2] platform/x86: intel-hid: Add support for Device Specific
> Methods
> 
> In some of the recent platforms, it is possible that stand alone methods
> for HEBC() or other methods used in this driver may not exist. In this
> case intel-hid driver will fail to load and power button will not be
> functional.
> 
> It is also possible that some quirks in this driver added for some
> platforms may have same issue in loading intel-hid driver.
> 
> There is an update to the ACPI details for the HID event filter driver.
> In the updated specification a _DSM is added, which has separate function
> indexes for each of the previous stand alone methods.
> 
> This change brings in support for the _DSM and allows usage of function
> index for corresponding stand alone methods.
> 
> Details of Device Specific Method:
> 
> Intel HID Event Filter Driver _DSM UUID:
> eeec56b3-4442-408f-a792-4edd4d758054
> 
> • Function index 0: Returns a buffer with a bit-field representing the
> supported function IDs.
> 
> Function Index	ASL Object
> --------------------------------
> 1		BTNL
> 2		HDMM
> 3		HDSM
> 4		HDEM
> 5		BTNS
> 6		BTNE
> 7		HEBC
> 8		VGBS
> 9		HEBC
> 
> One significant change is to query the supported methods implemented on
> the platform. So the previous HEBC() has two variants. HEBC v1 and
> HEBC v2. The v2 version allowed further define which of the 5-button
> are actually defined by the platform. HEBC v2 support is only available
> via new DSM.
> 
> v1 Button details:
> Bits [0] - Rotation Lock, Num Lock, Home, End, Page Up,
> Page Down
> Bits [1] - Wireless Radio Control
> Bits [2] - System Power Down
> Bits [3] - System Hibernate
> Bits [4] - System Sleep/ System Wake
> Bits [5] - Scan Next Track
> Bits [6] - Scan Previous Track
> Bits [7] - Stop
> Bits [8] - Play/Pause
> Bits [9] - Mute
> Bits [10] - Volume Increment
> Bits [11] - Volume Decrement
> Bits [12] - Display Brightness Increment
> Bits [13] - Display Brightness Decrement
> Bits [14] - Lock Tablet
> Bits [15] - Release Tablet
> Bits [16] - Toggle Bezel
> Bits [17] - 5 button array
> Bits [18-31] - reserved
> 
> v2 Buttom details:
> Bits [0-16] - Same as v1 version
> Bits [17] - 5 button array
> Bits [18] – Power Button
> Bits [19] - W Home Button
> Bits [20] - Volume Up Button
> Bits [21] - Volume Down Button
> Bits [22] – Rotation Lock Button
> Bits [23-31] – reserved
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

I verified this on XPS 9370 FW 1.3.2 (which contains this alternate HEBC interface).
Power button works again for wakeup from s2idle.

Tested-by: Mario Limonciello <mario.limonciello@dell.com>

> ---
> Accidently sent the patch without change of version, so please ignore
> the previous one sent today.
> v2:
> 	Minor changes suggested by Andy
> 
>  drivers/platform/x86/intel-hid.c | 178 ++++++++++++++++++++++++++++++++++----
> -
>  1 file changed, 157 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel-hid.c b/drivers/platform/x86/intel-hid.c
> index b5adba227783..6cf9b7fa5bf0 100644
> --- a/drivers/platform/x86/intel-hid.c
> +++ b/drivers/platform/x86/intel-hid.c
> @@ -96,13 +96,140 @@ struct intel_hid_priv {
>  	bool wakeup_mode;
>  };
> 
> -static int intel_hid_set_enable(struct device *device, bool enable)
> +#define HID_EVENT_FILTER_UUID	"eeec56b3-4442-408f-a792-
> 4edd4d758054"
> +
> +enum intel_hid_dsm_fn_codes {
> +	INTEL_HID_DSM_FN_INVALID,
> +	INTEL_HID_DSM_BTNL_FN,
> +	INTEL_HID_DSM_HDMM_FN,
> +	INTEL_HID_DSM_HDSM_FN,
> +	INTEL_HID_DSM_HDEM_FN,
> +	INTEL_HID_DSM_BTNS_FN,
> +	INTEL_HID_DSM_BTNE_FN,
> +	INTEL_HID_DSM_HEBC_V1_FN,
> +	INTEL_HID_DSM_VGBS_FN,
> +	INTEL_HID_DSM_HEBC_V2_FN,
> +	INTEL_HID_DSM_FN_MAX
> +};
> +
> +static const char *intel_hid_dsm_fn_to_method[INTEL_HID_DSM_FN_MAX] = {
> +	NULL,
> +	"BTNL",
> +	"HDMM",
> +	"HDSM",
> +	"HDEM",
> +	"BTNS",
> +	"BTNE",
> +	"HEBC",
> +	"VGBS",
> +	"HEBC"
> +};
> +
> +static unsigned long long intel_hid_dsm_fn_mask;
> +static guid_t intel_dsm_guid;
> +
> +static bool intel_hid_execute_method(acpi_handle handle,
> +				     enum intel_hid_dsm_fn_codes fn_index,
> +				     unsigned long long arg)
>  {
> +	union acpi_object *obj, argv4, req;
>  	acpi_status status;
> +	char *method_name;
> 
> -	status = acpi_execute_simple_method(ACPI_HANDLE(device), "HDSM",
> -					    enable);
> -	if (ACPI_FAILURE(status)) {
> +	if (fn_index <= INTEL_HID_DSM_FN_INVALID ||
> +	    fn_index >= INTEL_HID_DSM_FN_MAX)
> +		return false;
> +
> +	method_name = (char *)intel_hid_dsm_fn_to_method[fn_index];
> +
> +	if (!(intel_hid_dsm_fn_mask & fn_index))
> +		goto skip_dsm_exec;
> +
> +	/* All methods expects a package with one integer element */
> +	req.type = ACPI_TYPE_INTEGER;
> +	req.integer.value = arg;
> +
> +	argv4.type = ACPI_TYPE_PACKAGE;
> +	argv4.package.count = 1;
> +	argv4.package.elements = &req;
> +
> +	obj = acpi_evaluate_dsm(handle, &intel_dsm_guid, 1, fn_index, &argv4);
> +	if (obj) {
> +		acpi_handle_debug(handle, "Exec DSM Fn code: %d[%s]
> success\n",
> +				  fn_index, method_name);
> +		ACPI_FREE(obj);
> +		return true;
> +	}
> +
> +skip_dsm_exec:
> +	status = acpi_execute_simple_method(handle, method_name, arg);
> +	if (ACPI_SUCCESS(status))
> +		return true;
> +
> +	return false;
> +}
> +
> +static bool intel_hid_evaluate_method(acpi_handle handle,
> +				      enum intel_hid_dsm_fn_codes fn_index,
> +				      unsigned long long *result)
> +{
> +	union acpi_object *obj;
> +	acpi_status status;
> +	char *method_name;
> +
> +	if (fn_index <= INTEL_HID_DSM_FN_INVALID ||
> +	    fn_index >= INTEL_HID_DSM_FN_MAX)
> +		return false;
> +
> +	method_name = (char *)intel_hid_dsm_fn_to_method[fn_index];
> +
> +	if (!(intel_hid_dsm_fn_mask & fn_index))
> +		goto skip_dsm_eval;
> +
> +	obj = acpi_evaluate_dsm_typed(handle, &intel_dsm_guid,
> +				      1, fn_index,
> +				      NULL,  ACPI_TYPE_INTEGER);
> +	if (obj) {
> +		*result = obj->integer.value;
> +		acpi_handle_debug(handle,
> +				  "Eval DSM Fn code: %d[%s] results: 0x%llx\n",
> +				  fn_index, method_name, *result);
> +		ACPI_FREE(obj);
> +		return true;
> +	}
> +
> +skip_dsm_eval:
> +	status = acpi_evaluate_integer(handle, method_name, NULL, result);
> +	if (ACPI_SUCCESS(status))
> +		return true;
> +
> +	return false;
> +}
> +
> +static void intel_hid_init_dsm(acpi_handle handle)
> +{
> +	union acpi_object *obj;
> +
> +	guid_parse(HID_EVENT_FILTER_UUID, &intel_dsm_guid);
> +
> +	obj = acpi_evaluate_dsm_typed(handle, &intel_dsm_guid, 1, 0, NULL,
> +				      ACPI_TYPE_BUFFER);
> +	if (obj) {
> +		intel_hid_dsm_fn_mask = *obj->buffer.pointer;
> +		ACPI_FREE(obj);
> +	}
> +
> +	acpi_handle_debug(handle, "intel_hid_dsm_fn_mask = %llx\n",
> +			  intel_hid_dsm_fn_mask);
> +}
> +
> +static int intel_hid_set_enable(struct device *device, bool enable)
> +{
> +	acpi_handle handle = ACPI_HANDLE(device);
> +
> +	/* Enable|disable features - power button is always enabled */
> +	if (!intel_hid_execute_method(handle, INTEL_HID_DSM_HDSM_FN,
> +				      enable)) {
>  		dev_warn(device, "failed to %sable hotkeys\n",
>  			 enable ? "en" : "dis");
>  		return -EIO;
> @@ -129,9 +256,8 @@ static void intel_button_array_enable(struct device
> *device, bool enable)
>  	}
> 
>  	/* Enable|disable features - power button is always enabled */
> -	status = acpi_execute_simple_method(handle, "BTNE",
> -					    enable ? button_cap : 1);
> -	if (ACPI_FAILURE(status))
> +	if (!intel_hid_execute_method(handle, INTEL_HID_DSM_BTNE_FN,
> +				      enable ? button_cap : 1))
>  		dev_warn(device, "failed to set button capability\n");
>  }
> 
> @@ -217,7 +343,6 @@ static void notify_handler(acpi_handle handle, u32 event,
> void *context)
>  	struct platform_device *device = context;
>  	struct intel_hid_priv *priv = dev_get_drvdata(&device->dev);
>  	unsigned long long ev_index;
> -	acpi_status status;
> 
>  	if (priv->wakeup_mode) {
>  		/*
> @@ -269,8 +394,8 @@ static void notify_handler(acpi_handle handle, u32 event,
> void *context)
>  		return;
>  	}
> 
> -	status = acpi_evaluate_integer(handle, "HDEM", NULL, &ev_index);
> -	if (ACPI_FAILURE(status)) {
> +	if (!intel_hid_evaluate_method(handle, INTEL_HID_DSM_HDEM_FN,
> +				       &ev_index)) {
>  		dev_warn(&device->dev, "failed to get event index\n");
>  		return;
>  	}
> @@ -284,17 +409,24 @@ static bool button_array_present(struct platform_device
> *device)
>  {
>  	acpi_handle handle = ACPI_HANDLE(&device->dev);
>  	unsigned long long event_cap;
> -	acpi_status status;
> -	bool supported = false;
> 
> -	status = acpi_evaluate_integer(handle, "HEBC", NULL, &event_cap);
> -	if (ACPI_SUCCESS(status) && (event_cap & 0x20000))
> -		supported = true;
> +	if (intel_hid_evaluate_method(handle, INTEL_HID_DSM_HEBC_V2_FN,
> +				      &event_cap)) {
> +		/* Check presence of 5 button array or v2 power button */
> +		if (event_cap & 0x60000)
> +			return true;
> +	}
> +
> +	if (intel_hid_evaluate_method(handle, INTEL_HID_DSM_HEBC_V1_FN,
> +				      &event_cap)) {
> +		if (event_cap & 0x20000)
> +			return true;
> +	}
> 
>  	if (dmi_check_system(button_array_table))
> -		supported = true;
> +		return true;
> 
> -	return supported;
> +	return false;
>  }
> 
>  static int intel_hid_probe(struct platform_device *device)
> @@ -305,8 +437,9 @@ static int intel_hid_probe(struct platform_device *device)
>  	acpi_status status;
>  	int err;
> 
> -	status = acpi_evaluate_integer(handle, "HDMM", NULL, &mode);
> -	if (ACPI_FAILURE(status)) {
> +	intel_hid_init_dsm(handle);
> +
> +	if (!intel_hid_evaluate_method(handle, INTEL_HID_DSM_HDMM_FN,
> &mode)) {
>  		dev_warn(&device->dev, "failed to read mode\n");
>  		return -ENODEV;
>  	}
> @@ -352,13 +485,16 @@ static int intel_hid_probe(struct platform_device
> *device)
>  		goto err_remove_notify;
> 
>  	if (priv->array) {
> +		unsigned long long dummy;
> +
>  		intel_button_array_enable(&device->dev, true);
> 
>  		/* Call button load method to enable HID power button */
> -		status = acpi_evaluate_object(handle, "BTNL", NULL, NULL);
> -		if (ACPI_FAILURE(status))
> +		if (!intel_hid_evaluate_method(handle, INTEL_HID_DSM_BTNL_FN,
> +					       &dummy)) {
>  			dev_warn(&device->dev,
>  				 "failed to enable HID power button\n");
> +		}
>  	}
> 
>  	device_init_wakeup(&device->dev, true);
> --
> 2.14.1

Re: [PATCH v2] platform/x86: intel-hid: Add support for Device Specific Methods

[Srinivas Pandruvada]

On Fri, 2018-06-29 at 16:44 +0000, Mario.Limonciello@dell.com wrote:
> > 

[...]

> I verified this on XPS 9370 FW 1.3.2 (which contains this alternate
> HEBC interface).
> Power button works again for wakeup from s2idle.
> 
> Tested-by: Mario Limonciello <mario.limonciello@dell.com>
> 
So there are some customers who will have issue with power button
without this patch, so it should be also marked for stable also.
Also this may be a candidate for 4.18-rcX.

Thanks,
Srinivas

> > ---
> > Accidently sent the patch without change of version, so please
> > ignore
> > the previous one sent today.
> > v2:
> > 	Minor changes suggested by Andy
> > 
> >  drivers/platform/x86/intel-hid.c | 178
> > ++++++++++++++++++++++++++++++++++----
> > -
> >  1 file changed, 157 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/intel-hid.c
> > b/drivers/platform/x86/intel-hid.c
> > index b5adba227783..6cf9b7fa5bf0 100644
> > --- a/drivers/platform/x86/intel-hid.c
> > +++ b/drivers/platform/x86/intel-hid.c
> > @@ -96,13 +96,140 @@ struct intel_hid_priv {
> >  	bool wakeup_mode;
> >  };
> > 
> > -static int intel_hid_set_enable(struct device *device, bool
> > enable)
> > +#define HID_EVENT_FILTER_UUID	"eeec56b3-4442-408f-a792-
> > 4edd4d758054"
> > +
> > +enum intel_hid_dsm_fn_codes {
> > +	INTEL_HID_DSM_FN_INVALID,
> > +	INTEL_HID_DSM_BTNL_FN,
> > +	INTEL_HID_DSM_HDMM_FN,
> > +	INTEL_HID_DSM_HDSM_FN,
> > +	INTEL_HID_DSM_HDEM_FN,
> > +	INTEL_HID_DSM_BTNS_FN,
> > +	INTEL_HID_DSM_BTNE_FN,
> > +	INTEL_HID_DSM_HEBC_V1_FN,
> > +	INTEL_HID_DSM_VGBS_FN,
> > +	INTEL_HID_DSM_HEBC_V2_FN,
> > +	INTEL_HID_DSM_FN_MAX
> > +};
> > +
> > +static const char
> > *intel_hid_dsm_fn_to_method[INTEL_HID_DSM_FN_MAX] = {
> > +	NULL,
> > +	"BTNL",
> > +	"HDMM",
> > +	"HDSM",
> > +	"HDEM",
> > +	"BTNS",
> > +	"BTNE",
> > +	"HEBC",
> > +	"VGBS",
> > +	"HEBC"
> > +};
> > +
> > +static unsigned long long intel_hid_dsm_fn_mask;
> > +static guid_t intel_dsm_guid;
> > +
> > +static bool intel_hid_execute_method(acpi_handle handle,
> > +				     enum intel_hid_dsm_fn_codes
> > fn_index,
> > +				     unsigned long long arg)
> >  {
> > +	union acpi_object *obj, argv4, req;
> >  	acpi_status status;
> > +	char *method_name;
> > 
> > -	status = acpi_execute_simple_method(ACPI_HANDLE(device),
> > "HDSM",
> > -					    enable);
> > -	if (ACPI_FAILURE(status)) {
> > +	if (fn_index <= INTEL_HID_DSM_FN_INVALID ||
> > +	    fn_index >= INTEL_HID_DSM_FN_MAX)
> > +		return false;
> > +
> > +	method_name = (char
> > *)intel_hid_dsm_fn_to_method[fn_index];
> > +
> > +	if (!(intel_hid_dsm_fn_mask & fn_index))
> > +		goto skip_dsm_exec;
> > +
> > +	/* All methods expects a package with one integer element
> > */
> > +	req.type = ACPI_TYPE_INTEGER;
> > +	req.integer.value = arg;
> > +
> > +	argv4.type = ACPI_TYPE_PACKAGE;
> > +	argv4.package.count = 1;
> > +	argv4.package.elements = &req;
> > +
> > +	obj = acpi_evaluate_dsm(handle, &intel_dsm_guid, 1,
> > fn_index, &argv4);
> > +	if (obj) {
> > +		acpi_handle_debug(handle, "Exec DSM Fn code:
> > %d[%s]
> > success\n",
> > +				  fn_index, method_name);
> > +		ACPI_FREE(obj);
> > +		return true;
> > +	}
> > +
> > +skip_dsm_exec:
> > +	status = acpi_execute_simple_method(handle, method_name,
> > arg);
> > +	if (ACPI_SUCCESS(status))
> > +		return true;
> > +
> > +	return false;
> > +}
> > +
> > +static bool intel_hid_evaluate_method(acpi_handle handle,
> > +				      enum intel_hid_dsm_fn_codes
> > fn_index,
> > +				      unsigned long long *result)
> > +{
> > +	union acpi_object *obj;
> > +	acpi_status status;
> > +	char *method_name;
> > +
> > +	if (fn_index <= INTEL_HID_DSM_FN_INVALID ||
> > +	    fn_index >= INTEL_HID_DSM_FN_MAX)
> > +		return false;
> > +
> > +	method_name = (char
> > *)intel_hid_dsm_fn_to_method[fn_index];
> > +
> > +	if (!(intel_hid_dsm_fn_mask & fn_index))
> > +		goto skip_dsm_eval;
> > +
> > +	obj = acpi_evaluate_dsm_typed(handle, &intel_dsm_guid,
> > +				      1, fn_index,
> > +				      NULL,  ACPI_TYPE_INTEGER);
> > +	if (obj) {
> > +		*result = obj->integer.value;
> > +		acpi_handle_debug(handle,
> > +				  "Eval DSM Fn code: %d[%s]
> > results: 0x%llx\n",
> > +				  fn_index, method_name, *result);
> > +		ACPI_FREE(obj);
> > +		return true;
> > +	}
> > +
> > +skip_dsm_eval:
> > +	status = acpi_evaluate_integer(handle, method_name, NULL,
> > result);
> > +	if (ACPI_SUCCESS(status))
> > +		return true;
> > +
> > +	return false;
> > +}
> > +
> > +static void intel_hid_init_dsm(acpi_handle handle)
> > +{
> > +	union acpi_object *obj;
> > +
> > +	guid_parse(HID_EVENT_FILTER_UUID, &intel_dsm_guid);
> > +
> > +	obj = acpi_evaluate_dsm_typed(handle, &intel_dsm_guid, 1,
> > 0, NULL,
> > +				      ACPI_TYPE_BUFFER);
> > +	if (obj) {
> > +		intel_hid_dsm_fn_mask = *obj->buffer.pointer;
> > +		ACPI_FREE(obj);
> > +	}
> > +
> > +	acpi_handle_debug(handle, "intel_hid_dsm_fn_mask =
> > %llx\n",
> > +			  intel_hid_dsm_fn_mask);
> > +}
> > +
> > +static int intel_hid_set_enable(struct device *device, bool
> > enable)
> > +{
> > +	acpi_handle handle = ACPI_HANDLE(device);
> > +
> > +	/* Enable|disable features - power button is always
> > enabled */
> > +	if (!intel_hid_execute_method(handle,
> > INTEL_HID_DSM_HDSM_FN,
> > +				      enable)) {
> >  		dev_warn(device, "failed to %sable hotkeys\n",
> >  			 enable ? "en" : "dis");
> >  		return -EIO;
> > @@ -129,9 +256,8 @@ static void intel_button_array_enable(struct
> > device
> > *device, bool enable)
> >  	}
> > 
> >  	/* Enable|disable features - power button is always
> > enabled */
> > -	status = acpi_execute_simple_method(handle, "BTNE",
> > -					    enable ? button_cap :
> > 1);
> > -	if (ACPI_FAILURE(status))
> > +	if (!intel_hid_execute_method(handle,
> > INTEL_HID_DSM_BTNE_FN,
> > +				      enable ? button_cap : 1))
> >  		dev_warn(device, "failed to set button
> > capability\n");
> >  }
> > 
> > @@ -217,7 +343,6 @@ static void notify_handler(acpi_handle handle,
> > u32 event,
> > void *context)
> >  	struct platform_device *device = context;
> >  	struct intel_hid_priv *priv = dev_get_drvdata(&device-
> > >dev);
> >  	unsigned long long ev_index;
> > -	acpi_status status;
> > 
> >  	if (priv->wakeup_mode) {
> >  		/*
> > @@ -269,8 +394,8 @@ static void notify_handler(acpi_handle handle,
> > u32 event,
> > void *context)
> >  		return;
> >  	}
> > 
> > -	status = acpi_evaluate_integer(handle, "HDEM", NULL,
> > &ev_index);
> > -	if (ACPI_FAILURE(status)) {
> > +	if (!intel_hid_evaluate_method(handle,
> > INTEL_HID_DSM_HDEM_FN,
> > +				       &ev_index)) {
> >  		dev_warn(&device->dev, "failed to get event
> > index\n");
> >  		return;
> >  	}
> > @@ -284,17 +409,24 @@ static bool button_array_present(struct
> > platform_device
> > *device)
> >  {
> >  	acpi_handle handle = ACPI_HANDLE(&device->dev);
> >  	unsigned long long event_cap;
> > -	acpi_status status;
> > -	bool supported = false;
> > 
> > -	status = acpi_evaluate_integer(handle, "HEBC", NULL,
> > &event_cap);
> > -	if (ACPI_SUCCESS(status) && (event_cap & 0x20000))
> > -		supported = true;
> > +	if (intel_hid_evaluate_method(handle,
> > INTEL_HID_DSM_HEBC_V2_FN,
> > +				      &event_cap)) {
> > +		/* Check presence of 5 button array or v2 power
> > button */
> > +		if (event_cap & 0x60000)
> > +			return true;
> > +	}
> > +
> > +	if (intel_hid_evaluate_method(handle,
> > INTEL_HID_DSM_HEBC_V1_FN,
> > +				      &event_cap)) {
> > +		if (event_cap & 0x20000)
> > +			return true;
> > +	}
> > 
> >  	if (dmi_check_system(button_array_table))
> > -		supported = true;
> > +		return true;
> > 
> > -	return supported;
> > +	return false;
> >  }
> > 
> >  static int intel_hid_probe(struct platform_device *device)
> > @@ -305,8 +437,9 @@ static int intel_hid_probe(struct
> > platform_device *device)
> >  	acpi_status status;
> >  	int err;
> > 
> > -	status = acpi_evaluate_integer(handle, "HDMM", NULL,
> > &mode);
> > -	if (ACPI_FAILURE(status)) {
> > +	intel_hid_init_dsm(handle);
> > +
> > +	if (!intel_hid_evaluate_method(handle,
> > INTEL_HID_DSM_HDMM_FN,
> > &mode)) {
> >  		dev_warn(&device->dev, "failed to read mode\n");
> >  		return -ENODEV;
> >  	}
> > @@ -352,13 +485,16 @@ static int intel_hid_probe(struct
> > platform_device
> > *device)
> >  		goto err_remove_notify;
> > 
> >  	if (priv->array) {
> > +		unsigned long long dummy;
> > +
> >  		intel_button_array_enable(&device->dev, true);
> > 
> >  		/* Call button load method to enable HID power
> > button */
> > -		status = acpi_evaluate_object(handle, "BTNL",
> > NULL, NULL);
> > -		if (ACPI_FAILURE(status))
> > +		if (!intel_hid_evaluate_method(handle,
> > INTEL_HID_DSM_BTNL_FN,
> > +					       &dummy)) {
> >  			dev_warn(&device->dev,
> >  				 "failed to enable HID power
> > button\n");
> > +		}
> >  	}
> > 
> >  	device_init_wakeup(&device->dev, true);
> > --
> > 2.14.1
> 
> 

Re: [PATCH v2] platform/x86: intel-hid: Add support for Device Specific Methods

[Andy Shevchenko]

On Fri, Jun 29, 2018 at 9:41 PM, Srinivas Pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
> On Fri, 2018-06-29 at 16:44 +0000, Mario.Limonciello@dell.com wrote:
>> >
>
> [...]
>
>> I verified this on XPS 9370 FW 1.3.2 (which contains this alternate
>> HEBC interface).
>> Power button works again for wakeup from s2idle.
>>
>> Tested-by: Mario Limonciello <mario.limonciello@dell.com>
>>
> So there are some customers who will have issue with power button
> without this patch, so it should be also marked for stable also.
> Also this may be a candidate for 4.18-rcX.
>

I'm not sure Greg will take this selling point for rather big patch.
From changelog, honestly, I don't see any regression description,
looks like "it wasn't working before change anyway".

For now, I pushed this to my review and testing queue as is, thanks!

> Thanks,
> Srinivas
>
>> > ---
>> > Accidently sent the patch without change of version, so please
>> > ignore
>> > the previous one sent today.
>> > v2:
>> >     Minor changes suggested by Andy
>> >
>> >  drivers/platform/x86/intel-hid.c | 178
>> > ++++++++++++++++++++++++++++++++++----
>> > -
>> >  1 file changed, 157 insertions(+), 21 deletions(-)
>> >
>> > diff --git a/drivers/platform/x86/intel-hid.c
>> > b/drivers/platform/x86/intel-hid.c
>> > index b5adba227783..6cf9b7fa5bf0 100644
>> > --- a/drivers/platform/x86/intel-hid.c
>> > +++ b/drivers/platform/x86/intel-hid.c
>> > @@ -96,13 +96,140 @@ struct intel_hid_priv {
>> >     bool wakeup_mode;
>> >  };
>> >
>> > -static int intel_hid_set_enable(struct device *device, bool
>> > enable)
>> > +#define HID_EVENT_FILTER_UUID      "eeec56b3-4442-408f-a792-
>> > 4edd4d758054"
>> > +
>> > +enum intel_hid_dsm_fn_codes {
>> > +   INTEL_HID_DSM_FN_INVALID,
>> > +   INTEL_HID_DSM_BTNL_FN,
>> > +   INTEL_HID_DSM_HDMM_FN,
>> > +   INTEL_HID_DSM_HDSM_FN,
>> > +   INTEL_HID_DSM_HDEM_FN,
>> > +   INTEL_HID_DSM_BTNS_FN,
>> > +   INTEL_HID_DSM_BTNE_FN,
>> > +   INTEL_HID_DSM_HEBC_V1_FN,
>> > +   INTEL_HID_DSM_VGBS_FN,
>> > +   INTEL_HID_DSM_HEBC_V2_FN,
>> > +   INTEL_HID_DSM_FN_MAX
>> > +};
>> > +
>> > +static const char
>> > *intel_hid_dsm_fn_to_method[INTEL_HID_DSM_FN_MAX] = {
>> > +   NULL,
>> > +   "BTNL",
>> > +   "HDMM",
>> > +   "HDSM",
>> > +   "HDEM",
>> > +   "BTNS",
>> > +   "BTNE",
>> > +   "HEBC",
>> > +   "VGBS",
>> > +   "HEBC"
>> > +};
>> > +
>> > +static unsigned long long intel_hid_dsm_fn_mask;
>> > +static guid_t intel_dsm_guid;
>> > +
>> > +static bool intel_hid_execute_method(acpi_handle handle,
>> > +                                enum intel_hid_dsm_fn_codes
>> > fn_index,
>> > +                                unsigned long long arg)
>> >  {
>> > +   union acpi_object *obj, argv4, req;
>> >     acpi_status status;
>> > +   char *method_name;
>> >
>> > -   status = acpi_execute_simple_method(ACPI_HANDLE(device),
>> > "HDSM",
>> > -                                       enable);
>> > -   if (ACPI_FAILURE(status)) {
>> > +   if (fn_index <= INTEL_HID_DSM_FN_INVALID ||
>> > +       fn_index >= INTEL_HID_DSM_FN_MAX)
>> > +           return false;
>> > +
>> > +   method_name = (char
>> > *)intel_hid_dsm_fn_to_method[fn_index];
>> > +
>> > +   if (!(intel_hid_dsm_fn_mask & fn_index))
>> > +           goto skip_dsm_exec;
>> > +
>> > +   /* All methods expects a package with one integer element
>> > */
>> > +   req.type = ACPI_TYPE_INTEGER;
>> > +   req.integer.value = arg;
>> > +
>> > +   argv4.type = ACPI_TYPE_PACKAGE;
>> > +   argv4.package.count = 1;
>> > +   argv4.package.elements = &req;
>> > +
>> > +   obj = acpi_evaluate_dsm(handle, &intel_dsm_guid, 1,
>> > fn_index, &argv4);
>> > +   if (obj) {
>> > +           acpi_handle_debug(handle, "Exec DSM Fn code:
>> > %d[%s]
>> > success\n",
>> > +                             fn_index, method_name);
>> > +           ACPI_FREE(obj);
>> > +           return true;
>> > +   }
>> > +
>> > +skip_dsm_exec:
>> > +   status = acpi_execute_simple_method(handle, method_name,
>> > arg);
>> > +   if (ACPI_SUCCESS(status))
>> > +           return true;
>> > +
>> > +   return false;
>> > +}
>> > +
>> > +static bool intel_hid_evaluate_method(acpi_handle handle,
>> > +                                 enum intel_hid_dsm_fn_codes
>> > fn_index,
>> > +                                 unsigned long long *result)
>> > +{
>> > +   union acpi_object *obj;
>> > +   acpi_status status;
>> > +   char *method_name;
>> > +
>> > +   if (fn_index <= INTEL_HID_DSM_FN_INVALID ||
>> > +       fn_index >= INTEL_HID_DSM_FN_MAX)
>> > +           return false;
>> > +
>> > +   method_name = (char
>> > *)intel_hid_dsm_fn_to_method[fn_index];
>> > +
>> > +   if (!(intel_hid_dsm_fn_mask & fn_index))
>> > +           goto skip_dsm_eval;
>> > +
>> > +   obj = acpi_evaluate_dsm_typed(handle, &intel_dsm_guid,
>> > +                                 1, fn_index,
>> > +                                 NULL,  ACPI_TYPE_INTEGER);
>> > +   if (obj) {
>> > +           *result = obj->integer.value;
>> > +           acpi_handle_debug(handle,
>> > +                             "Eval DSM Fn code: %d[%s]
>> > results: 0x%llx\n",
>> > +                             fn_index, method_name, *result);
>> > +           ACPI_FREE(obj);
>> > +           return true;
>> > +   }
>> > +
>> > +skip_dsm_eval:
>> > +   status = acpi_evaluate_integer(handle, method_name, NULL,
>> > result);
>> > +   if (ACPI_SUCCESS(status))
>> > +           return true;
>> > +
>> > +   return false;
>> > +}
>> > +
>> > +static void intel_hid_init_dsm(acpi_handle handle)
>> > +{
>> > +   union acpi_object *obj;
>> > +
>> > +   guid_parse(HID_EVENT_FILTER_UUID, &intel_dsm_guid);
>> > +
>> > +   obj = acpi_evaluate_dsm_typed(handle, &intel_dsm_guid, 1,
>> > 0, NULL,
>> > +                                 ACPI_TYPE_BUFFER);
>> > +   if (obj) {
>> > +           intel_hid_dsm_fn_mask = *obj->buffer.pointer;
>> > +           ACPI_FREE(obj);
>> > +   }
>> > +
>> > +   acpi_handle_debug(handle, "intel_hid_dsm_fn_mask =
>> > %llx\n",
>> > +                     intel_hid_dsm_fn_mask);
>> > +}
>> > +
>> > +static int intel_hid_set_enable(struct device *device, bool
>> > enable)
>> > +{
>> > +   acpi_handle handle = ACPI_HANDLE(device);
>> > +
>> > +   /* Enable|disable features - power button is always
>> > enabled */
>> > +   if (!intel_hid_execute_method(handle,
>> > INTEL_HID_DSM_HDSM_FN,
>> > +                                 enable)) {
>> >             dev_warn(device, "failed to %sable hotkeys\n",
>> >                      enable ? "en" : "dis");
>> >             return -EIO;
>> > @@ -129,9 +256,8 @@ static void intel_button_array_enable(struct
>> > device
>> > *device, bool enable)
>> >     }
>> >
>> >     /* Enable|disable features - power button is always
>> > enabled */
>> > -   status = acpi_execute_simple_method(handle, "BTNE",
>> > -                                       enable ? button_cap :
>> > 1);
>> > -   if (ACPI_FAILURE(status))
>> > +   if (!intel_hid_execute_method(handle,
>> > INTEL_HID_DSM_BTNE_FN,
>> > +                                 enable ? button_cap : 1))
>> >             dev_warn(device, "failed to set button
>> > capability\n");
>> >  }
>> >
>> > @@ -217,7 +343,6 @@ static void notify_handler(acpi_handle handle,
>> > u32 event,
>> > void *context)
>> >     struct platform_device *device = context;
>> >     struct intel_hid_priv *priv = dev_get_drvdata(&device-
>> > >dev);
>> >     unsigned long long ev_index;
>> > -   acpi_status status;
>> >
>> >     if (priv->wakeup_mode) {
>> >             /*
>> > @@ -269,8 +394,8 @@ static void notify_handler(acpi_handle handle,
>> > u32 event,
>> > void *context)
>> >             return;
>> >     }
>> >
>> > -   status = acpi_evaluate_integer(handle, "HDEM", NULL,
>> > &ev_index);
>> > -   if (ACPI_FAILURE(status)) {
>> > +   if (!intel_hid_evaluate_method(handle,
>> > INTEL_HID_DSM_HDEM_FN,
>> > +                                  &ev_index)) {
>> >             dev_warn(&device->dev, "failed to get event
>> > index\n");
>> >             return;
>> >     }
>> > @@ -284,17 +409,24 @@ static bool button_array_present(struct
>> > platform_device
>> > *device)
>> >  {
>> >     acpi_handle handle = ACPI_HANDLE(&device->dev);
>> >     unsigned long long event_cap;
>> > -   acpi_status status;
>> > -   bool supported = false;
>> >
>> > -   status = acpi_evaluate_integer(handle, "HEBC", NULL,
>> > &event_cap);
>> > -   if (ACPI_SUCCESS(status) && (event_cap & 0x20000))
>> > -           supported = true;
>> > +   if (intel_hid_evaluate_method(handle,
>> > INTEL_HID_DSM_HEBC_V2_FN,
>> > +                                 &event_cap)) {
>> > +           /* Check presence of 5 button array or v2 power
>> > button */
>> > +           if (event_cap & 0x60000)
>> > +                   return true;
>> > +   }
>> > +
>> > +   if (intel_hid_evaluate_method(handle,
>> > INTEL_HID_DSM_HEBC_V1_FN,
>> > +                                 &event_cap)) {
>> > +           if (event_cap & 0x20000)
>> > +                   return true;
>> > +   }
>> >
>> >     if (dmi_check_system(button_array_table))
>> > -           supported = true;
>> > +           return true;
>> >
>> > -   return supported;
>> > +   return false;
>> >  }
>> >
>> >  static int intel_hid_probe(struct platform_device *device)
>> > @@ -305,8 +437,9 @@ static int intel_hid_probe(struct
>> > platform_device *device)
>> >     acpi_status status;
>> >     int err;
>> >
>> > -   status = acpi_evaluate_integer(handle, "HDMM", NULL,
>> > &mode);
>> > -   if (ACPI_FAILURE(status)) {
>> > +   intel_hid_init_dsm(handle);
>> > +
>> > +   if (!intel_hid_evaluate_method(handle,
>> > INTEL_HID_DSM_HDMM_FN,
>> > &mode)) {
>> >             dev_warn(&device->dev, "failed to read mode\n");
>> >             return -ENODEV;
>> >     }
>> > @@ -352,13 +485,16 @@ static int intel_hid_probe(struct
>> > platform_device
>> > *device)
>> >             goto err_remove_notify;
>> >
>> >     if (priv->array) {
>> > +           unsigned long long dummy;
>> > +
>> >             intel_button_array_enable(&device->dev, true);
>> >
>> >             /* Call button load method to enable HID power
>> > button */
>> > -           status = acpi_evaluate_object(handle, "BTNL",
>> > NULL, NULL);
>> > -           if (ACPI_FAILURE(status))
>> > +           if (!intel_hid_evaluate_method(handle,
>> > INTEL_HID_DSM_BTNL_FN,
>> > +                                          &dummy)) {
>> >                     dev_warn(&device->dev,
>> >                              "failed to enable HID power
>> > button\n");
>> > +           }
>> >     }
>> >
>> >     device_init_wakeup(&device->dev, true);
>> > --
>> > 2.14.1
>>
>>



-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v2] platform/x86: intel-hid: Add support for Device Specific Methods

[Mario.Limonciello]

> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> Sent: Monday, July 2, 2018 7:41 AM
> To: Srinivas Pandruvada
> Cc: Limonciello, Mario; Alex Hung; Darren Hart; Andy Shevchenko; Platform Driver;
> Linux Kernel Mailing List; Rafael J. Wysocki
> Subject: Re: [PATCH v2] platform/x86: intel-hid: Add support for Device Specific
> Methods
> 
> On Fri, Jun 29, 2018 at 9:41 PM, Srinivas Pandruvada
> <srinivas.pandruvada@linux.intel.com> wrote:
> > On Fri, 2018-06-29 at 16:44 +0000, Mario.Limonciello@dell.com wrote:
> >> >
> >
> > [...]
> >
> >> I verified this on XPS 9370 FW 1.3.2 (which contains this alternate
> >> HEBC interface).
> >> Power button works again for wakeup from s2idle.
> >>
> >> Tested-by: Mario Limonciello <mario.limonciello@dell.com>
> >>
> > So there are some customers who will have issue with power button
> > without this patch, so it should be also marked for stable also.
> > Also this may be a candidate for 4.18-rcX.
> >
> 
> I'm not sure Greg will take this selling point for rather big patch.
> From changelog, honestly, I don't see any regression description,
> looks like "it wasn't working before change anyway".
> 

Just for adding some context.

Some platforms have moved to different interface in ASL in FW upgrade
due to deficiencies/bugs present with old interface.  So yes it's platform FW
change in behavior that leads to Linux kernel regression.  Windows driver has supported
both interfaces for a long time.  Linux kernel however doesn't support this interface until now.

> For now, I pushed this to my review and testing queue as is, thanks!

If not stable I think it would at least be ideal to try to bring this to 4.18-rcX if possible for
compatibility with more platforms that will come with this other interface instead.

> 
> > Thanks,
> > Srinivas
> >
> >> > ---
> >> > Accidently sent the patch without change of version, so please
> >> > ignore
> >> > the previous one sent today.
> >> > v2:
> >> >     Minor changes suggested by Andy
> >> >
> >> >  drivers/platform/x86/intel-hid.c | 178
> >> > ++++++++++++++++++++++++++++++++++----
> >> > -
> >> >  1 file changed, 157 insertions(+), 21 deletions(-)
> >> >
> >> > diff --git a/drivers/platform/x86/intel-hid.c
> >> > b/drivers/platform/x86/intel-hid.c
> >> > index b5adba227783..6cf9b7fa5bf0 100644
> >> > --- a/drivers/platform/x86/intel-hid.c
> >> > +++ b/drivers/platform/x86/intel-hid.c
> >> > @@ -96,13 +96,140 @@ struct intel_hid_priv {
> >> >     bool wakeup_mode;
> >> >  };
> >> >
> >> > -static int intel_hid_set_enable(struct device *device, bool
> >> > enable)
> >> > +#define HID_EVENT_FILTER_UUID      "eeec56b3-4442-408f-a792-
> >> > 4edd4d758054"
> >> > +
> >> > +enum intel_hid_dsm_fn_codes {
> >> > +   INTEL_HID_DSM_FN_INVALID,
> >> > +   INTEL_HID_DSM_BTNL_FN,
> >> > +   INTEL_HID_DSM_HDMM_FN,
> >> > +   INTEL_HID_DSM_HDSM_FN,
> >> > +   INTEL_HID_DSM_HDEM_FN,
> >> > +   INTEL_HID_DSM_BTNS_FN,
> >> > +   INTEL_HID_DSM_BTNE_FN,
> >> > +   INTEL_HID_DSM_HEBC_V1_FN,
> >> > +   INTEL_HID_DSM_VGBS_FN,
> >> > +   INTEL_HID_DSM_HEBC_V2_FN,
> >> > +   INTEL_HID_DSM_FN_MAX
> >> > +};
> >> > +
> >> > +static const char
> >> > *intel_hid_dsm_fn_to_method[INTEL_HID_DSM_FN_MAX] = {
> >> > +   NULL,
> >> > +   "BTNL",
> >> > +   "HDMM",
> >> > +   "HDSM",
> >> > +   "HDEM",
> >> > +   "BTNS",
> >> > +   "BTNE",
> >> > +   "HEBC",
> >> > +   "VGBS",
> >> > +   "HEBC"
> >> > +};
> >> > +
> >> > +static unsigned long long intel_hid_dsm_fn_mask;
> >> > +static guid_t intel_dsm_guid;
> >> > +
> >> > +static bool intel_hid_execute_method(acpi_handle handle,
> >> > +                                enum intel_hid_dsm_fn_codes
> >> > fn_index,
> >> > +                                unsigned long long arg)
> >> >  {
> >> > +   union acpi_object *obj, argv4, req;
> >> >     acpi_status status;
> >> > +   char *method_name;
> >> >
> >> > -   status = acpi_execute_simple_method(ACPI_HANDLE(device),
> >> > "HDSM",
> >> > -                                       enable);
> >> > -   if (ACPI_FAILURE(status)) {
> >> > +   if (fn_index <= INTEL_HID_DSM_FN_INVALID ||
> >> > +       fn_index >= INTEL_HID_DSM_FN_MAX)
> >> > +           return false;
> >> > +
> >> > +   method_name = (char
> >> > *)intel_hid_dsm_fn_to_method[fn_index];
> >> > +
> >> > +   if (!(intel_hid_dsm_fn_mask & fn_index))
> >> > +           goto skip_dsm_exec;
> >> > +
> >> > +   /* All methods expects a package with one integer element
> >> > */
> >> > +   req.type = ACPI_TYPE_INTEGER;
> >> > +   req.integer.value = arg;
> >> > +
> >> > +   argv4.type = ACPI_TYPE_PACKAGE;
> >> > +   argv4.package.count = 1;
> >> > +   argv4.package.elements = &req;
> >> > +
> >> > +   obj = acpi_evaluate_dsm(handle, &intel_dsm_guid, 1,
> >> > fn_index, &argv4);
> >> > +   if (obj) {
> >> > +           acpi_handle_debug(handle, "Exec DSM Fn code:
> >> > %d[%s]
> >> > success\n",
> >> > +                             fn_index, method_name);
> >> > +           ACPI_FREE(obj);
> >> > +           return true;
> >> > +   }
> >> > +
> >> > +skip_dsm_exec:
> >> > +   status = acpi_execute_simple_method(handle, method_name,
> >> > arg);
> >> > +   if (ACPI_SUCCESS(status))
> >> > +           return true;
> >> > +
> >> > +   return false;
> >> > +}
> >> > +
> >> > +static bool intel_hid_evaluate_method(acpi_handle handle,
> >> > +                                 enum intel_hid_dsm_fn_codes
> >> > fn_index,
> >> > +                                 unsigned long long *result)
> >> > +{
> >> > +   union acpi_object *obj;
> >> > +   acpi_status status;
> >> > +   char *method_name;
> >> > +
> >> > +   if (fn_index <= INTEL_HID_DSM_FN_INVALID ||
> >> > +       fn_index >= INTEL_HID_DSM_FN_MAX)
> >> > +           return false;
> >> > +
> >> > +   method_name = (char
> >> > *)intel_hid_dsm_fn_to_method[fn_index];
> >> > +
> >> > +   if (!(intel_hid_dsm_fn_mask & fn_index))
> >> > +           goto skip_dsm_eval;
> >> > +
> >> > +   obj = acpi_evaluate_dsm_typed(handle, &intel_dsm_guid,
> >> > +                                 1, fn_index,
> >> > +                                 NULL,  ACPI_TYPE_INTEGER);
> >> > +   if (obj) {
> >> > +           *result = obj->integer.value;
> >> > +           acpi_handle_debug(handle,
> >> > +                             "Eval DSM Fn code: %d[%s]
> >> > results: 0x%llx\n",
> >> > +                             fn_index, method_name, *result);
> >> > +           ACPI_FREE(obj);
> >> > +           return true;
> >> > +   }
> >> > +
> >> > +skip_dsm_eval:
> >> > +   status = acpi_evaluate_integer(handle, method_name, NULL,
> >> > result);
> >> > +   if (ACPI_SUCCESS(status))
> >> > +           return true;
> >> > +
> >> > +   return false;
> >> > +}
> >> > +
> >> > +static void intel_hid_init_dsm(acpi_handle handle)
> >> > +{
> >> > +   union acpi_object *obj;
> >> > +
> >> > +   guid_parse(HID_EVENT_FILTER_UUID, &intel_dsm_guid);
> >> > +
> >> > +   obj = acpi_evaluate_dsm_typed(handle, &intel_dsm_guid, 1,
> >> > 0, NULL,
> >> > +                                 ACPI_TYPE_BUFFER);
> >> > +   if (obj) {
> >> > +           intel_hid_dsm_fn_mask = *obj->buffer.pointer;
> >> > +           ACPI_FREE(obj);
> >> > +   }
> >> > +
> >> > +   acpi_handle_debug(handle, "intel_hid_dsm_fn_mask =
> >> > %llx\n",
> >> > +                     intel_hid_dsm_fn_mask);
> >> > +}
> >> > +
> >> > +static int intel_hid_set_enable(struct device *device, bool
> >> > enable)
> >> > +{
> >> > +   acpi_handle handle = ACPI_HANDLE(device);
> >> > +
> >> > +   /* Enable|disable features - power button is always
> >> > enabled */
> >> > +   if (!intel_hid_execute_method(handle,
> >> > INTEL_HID_DSM_HDSM_FN,
> >> > +                                 enable)) {
> >> >             dev_warn(device, "failed to %sable hotkeys\n",
> >> >                      enable ? "en" : "dis");
> >> >             return -EIO;
> >> > @@ -129,9 +256,8 @@ static void intel_button_array_enable(struct
> >> > device
> >> > *device, bool enable)
> >> >     }
> >> >
> >> >     /* Enable|disable features - power button is always
> >> > enabled */
> >> > -   status = acpi_execute_simple_method(handle, "BTNE",
> >> > -                                       enable ? button_cap :
> >> > 1);
> >> > -   if (ACPI_FAILURE(status))
> >> > +   if (!intel_hid_execute_method(handle,
> >> > INTEL_HID_DSM_BTNE_FN,
> >> > +                                 enable ? button_cap : 1))
> >> >             dev_warn(device, "failed to set button
> >> > capability\n");
> >> >  }
> >> >
> >> > @@ -217,7 +343,6 @@ static void notify_handler(acpi_handle handle,
> >> > u32 event,
> >> > void *context)
> >> >     struct platform_device *device = context;
> >> >     struct intel_hid_priv *priv = dev_get_drvdata(&device-
> >> > >dev);
> >> >     unsigned long long ev_index;
> >> > -   acpi_status status;
> >> >
> >> >     if (priv->wakeup_mode) {
> >> >             /*
> >> > @@ -269,8 +394,8 @@ static void notify_handler(acpi_handle handle,
> >> > u32 event,
> >> > void *context)
> >> >             return;
> >> >     }
> >> >
> >> > -   status = acpi_evaluate_integer(handle, "HDEM", NULL,
> >> > &ev_index);
> >> > -   if (ACPI_FAILURE(status)) {
> >> > +   if (!intel_hid_evaluate_method(handle,
> >> > INTEL_HID_DSM_HDEM_FN,
> >> > +                                  &ev_index)) {
> >> >             dev_warn(&device->dev, "failed to get event
> >> > index\n");
> >> >             return;
> >> >     }
> >> > @@ -284,17 +409,24 @@ static bool button_array_present(struct
> >> > platform_device
> >> > *device)
> >> >  {
> >> >     acpi_handle handle = ACPI_HANDLE(&device->dev);
> >> >     unsigned long long event_cap;
> >> > -   acpi_status status;
> >> > -   bool supported = false;
> >> >
> >> > -   status = acpi_evaluate_integer(handle, "HEBC", NULL,
> >> > &event_cap);
> >> > -   if (ACPI_SUCCESS(status) && (event_cap & 0x20000))
> >> > -           supported = true;
> >> > +   if (intel_hid_evaluate_method(handle,
> >> > INTEL_HID_DSM_HEBC_V2_FN,
> >> > +                                 &event_cap)) {
> >> > +           /* Check presence of 5 button array or v2 power
> >> > button */
> >> > +           if (event_cap & 0x60000)
> >> > +                   return true;
> >> > +   }
> >> > +
> >> > +   if (intel_hid_evaluate_method(handle,
> >> > INTEL_HID_DSM_HEBC_V1_FN,
> >> > +                                 &event_cap)) {
> >> > +           if (event_cap & 0x20000)
> >> > +                   return true;
> >> > +   }
> >> >
> >> >     if (dmi_check_system(button_array_table))
> >> > -           supported = true;
> >> > +           return true;
> >> >
> >> > -   return supported;
> >> > +   return false;
> >> >  }
> >> >
> >> >  static int intel_hid_probe(struct platform_device *device)
> >> > @@ -305,8 +437,9 @@ static int intel_hid_probe(struct
> >> > platform_device *device)
> >> >     acpi_status status;
> >> >     int err;
> >> >
> >> > -   status = acpi_evaluate_integer(handle, "HDMM", NULL,
> >> > &mode);
> >> > -   if (ACPI_FAILURE(status)) {
> >> > +   intel_hid_init_dsm(handle);
> >> > +
> >> > +   if (!intel_hid_evaluate_method(handle,
> >> > INTEL_HID_DSM_HDMM_FN,
> >> > &mode)) {
> >> >             dev_warn(&device->dev, "failed to read mode\n");
> >> >             return -ENODEV;
> >> >     }
> >> > @@ -352,13 +485,16 @@ static int intel_hid_probe(struct
> >> > platform_device
> >> > *device)
> >> >             goto err_remove_notify;
> >> >
> >> >     if (priv->array) {
> >> > +           unsigned long long dummy;
> >> > +
> >> >             intel_button_array_enable(&device->dev, true);
> >> >
> >> >             /* Call button load method to enable HID power
> >> > button */
> >> > -           status = acpi_evaluate_object(handle, "BTNL",
> >> > NULL, NULL);
> >> > -           if (ACPI_FAILURE(status))
> >> > +           if (!intel_hid_evaluate_method(handle,
> >> > INTEL_HID_DSM_BTNL_FN,
> >> > +                                          &dummy)) {
> >> >                     dev_warn(&device->dev,
> >> >                              "failed to enable HID power
> >> > button\n");
> >> > +           }
> >> >     }
> >> >
> >> >     device_init_wakeup(&device->dev, true);
> >> > --
> >> > 2.14.1
> >>
> >>
> 
> 
> 
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v2] platform/x86: intel-hid: Add support for Device Specific Methods

[Andy Shevchenko]

On Mon, Jul 2, 2018 at 4:51 PM,  <Mario.Limonciello@dell.com> wrote:

>> > So there are some customers who will have issue with power button
>> > without this patch, so it should be also marked for stable also.
>> > Also this may be a candidate for 4.18-rcX.
>> >
>>
>> I'm not sure Greg will take this selling point for rather big patch.
>> From changelog, honestly, I don't see any regression description,
>> looks like "it wasn't working before change anyway".
>>
>
> Just for adding some context.
>
> Some platforms have moved to different interface in ASL in FW upgrade
> due to deficiencies/bugs present with old interface.  So yes it's platform FW
> change in behavior that leads to Linux kernel regression.  Windows driver has supported
> both interfaces for a long time.  Linux kernel however doesn't support this interface until now.
>
>> For now, I pushed this to my review and testing queue as is, thanks!
>
> If not stable I think it would at least be ideal to try to bring this to 4.18-rcX if possible for
> compatibility with more platforms that will come with this other interface instead.

Citing Linus:

--- 8< --- 8< ---
So please, people, the "fixes" during the rc series really should be
things that are _regressions_. If it used to work, and it no longer does,
then fixing that is a good and proper fix. Or if something oopses or has a
security implication, then the fix for that is a real fix.

But if it's something that has never worked, even if it "fixes" some
behavior, then it's new development, and that should come in during the
merge window. Just because you think it's a "fix" doesn't mean that it
really is one, at least in the "during the rc series" sense.
--- 8< --- 8< ---

So, if we can sell him that it used to work and firmware fix is a
Linux regression I'm fine.

Darren, what do you think?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] platform/x86: intel-hid: Add support for Device Specific Methods

[Darren Hart]

On Mon, Jul 02, 2018 at 05:06:14PM +0300, Andy Shevchenko wrote:
> On Mon, Jul 2, 2018 at 4:51 PM,  <Mario.Limonciello@dell.com> wrote:
> 
> >> > So there are some customers who will have issue with power button
> >> > without this patch, so it should be also marked for stable also.
> >> > Also this may be a candidate for 4.18-rcX.
> >> >
> >>
> >> I'm not sure Greg will take this selling point for rather big patch.
> >> From changelog, honestly, I don't see any regression description,
> >> looks like "it wasn't working before change anyway".
> >>
> >
> > Just for adding some context.
> >
> > Some platforms have moved to different interface in ASL in FW upgrade
> > due to deficiencies/bugs present with old interface.  So yes it's platform FW
> > change in behavior that leads to Linux kernel regression.  Windows driver has supported
> > both interfaces for a long time.  Linux kernel however doesn't support this interface until now.
> >
> >> For now, I pushed this to my review and testing queue as is, thanks!
> >
> > If not stable I think it would at least be ideal to try to bring this to 4.18-rcX if possible for
> > compatibility with more platforms that will come with this other interface instead.
> 
> Citing Linus:
> 
> --- 8< --- 8< ---
> So please, people, the "fixes" during the rc series really should be
> things that are _regressions_. If it used to work, and it no longer does,
> then fixing that is a good and proper fix. Or if something oopses or has a
> security implication, then the fix for that is a real fix.
> 
> But if it's something that has never worked, even if it "fixes" some
> behavior, then it's new development, and that should come in during the
> merge window. Just because you think it's a "fix" doesn't mean that it
> really is one, at least in the "during the rc series" sense.
> --- 8< --- 8< ---
> 
> So, if we can sell him that it used to work and firmware fix is a
> Linux regression I'm fine.
> 
> Darren, what do you think?

So if I understand this correctly, we have this timeline.

Linux v4.16
- Machine A works
Linux v4.17
- Machine A works
- Machine A updates firmware
- Machine A stops working
Linux v4.18
- Machine A still doesn't work

So it is not a *Linux kernel* regression.

The patch is too large for standard stable rules.

It is a regression from any user's perspective - the machine worked,
they followed good digital hygiene and updated their firmware, and now
it doesn't. This user will now think twice before they update their BIOS
again, since it may fundamentally change the platform, rather than
committing to only fixing things below the OS Interface layer. :-(

The risk of course is that this introduces new bugs - and as with
anything that still uses _DSM (sigh, why?) that is quite possible due to
the opaque interface. Very unfortunate to see _DSM ADDED to a previously
_DSM free implementation. Linus is right, this is not a fix, this is
feature development.

I strongly advocate for vendors to have more control over their drivers,
but this scenario really frustrates me. I don't think I can justify this
to Linus as a fix. But before we just say "no" (because hey, I want
these fixes available as early as possible too), let's ask Rafael if he
has an opinion or if there is precedent for this in his experience with
ACPI drivers in general:

Rafael?

Finally, we can also just ask Linus. The firmware update broke the power
button, we can get it working again by supporting the new API with this
patch... see what he says.

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH v2] platform/x86: intel-hid: Add support for Device Specific Methods

[Mario.Limonciello]

>I strongly advocate for vendors to have more control over their drivers,
>but this scenario really frustrates me. I don't think I can justify this
>to Linus as a fix. But before we just say "no" (because hey, I want
>these fixes available as early as possible too), let's ask Rafael if he
>has an opinion or if there is precedent for this in his experience with
>ACPI drivers in general:

Full disclosure - an updated FW has since been rolled out that reverted this
behavior back to previous FW behavior due to lack of Linux support for the
new _DSM.  There is desire to use the new interface (as it did fix actual
problems with the old one) so at some point it may return.  When that happens
it would be ideal that people who are (for example) running an LTS kernel
or distro kernel that tracks stable can pick it up too.

Re: [PATCH v2] platform/x86: intel-hid: Add support for Device Specific Methods

[Srinivas Pandruvada]

On Fri, 2018-07-06 at 16:59 -0700, Darren Hart wrote:
> On Mon, Jul 02, 2018 at 05:06:14PM +0300, Andy Shevchenko wrote:
> > On Mon, Jul 2, 2018 at 4:51 PM,  <Mario.Limonciello@dell.com>
> > wrote:
> > 
> > > > > So there are some customers who will have issue with power
> > > > > button
> > > > > without this patch, so it should be also marked for stable
> > > > > also.
> > > > > Also this may be a candidate for 4.18-rcX.
> > > > > 
> > > > 
> > > > I'm not sure Greg will take this selling point for rather big
> > > > patch.
> > > > From changelog, honestly, I don't see any regression
> > > > description,
> > > > looks like "it wasn't working before change anyway".
> > > > 
> > > 
> > > Just for adding some context.
> > > 
> > > Some platforms have moved to different interface in ASL in FW
> > > upgrade
> > > due to deficiencies/bugs present with old interface.  So yes it's
> > > platform FW
> > > change in behavior that leads to Linux kernel
> > > regression.  Windows driver has supported
> > > both interfaces for a long time.  Linux kernel however doesn't
> > > support this interface until now.
> > > 
> > > > For now, I pushed this to my review and testing queue as is,
> > > > thanks!
> > > 
> > > If not stable I think it would at least be ideal to try to bring
> > > this to 4.18-rcX if possible for
> > > compatibility with more platforms that will come with this other
> > > interface instead.
> > 
> > Citing Linus:
> > 
> > --- 8< --- 8< ---
> > So please, people, the "fixes" during the rc series really should
> > be
> > things that are _regressions_. If it used to work, and it no longer
> > does,
> > then fixing that is a good and proper fix. Or if something oopses
> > or has a
> > security implication, then the fix for that is a real fix.
> > 
> > But if it's something that has never worked, even if it "fixes"
> > some
> > behavior, then it's new development, and that should come in during
> > the
> > merge window. Just because you think it's a "fix" doesn't mean that
> > it
> > really is one, at least in the "during the rc series" sense.
> > --- 8< --- 8< ---
> > 
> > So, if we can sell him that it used to work and firmware fix is a
> > Linux regression I'm fine.
> > 
> > Darren, what do you think?
> 
> So if I understand this correctly, we have this timeline.
> 
> Linux v4.16
> - Machine A works
> Linux v4.17
> - Machine A works
> - Machine A updates firmware
> - Machine A stops working
> Linux v4.18
> - Machine A still doesn't work
> 
> So it is not a *Linux kernel* regression.
> 
> The patch is too large for standard stable rules.
> 
> It is a regression from any user's perspective - the machine worked,
> they followed good digital hygiene and updated their firmware, and
> now
> it doesn't. This user will now think twice before they update their
> BIOS
> again, since it may fundamentally change the platform, rather than
> committing to only fixing things below the OS Interface layer. :-(
> 
> The risk of course is that this introduces new bugs - and as with
> anything that still uses _DSM (sigh, why?) that is quite possible due
> to
> the opaque interface. Very unfortunate to see _DSM ADDED to a
> previously
> _DSM free implementation. Linus is right, this is not a fix, this is
> feature development.
> 
> I strongly advocate for vendors to have more control over their
> drivers,
> but this scenario really frustrates me. I don't think I can justify
> this
> to Linus as a fix. But before we just say "no" (because hey, I want
> these fixes available as early as possible too), let's ask Rafael if
> he
> has an opinion or if there is precedent for this in his experience
> with
> ACPI drivers in general:
> 
> Rafael?
> 
> Finally, we can also just ask Linus. The firmware update broke the
> power
> button, we can get it working again by supporting the new API with
> this
> patch... see what he says.
Mario can add more.
But I think Dell has released a BIOS fix, so that power button can
still work using non _DSM way. So I think we can wait for normal
release cycle.

Thanks,
Srinivas

> 

Re: [PATCH v2] platform/x86: intel-hid: Add support for Device Specific Methods

[Andy Shevchenko]

On Sat, Jul 7, 2018 at 6:48 AM,  <Mario.Limonciello@dell.com> wrote:
>>I strongly advocate for vendors to have more control over their drivers,
>>but this scenario really frustrates me. I don't think I can justify this
>>to Linus as a fix. But before we just say "no" (because hey, I want
>>these fixes available as early as possible too), let's ask Rafael if he
>>has an opinion or if there is precedent for this in his experience with
>>ACPI drivers in general:
>
> Full disclosure - an updated FW has since been rolled out that reverted this
> behavior back to previous FW behavior due to lack of Linux support for the
> new _DSM.  There is desire to use the new interface (as it did fix actual
> problems with the old one) so at some point it may return.  When that happens
> it would be ideal that people who are (for example) running an LTS kernel
> or distro kernel that tracks stable can pick it up too.

I am all for the better user experience and bugs especially in FW
should be fixed, but I think to support all kind of behaviour the new
FW release should always provide a knob by which we can check if some
feature is supported or not.

Above sounds to me as mistake in design of the fix.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] platform/x86: intel-hid: Add support for Device Specific Methods

[Srinivas Pandruvada]

On Sat, 2018-07-07 at 19:24 +0300, Andy Shevchenko wrote:
> On Sat, Jul 7, 2018 at 6:48 AM,  <Mario.Limonciello@dell.com> wrote:
> > > I strongly advocate for vendors to have more control over their
> > > drivers,
> > > but this scenario really frustrates me. I don't think I can
> > > justify this
> > > to Linus as a fix. But before we just say "no" (because hey, I
> > > want
> > > these fixes available as early as possible too), let's ask Rafael
> > > if he
> > > has an opinion or if there is precedent for this in his
> > > experience with
> > > ACPI drivers in general:
> > 
> > Full disclosure - an updated FW has since been rolled out that
> > reverted this
> > behavior back to previous FW behavior due to lack of Linux support
> > for the
> > new _DSM.  There is desire to use the new interface (as it did fix
> > actual
> > problems with the old one) so at some point it may return.  When
> > that happens
> > it would be ideal that people who are (for example) running an LTS
> > kernel
> > or distro kernel that tracks stable can pick it up too.
> 
> I am all for the better user experience and bugs especially in FW
> should be fixed, but I think to support all kind of behaviour the new
> FW release should always provide a knob by which we can check if some
> feature is supported or not.
> 
> Above sounds to me as mistake in design of the fix.
The fix here takes care of all the combination. If the new _DSM is
available, it checks what functionality the _DSM interface can provide
(Fn 0). For those functionality it gets using _DSM matching new FW
interface. If there is no _DSM or _DSM doesn't provide the
functionality or _DSM provided method fails, it still revert to old FW
interface style using standalone method. Eventually because of
configurable 5-button array, all platforms will be using new FW
interface with consideration of backward compatibility.

Thanks,
Srinivas

Re: [PATCH v2] platform/x86: intel-hid: Add support for Device Specific Methods

[Andy Shevchenko]

On Sun, Jul 8, 2018 at 2:37 AM, Srinivas Pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
> On Sat, 2018-07-07 at 19:24 +0300, Andy Shevchenko wrote:
>> On Sat, Jul 7, 2018 at 6:48 AM,  <Mario.Limonciello@dell.com> wrote:
>> > > I strongly advocate for vendors to have more control over their
>> > > drivers,
>> > > but this scenario really frustrates me. I don't think I can
>> > > justify this
>> > > to Linus as a fix. But before we just say "no" (because hey, I
>> > > want
>> > > these fixes available as early as possible too), let's ask Rafael
>> > > if he
>> > > has an opinion or if there is precedent for this in his
>> > > experience with
>> > > ACPI drivers in general:
>> >
>> > Full disclosure - an updated FW has since been rolled out that
>> > reverted this
>> > behavior back to previous FW behavior due to lack of Linux support
>> > for the
>> > new _DSM.  There is desire to use the new interface (as it did fix
>> > actual
>> > problems with the old one) so at some point it may return.  When
>> > that happens
>> > it would be ideal that people who are (for example) running an LTS
>> > kernel
>> > or distro kernel that tracks stable can pick it up too.
>>
>> I am all for the better user experience and bugs especially in FW
>> should be fixed, but I think to support all kind of behaviour the new
>> FW release should always provide a knob by which we can check if some
>> feature is supported or not.
>>
>> Above sounds to me as mistake in design of the fix.
> The fix here takes care of all the combination. If the new _DSM is
> available, it checks what functionality the _DSM interface can provide
> (Fn 0). For those functionality it gets using _DSM matching new FW
> interface. If there is no _DSM or _DSM doesn't provide the
> functionality or _DSM provided method fails, it still revert to old FW
> interface style using standalone method. Eventually because of
> configurable 5-button array, all platforms will be using new FW
> interface with consideration of backward compatibility.

Srinivas, thanks for elaboration.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v2] platform/x86: intel-hid: Add support for Device Specific Methods

[Mario.Limonciello]

> -----Original Message-----
> From: Srinivas Pandruvada [mailto:srinivas.pandruvada@linux.intel.com]
> Sent: Saturday, July 7, 2018 8:44 AM
> To: Darren Hart; Andy Shevchenko
> Cc: Limonciello, Mario; Alex Hung; Andy Shevchenko; Platform Driver; Linux Kernel
> Mailing List; Rafael J. Wysocki
> Subject: Re: [PATCH v2] platform/x86: intel-hid: Add support for Device Specific
> Methods
> 
> On Fri, 2018-07-06 at 16:59 -0700, Darren Hart wrote:
> > On Mon, Jul 02, 2018 at 05:06:14PM +0300, Andy Shevchenko wrote:
> > > On Mon, Jul 2, 2018 at 4:51 PM,  <Mario.Limonciello@dell.com>
> > > wrote:
> > >
> > > > > > So there are some customers who will have issue with power
> > > > > > button
> > > > > > without this patch, so it should be also marked for stable
> > > > > > also.
> > > > > > Also this may be a candidate for 4.18-rcX.
> > > > > >
> > > > >
> > > > > I'm not sure Greg will take this selling point for rather big
> > > > > patch.
> > > > > From changelog, honestly, I don't see any regression
> > > > > description,
> > > > > looks like "it wasn't working before change anyway".
> > > > >
> > > >
> > > > Just for adding some context.
> > > >
> > > > Some platforms have moved to different interface in ASL in FW
> > > > upgrade
> > > > due to deficiencies/bugs present with old interface.  So yes it's
> > > > platform FW
> > > > change in behavior that leads to Linux kernel
> > > > regression.  Windows driver has supported
> > > > both interfaces for a long time.  Linux kernel however doesn't
> > > > support this interface until now.
> > > >
> > > > > For now, I pushed this to my review and testing queue as is,
> > > > > thanks!
> > > >
> > > > If not stable I think it would at least be ideal to try to bring
> > > > this to 4.18-rcX if possible for
> > > > compatibility with more platforms that will come with this other
> > > > interface instead.
> > >
> > > Citing Linus:
> > >
> > > --- 8< --- 8< ---
> > > So please, people, the "fixes" during the rc series really should
> > > be
> > > things that are _regressions_. If it used to work, and it no longer
> > > does,
> > > then fixing that is a good and proper fix. Or if something oopses
> > > or has a
> > > security implication, then the fix for that is a real fix.
> > >
> > > But if it's something that has never worked, even if it "fixes"
> > > some
> > > behavior, then it's new development, and that should come in during
> > > the
> > > merge window. Just because you think it's a "fix" doesn't mean that
> > > it
> > > really is one, at least in the "during the rc series" sense.
> > > --- 8< --- 8< ---
> > >
> > > So, if we can sell him that it used to work and firmware fix is a
> > > Linux regression I'm fine.
> > >
> > > Darren, what do you think?
> >
> > So if I understand this correctly, we have this timeline.
> >
> > Linux v4.16
> > - Machine A works
> > Linux v4.17
> > - Machine A works
> > - Machine A updates firmware
> > - Machine A stops working
> > Linux v4.18
> > - Machine A still doesn't work
> >
> > So it is not a *Linux kernel* regression.
> >
> > The patch is too large for standard stable rules.
> >
> > It is a regression from any user's perspective - the machine worked,
> > they followed good digital hygiene and updated their firmware, and
> > now
> > it doesn't. This user will now think twice before they update their
> > BIOS
> > again, since it may fundamentally change the platform, rather than
> > committing to only fixing things below the OS Interface layer. :-(
> >
> > The risk of course is that this introduces new bugs - and as with
> > anything that still uses _DSM (sigh, why?) that is quite possible due
> > to
> > the opaque interface. Very unfortunate to see _DSM ADDED to a
> > previously
> > _DSM free implementation. Linus is right, this is not a fix, this is
> > feature development.
> >
> > I strongly advocate for vendors to have more control over their
> > drivers,
> > but this scenario really frustrates me. I don't think I can justify
> > this
> > to Linus as a fix. But before we just say "no" (because hey, I want
> > these fixes available as early as possible too), let's ask Rafael if
> > he
> > has an opinion or if there is precedent for this in his experience
> > with
> > ACPI drivers in general:
> >
> > Rafael?
> >
> > Finally, we can also just ask Linus. The firmware update broke the
> > power
> > button, we can get it working again by supporting the new API with
> > this
> > patch... see what he says.

> Mario can add more.
> But I think Dell has released a BIOS fix, so that power button can
> still work using non _DSM way. So I think we can wait for normal
> release cycle.

Yeah, I added some comments on a separate reply already indicating this.

The affected system we know about has reverted the new interface.

This all stems from an expectation that the _DSM has been there for "many"
releases on the Windows side.  Long enough that even all the corporate downgrade
scenarios to older Windows versions and the drivers with them all worked properly.

If this doesn't end up being a candidate for backporting to stable we'll probably
end up asking our various OS partners to backport it as a SAUCE type patch in distro
kernels to eventually be able to turn this back on.

Re: [PATCH v2] platform/x86: intel-hid: Add support for Device Specific Methods

[Darren Hart]

On Mon, Jul 09, 2018 at 04:38:16AM +0000, Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Srinivas Pandruvada [mailto:srinivas.pandruvada@linux.intel.com]
> > Sent: Saturday, July 7, 2018 8:44 AM
> > To: Darren Hart; Andy Shevchenko
> > Cc: Limonciello, Mario; Alex Hung; Andy Shevchenko; Platform Driver; Linux Kernel
> > Mailing List; Rafael J. Wysocki
> > Subject: Re: [PATCH v2] platform/x86: intel-hid: Add support for Device Specific
> > Methods
> > 
> > On Fri, 2018-07-06 at 16:59 -0700, Darren Hart wrote:
> > > On Mon, Jul 02, 2018 at 05:06:14PM +0300, Andy Shevchenko wrote:
> > > > On Mon, Jul 2, 2018 at 4:51 PM,  <Mario.Limonciello@dell.com>
> > > > wrote:
...
> > Mario can add more.
> > But I think Dell has released a BIOS fix, so that power button can
> > still work using non _DSM way. So I think we can wait for normal
> > release cycle.
> 
> Yeah, I added some comments on a separate reply already indicating this.
> 
> The affected system we know about has reverted the new interface.
> 
> This all stems from an expectation that the _DSM has been there for "many"
> releases on the Windows side.  Long enough that even all the corporate downgrade
> scenarios to older Windows versions and the drivers with them all worked properly.
> 
> If this doesn't end up being a candidate for backporting to stable we'll probably
> end up asking our various OS partners to backport it as a SAUCE type patch in distro
> kernels to eventually be able to turn this back on.

Thanks Mario. We'll skip the RC. We can discuss -stable as a separate
issue.  This patch fixes a demonstrable bug, the biggest obstacle here
is the size. At 240 lines with context, it more than doubles the maximum
patch size for stable.

My recommendation is that we treat the stable backport of this as a
special case, e.g. Andy and I don't tag it, but you or Srinivas propose
it and specifically make the case for why an exception should be made to
Greg.

How self contained and isolated this change is will be an important part
of the argument. On the one hand, it's all one file, on the other hand,
this is a fairly generic driver in use in more and more platforms, with
the potential for widespread impact if this introduces a new bug.

-- 
Darren Hart
VMware Open Source Technology Center

RE: [PATCH v2] platform/x86: intel-hid: Add support for Device Specific Methods

[Mario.Limonciello]

> -----Original Message-----
> From: Darren Hart [mailto:dvhart@infradead.org]
> Sent: Monday, July 9, 2018 4:07 PM
> To: Limonciello, Mario
> Cc: srinivas.pandruvada@linux.intel.com; andy.shevchenko@gmail.com;
> alex.hung@canonical.com; andy@infradead.org; platform-driver-
> x86@vger.kernel.org; linux-kernel@vger.kernel.org; rjw@rjwysocki.net
> Subject: Re: [PATCH v2] platform/x86: intel-hid: Add support for Device Specific
> Methods
> 
> On Mon, Jul 09, 2018 at 04:38:16AM +0000, Mario.Limonciello@dell.com wrote:
> > > -----Original Message-----
> > > From: Srinivas Pandruvada [mailto:srinivas.pandruvada@linux.intel.com]
> > > Sent: Saturday, July 7, 2018 8:44 AM
> > > To: Darren Hart; Andy Shevchenko
> > > Cc: Limonciello, Mario; Alex Hung; Andy Shevchenko; Platform Driver; Linux
> Kernel
> > > Mailing List; Rafael J. Wysocki
> > > Subject: Re: [PATCH v2] platform/x86: intel-hid: Add support for Device Specific
> > > Methods
> > >
> > > On Fri, 2018-07-06 at 16:59 -0700, Darren Hart wrote:
> > > > On Mon, Jul 02, 2018 at 05:06:14PM +0300, Andy Shevchenko wrote:
> > > > > On Mon, Jul 2, 2018 at 4:51 PM,  <Mario.Limonciello@dell.com>
> > > > > wrote:
> ...
> > > Mario can add more.
> > > But I think Dell has released a BIOS fix, so that power button can
> > > still work using non _DSM way. So I think we can wait for normal
> > > release cycle.
> >
> > Yeah, I added some comments on a separate reply already indicating this.
> >
> > The affected system we know about has reverted the new interface.
> >
> > This all stems from an expectation that the _DSM has been there for "many"
> > releases on the Windows side.  Long enough that even all the corporate
> downgrade
> > scenarios to older Windows versions and the drivers with them all worked
> properly.
> >
> > If this doesn't end up being a candidate for backporting to stable we'll probably
> > end up asking our various OS partners to backport it as a SAUCE type patch in
> distro
> > kernels to eventually be able to turn this back on.
> 
> Thanks Mario. We'll skip the RC. We can discuss -stable as a separate
> issue.  This patch fixes a demonstrable bug, the biggest obstacle here
> is the size. At 240 lines with context, it more than doubles the maximum
> patch size for stable.
> 
> My recommendation is that we treat the stable backport of this as a
> special case, e.g. Andy and I don't tag it, but you or Srinivas propose
> it and specifically make the case for why an exception should be made to
> Greg.
> 
> How self contained and isolated this change is will be an important part
> of the argument. On the one hand, it's all one file, on the other hand,
> this is a fairly generic driver in use in more and more platforms, with
> the potential for widespread impact if this introduces a new bug.
> 

In that case, how about we let it bake for a while and maybe by the time
4.19 comes out is when we do the proposal of a stable backport?

Re: [PATCH v2] platform/x86: intel-hid: Add support for Device Specific Methods

[Darren Hart]

On Mon, Jul 09, 2018 at 09:08:56PM +0000, Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Darren Hart [mailto:dvhart@infradead.org]
> > Sent: Monday, July 9, 2018 4:07 PM
> > To: Limonciello, Mario
> > Cc: srinivas.pandruvada@linux.intel.com; andy.shevchenko@gmail.com;
> > alex.hung@canonical.com; andy@infradead.org; platform-driver-
> > x86@vger.kernel.org; linux-kernel@vger.kernel.org; rjw@rjwysocki.net
> > Subject: Re: [PATCH v2] platform/x86: intel-hid: Add support for Device Specific
> > Methods
> > 
> > On Mon, Jul 09, 2018 at 04:38:16AM +0000, Mario.Limonciello@dell.com wrote:
> > > > -----Original Message-----
> > > > From: Srinivas Pandruvada [mailto:srinivas.pandruvada@linux.intel.com]
> > > > Sent: Saturday, July 7, 2018 8:44 AM
> > > > To: Darren Hart; Andy Shevchenko
> > > > Cc: Limonciello, Mario; Alex Hung; Andy Shevchenko; Platform Driver; Linux
> > Kernel
> > > > Mailing List; Rafael J. Wysocki
> > > > Subject: Re: [PATCH v2] platform/x86: intel-hid: Add support for Device Specific
> > > > Methods
> > > >
> > > > On Fri, 2018-07-06 at 16:59 -0700, Darren Hart wrote:
> > > > > On Mon, Jul 02, 2018 at 05:06:14PM +0300, Andy Shevchenko wrote:
> > > > > > On Mon, Jul 2, 2018 at 4:51 PM,  <Mario.Limonciello@dell.com>
> > > > > > wrote:
> > ...
> > > > Mario can add more.
> > > > But I think Dell has released a BIOS fix, so that power button can
> > > > still work using non _DSM way. So I think we can wait for normal
> > > > release cycle.
> > >
> > > Yeah, I added some comments on a separate reply already indicating this.
> > >
> > > The affected system we know about has reverted the new interface.
> > >
> > > This all stems from an expectation that the _DSM has been there for "many"
> > > releases on the Windows side.  Long enough that even all the corporate
> > downgrade
> > > scenarios to older Windows versions and the drivers with them all worked
> > properly.
> > >
> > > If this doesn't end up being a candidate for backporting to stable we'll probably
> > > end up asking our various OS partners to backport it as a SAUCE type patch in
> > distro
> > > kernels to eventually be able to turn this back on.
> > 
> > Thanks Mario. We'll skip the RC. We can discuss -stable as a separate
> > issue.  This patch fixes a demonstrable bug, the biggest obstacle here
> > is the size. At 240 lines with context, it more than doubles the maximum
> > patch size for stable.
> > 
> > My recommendation is that we treat the stable backport of this as a
> > special case, e.g. Andy and I don't tag it, but you or Srinivas propose
> > it and specifically make the case for why an exception should be made to
> > Greg.
> > 
> > How self contained and isolated this change is will be an important part
> > of the argument. On the one hand, it's all one file, on the other hand,
> > this is a fairly generic driver in use in more and more platforms, with
> > the potential for widespread impact if this introduces a new bug.
> > 
> 
> In that case, how about we let it bake for a while and maybe by the time
> 4.19 comes out is when we do the proposal of a stable backport?

Works for me.
-- 
Darren Hart
VMware Open Source Technology Center