| Message ID | 20211229231141.303919-4-dmanti@microsoft.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add spi-hid, transport for HID over SPI bus | expand |
On Thu, Dec 30, 2021 at 12:11 AM Dmitry Antipov <daantipov@gmail.com> 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. > > Signed-off-by: Dmitry Antipov <dmanti@microsoft.com> > --- > 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 >
> -----Original Message----- > From: Benjamin Tissoires <benjamin.tissoires@redhat.com> > Sent: Monday, January 3, 2022 7:27 AM > To: Dmitry Antipov <daantipov@gmail.com> > Cc: Jiri Kosina <jikos@kernel.org>; open list:HID CORE LAYER <linux- > input@vger.kernel.org>; Felipe Balbi <balbi@kernel.org>; Dmitry Antipov > <dmanti@microsoft.com> > 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@gmail.com> > 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@microsoft.com> > > --- > > 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 > >
On Tue, Jan 4, 2022 at 3:08 AM Dmitry Antipov <dmanti@microsoft.com> wrote: > > > -----Original Message----- > > From: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > Sent: Monday, January 3, 2022 7:27 AM > > To: Dmitry Antipov <daantipov@gmail.com> > > Cc: Jiri Kosina <jikos@kernel.org>; open list:HID CORE LAYER <linux- > > input@vger.kernel.org>; Felipe Balbi <balbi@kernel.org>; Dmitry Antipov > > <dmanti@microsoft.com> > > 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@gmail.com> > > 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. Looking at section 9 (Error handling) of the HID SPI protocol spec, it seems that the only time the host may (or not) decide to reset the device is when receiving a timeout error. And looking at the phrasing there, I think we ought to simply reset the device anyway. So now that I have the spec under my eyes, I would think that for this part, the host is expected to reset the device, which in turn makes this a spi-hid responsibility. So I would suggest adding a callback notifying that the device has been reset, and with a flag telling whether it's host or device initiated. Then in hid-microsoft, hid-multitouch we can deal with that situation. Putting this at the transport layer allows for a common behavior which won't depend on the leaf HID driver in use. > 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: Oh, that's interesting. How about we put those stats in api-hid-core.c, so that anybody can benefit from it? Those are per-device anyway so that might be a useful way to debug issues when there are weird behaviors. > > 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? I would suggest we stay as close as possible to the spec. When the spec says we need to reset, we do it, and notify the driver. TBH, the only thing that works in the long run is to map the implementation from Windows, when this gets more widespread. And we can always quirk the devices that need a special error handling or revisit at that particular time when we get the device in question. > > If you have a suggestion on how to pipe telemetry counters to userspace > without this generic callback I can try it out as well. So as I mentioned we should probably set those in spi-hid. The other and more modern approach is to use BPF, but that would be only when the program is loaded. So I would keep the raw values in spi-hid, export them through sysfs, and possibly allow for some tracing through BPF if we want to get something more dynamic (like real time reading of values). Cheers, Benjamin > > > > > > > Signed-off-by: Dmitry Antipov <dmanti@microsoft.com> > > > --- > > > 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 > > > >
On Tue, Jan 4, 2022 at 7:52 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > On Tue, Jan 4, 2022 at 3:08 AM Dmitry Antipov <dmanti@microsoft.com> > wrote: > > > > > -----Original Message----- > > > From: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > Sent: Monday, January 3, 2022 7:27 AM > > > To: Dmitry Antipov <daantipov@gmail.com> > > > Cc: Jiri Kosina <jikos@kernel.org>; open list:HID CORE LAYER <linux- > > > input@vger.kernel.org>; Felipe Balbi <balbi@kernel.org>; Dmitry > > > Antipov <dmanti@microsoft.com> > > > 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@gmail.com> > > > 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. > > Looking at section 9 (Error handling) of the HID SPI protocol spec, it seems that > the only time the host may (or not) decide to reset the device is when receiving > a timeout error. > And looking at the phrasing there, I think we ought to simply reset the device > anyway. > > So now that I have the spec under my eyes, I would think that for this part, the > host is expected to reset the device, which in turn makes this a spi-hid > responsibility. > > So I would suggest adding a callback notifying that the device has been reset, > and with a flag telling whether it's host or device initiated. > Then in hid-microsoft, hid-multitouch we can deal with that situation. > > Putting this at the transport layer allows for a common behavior which won't > depend on the leaf HID driver in use. Please note the "ready" flag that is wired to a sysfs attribute in spi-hid in patch 5/5. In our case the touch digitizer sends the raw data, so we process it and convert it into input events in a userspace service we call the touch daemon. The touch daemon detects digitizer resets via the ready flag: any time the flag goes from "not ready" to "ready", it is interpreted as digitizer coming out of reset and the touch daemon then sends some system state info to the digitizer, among other things. While the ready flag is "not ready", in our architecture, the userspace will not send ioctl's or write into the hidraw device. All this means that the code in hid-microsoft won't be implementing this new notify_of_reset() callback. Since in the final submission there won't be an implementation of this callback, is it worth adding at this stage? Can it go in as a REVISIT or a FIXME comment until such notification to the leaf driver is needed? > > 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: > > Oh, that's interesting. How about we put those stats in api-hid-core.c, so that > anybody can benefit from it? > Those are per-device anyway so that might be a useful way to debug issues > when there are weird behaviors. I haven't found an api-hid-core.c. Are you suggesting I create a new file at drivers/hid that would extend hid-core.c? If yes, can you please tell what you expect to be in the HID core vs the transport driver? > > > > 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? > > I would suggest we stay as close as possible to the spec. When the spec says we > need to reset, we do it, and notify the driver. > TBH, the only thing that works in the long run is to map the implementation > from Windows, when this gets more widespread. > And we can always quirk the devices that need a special error handling or > revisit at that particular time when we get the device in question. > > > > > > If you have a suggestion on how to pipe telemetry counters to > > userspace without this generic callback I can try it out as well. > > So as I mentioned we should probably set those in spi-hid. The other and more > modern approach is to use BPF, but that would be only when the program is > loaded. So I would keep the raw values in spi-hid, export them through sysfs, > and possibly allow for some tracing through BPF if we want to get something > more dynamic (like real time reading of values). Does api-hid-core.c play a role in the suggested non-BPF, basic approach? > > Cheers, > Benjamin > > > > > > > > > > > Signed-off-by: Dmitry Antipov <dmanti@microsoft.com> > > > > --- > > > > 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 > > > > > >
On Sat, Jan 8, 2022 at 2:10 AM Dmitry Antipov <dmanti@microsoft.com> wrote: > > On Tue, Jan 4, 2022 at 7:52 AM Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: > > > > On Tue, Jan 4, 2022 at 3:08 AM Dmitry Antipov <dmanti@microsoft.com> > > wrote: > > > > > > > -----Original Message----- > > > > From: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > > Sent: Monday, January 3, 2022 7:27 AM > > > > To: Dmitry Antipov <daantipov@gmail.com> > > > > Cc: Jiri Kosina <jikos@kernel.org>; open list:HID CORE LAYER <linux- > > > > input@vger.kernel.org>; Felipe Balbi <balbi@kernel.org>; Dmitry > > > > Antipov <dmanti@microsoft.com> > > > > 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@gmail.com> > > > > 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. > > > > Looking at section 9 (Error handling) of the HID SPI protocol spec, it seems that > > the only time the host may (or not) decide to reset the device is when receiving > > a timeout error. > > And looking at the phrasing there, I think we ought to simply reset the device > > anyway. > > > > So now that I have the spec under my eyes, I would think that for this part, the > > host is expected to reset the device, which in turn makes this a spi-hid > > responsibility. > > > > So I would suggest adding a callback notifying that the device has been reset, > > and with a flag telling whether it's host or device initiated. > > Then in hid-microsoft, hid-multitouch we can deal with that situation. > > > > Putting this at the transport layer allows for a common behavior which won't > > depend on the leaf HID driver in use. > > Please note the "ready" flag that is wired to a sysfs attribute in > spi-hid in patch 5/5. In our case the touch digitizer sends the raw > data, so we process it and convert it into input events in a userspace > service we call the touch daemon. The touch daemon detects digitizer > resets via the ready flag: any time the flag goes from "not ready" to > "ready", it is interpreted as digitizer coming out of reset and the > touch daemon then sends some system state info to the digitizer, among > other things. While the ready flag is "not ready", in our architecture, > the userspace will not send ioctl's or write into the hidraw device. So that means that this device is forwarding the raw touch map? > > All this means that the code in hid-microsoft won't be implementing this > new notify_of_reset() callback. Since in the final submission there > won't be an implementation of this callback, is it worth adding at this > stage? Can it go in as a REVISIT or a FIXME comment until such > notification to the leaf driver is needed? If there is no users, then it's probably best to not implement it. We could add a comment, yes, but maybe not a FIXME, just a regular comment. > > > > 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: > > > > Oh, that's interesting. How about we put those stats in api-hid-core.c, so that > > anybody can benefit from it? > > Those are per-device anyway so that might be a useful way to debug issues > > when there are weird behaviors. > > I haven't found an api-hid-core.c. Are you suggesting I create a new > file at drivers/hid that would extend hid-core.c? If yes, can you please > tell what you expect to be in the HID core vs the transport driver? Sorry I meant i2c-hid-core.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? > > > > I would suggest we stay as close as possible to the spec. When the spec says we > > need to reset, we do it, and notify the driver. > > TBH, the only thing that works in the long run is to map the implementation > > from Windows, when this gets more widespread. > > And we can always quirk the devices that need a special error handling or > > revisit at that particular time when we get the device in question. > > > > > > > > > > If you have a suggestion on how to pipe telemetry counters to > > > userspace without this generic callback I can try it out as well. > > > > So as I mentioned we should probably set those in spi-hid. The other and more > > modern approach is to use BPF, but that would be only when the program is > > loaded. So I would keep the raw values in spi-hid, export them through sysfs, > > and possibly allow for some tracing through BPF if we want to get something > > more dynamic (like real time reading of values). > > Does api-hid-core.c play a role in the suggested non-BPF, basic approach? Again, sorry for the confusion. I think you should keep in mind that BPF might be a solution in the long run, but right now it's not merged, so please ignore it for the time being :) Cheers, Benjamin > > > > > Cheers, > > Benjamin > > > > > > > > > > > > > > > Signed-off-by: Dmitry Antipov <dmanti@microsoft.com> > > > > > --- > > > > > 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 > > > > > > > > >
Hi, Benjamin Tissoires <benjamin.tissoires@redhat.com> writes: > On Sat, Jan 8, 2022 at 2:10 AM Dmitry Antipov <dmanti@microsoft.com> wrote: >> >> On Tue, Jan 4, 2022 at 7:52 AM Benjamin Tissoires >> <benjamin.tissoires@redhat.com> wrote: >> > >> > On Tue, Jan 4, 2022 at 3:08 AM Dmitry Antipov <dmanti@microsoft.com> >> > wrote: >> > > >> > > > -----Original Message----- >> > > > From: Benjamin Tissoires <benjamin.tissoires@redhat.com> >> > > > Sent: Monday, January 3, 2022 7:27 AM >> > > > To: Dmitry Antipov <daantipov@gmail.com> >> > > > Cc: Jiri Kosina <jikos@kernel.org>; open list:HID CORE LAYER <linux- >> > > > input@vger.kernel.org>; Felipe Balbi <balbi@kernel.org>; Dmitry >> > > > Antipov <dmanti@microsoft.com> >> > > > 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@gmail.com> >> > > > 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. >> > >> > Looking at section 9 (Error handling) of the HID SPI protocol spec, it seems that >> > the only time the host may (or not) decide to reset the device is when receiving >> > a timeout error. >> > And looking at the phrasing there, I think we ought to simply reset the device >> > anyway. >> > >> > So now that I have the spec under my eyes, I would think that for this part, the >> > host is expected to reset the device, which in turn makes this a spi-hid >> > responsibility. >> > >> > So I would suggest adding a callback notifying that the device has been reset, >> > and with a flag telling whether it's host or device initiated. >> > Then in hid-microsoft, hid-multitouch we can deal with that situation. >> > >> > Putting this at the transport layer allows for a common behavior which won't >> > depend on the leaf HID driver in use. >> >> Please note the "ready" flag that is wired to a sysfs attribute in >> spi-hid in patch 5/5. In our case the touch digitizer sends the raw >> data, so we process it and convert it into input events in a userspace >> service we call the touch daemon. The touch daemon detects digitizer >> resets via the ready flag: any time the flag goes from "not ready" to >> "ready", it is interpreted as digitizer coming out of reset and the >> touch daemon then sends some system state info to the digitizer, among >> other things. While the ready flag is "not ready", in our architecture, >> the userspace will not send ioctl's or write into the hidraw device. > > So that means that this device is forwarding the raw touch map? yes it is. Raw touch map for fingers, some other not-truly-raw reports for pen, and some vendor specific messages (mostly tuning-related and some telemetry/debug data). >> All this means that the code in hid-microsoft won't be implementing this >> new notify_of_reset() callback. Since in the final submission there >> won't be an implementation of this callback, is it worth adding at this >> stage? Can it go in as a REVISIT or a FIXME comment until such >> notification to the leaf driver is needed? > > If there is no users, then it's probably best to not implement it. We > could add a comment, yes, but maybe not a FIXME, just a regular > comment. +1 [snip]
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, + HID_TRANSPORT_ERROR_TYPE_BUS_OUTPUT_TRANSFER, + HID_TRANSPORT_ERROR_TYPE_DEVICE_INITIATED_RESET, + HID_TRANSPORT_ERROR_TYPE_HEADER_DATA, + HID_TRANSPORT_ERROR_TYPE_INPUT_REPORT_DATA, + HID_TRANSPORT_ERROR_TYPE_REPORT_TYPE, + HID_TRANSPORT_ERROR_TYPE_GET_FEATURE_RESPONSE, + HID_TRANSPORT_ERROR_TYPE_REGULATOR_ENABLE, + HID_TRANSPORT_ERROR_TYPE_REGULATOR_DISABLE +}; + /** * 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);
This new API allows a transport driver to notify the HID device driver about a transport layer error. Signed-off-by: Dmitry Antipov <dmanti@microsoft.com> --- include/linux/hid.h | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)