HID: Transport Driver Cleanup

usbhid_set_leds() is only used inside of usbhid/hid-core.c so no need to export it.

Signed-off-by David Herrmann <[email protected]>

---

Report fields can be updated from HID drivers unlocked via hid_set_field(). It is protected by input_lock in HID core so only a single input event is handled at a time. USBHID can thus update the field unlocked and doesn't conflict with any HID vendor/device drivers. Note, many HID drivers make heavy use of hid_set_field() in that way.

But usbhid also schedules a work to gather multiple LED changes in a single report. Hence, we used to lock the LED field update so the work can read a consistent state. However, hid_set_field() only writes a single integer field, which is guaranteed to be allocated all the time. So the worst possible race-condition is a garbage read on the LED field.

Therefore, there is no need to protect the update. In fact, the only thing that is prevented by locking hid_set_field(), is an LED update while the scheduled work currently writes an older LED update out. However, this means, a new work is scheduled directly when the old one is done writing the new state to the device. So we actually _win_ by not protecting the write and allowing the write to be combined with the current write. A new worker is still scheduled, but will not write any new state. So the LED will not blink unnecessarily on the device.

Assume we have the LED set to 0. Two request come in which enable the LED and immediately disable it. The current situation with two CPUs would be:

  usb_hidinput_input_event()       |      hid_led()
  ---------------------------------+----------------------------------
    spin_lock(&usbhid->lock);
    hid_set_field(1);
    spin_unlock(&usbhid->lock);
    schedule_work(...);
                                      spin_lock(&usbhid->lock);
                                      __usbhid_submit_report(..1..);
                                      spin_unlock(&usbhid->lock);
    spin_lock(&usbhid->lock);
    hid_set_field(0);
    spin_unlock(&usbhid->lock);
    schedule_work(...);
                                      spin_lock(&usbhid->lock);
                                      __usbhid_submit_report(..0..);
                                      spin_unlock(&usbhid->lock);

With the locking removed, we _might_ end up with (look at the changed __usbhid_submit_report() parameters in the first try!):

  usb_hidinput_input_event()       |      hid_led()
  ---------------------------------+----------------------------------
    hid_set_field(1);
    schedule_work(...);
                                      spin_lock(&usbhid->lock);
    hid_set_field(0);
    schedule_work(...);
                                      __usbhid_submit_report(..0..);
                                      spin_unlock(&usbhid->lock);

                                      ... next work ...

                                      spin_lock(&usbhid->lock);
                                      __usbhid_submit_report(..0..);
                                      spin_unlock(&usbhid->lock);

As one can see, we no longer send the "LED ON" signal as it is disabled immediately afterwards and the following "LED OFF" request overwrites the pending "LED ON".

It is important to note that hid_set_field() is not atomic, so we might also end up with any other value. But that doesn't matter either as we _always_ schedule the next work with a correct value and schedule_work() acts as memory barrier, anyways. So in the worst case, we run __usbhid_submit_report(..<garbage>..) in the first case and the following __usbhid_submit_report() will write the correct value. But LED states are booleans so any garbage will be converted to either 0 or 1 and the remote device will never see invalid requests.

Why all this? It avoids any custom locking around hid_set_field() in usbhid and finally allows us to provide a generic hidinput_input_event() handler for all HID transport drivers.

Signed-off-by David Herrmann <[email protected]>

---

Quoted 77 lines

Report fields can be updated from HID drivers unlocked via hid_set_field(). It is protected by input_lock in HID core so only a single input event is handled at a time. USBHID can thus update the field unlocked and doesn't conflict with any HID vendor/device drivers. Note, many HID drivers make heavy use of hid_set_field() in that way.

But usbhid also schedules a work to gather multiple LED changes in a single report. Hence, we used to lock the LED field update so the work can read a consistent state. However, hid_set_field() only writes a single integer field, which is guaranteed to be allocated all the time. So the worst possible race-condition is a garbage read on the LED field.

Therefore, there is no need to protect the update. In fact, the only thing that is prevented by locking hid_set_field(), is an LED update while the scheduled work currently writes an older LED update out. However, this means, a new work is scheduled directly when the old one is done writing the new state to the device. So we actually _win_ by not protecting the write and allowing the write to be combined with the current write. A new worker is still scheduled, but will not write any new state. So the LED will not blink unnecessarily on the device.

Assume we have the LED set to 0. Two request come in which enable the LED and immediately disable it. The current situation with two CPUs would be:

  usb_hidinput_input_event()       |      hid_led()
  ---------------------------------+----------------------------------
    spin_lock(&usbhid->lock);
    hid_set_field(1);
    spin_unlock(&usbhid->lock);
    schedule_work(...);
                                      spin_lock(&usbhid->lock);
                                      __usbhid_submit_report(..1..);
                                      spin_unlock(&usbhid->lock);
    spin_lock(&usbhid->lock);
    hid_set_field(0);
    spin_unlock(&usbhid->lock);
    schedule_work(...);
                                      spin_lock(&usbhid->lock);
                                      __usbhid_submit_report(..0..);
                                      spin_unlock(&usbhid->lock);

With the locking removed, we _might_ end up with (look at the changed __usbhid_submit_report() parameters in the first try!):

  usb_hidinput_input_event()       |      hid_led()
  ---------------------------------+----------------------------------
    hid_set_field(1);
    schedule_work(...);
                                      spin_lock(&usbhid->lock);
    hid_set_field(0);
    schedule_work(...);
                                      __usbhid_submit_report(..0..);
                                      spin_unlock(&usbhid->lock);

                                      ... next work ...

                                      spin_lock(&usbhid->lock);
                                      __usbhid_submit_report(..0..);
                                      spin_unlock(&usbhid->lock);

As one can see, we no longer send the "LED ON" signal as it is disabled immediately afterwards and the following "LED OFF" request overwrites the pending "LED ON".

It is important to note that hid_set_field() is not atomic, so we might also end up with any other value. But that doesn't matter either as we _always_ schedule the next work with a correct value and schedule_work() acts as memory barrier, anyways. So in the worst case, we run __usbhid_submit_report(..<garbage>..) in the first case and the following __usbhid_submit_report() will write the correct value. But LED states are booleans so any garbage will be converted to either 0 or 1 and the remote device will never see invalid requests.

Why all this? It avoids any custom locking around hid_set_field() in usbhid and finally allows us to provide a generic hidinput_input_event() handler for all HID transport drivers.

Quoted 2 lines

Signed-off-by David Herrmann <[email protected]>

---

Hi David,

that was a very good commit message!

Reviewed-by Benjamin Tissoires <[email protected]>

The hidinput_input_event() callback converts input events written from userspace into HID reports and sends them to the device. We currently implement this in every HID transport driver, even though most of them do the same.

This provides a generic hidinput_input_event() implementation which is mostly copied from usbhid. It uses a delayed worker to allow multiple LED events to be collected into a single output event.
We use the custom ->request() transport driver callback to allow drivers to adjust the outgoing report and handle the request asynchronously. If no custom ->request() callback is available, we fall back to the generic raw output report handler (which is synchronous).

Drivers can still provide custom hidinput_input_event() handlers (see logitech-dj) if the generic implementation doesn't fit their needs.

Signed-off-by David Herrmann <[email protected]>

---
 drivers/hid/hid-input.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/hid.h     |  1 +
 2 files changed, 80 insertions(+), 1 deletion(-)

drivers/hid/hid-input.c +33 -0

index 7480799..308eee8 100644
@@ -1137,6 +1137,74 @@ unsigned int hidinput_count_leds(struct hid_device *hid)
 }
 EXPORT_SYMBOL_GPL(hidinput_count_leds);
 
+static void hidinput_led_worker(struct work_struct *work)
+{
+       struct hid_device *hid = container_of(work, struct hid_device,
+                                             led_work);
+       struct hid_field *field;
+       struct hid_report *report;
+       int len;
+       __u8 *buf;
+
+       field = hidinput_get_led_field(hid);
+       if (!field)
+               return;
+
+       /*
+        * field->report is accessed unlocked regarding HID core. So there might
+        * be another incoming SET-LED request from user-space, which changes
+        * the LED state while we assemble our outgoing buffer. However, this
+        * doesn't matter as hid_output_report() correctly converts it into a
+        * boolean value no matter what information is currently set on the LED
+        * field (even garbage). So the remote device will always get a valid
+        * request.
+        * And in case we send a wrong value, a next led worker is spawned
+        * for every SET-LED request so the following worker will send the
+        * correct value, guaranteed!
+        */
+
+       report = field->report;
+
+       /* use custom SET_REPORT request if possible (asynchronous) */
+       if (hid->ll_driver->request)
+               return hid->ll_driver->request(hid, report, HID_REQ_SET_REPORT);
+
+       /* fall back to generic raw-output-report */

drivers/hid/hid-input.c +25 -0

+
+static int hidinput_input_event(struct input_dev *dev, unsigned int type,
+                               unsigned int code, int value)
+{
+       struct hid_device *hid = input_get_drvdata(dev);
+       struct hid_field *field;
+       int offset;
+
+       if (type == EV_FF)
+               return input_ff_event(dev, type, code, value);
+
+       if (type != EV_LED)
+               return -1;
+
+       if ((offset = hidinput_find_field(hid, type, code, &field)) == -1) {
+               hid_warn(dev, "event field not found\n");
+               return -1;
+       }
+
+       hid_set_field(field, offset, value);
+
+       schedule_work(&hid->led_work);
+       return 0;
+}
+
 static int hidinput_open(struct input_dev *dev)
 {
        struct hid_device *hid = input_get_drvdata(dev);

drivers/hid/hid-input.c +1 -0

        input_dev->open = hidinput_open;
        input_dev->close = hidinput_close;
        input_dev->setkeycode = hidinput_setkeycode;

@@ -1278,6 +1349,7 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
        int i, j, k;
 
        INIT_LIST_HEAD(&hid->inputs);
+       INIT_WORK(&hid->led_work, hidinput_led_worker);
 
        if (!force) {
                for (i = 0; i < hid->maxcollection; i++) {

drivers/hid/hid-input.c +0 -0

 }
 EXPORT_SYMBOL_GPL(hidinput_disconnect);
 

include/linux/hid.h +1 -1

index b8058c5..ea4b828 100644
@@ -456,6 +456,7 @@ struct hid_device {                                                 /* device report descriptor */
        enum hid_type type;                                             /* device type (mouse, kbd, ...) */
        unsigned country;                                               /* HID country */
        struct hid_report_enum report_enum[HID_REPORT_TYPES];
+       struct work_struct led_work;                                    /* delayed LED worker */
 
        struct semaphore driver_lock;                                   /* protects the current driver, except during input */
        struct semaphore driver_input_lock;                             /* protects the current driver */
--

1.8.3.2

Quoted 72 lines · David Herrmann

The hidinput_input_event() callback converts input events written from userspace into HID reports and sends them to the device. We currently implement this in every HID transport driver, even though most of them do the same.

This provides a generic hidinput_input_event() implementation which is mostly copied from usbhid. It uses a delayed worker to allow multiple LED events to be collected into a single output event.
We use the custom ->request() transport driver callback to allow drivers to adjust the outgoing report and handle the request asynchronously. If no custom ->request() callback is available, we fall back to the generic raw output report handler (which is synchronous).

Drivers can still provide custom hidinput_input_event() handlers (see logitech-dj) if the generic implementation doesn't fit their needs.

Signed-off-by David Herrmann <[email protected]>

---
 drivers/hid/hid-input.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/hid.h     |  1 +
 2 files changed, 80 insertions(+), 1 deletion(-)

drivers/hid/hid-input.c +42 -0

index 7480799..308eee8 100644
@@ -1137,6 +1137,74 @@ unsigned int hidinput_count_leds(struct hid_device *hid)
 }
 EXPORT_SYMBOL_GPL(hidinput_count_leds);
 
+static void hidinput_led_worker(struct work_struct *work)
+{
+       struct hid_device *hid = container_of(work, struct hid_device,
+                                             led_work);
+       struct hid_field *field;
+       struct hid_report *report;
+       int len;
+       __u8 *buf;
+
+       field = hidinput_get_led_field(hid);
+       if (!field)
+               return;
+
+       /*
+        * field->report is accessed unlocked regarding HID core. So there might
+        * be another incoming SET-LED request from user-space, which changes
+        * the LED state while we assemble our outgoing buffer. However, this
+        * doesn't matter as hid_output_report() correctly converts it into a
+        * boolean value no matter what information is currently set on the LED
+        * field (even garbage). So the remote device will always get a valid
+        * request.
+        * And in case we send a wrong value, a next led worker is spawned
+        * for every SET-LED request so the following worker will send the
+        * correct value, guaranteed!
+        */
+
+       report = field->report;
+
+       /* use custom SET_REPORT request if possible (asynchronous) */
+       if (hid->ll_driver->request)
+               return hid->ll_driver->request(hid, report, HID_REQ_SET_REPORT);
+
+       /* fall back to generic raw-output-report */
+       len = ((report->size - 1) >> 3) + 1 + (report->id > 0);
+       buf = kmalloc(len, GFP_KERNEL);
+       if (!buf)
+               return;
+
+       hid_output_report(report, buf);
+       /* synchronous output report */
+       hid->hid_output_raw_report(hid, buf, len, HID_OUTPUT_REPORT);
+       kfree(buf);

drivers/hid/hid-input.c unsigned int hidinput_count_leds(struct hid_device *hid) · line 1182

Quoted 1 line

drivers/hid/hid-input.c +1 -0

+}

Instead of writing a specific fallback in case hid->ll_driver->request does not exist, I think it would make sense to implement a generic hid_hw_request function in hid-input, so that HIDP and UHID will benefit from it. I think it will be better because the implementation I made in i2c-hid.c is nearly the exact same calls than the fallback here.

Quoted 29 lines

drivers/hid/hid-input.c +25 -0

+
+static int hidinput_input_event(struct input_dev *dev, unsigned int type,
+                               unsigned int code, int value)
+{
+       struct hid_device *hid = input_get_drvdata(dev);
+       struct hid_field *field;
+       int offset;
+
+       if (type == EV_FF)
+               return input_ff_event(dev, type, code, value);
+
+       if (type != EV_LED)
+               return -1;
+
+       if ((offset = hidinput_find_field(hid, type, code, &field)) == -1) {
+               hid_warn(dev, "event field not found\n");
+               return -1;
+       }
+
+       hid_set_field(field, offset, value);
+
+       schedule_work(&hid->led_work);
+       return 0;
+}
+
 static int hidinput_open(struct input_dev *dev)
 {
        struct hid_device *hid = input_get_drvdata(dev);

drivers/hid/hid-input.c static struct hid_input *hidinput_allocate(struct hid_device *hid) · line 1257

Quoted 8 lines

drivers/hid/hid-input.c +4 -1

@@ -1183,7 +1251,10 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid)
        }
 
        input_set_drvdata(input_dev, hid);
-       input_dev->event = hid->ll_driver->hidinput_input_event;
+       if (hid->ll_driver->hidinput_input_event)
+               input_dev->event = hid->ll_driver->hidinput_input_event;
+       else if (hid->ll_driver->request || hid->hid_output_raw_report)
+               input_dev->event = hidinput_input_event;

with a generic hid_hw_request in hid-input, the else is simpler here.

Quoted 9 lines

drivers/hid/hid-input.c +1 -0

        input_dev->open = hidinput_open;
        input_dev->close = hidinput_close;
        input_dev->setkeycode = hidinput_setkeycode;

@@ -1278,6 +1349,7 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
        int i, j, k;
 
        INIT_LIST_HEAD(&hid->inputs);
+       INIT_WORK(&hid->led_work, hidinput_led_worker);
 

drivers/hid/hid-input.c void hidinput_disconnect(struct hid_device *hid) · line 1459

Quoted 12 lines

drivers/hid/hid-input.c +6 -0

        if (!force) {
                for (i = 0; i < hid->maxcollection; i++) {

@@ -1379,6 +1451,12 @@ void hidinput_disconnect(struct hid_device *hid)
                input_unregister_device(hidinput->input);
                kfree(hidinput);
        }
+
+       /* led_work is spawned by input_dev callbacks, but doesn't access the
+        * parent input_dev at all. Once all input devices are removed, we
+        * know that led_work will never get restarted, so we can cancel it
+        * synchronously and are safe. */
+       cancel_work_sync(&hid->led_work);

You missed the multi-lines comment formatting style on this one :)

Quoted 16 lines

drivers/hid/hid-input.c +0 -0

 }
 EXPORT_SYMBOL_GPL(hidinput_disconnect);
 

include/linux/hid.h +1 -1

index b8058c5..ea4b828 100644
@@ -456,6 +456,7 @@ struct hid_device {                                                 /* device report descriptor */
        enum hid_type type;                                             /* device type (mouse, kbd, ...) */
        unsigned country;                                               /* HID country */
        struct hid_report_enum report_enum[HID_REPORT_TYPES];
+       struct work_struct led_work;                                    /* delayed LED worker */
 
        struct semaphore driver_lock;                                   /* protects the current driver, except during input */
        struct semaphore driver_input_lock;                             /* protects the current driver */
--

Quoted 2 lines

1.8.3.2

Cheers,
Benjamin

Quoted 7 lines

Quoted 6 lines

drivers/hid/hid-input.c +6 -0

+
+       /* led_work is spawned by input_dev callbacks, but doesn't access the
+        * parent input_dev at all. Once all input devices are removed, we
+        * know that led_work will never get restarted, so we can cancel it
+        * synchronously and are safe. */
+       cancel_work_sync(&hid->led_work);

Quoted 1 line

You missed the multi-lines comment formatting style on this one :)

The ./net/ subsystem uses these comments quite a lot and there was a discussion between davem and linus with the conclusion that these comments are ok. But I actually don't care, so I can change to normal CodingStyle.

HID core provides the same functionality as we do, so drop the custom hidinput_input_event() handler.

Signed-off-by David Herrmann <[email protected]>

---

HID core provides the same functionality, so drop the custom handler. Besides, the current handler doesn't schedule any outgoing report so it did not work, anyway.

Signed-off-by David Herrmann <[email protected]>

---

HID core provides the same functionality and can convert the input event to a raw output report. We can thus drop UHID_OUTPUT_EV and rely on the mandatory UHID_OUTPUT.

User-space wasn't able to do anything with UHID_OUTPUT_EV, anyway. They don't have access to the report fields.

Signed-off-by David Herrmann <[email protected]>

---

HID core provides the same functionality and can convert the input event to a raw output report. We can thus drop UHID_OUTPUT_EV and rely on the mandatory UHID_OUTPUT.

User-space wasn't able to do anything with UHID_OUTPUT_EV, anyway. They don't have access to the report fields.

Hi David,

Quoted 6 lines

HID core provides the same functionality and can convert the input event to a raw output report. We can thus drop UHID_OUTPUT_EV and rely on the mandatory UHID_OUTPUT.

User-space wasn't able to do anything with UHID_OUTPUT_EV, anyway. They don't have access to the report fields.

I like your patchset overall, but this looks like userspace breakage?

hid-transport.txt describes the transport driver layout which is required by HID-core to work correctly. HID-core supports many different transport drivers and new drivers can be added for any kind of I/O system.

The current layout doesn't really differentiate between intr and ctrl channels, which confuses some hid-drivers and may break devices. HIDP, USBHID and I2DHID all implement different semantics for each callback, so

our device drivers aren't really transport-driver-agnostic.

To solve that, hid-transport.txt describes the layout of transport drivers that is required by HID core. Furthermore, it introduces some new callbacks which replace the current hid_get_raw_report() and hid_output_raw_report() hooks. These will be implemented in follow-up patches.

Signed-off-by David Herrmann <[email protected]>

---
 Documentation/hid/hid-transport.txt | 299 ++++++++++++++++++++++++++++++++++++
 1 file changed, 299 insertions(+)
 create mode 100644 Documentation/hid/hid-transport.txt

Documentation/hid/hid-transport.txt new file +0 -0

new file mode 100644
index 0000000..034f11d

Documentation/hid/hid-transport.txt new file +23 -0

+
+ +-----------+  +-----------+            +-----------+  +-----------+
+ | Device #1 |  | Device #i |            | Device #j |  | Device #k |
+ +-----------+  +-----------+            +-----------+  +-----------+
+          \\      //                              \\      //
+        +------------+                          +------------+
+        | I/O Driver |                          | I/O Driver |
+        +------------+                          +------------+
+              ||                                      ||
+     +------------------+                    +------------------+
+     | Transport Driver |                    | Transport Driver |
+     +------------------+                    +------------------+
+                       \___                ___/
+                           \              /
+                          +----------------+
+                          |    HID Core    |
+                          +----------------+
+                           /  |        |  \
+                          /   |        |   \
+             ____________/    |        |    \_________________
+            /                 |        |                      \
+           /                  |        |                       \
+ +----------------+  +-----------+  +------------------+  +------------------+

Documentation/hid/hid-transport.txt new file +11 -0

+
+Everything below "HID Core" is simplified in this graph as it is only of
+interest to HID device drivers. Transport drivers do not need to know the
+specifics.
+
+1.1) Device Setup
+-----------------
+
+I/O drivers normally provide hotplug detection or device enumeration APIs to the
+transport drivers. Transport drivers use this to find any suitable HID device.
+They allocate HID device objects and register them with HID core. Transport

Documentation/hid/hid-transport.txt new file +13 -0

+device failures. Once transport drivers detect unplug or failure events, they
+must unregister the device from HID core and HID core will stop using the
+provided callbacks.
+
+1.2) Transport Driver Requirements
+----------------------------------
+
+HID core requires transport drivers to follow a given design. A Transport
+drivers must provide two bi-directional I/O channels to each HID device. These
+channels might be multiplexed on a single physical channel, though.
+
+ - Interrupt Channel (intr): The intr channel is used for asynchronous data
+   reports. No management commands or data acknowledgements are sent on this

Documentation/hid/hid-transport.txt new file +19 -0

+
+2) HID API
+==========
+
+2.1) Initialization
+-------------------
+
+Transport drivers normally use the following procedure to register a new device
+with HID core:
+
+       struct hid_device *hid;
+       int ret;
+
+       hid = hid_allocate_device();
+       if (IS_ERR(hid)) {
+               ret = PTR_ERR(hid);
+               goto err_<...>;
+       }
+

Documentation/hid/hid-transport.txt new file +21 -0

+
+       hid_destroy_device(hid);
+
+Once hid_destroy_device() returns, HID core will no longer make use of any
+driver callbacks.
+
+2.2) hid_ll_driver operations
+-----------------------------
+
+The available HID callbacks are:
+ - int (*start) (struct hid_device *hdev)
+   Called from HID device drivers once they want to use the device. Transport
+   drivers can choose to setup their device in this callback. However, normally
+   devices are already set up before transport drivers register them to HID core
+   so this is mostly only used by USB-HID.
+
+ - void (*stop) (struct hid_device *hdev)
+   Called from HID device drivers once they are done with a device. Transport
+   drivers can free any buffers and deinitialize the device. But note that
+   ->start() might be called again if another HID device driver is loaded on the
+   device.

Documentation/hid/hid-transport.txt new file +28 -0

+
+ - void (*close) (struct hid_device *hdev)
+   Called from HID device drivers after ->open() was called but they are no
+   longer interested in device reports. (Usually if user-space closed any input
+   devices of the driver).
+   Transport drivers can put devices asleep and terminate any I/O. However,
+   ->start() may be called again if the device driver is interested in input
+   reports again.
+
+ - int (*parse) (struct hid_device *hdev)
+   Called once during device setup after ->start() has been called. Transport
+   drivers must read the HID report-descriptor from the device and tell HID core
+   about it via hid_parse_report().
+
+ - int (*power) (struct hid_device *hdev, int level)
+   Called by HID core to give PM hints to transport drivers. Usually this is
+   analogical to the ->open() and ->close() hints and redundant.
+
+ - void (*request) (struct hid_device *hdev, struct hid_report *report,
+                    int reqtype)
+   Send an HID request on the ctrl channel. "report" contains the report that
+   should be sent and "reqtype" the request type. Request-type can be
+   HID_REQ_SET_REPORT or HID_REQ_GET_REPORT.
+   This callback is optional. If not provided, HID core will assemble a raw
+   report following the HID specs and send it via the ->raw_request() callback.
+   The transport driver is free to implement this asynchronously.
+
+ - int (*wait) (struct hid_device *hdev)

Documentation/hid/hid-transport.txt new file +15 -1

+handshakes or other management commands which can be required by the given HID
+transport specification.
+
+Every raw data packet read from a device must be fed into HID core via
+hid_input_report(). You must specify the channel-type (intr or ctrl) and report
+type (input/output/feature). Under normal conditions, only input reports are
+provided via this API.
+
+Responses to GET_REPORT requests via ->request() must also be provided via this
+API. Responses to ->raw_request() are synchronous and must be intercepted by the
+transport driver and not passed to hid_input_report().
+Acknowledgements to SET_REPORT requests are not of interest to HID core.
+
+----------------------------------------------------
+Written 2013, David Herrmann <[email protected]>
--

1.8.3.2

Hi David,

Quoted 4 lines · David Herrmann

hid-transport.txt describes the transport driver layout which is required by HID-core to work correctly. HID-core supports many different transport drivers and new drivers can be added for any kind of I/O system.

Quoted 3 lines

The current layout doesn't really differentiate between intr and ctrl channels, which confuses some hid-drivers and may break devices. HIDP, USBHID and I2DHID all implement different semantics for each callback, so

typo: I2CHID

Quoted 14 lines

our device drivers aren't really transport-driver-agnostic.

To solve that, hid-transport.txt describes the layout of transport drivers that is required by HID core. Furthermore, it introduces some new callbacks which replace the current hid_get_raw_report() and hid_output_raw_report() hooks. These will be implemented in follow-up patches.

Signed-off-by David Herrmann <[email protected]>

---
 Documentation/hid/hid-transport.txt | 299 ++++++++++++++++++++++++++++++++++++
 1 file changed, 299 insertions(+)
 create mode 100644 Documentation/hid/hid-transport.txt

Documentation/hid/hid-transport.txt line 7

Quoted 13 lines

Documentation/hid/hid-transport.txt new file +7 -0

new file mode 100644
index 0000000..034f11d
@@ -0,0 +1,299 @@
+                          HID I/O Transport Drivers
+                         ===========================
+
+The HID subsystem is independent of the underlying transport driver. Initially,
+only USB was supported, but other specifications adopted the HID design and
+provided new transport drivers. The kernel includes at least support for USB,
+Bluetooth, i2c and user-space I/O drivers.

IIRC, we should put I2C in capital letters.

Documentation/hid/hid-transport.txt line 17

Quoted 10 lines

Documentation/hid/hid-transport.txt new file +10 -0

+
+1) HID Bus
+==========
+
+The HID subsystem is designed as a bus. Any I/O subsystem may provide HID
+devices and register them with the HID bus. HID core then loads generic device
+drivers on top of it. The transport drivers are responsible of raw data
+transport and device setup/management. HID core is responsible of
+report-parsing, report interpretation and the user-space API. Device specifics
+and quirks are also handled by HID core and the HID device drivers.

Hmm, the quirks part is not exactly what is currently implemented. Usbhid (Transport Driver) set and use quirks while you say here that they are handled by hid-core.

I personally prefer the way it is documented here (quirks handled in hid-core) so HIDP, i2c-hid and uhid will benefit from the dynamic quirks already implemented in usbhid. But that will require another patch :)

Quoted 27 lines

Documentation/hid/hid-transport.txt new file +27 -0

+
+ +-----------+  +-----------+            +-----------+  +-----------+
+ | Device #1 |  | Device #i |            | Device #j |  | Device #k |
+ +-----------+  +-----------+            +-----------+  +-----------+
+          \\      //                              \\      //
+        +------------+                          +------------+
+        | I/O Driver |                          | I/O Driver |
+        +------------+                          +------------+
+              ||                                      ||
+     +------------------+                    +------------------+
+     | Transport Driver |                    | Transport Driver |
+     +------------------+                    +------------------+
+                       \___                ___/
+                           \              /
+                          +----------------+
+                          |    HID Core    |
+                          +----------------+
+                           /  |        |  \
+                          /   |        |   \
+             ____________/    |        |    \_________________
+            /                 |        |                      \
+           /                  |        |                       \
+ +----------------+  +-----------+  +------------------+  +------------------+
+ | Generic Driver |  | MT Driver |  | Custom Driver #1 |  | Custom Driver #2 |
+ +----------------+  +-----------+  +------------------+  +------------------+
+
+Example Drivers:

Documentation/hid/hid-transport.txt line 50

Quoted 6 lines

Documentation/hid/hid-transport.txt new file +6 -0

+  I/O: USB, I2C, Bluetooth-l2cap
+  Transport: USB-HID, I2C-HID, BT-HIDP
+
+Normally, a transport driver is specific for a given I/O driver. But the HID
+design also allows transport drivers to work with different I/O systems at the
+same time.

I do not follow you here. Which transport driver you have in mind?

Quoted 15 lines

Documentation/hid/hid-transport.txt new file +15 -0

+
+Everything below "HID Core" is simplified in this graph as it is only of
+interest to HID device drivers. Transport drivers do not need to know the
+specifics.
+
+1.1) Device Setup
+-----------------
+
+I/O drivers normally provide hotplug detection or device enumeration APIs to the
+transport drivers. Transport drivers use this to find any suitable HID device.
+They allocate HID device objects and register them with HID core. Transport
+drivers are not required to register themselves with HID core. HID core is never
+aware of which transport drivers are available and is not interested in it. It
+is only interested in devices.
+

Documentation/hid/hid-transport.txt line 71

Quoted 6 lines

Documentation/hid/hid-transport.txt new file +6 -0

+Transport drivers attach a constant "struct hid_ll_driver" object with each
+device. Once a device is registered with HID core, the callbacks provided via
+this struct are used by HID core to communicate with the device.
+
+Transport drivers are responsible of detecting device failures and unplugging.
+HID core will operate an device as long as it is registered regardless of any

typo: ^^ "a"

Quoted 13 lines

Documentation/hid/hid-transport.txt new file +13 -0

+device failures. Once transport drivers detect unplug or failure events, they
+must unregister the device from HID core and HID core will stop using the
+provided callbacks.
+
+1.2) Transport Driver Requirements
+----------------------------------
+
+HID core requires transport drivers to follow a given design. A Transport
+drivers must provide two bi-directional I/O channels to each HID device. These
+channels might be multiplexed on a single physical channel, though.
+
+ - Interrupt Channel (intr): The intr channel is used for asynchronous data
+   reports. No management commands or data acknowledgements are sent on this

Documentation/hid/hid-transport.txt line 94

Quoted 10 lines

Documentation/hid/hid-transport.txt new file +10 -0

+   channel. Any unrequested incoming or outgoing data report must be sent on
+   this channel and is never acknowledged by the remote side. Devices usually
+   send their input events on this channel. Outgoing events are normally
+   not send via intr, except if high throughput is required.
+ - Control Channel (ctrl): The ctrl channel is used for synchronous requests and
+   device management. Unrequested data input events must not be sent on this
+   channel and are normally ignored. Instead, devices only send management
+   events or answers to host requests on this channel.
+   Outgoing reports are usually sent on the ctrl channel via synchronous
+   SET_REPORT requests.

This is not entirely true for USB and I2C:
- for USB, OUTPUT reports can also be sent through a direct write on an URB.
- for I2C, the notion of intr vs ctrl is slightly different, but it remains roughly the same. Let me detail it so that you can make your idea:
The HID descriptors declares three I2C registers:
* the "Input" register: this one correspond to the intr you are
describing. The difference is that this register is only used from the communication from the device to the host (no outputs here). The big interest in this register is that the communication is device initiated thanks to an external line triggered by the device (in I2C, the communication is only master (host) initiated).
* the "Command" register: this one corresponds to the ctrl channel.
This channel/register is host initiated too (like on the USB side), and there is a slight head over due to the HID protocol regarding sending output events.
* the "Output" register: this one corresponds to the URBs described in
USB. Every communication coming from the host on this channel is considered as an OUTPUT report, reducing the HID protocol over head.

The HID/I2C also declares a forth register (the "data") register which
is used by the HID protocol while using the "Command" register.

To sum up, I think the differences between the channels are:
- Interrupt Channel (intr): asynchronous, device initiated, no acknowledgements (beside the fact that data has been read), read only.
- Control Channel (ctrl): synchronous, host initiated, acknowledged (because synchronous), read/write
- Output Channel (out): synchronous, host initiated, acknowledged (because synchronous), write only

Then, the usbhid transport layer makes the ctrl and out channels asynchronous...

Documentation/hid/hid-transport.txt line 101

Quoted 7 lines

Documentation/hid/hid-transport.txt new file +7 -0

+
+Communication between devices and HID core is mostly done via HID reports. A
+report can be of one of three types:
+
+ - INPUT Report: Input reports provide data from device to host. This
+   data may include button events, axis events, battery status or more. This
+   data is generated by the device and sent to the host without requiring

_with_ or without requiring explicit requests.
On I2C, the command GET_REPORT is even mandatory (which does not seems to be the case for USB, given the amount of QUIRK_NO_INIT_REPORT we have.)

Documentation/hid/hid-transport.txt line 102

Quoted 1 line

Documentation/hid/hid-transport.txt new file +1 -0

+   explicit requests. Devices can choose to send data continously or only on

My spell checker gives me a typo on: "continuously"

Documentation/hid/hid-transport.txt line 106

Quoted 4 lines

Documentation/hid/hid-transport.txt new file +4 -0

+   change.
+ - OUTPUT Report: Output reports change device states. They are sent from host
+   to device and may include LED requests, rumble requests or more. Output
+   reports are never sent from device to host, except if explicitly requested.

In this case, I think it would be better to say that Output reports are never sent from device to host, but the host can _retrieve_ its current state.

Documentation/hid/hid-transport.txt line 107

Quoted 1 line

Documentation/hid/hid-transport.txt new file +1 -0

+   Hosts may choose to send output reports either continously or only on change.

My spell checker gives me a typo on: "continuously"

Documentation/hid/hid-transport.txt line 111

Quoted 4 lines

Documentation/hid/hid-transport.txt new file +4 -0

+ - FEATURE Report: Feature reports can be anything that doesn't belong into the
+   other two categories. Some of them are generic and defined by the HID spec,
+   but most of them are used as custom device extensions. For instance, they may
+   provide battery status information.

I disagree. FEATURE are specific reports which are read/write, and not spontaneously emitted by the device. For instance, this allows the host to change the current mode of the device (mouse emulation or multitouch in the case I know most), or to set constants in the device (max reported touches in my case, but this can be hardware macros for keyboards, or something else).

Documentation/hid/hid-transport.txt line 113

Quoted 2 lines

Documentation/hid/hid-transport.txt new file +2 -0

+   Feature reports are never sent without requests. A host must explicitly
+   request a device to set or send a feature report. This also means, feature

I would prefer a "host must explicitly request a device to set or _retrieve_ a feature report".

Documentation/hid/hid-transport.txt line 116

Quoted 3 lines

Documentation/hid/hid-transport.txt new file +3 -0

+   reports are never sent on the intr channel as this channel is asynchronous.
+
+INPUT and OUTPUT reports can be sent as pure data reports on the intr channel.

In my mind, OUTPUT reports are not sent through the intr channel (because write only), but you already get my point I think :)

Documentation/hid/hid-transport.txt line 120

Quoted 4 lines

Documentation/hid/hid-transport.txt new file +4 -0

+For INPUT reports this is the usual operational mode. But for OUTPUT reports,
+this is rarely done as OUTPUT reports are normally quite rare. But devices are
+free to make excessive use of asynchronous OUTPUT reports (for instance, custom
+HID audio speakers make great use of it).

Ok, did not know about it.

Documentation/hid/hid-transport.txt line 127

Quoted 7 lines

Documentation/hid/hid-transport.txt new file +7 -0

+
+Raw reports must not be sent on the ctrl channel, though. Instead, the ctrl
+channel provides synchronous GET/SET_REPORT requests.
+
+ - GET_REPORT: A GET_REPORT request has a report ID as payload and is sent
+   from host to device. The device must answer with a data report for the
+   requested report ID on the ctrl channel as a synchronous acknowledgement.

Beware that the report ID is not mandatory in case the HID report descriptors declares only 1 report without report ID. But I'm fine with it anyway (it's understandable that way).

Documentation/hid/hid-transport.txt line 137

Quoted 10 lines

Documentation/hid/hid-transport.txt new file +10 -0

+   Only one GET_REPORT request can be pending for each device. This restriction
+   is enforced by HID core as several transport drivers don't allow multiple
+   simultaneous GET_REPORT requests.
+   Note that data reports which are sent as answer to a GET_REPORT request are
+   not handled as generic device events. That is, if a device does not operate
+   in continuous data reporting mode, an answer to GET_REPORT does not replace
+   the raw data report on the intr channel on state change.
+   GET_REPORT is only used by custom HID device drivers to query device state.
+   Normally, HID core caches any device state so this request is not necessary
+   on devices that follow the HID specs.

FYI, HID/I2C spec says: "GET_REPORT is often used by applications on startup to retrieve the ``current state'' of the device rather than waiting for the device to generate the next input/feature report".

And as under Linux applications do not talk directly to the hid devices, I fully concurs to your point.

Documentation/hid/hid-transport.txt line 139

Quoted 2 lines

Documentation/hid/hid-transport.txt new file +2 -0

+   GET_REPORT requests can be sent for any of the 3 report types and shall
+   return the current report state of the device.

The HID/I2C spec explicitly says: "the DEVICE shall ignore a GET_REPORT requests with the REPORT TYPE set to Output, as it is not used in this specification." So under i2c, we can send GET_REPORT with OUTPUT, but we will not get anything from the device (this is why it is forbidden by the transport driver).

Documentation/hid/hid-transport.txt line 142

Quoted 3 lines

Documentation/hid/hid-transport.txt new file +3 -0

+ - SET_REPORT: A SET_REPORT request has a report ID plus data as payload. It is
+   sent from host to device and a device must update it's current report state
+   according to the given data. Any of the 3 report types can be used.

The HID/I2C spec explicitly says: "the DEVICE might choose to ignore input SET_REPORT requests as meaningless."...

Quoted 9 lines

Documentation/hid/hid-transport.txt new file +9 -0

+   A device must answer with a synchronous acknowledgement. However, HID core
+   does not require transport drivers to forward this acknowledgement to HID
+   core.
+   Same as for GET_REPORT, only one SET_REPORT can be pending at a time. This
+   restriction is enforced by HID core as some transport drivers do not support
+   multiple synchronous SET_REPORT requests.
+
+Other ctrl-channel requests are supported by USB-HID but are not available
+(or deprecated) in most other transport level specifications:

Documentation/hid/hid-transport.txt line 154

Quoted 3 lines

Documentation/hid/hid-transport.txt new file +3 -0

+
+ - GET/SET_IDLE: Only used by USB-HID. Do not implement!
+ - GET/SET_PROTOCOL: Not used by HID core. Do not implement!

The I2C declares also:
- RESET: mandatory (reset the device at any time)
- SET_POWER: mandatory on the device side (request from host to device to indicate preferred power setting).

Quoted 24 lines

Documentation/hid/hid-transport.txt new file +24 -0

+
+2) HID API
+==========
+
+2.1) Initialization
+-------------------
+
+Transport drivers normally use the following procedure to register a new device
+with HID core:
+
+       struct hid_device *hid;
+       int ret;
+
+       hid = hid_allocate_device();
+       if (IS_ERR(hid)) {
+               ret = PTR_ERR(hid);
+               goto err_<...>;
+       }
+
+       strlcpy(hid->name, <device-name-src>, 127);
+       strlcpy(hid->phys, <device-phys-src>, 63);
+       strlcpy(hid->uniq, <device-uniq-src>, 63);
+
+       hid->ll_driver = &custom_ll_driver;

Documentation/hid/hid-transport.txt line 183

Quoted 5 lines

Documentation/hid/hid-transport.txt new file +5 -0

+       hid->bus = <device-bus>;
+       hid->vendor = <device-vendor>;
+       hid->product = <device-product>;
+       hid->version = <device-version>;
+       hid->country = <device-country>;

FYI, HID/I2C does not define any device-country field (I guess it will come in a later release...)

Documentation/hid/hid-transport.txt line 184

Quoted 1 line

Documentation/hid/hid-transport.txt new file +1 -0

+       hid->dev.parent = <pointer-to-parent-device>;

FYI, I have started implementing a devm API for HID (in the same way the input devm API is implemented), and dev.parent should not be overwritten.

Anyway the two last comments are not requesting any changes in the document.

Documentation/hid/hid-transport.txt line 192

Quoted 8 lines

Documentation/hid/hid-transport.txt new file +8 -0

+       hid->driver_data = <transport-driver-data-field>;
+
+       ret = hid_add_device(hid);
+       if (ret)
+               goto err_<...>;
+
+Once hid_add_device() is entered, HID core might use the callbacks provided in
+"custom_ll_driver". To unregister a device, use:

                    ^^^^

Maybe introduce a new paragraph (otherwise, it looks like hid_destroy_device is called from HID core).

Quoted 21 lines

Documentation/hid/hid-transport.txt new file +21 -0

+
+       hid_destroy_device(hid);
+
+Once hid_destroy_device() returns, HID core will no longer make use of any
+driver callbacks.
+
+2.2) hid_ll_driver operations
+-----------------------------
+
+The available HID callbacks are:
+ - int (*start) (struct hid_device *hdev)
+   Called from HID device drivers once they want to use the device. Transport
+   drivers can choose to setup their device in this callback. However, normally
+   devices are already set up before transport drivers register them to HID core
+   so this is mostly only used by USB-HID.
+
+ - void (*stop) (struct hid_device *hdev)
+   Called from HID device drivers once they are done with a device. Transport
+   drivers can free any buffers and deinitialize the device. But note that
+   ->start() might be called again if another HID device driver is loaded on the
+   device.

Documentation/hid/hid-transport.txt line 223

Quoted 10 lines

Documentation/hid/hid-transport.txt new file +10 -0

+   Transport drivers are free to ignore it and deinitialize devices after they
+   destroyed them via hid_destroy_device().
+
+ - int (*open) (struct hid_device *hdev)
+   Called from HID device drivers once they are interested in data reports.
+   Usually, while user-space didn't open any input API/etc., device drivers are
+   not interested in device data and transport drivers can put devices asleep.
+   However, once ->open() is called, transport drivers must be ready for I/O.
+   ->open() calls are never nested. So in between two ->open() calls there must
+   be a call to ->close().

This is not true in either USB or I2C. And I guess with every BT or UHID devices. ->open() and ->close() are called upon open/close of the input node (or hidraw, or hiddev). So If two clients are opening the same input node, there will be two calls to ->open() without any call to ->close() in between.

I guess you mixed up this part with the ->start() ->stop() :)

Quoted 34 lines

Documentation/hid/hid-transport.txt new file +34 -0

+
+ - void (*close) (struct hid_device *hdev)
+   Called from HID device drivers after ->open() was called but they are no
+   longer interested in device reports. (Usually if user-space closed any input
+   devices of the driver).
+   Transport drivers can put devices asleep and terminate any I/O. However,
+   ->start() may be called again if the device driver is interested in input
+   reports again.
+
+ - int (*parse) (struct hid_device *hdev)
+   Called once during device setup after ->start() has been called. Transport
+   drivers must read the HID report-descriptor from the device and tell HID core
+   about it via hid_parse_report().
+
+ - int (*power) (struct hid_device *hdev, int level)
+   Called by HID core to give PM hints to transport drivers. Usually this is
+   analogical to the ->open() and ->close() hints and redundant.
+
+ - void (*request) (struct hid_device *hdev, struct hid_report *report,
+                    int reqtype)
+   Send an HID request on the ctrl channel. "report" contains the report that
+   should be sent and "reqtype" the request type. Request-type can be
+   HID_REQ_SET_REPORT or HID_REQ_GET_REPORT.
+   This callback is optional. If not provided, HID core will assemble a raw
+   report following the HID specs and send it via the ->raw_request() callback.
+   The transport driver is free to implement this asynchronously.
+
+ - int (*wait) (struct hid_device *hdev)
+   Used by HID core before calling ->request() again. A transport driver can use
+   it to wait for any pending requests to complete if only one request is
+   allowed at a time.
+
+ - int (*raw_request) (struct hid_device *hdev, unsigned char reportnum,
+                       __u8 *buf, size_t count, unsigned char rtype,

Documentation/hid/hid-transport.txt line 261

Quoted 4 lines

Documentation/hid/hid-transport.txt new file +4 -0

+                       int reqtype)
+   Same as ->request() but provides the report as raw buffer. This request shall
+   be synchronous. A transport driver must not use ->wait() to complete such
+   requests.

2 questions/remarks here:
- is raw_request() meant to replace ->hid_output_raw_report() and ->hid_get_raw_report()?

- reportnum declared as an unsigned will be problematic regarding the rare devices not having any report ID in their report descriptors.

Documentation/hid/hid-transport.txt line 267

Quoted 6 lines

Documentation/hid/hid-transport.txt new file +6 -0

+
+ - int (*output_report) (struct hid_device *hdev, __u8 *buf, size_t len)
+   Send raw output report via intr channel. Used by some HID device drivers
+   which require high throughput for outgoing requests on the intr channel. This
+   must not cause SET_REPORT calls! This must be implemented as asynchronous
+   output report on the intr channel!

For me, there is something wrong here. The name infers that we are trying to send an output_report directly (so through the USB URB or through the output i2c register), but you are implementing it through the intr channel... :-S

Documentation/hid/hid-transport.txt line 275

Quoted 8 lines

Documentation/hid/hid-transport.txt new file +8 -0

+
+ - int (*hidinput_input_event) (struct input_dev *idev, unsigned int type,
+                                unsigned int code, int value)
+   Obsolete callback used by logitech converters. It is called when userspace
+   writes input events to the input device (eg., EV_LED). A driver can use this
+   callback to convert it into an output report and send it to the device. If
+   this callback is not provided, HID core will use ->request() or
+   ->raw_request() respectively.

I bet there will be a way to make this work with logitech devices too (if we implement a proper ->hid_output_raw_report() in each paired devices).

Quoted 5 lines

Documentation/hid/hid-transport.txt new file +5 -0

+
+ - int (*idle) (struct hid_device *hdev, int report, int idle, int reqtype)
+   Perform SET/GET_IDLE request. Only used by USB-HID, do not implement!
+
+2.3) Data Path

Documentation/hid/hid-transport.txt line 284

Quoted 4 lines

Documentation/hid/hid-transport.txt new file +4 -0

+--------------
+
+Transport drivers are responsible of reading data from I/O devices. They must
+handle any state-tracking themselves. HID core does not implement protocol

I don't get the "state-tracking" here. The reports states should be handled by core, and I do not see the other states (or you meant PM states?).

Quoted 14 lines

Documentation/hid/hid-transport.txt new file +14 -0

+handshakes or other management commands which can be required by the given HID
+transport specification.
+
+Every raw data packet read from a device must be fed into HID core via
+hid_input_report(). You must specify the channel-type (intr or ctrl) and report
+type (input/output/feature). Under normal conditions, only input reports are
+provided via this API.
+
+Responses to GET_REPORT requests via ->request() must also be provided via this
+API. Responses to ->raw_request() are synchronous and must be intercepted by the
+transport driver and not passed to hid_input_report().
+Acknowledgements to SET_REPORT requests are not of interest to HID core.
+
+----------------------------------------------------

Quoted 4 lines

Documentation/hid/hid-transport.txt new file +1 -1

+Written 2013, David Herrmann <[email protected]>
--

1.8.3.2

done!
Many thanks for this David. It was very interesting and detailed. It will make a great documentation.

Quoted 15 lines

Documentation/hid/hid-transport.txt line 17

Quoted 10 lines

Documentation/hid/hid-transport.txt new file +10 -0

+
+1) HID Bus
+==========
+
+The HID subsystem is designed as a bus. Any I/O subsystem may provide HID
+devices and register them with the HID bus. HID core then loads generic device
+drivers on top of it. The transport drivers are responsible of raw data
+transport and device setup/management. HID core is responsible of
+report-parsing, report interpretation and the user-space API. Device specifics
+and quirks are also handled by HID core and the HID device drivers.

Hmm, the quirks part is not exactly what is currently implemented. Usbhid (Transport Driver) set and use quirks while you say here that they are handled by hid-core.

Quoted 3 lines

I personally prefer the way it is documented here (quirks handled in hid-core) so HIDP, i2c-hid and uhid will benefit from the dynamic quirks already implemented in usbhid. But that will require another patch :)

I actually meant things like wrong report-descriptors or special device drivers. Of course, there are also quirks that apply to the transport driver layer. I will rephrase that.

Quoted 7 lines

To sum up, I think the differences between the channels are:
- Interrupt Channel (intr): asynchronous, device initiated, no acknowledgements (beside the fact that data has been read), read only.
- Control Channel (ctrl): synchronous, host initiated, acknowledged (because synchronous), read/write
- Output Channel (out): synchronous, host initiated, acknowledged (because synchronous), write only

I intetionally described the channels as "bi-directional". So you actually describe the same scenario, but from a different view-point (mine obviously is HIDP). So if you consider the OUT channel to be the same as writing on INTR, you will get the same scenario. I am not entirely sure which is better, but I can change it to your description if it helps?

Regarding asynchronous vs. synchronous: I actually don't care how the I/O layer implements it. In this document, "asynchronous" means that I don't care for any acknowledgements. "synchronous" means, the remote device acknowledges the receival. Obviously, if an I/O system always acknowledges a SENT, then a transport driver can implement asynchronous transports as synchronous transports (which might mean having a separate buffer like USBHID does). But HID-core does not care.
Same situation if synchronous is not supported. HID-core assumes it is synchronous so the transport layer can simply fake an acknowledgement. However, this means that the transport-layer might have to take care of retransmissions if an asynchronous transport fails (which L2CAP in Bluetooth-Core does tranparently for HIDP).

I will try to be more verbose, but I intentionally posted all the callbacks below which should explain that "asynchronous" means it can be called in atomic-context and "synchronous" means it is allowed to sleep and wait for acknowledgement (speaking of code).

Thanks a lot for the I2C explanation, I think I understand it now and it does resemble USBHID and HIDP very much!

Quoted 14 lines

Then, the usbhid transport layer makes the ctrl and out channels asynchronous...

Documentation/hid/hid-transport.txt line 101

Quoted 7 lines

Documentation/hid/hid-transport.txt new file +7 -0

+
+Communication between devices and HID core is mostly done via HID reports. A
+report can be of one of three types:
+
+ - INPUT Report: Input reports provide data from device to host. This
+   data may include button events, axis events, battery status or more. This
+   data is generated by the device and sent to the host without requiring

_with_ or without requiring explicit requests.
On I2C, the command GET_REPORT is even mandatory (which does not seems to be the case for USB, given the amount of QUIRK_NO_INIT_REPORT we have.)

You're right, they can be sent synchronously via GET_REPORT, too.

But I don't know what you mean that GET_REPORT is mandatory? I thought an I2C device sends the data and fires an IRQ? Or is, upon interrupt receival, the host required to send a GET_REPORT to receive the pending input-event? I sadly have no idea how I2C works, but if it is _only_ host initiated, then this makes sense. Just want to go sure.

Quoted 6 lines

Documentation/hid/hid-transport.txt line 116

Quoted 3 lines

Documentation/hid/hid-transport.txt new file +3 -0

+   reports are never sent on the intr channel as this channel is asynchronous.
+
+INPUT and OUTPUT reports can be sent as pure data reports on the intr channel.

In my mind, OUTPUT reports are not sent through the intr channel (because write only), but you already get my point I think :)

Yeah, as explained above I merged INTR and OUTPUT into a bi-directional channel.

Quoted 8 lines

Documentation/hid/hid-transport.txt line 120

Quoted 5 lines

Documentation/hid/hid-transport.txt new file +4 -0

+For INPUT reports this is the usual operational mode. But for OUTPUT reports,
+this is rarely done as OUTPUT reports are normally quite rare. But devices are
+free to make excessive use of asynchronous OUTPUT reports (for instance, custom
+HID audio speakers make great use of it).

Ok, did not know about it.

Quoted 11 lines

Documentation/hid/hid-transport.txt line 127

Quoted 7 lines

Documentation/hid/hid-transport.txt new file +7 -0

+
+Raw reports must not be sent on the ctrl channel, though. Instead, the ctrl
+channel provides synchronous GET/SET_REPORT requests.
+
+ - GET_REPORT: A GET_REPORT request has a report ID as payload and is sent
+   from host to device. The device must answer with a data report for the
+   requested report ID on the ctrl channel as a synchronous acknowledgement.

Beware that the report ID is not mandatory in case the HID report descriptors declares only 1 report without report ID. But I'm fine with it anyway (it's understandable that way).

Ugh, I am not entirely sure but afaik HIDP doesn't support implicit IDs. Could you tell me whether I2C supports it?

A report-descriptor can skip report IDs if there is only a single report? Didn't know that, but we definitely need to document it.

Quoted 12 lines

Documentation/hid/hid-transport.txt line 137

Quoted 11 lines

Documentation/hid/hid-transport.txt new file +10 -0

+   Only one GET_REPORT request can be pending for each device. This restriction
+   is enforced by HID core as several transport drivers don't allow multiple
+   simultaneous GET_REPORT requests.
+   Note that data reports which are sent as answer to a GET_REPORT request are
+   not handled as generic device events. That is, if a device does not operate
+   in continuous data reporting mode, an answer to GET_REPORT does not replace
+   the raw data report on the intr channel on state change.
+   GET_REPORT is only used by custom HID device drivers to query device state.
+   Normally, HID core caches any device state so this request is not necessary
+   on devices that follow the HID specs.

Quoted 3 lines

FYI, HID/I2C spec says: "GET_REPORT is often used by applications on startup to retrieve the ``current state'' of the device rather than waiting for the device to generate the next input/feature report".

Yeah, for startup this makes sense. I will add a short note.

Quoted 11 lines

And as under Linux applications do not talk directly to the hid devices, I fully concurs to your point.

Documentation/hid/hid-transport.txt line 139

Quoted 2 lines

Documentation/hid/hid-transport.txt new file +2 -0

+   GET_REPORT requests can be sent for any of the 3 report types and shall
+   return the current report state of the device.

The HID/I2C spec explicitly says: "the DEVICE shall ignore a GET_REPORT requests with the REPORT TYPE set to Output, as it is not used in this specification." So under i2c, we can send GET_REPORT with OUTPUT, but we will not get anything from the device (this is why it is forbidden by the transport driver).

Heh, didn't know that, either. It's fine if I2C ignores it. However, I'd like to avoid forbidding it. There can be devices which use this (companies to crazy things..) and I cannot see a reason to forbid it in HID core. The transport drivers are free to adhere to their specifications, though. I guess that's fine?

Quoted 7 lines

Documentation/hid/hid-transport.txt line 142

Quoted 4 lines

+ - SET_REPORT: A SET_REPORT request has a report ID plus data as payload. It is

Documentation/hid/hid-transport.txt new file +2 -0

+   sent from host to device and a device must update it's current report state
+   according to the given data. Any of the 3 report types can be used.

The HID/I2C spec explicitly says: "the DEVICE might choose to ignore input SET_REPORT requests as meaningless."...

Same as above I think. But thanks for the clarification! I guess I will add a note that it is discouraged and devices are not supposed to handle it. This should clear all doubts.

Quoted 10 lines

Quoted 10 lines

Documentation/hid/hid-transport.txt new file +9 -0

+   A device must answer with a synchronous acknowledgement. However, HID core
+   does not require transport drivers to forward this acknowledgement to HID
+   core.
+   Same as for GET_REPORT, only one SET_REPORT can be pending at a time. This
+   restriction is enforced by HID core as some transport drivers do not support
+   multiple synchronous SET_REPORT requests.
+
+Other ctrl-channel requests are supported by USB-HID but are not available
+(or deprecated) in most other transport level specifications:

Quoted 8 lines

Quoted 3 lines

+
+ - GET/SET_IDLE: Only used by USB-HID. Do not implement!
+ - GET/SET_PROTOCOL: Not used by HID core. Do not implement!

The I2C declares also:
- RESET: mandatory (reset the device at any time)
- SET_POWER: mandatory on the device side (request from host to device to indicate preferred power setting).

Are they hooked up to HID-core? I'd like to avoid any commands which

are handled transparently in the transport driver (like suspend/resume). However, SET_POWER sounds related to ->power() callback. I will look through it again and include it in the next revision if it is hooked up.

Quoted 25 lines

Quoted 25 lines

Documentation/hid/hid-transport.txt new file +24 -0

+
+2) HID API
+==========
+
+2.1) Initialization
+-------------------
+
+Transport drivers normally use the following procedure to register a new device
+with HID core:
+
+       struct hid_device *hid;
+       int ret;
+
+       hid = hid_allocate_device();
+       if (IS_ERR(hid)) {
+               ret = PTR_ERR(hid);
+               goto err_<...>;
+       }
+
+       strlcpy(hid->name, <device-name-src>, 127);
+       strlcpy(hid->phys, <device-phys-src>, 63);
+       strlcpy(hid->uniq, <device-uniq-src>, 63);
+
+       hid->ll_driver = &custom_ll_driver;

Quoted 8 lines

Documentation/hid/hid-transport.txt line 183

Quoted 5 lines

Documentation/hid/hid-transport.txt new file +5 -0

+       hid->bus = <device-bus>;
+       hid->vendor = <device-vendor>;
+       hid->product = <device-product>;
+       hid->version = <device-version>;
+       hid->country = <device-country>;

FYI, HID/I2C does not define any device-country field (I guess it will come in a later release...)

Right, I think BT also sets it to 0 in bluez. I will add a note that "0" is the default value.

Quoted 4 lines

Quoted 1 line

+ hid->dev.parent = <pointer-to-parent-device>;

FYI, I have started implementing a devm API for HID (in the same way the input devm API is implemented), and dev.parent should not be overwritten.

Cool! I will adjust the document once it is merged.

Quoted 11 lines

Anyway the two last comments are not requesting any changes in the document.

Documentation/hid/hid-transport.txt line 192

Quoted 8 lines

Documentation/hid/hid-transport.txt new file +8 -0

+       hid->driver_data = <transport-driver-data-field>;
+
+       ret = hid_add_device(hid);
+       if (ret)
+               goto err_<...>;
+
+Once hid_add_device() is entered, HID core might use the callbacks provided in
+"custom_ll_driver". To unregister a device, use:

                    ^^^^

Quoted 2 lines

Maybe introduce a new paragraph (otherwise, it looks like hid_destroy_device is called from HID core).

Indeed, will do that.

Quoted 38 lines

Quoted 21 lines

Documentation/hid/hid-transport.txt new file +21 -0

+
+       hid_destroy_device(hid);
+
+Once hid_destroy_device() returns, HID core will no longer make use of any
+driver callbacks.
+
+2.2) hid_ll_driver operations
+-----------------------------
+
+The available HID callbacks are:
+ - int (*start) (struct hid_device *hdev)
+   Called from HID device drivers once they want to use the device. Transport
+   drivers can choose to setup their device in this callback. However, normally
+   devices are already set up before transport drivers register them to HID core
+   so this is mostly only used by USB-HID.
+
+ - void (*stop) (struct hid_device *hdev)
+   Called from HID device drivers once they are done with a device. Transport
+   drivers can free any buffers and deinitialize the device. But note that
+   ->start() might be called again if another HID device driver is loaded on the
+   device.

Documentation/hid/hid-transport.txt line 223

Quoted 10 lines

Documentation/hid/hid-transport.txt new file +10 -0

+   Transport drivers are free to ignore it and deinitialize devices after they
+   destroyed them via hid_destroy_device().
+
+ - int (*open) (struct hid_device *hdev)
+   Called from HID device drivers once they are interested in data reports.
+   Usually, while user-space didn't open any input API/etc., device drivers are
+   not interested in device data and transport drivers can put devices asleep.
+   However, once ->open() is called, transport drivers must be ready for I/O.
+   ->open() calls are never nested. So in between two ->open() calls there must
+   be a call to ->close().

This is not true in either USB or I2C. And I guess with every BT or UHID devices. ->open() and ->close() are called upon open/close of the input node (or hidraw, or hiddev). So If two clients are opening the same input node, there will be two calls to ->open() without any call to ->close() in between.

Quoted 1 line

I guess you mixed up this part with the ->start() ->stop() :)

I got confused, indeed. input-core only calls ->open() on first-open, but there might be multiple input devices. So you're right. I will fix this. Thanks for catching it!

Quoted 34 lines

Quoted 34 lines

Documentation/hid/hid-transport.txt new file +34 -0

+
+ - void (*close) (struct hid_device *hdev)
+   Called from HID device drivers after ->open() was called but they are no
+   longer interested in device reports. (Usually if user-space closed any input
+   devices of the driver).
+   Transport drivers can put devices asleep and terminate any I/O. However,
+   ->start() may be called again if the device driver is interested in input
+   reports again.
+
+ - int (*parse) (struct hid_device *hdev)
+   Called once during device setup after ->start() has been called. Transport
+   drivers must read the HID report-descriptor from the device and tell HID core
+   about it via hid_parse_report().
+
+ - int (*power) (struct hid_device *hdev, int level)
+   Called by HID core to give PM hints to transport drivers. Usually this is
+   analogical to the ->open() and ->close() hints and redundant.
+
+ - void (*request) (struct hid_device *hdev, struct hid_report *report,
+                    int reqtype)
+   Send an HID request on the ctrl channel. "report" contains the report that
+   should be sent and "reqtype" the request type. Request-type can be
+   HID_REQ_SET_REPORT or HID_REQ_GET_REPORT.
+   This callback is optional. If not provided, HID core will assemble a raw
+   report following the HID specs and send it via the ->raw_request() callback.
+   The transport driver is free to implement this asynchronously.
+
+ - int (*wait) (struct hid_device *hdev)
+   Used by HID core before calling ->request() again. A transport driver can use
+   it to wait for any pending requests to complete if only one request is
+   allowed at a time.
+
+ - int (*raw_request) (struct hid_device *hdev, unsigned char reportnum,
+                       __u8 *buf, size_t count, unsigned char rtype,

Quoted 8 lines

Documentation/hid/hid-transport.txt line 261

Quoted 4 lines

Documentation/hid/hid-transport.txt new file +4 -0

+                       int reqtype)
+   Same as ->request() but provides the report as raw buffer. This request shall
+   be synchronous. A transport driver must not use ->wait() to complete such
+   requests.

2 questions/remarks here:
- is raw_request() meant to replace ->hid_output_raw_report() and ->hid_get_raw_report()?

Both. raw_request() with HID_REQ_GET_REPORT replaces hid_get_raw_report() and with HID_REQ_SET_REPORT it replaces hid_output_raw_report().
However, at least HIDP implements hid_output_raw_report() with HID_OUTPUT_REPORT as raw output report instead of SET_REPORT. For this, "output_report()" below can be used.

Quoted 3 lines

- reportnum declared as an unsigned will be problematic regarding the rare devices not having any report ID in their report descriptors.

I thought reportnum 0 is an invalid ID? I will check again and change to signed if needed. Thanks!

Quoted 8 lines

Quoted 7 lines

Documentation/hid/hid-transport.txt new file +6 -0

+
+ - int (*output_report) (struct hid_device *hdev, __u8 *buf, size_t len)
+   Send raw output report via intr channel. Used by some HID device drivers
+   which require high throughput for outgoing requests on the intr channel. This
+   must not cause SET_REPORT calls! This must be implemented as asynchronous
+   output report on the intr channel!

Quoted 4 lines

For me, there is something wrong here. The name infers that we are trying to send an output_report directly (so through the USB URB or through the output i2c register), but you are implementing it through the intr channel... :-S

Again, "write on INTR" is OUTPUT channel for me. So I guess with the explanation above we are fine here?

Quoted 10 lines

Quoted 9 lines

Documentation/hid/hid-transport.txt new file +8 -0

+
+ - int (*hidinput_input_event) (struct input_dev *idev, unsigned int type,
+                                unsigned int code, int value)
+   Obsolete callback used by logitech converters. It is called when userspace
+   writes input events to the input device (eg., EV_LED). A driver can use this
+   callback to convert it into an output report and send it to the device. If
+   this callback is not provided, HID core will use ->request() or
+   ->raw_request() respectively.

Quoted 2 lines

I bet there will be a way to make this work with logitech devices too (if we implement a proper ->hid_output_raw_report() in each paired devices).

Yeah, I thought so, but I have no idea what the logitech-dj driver does. I guess we can drop this once we have no more users, but I wanted to avoid pushing to hard on it.

Quoted 14 lines

Documentation/hid/hid-transport.txt line 284

Quoted 10 lines

Documentation/hid/hid-transport.txt new file +9 -0

+
+ - int (*idle) (struct hid_device *hdev, int report, int idle, int reqtype)
+   Perform SET/GET_IDLE request. Only used by USB-HID, do not implement!
+
+2.3) Data Path
+--------------
+
+Transport drivers are responsible of reading data from I/O devices. They must
+handle any state-tracking themselves. HID core does not implement protocol

I don't get the "state-tracking" here. The reports states should be handled by core, and I do not see the other states (or you meant PM states?).

Ugh, I meant I/O-related state-tracking (or PM). I will rephrase that.

Quoted 15 lines

Quoted 15 lines

Documentation/hid/hid-transport.txt new file +14 -0

+handshakes or other management commands which can be required by the given HID
+transport specification.
+
+Every raw data packet read from a device must be fed into HID core via
+hid_input_report(). You must specify the channel-type (intr or ctrl) and report
+type (input/output/feature). Under normal conditions, only input reports are
+provided via this API.
+
+Responses to GET_REPORT requests via ->request() must also be provided via this
+API. Responses to ->raw_request() are synchronous and must be intercepted by the
+transport driver and not passed to hid_input_report().
+Acknowledgements to SET_REPORT requests are not of interest to HID core.
+
+----------------------------------------------------

Quoted 8 lines

Quoted 4 lines

+Written 2013, David Herrmann <[email protected]> --
1.8.3.2

done!
Many thanks for this David. It was very interesting and detailed. It will make a great documentation.

Thanks a lot for reviewing. I will fix the remaining issues and with the "write-on-INTR is OUTPUT" I guess we are on the same page (except for minor issues)?

Hi

This series provides some cleanups for HID transport drivers:
Patch #1: Cleanup which can be picked up
Patch #2-#6: Provide generic hidinput_input_event() helpers Patch #7-#8: Transport-driver Cleanup

Patch #7-#8 is an attempt to clean up the transport driver callbacks. Lets look at how the following hooks are currently implemented:
1) GET_REPORT: Issue a synchronous GET_REPORT via the ctrl-channel
2) SET_REPORT: Issue a synchronous SET_REPORT via the ctrl-channel

3) OUTPUT: Issue an asynchronous OUTPUT report on the intr-channel

USB-HID:
1) GET_REPORT can be issued via ->hid_get_raw_report() for all three report types correctly.
2) SET_REPORT can only be issued for FEATURE reports via ->hid_output_raw_report(). INPUT and OUTPUT reports on the same callback cause an asynchronous report on the intr channel.
3) OUTPUT reports can be issued via ->hid_output_raw_report() as described above in SET_REPORT.

HIDP:
1) GET_REPORT: same as for USB-HID
2) SET_REPORT can only be issued for FEATURE reports via ->hid_output_raw_report(). SET_REPORT for INPUT reports is forbidden (why?) and SET_REPORT for OUTPUT reports is actually implemented as 3)
3) OUTPUT reports can be issued via ->hid_output_raw_report().

I2C:
1) GET_REPORT implemented for INPUT and FEATURE via ->hid_get_raw_report(). Forbidden for OUTPUT reports (why?).

2) SET_REPORT can be issued for OUTPUT and FEATURE reports via

    ->hid_output_raw_report() but is forbidden for INPUT reports (why?).

3) OUTPUT reports cannot be issued at all.

UHID:
1) GET_REPORT only supported for FEATURE via ->hid_get_raw_report()
2) SET_REPORT not supported at all
3) OUTPUT supported via ->hid_output_raw_report()

As this is all very inconsistent, I added two new callbacks:
->raw_request() is the same as ->request() but with a raw buffer. It should be implemented by the transport drivers as SET/GET_REPORT calls in the ctrl-channel.

->output_report() is supposed to send an OUTPUT report on the intr-channel (same channel asynchronous input reports are received from).

Please see the hid-transport.txt for a description. The last patch tries to implement them. I have no idea how to implement it for i2c, it seems that i2c multiplexes ctrl and intr channels on a single i2c channel. I don't know how to send raw output reports at all.. Help welcome!

If there is more interest in this work, I will clean it up, try to convert the other hid_ll_driver drivers and resend as a proper patchset.

Cheers
David

David Herrmann (8):
HID: usbhid: make usbhid_set_leds() static
HID: usbhid: update LED fields unlocked
HID: input: generic hidinput_input_event handler
HID: usbhid: use generic hidinput_input_event()
HID: i2c: use generic hidinput_input_event()
HID: uhid: use generic hidinput_input_event()
HID: add transport driver documentation
HID: implement new transport-driver callbacks

 Documentation/hid/hid-transport.txt | 299 ++++++++++++++++++++++++++++++++++++
 Documentation/hid/uhid.txt          |   4 +-
 drivers/hid/hid-input.c             |  80 +++++++++-
 drivers/hid/i2c-hid/i2c-hid.c       |  24 ---
 drivers/hid/uhid.c                  |  52 ++++---
 drivers/hid/usbhid/hid-core.c       | 144 +++++++++--------
 drivers/hid/usbhid/usbhid.h         |   3 -
 include/linux/hid.h                 |  10 +-
 include/uapi/linux/uhid.h           |   4 +-
 net/bluetooth/hidp/core.c           |  90 +++++++++++
 10 files changed, 590 insertions(+), 120 deletions(-)
 create mode 100644 Documentation/hid/hid-transport.txt

--
1.8.3.2

This series provides some cleanups for HID transport drivers:

Quoted 1 line · David Herrmann

This series provides some cleanups for HID transport drivers:

Hi David,

thanks a lot for your work, again.

I have now applied all the patches from the series, except for:

- 3/8, waiting for v2

- 7/8, waiting for v2 as well, plus haven't finished reviewing it fully
- 8/8, still thinking about it and reviewing

Quoted 9 lines

Quoted 1 line

This series provides some cleanups for HID transport drivers:

Hi David,

thanks a lot for your work, again.

I have now applied all the patches from the series, except for:

- 3/8, waiting for v2

Ugh, patches 4-6 use the generic helper from 3 implicitly. So with the current tree, they will not be able to send any LED events. I think my conclusion in the discussion on #3 was wrong. I cannot rebase it on the end of the series. Sorry, I missed that.

For v2, as mentioned in the thread, I cannot write a generic fallback for hid_hw_request() without patch #8. That's because the transport drivers handle the raw reports differently. #8 fixes that.

So imho we should apply #3 as it is now. If we ever have a generic hid_hw_request(), we can just move the code from the LED handler to hid_hw_request_generic().