> -----Original Message-----
> From: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>
> Sent: Monday, January 3, 2022 7:27 AM
> To: Dmitry Antipov <daantipov@xxxxxxxxx>
> Cc: Jiri Kosina <jikos@xxxxxxxxxx>; open list:HID CORE LAYER <linux-
> input@xxxxxxxxxxxxxxx>; Felipe Balbi <balbi@xxxxxxxxxx>; Dmitry Antipov
> <dmanti@xxxxxxxxxxxxx>
> 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 <daantipov@xxxxxxxxx>
> wrote:
> >
> > 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.
> >
> > Signed-off-by: Dmitry Antipov <dmanti@xxxxxxxxxxxxx>
> > ---
> > 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.
>
> > + HID_TRANSPORT_ERROR_TYPE_BUS_OUTPUT_TRANSFER,
>
> Seems like this would better handled as a return code than an async callback
>
> > + HID_TRANSPORT_ERROR_TYPE_DEVICE_INITIATED_RESET,
>
> OK for this (but see my comment in the commit description)
>
> > + HID_TRANSPORT_ERROR_TYPE_HEADER_DATA,
> > + HID_TRANSPORT_ERROR_TYPE_INPUT_REPORT_DATA,
> > + HID_TRANSPORT_ERROR_TYPE_REPORT_TYPE,
>
> Those look like SPI specifics
>
> > + 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).
>
> > + 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
>
> > +};
> > +
> > /**
> > * 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
> >
