| Message ID | 20211229231141.303919-4-dmanti@microsoft.com |
|---|---|
| State | New |
| 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 >
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 > > > > > > > > >
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(+)