RE: [EXTERNAL] Re: [PATCH v1 3/5] HID: add on_transport_error() field to struct hid_driver

-----Original Message-----
From: Benjamin Tissoires <redacted>
Sent: Monday, January 3, 2022 7:27 AM
To: Dmitry Antipov <redacted>
Cc: Jiri Kosina <jikos@kernel.org>; open list:HID CORE LAYER <linux-
input@vger.kernel.org>; Felipe Balbi [off-list ref]; Dmitry Antipov
[off-list ref]
Subject: [EXTERNAL] Re: [PATCH v1 3/5] HID: add on_transport_error() field to
struct hid_driver

On Thu, Dec 30, 2021 at 12:11 AM Dmitry Antipov [off-list ref]
wrote:

quoted

This new API allows a transport driver to notify the HID device driver
about a transport layer error.

I do not see entirely the purpose of this new callback:

- when we receive the device initiated reset, this is a specific device event, so it
would make sense...
- but for things like HID_TRANSPORT_ERROR_TYPE_BUS_OUTPUT_TRANSFER, I
would expect the caller to return that error code instead of having an async
function called.

I think it might be simpler to add a more specific
.device_initiated_reset() callback instead of trying to be generic.

The intention of this new callback is to notify the device driver of a
transport-layer error for at least two reasons:
1. Delegating the decision making. For certain types of errors the spec
states that the host _may_ reset the device. Right now there are not
many devices that support HID over SPI, but I wanted to allow the
flexibility for each vendor to decide what cases to error-handle.
2. Telemetry instrumentation to gather statistics on various error
conditions hit in spi-hid. The way we implement this is by publishing
sysfs attributes with error counters from the device driver and epoll
on these attributes from userspace. Here is a snippet from a yet-to-be-
sent patch to hid-microsoft.c:

static void ms_on_transport_error(struct hid_device *hdev,
					int err_type,
					int err_code,
					bool handled)
{
	struct ms_data *ms = hid_get_drvdata(hdev);

	if (ms->quirks & MS_TRANSPORT_ERROR_HANDLING) {

		switch (err_type) {
			case HID_TRANSPORT_ERROR_TYPE_BUS_INPUT_TRANSFER_START:
			case HID_TRANSPORT_ERROR_TYPE_BUS_INPUT_TRANSFER_BODY:
			case HID_TRANSPORT_ERROR_TYPE_BUS_INPUT_TRANSFER_HEADER:
			case HID_TRANSPORT_ERROR_TYPE_HEADER_DATA:
			case HID_TRANSPORT_ERROR_TYPE_BUS_OUTPUT_TRANSFER:
				ms->bus_error_count++;
				ms->bus_last_error = err_code;
				break;
			case HID_TRANSPORT_ERROR_TYPE_DEVICE_INITIATED_RESET:
				ms->dir_count++;
				break;
			case HID_TRANSPORT_ERROR_TYPE_INPUT_REPORT_DATA:
			case HID_TRANSPORT_ERROR_TYPE_REPORT_TYPE:
			case HID_TRANSPORT_ERROR_TYPE_GET_FEATURE_RESPONSE:
				if (!handled && (hdev->ll_driver->reset != 0))
					hdev->ll_driver->reset(hdev);
				break;
			case HID_TRANSPORT_ERROR_TYPE_REGULATOR_ENABLE:
			case HID_TRANSPORT_ERROR_TYPE_REGULATOR_DISABLE:
				ms->regulator_error_count++;
				ms->regulator_last_error = err_code;
				break;
		}
	}
}

Please let me know what you think. Would it be ok to make a decision to
error-handle (reset the device) at a transport layer certain cases that
are not required by the spec?

If you have a suggestion on how to pipe telemetry counters to userspace
without this generic callback I can try it out as well.

quoted

Signed-off-by: Dmitry Antipov <redacted>
---
 include/linux/hid.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/include/linux/hid.h b/include/linux/hid.h index

1f134c8f8972..97041c322a0f 100644

--- a/include/linux/hid.h
+++ b/include/linux/hid.h

@@ -703,6 +703,20 @@ struct hid_usage_id {
        __u32 usage_code;
 };

+enum hid_transport_error_type {
+       HID_TRANSPORT_ERROR_TYPE_BUS_INPUT_TRANSFER_START = 0,
+       HID_TRANSPORT_ERROR_TYPE_BUS_INPUT_TRANSFER_BODY,
+       HID_TRANSPORT_ERROR_TYPE_BUS_INPUT_TRANSFER_HEADER,

Those 3 enums above are completely SPI specifics, but they are declared in the
generic hid.h header.
Also, if I am a driver, what am I supposed to do when I receive such an error?
Up till now, the most we did was to raise a warning to the user, and paper over
it. I am open to some smarter behavior, but I do not see what a mouse driver is
supposed to do with that kind of error.

quoted

+       HID_TRANSPORT_ERROR_TYPE_BUS_OUTPUT_TRANSFER,

Seems like this would better handled as a return code than an async callback

quoted

+       HID_TRANSPORT_ERROR_TYPE_DEVICE_INITIATED_RESET,

OK for this (but see my comment in the commit description)

quoted

+       HID_TRANSPORT_ERROR_TYPE_HEADER_DATA,
+       HID_TRANSPORT_ERROR_TYPE_INPUT_REPORT_DATA,
+       HID_TRANSPORT_ERROR_TYPE_REPORT_TYPE,

Those look like SPI specifics

quoted

+       HID_TRANSPORT_ERROR_TYPE_GET_FEATURE_RESPONSE,

Seems like this would be better handled as a return code than an async callback
(and it should already be the case because
hid_ll_raw_request() is synchronous and can fail if the HW complains).

quoted

+       HID_TRANSPORT_ERROR_TYPE_REGULATOR_ENABLE,
+       HID_TRANSPORT_ERROR_TYPE_REGULATOR_DISABLE

Again, what am I supposed to do with those 2 if they fail, besides emitting a
dev_err(), which the low level transport driver can do?


Cheers,
Benjamin

quoted

+};
+
 /**
  * struct hid_driver
  * @name: driver name (e.g. "Footech_bar-wheel") @@ -726,6 +740,7 @@
struct hid_usage_id {
  * @suspend: invoked on suspend (NULL means nop)
  * @resume: invoked on resume if device was not reset (NULL means nop)
  * @reset_resume: invoked on resume if device was reset (NULL means
nop)
+ * @on_transport_error: invoked on error hit by transport driver
  *
  * probe should return -errno on error, or 0 on success. During probe,
  * input will not be passed to raw_event unless hid_device_io_start
is @@ -777,6 +792,10 @@ struct hid_driver {
        void (*feature_mapping)(struct hid_device *hdev,
                        struct hid_field *field,
                        struct hid_usage *usage);
+       void (*on_transport_error)(struct hid_device *hdev,
+                       int err_type,
+                       int err_code,
+                       bool handled);
 #ifdef CONFIG_PM
        int (*suspend)(struct hid_device *hdev, pm_message_t message);
        int (*resume)(struct hid_device *hdev);
--
2.25.1