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.
>
> 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
>
