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

Hi,

Benjamin Tissoires [off-list ref] writes:

On Sat, Jan 8, 2022 at 2:10 AM Dmitry Antipov [off-list ref] wrote:

quoted

On Tue, Jan 4, 2022 at 7:52 AM Benjamin Tissoires
[off-list ref] wrote:

quoted

On Tue, Jan 4, 2022 at 3:08 AM Dmitry Antipov [off-list ref]
wrote:

quoted

quoted

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

quoted

quoted

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

quoted

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]

-- 
balbi