* Re: [RFC 4/8] HID: usbhid: use generic hidinput_input_event()
From: Benjamin Tissoires @ 2013-07-16 8:06 UTC (permalink / raw)
To: David Herrmann; +Cc: linux-input, Jiri Kosina, Henrik Rydberg, Oliver Neukum
In-Reply-To: <1373908217-16748-5-git-send-email-dh.herrmann@gmail.com>
On Mon, Jul 15, 2013 at 7:10 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> HID core provides the same functionality as we do, so drop the custom
> hidinput_input_event() handler.
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cheers,
Benjamin
^ permalink raw reply
* Re: [RFC 3/8] HID: input: generic hidinput_input_event handler
From: Benjamin Tissoires @ 2013-07-16 8:04 UTC (permalink / raw)
To: David Herrmann; +Cc: linux-input, Jiri Kosina, Henrik Rydberg, Oliver Neukum
In-Reply-To: <1373908217-16748-4-git-send-email-dh.herrmann@gmail.com>
On Mon, Jul 15, 2013 at 7:10 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> 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 <dh.herrmann@gmail.com>
> ---
> drivers/hid/hid-input.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++-
> include/linux/hid.h | 1 +
> 2 files changed, 80 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 7480799..308eee8 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -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);
> +}
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.
> +
> +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);
> @@ -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.
> 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++) {
> @@ -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 :)
> }
> EXPORT_SYMBOL_GPL(hidinput_disconnect);
>
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index b8058c5..ea4b828 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -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
>
Cheers,
Benjamin
^ permalink raw reply
* Re: [RFC 2/8] HID: usbhid: update LED fields unlocked
From: Benjamin Tissoires @ 2013-07-16 7:46 UTC (permalink / raw)
To: David Herrmann; +Cc: linux-input, Jiri Kosina, Henrik Rydberg, Oliver Neukum
In-Reply-To: <1373908217-16748-3-git-send-email-dh.herrmann@gmail.com>
On Mon, Jul 15, 2013 at 7:10 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> 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 <dh.herrmann@gmail.com>
> ---
Hi David,
that was a very good commit message!
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cheers,
Benjamin
^ permalink raw reply
* Re: [RFC 1/8] HID: usbhid: make usbhid_set_leds() static
From: Benjamin Tissoires @ 2013-07-16 7:41 UTC (permalink / raw)
To: David Herrmann; +Cc: linux-input, Jiri Kosina, Henrik Rydberg, Oliver Neukum
In-Reply-To: <1373908217-16748-2-git-send-email-dh.herrmann@gmail.com>
On Mon, Jul 15, 2013 at 7:10 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> usbhid_set_leds() is only used inside of usbhid/hid-core.c so no need to
> export it.
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
Hi David,
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH 1/1] MAINTAINERS: Change maintainer for cyttsp driver
From: Ferruh Yigit @ 2013-07-16 6:57 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Dmitry Torokhov, Henrik Rydberg, ttdrivers, linux-input,
linux-kernel
In-Reply-To: <1373838082-12372-2-git-send-email-javier@dowhile0.org>
On 07/15/2013 12:41 AM, Javier Martinez Canillas wrote:
> I haven't had time to work on this driver for a long time and
> Ferruh has been doing a great job making it more generic,
> adding support for new hardware and providing bug fixes.
Thank you a lot for your work on cyttsp drivers,
we would like to see your support back whenever you have time again.
Your expertise/know-how on issue is valuable.
>
> So, let's make MAINTAINERS reflect reality and add him as the
> cyttsp maintainer instead of me.
>
> Also, since Ferruh works for Cypress, we may change the driver
> status from Maintained to Supported.
>
> Signed-off-by: Javier Martinez Canillas <javier@dowhile0.org>
> ---
>
> Ferruh, please send your ack if you are willing to take over
> maintainance of this driver.
Acked-by: Ferruh Yigit <fery@cypress.com>
>
> Also, please confirm that you have been working on behalf of
> Cypress instead of doing it on your free time. Otherwise we
> should keep the driver status to maintained instead supported.
Right, I am a Cypress employee and working on TrueTouch drivers.
>
> Thanks a lot and best regards,
> Javier
>
> MAINTAINERS | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9d771d9..4ba996c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2458,9 +2458,9 @@ S: Maintained
> F: drivers/media/common/cypress_firmware*
>
> CYTTSP TOUCHSCREEN DRIVER
> -M: Javier Martinez Canillas <javier@dowhile0.org>
> +M: Ferruh Yigit <fery@cypress.com>
> L: linux-input@vger.kernel.org
> -S: Maintained
> +S: Supported
> F: drivers/input/touchscreen/cyttsp*
> F: include/linux/input/cyttsp.h
>
--
Regards,
ferruh
^ permalink raw reply
* Re: [PATCH v4] Input: sysrq - DT binding for key sequence
From: Rob Herring @ 2013-07-16 2:43 UTC (permalink / raw)
To: mathieu.poirier
Cc: grant.likely, dmitry.torokhov, kernel-team, devicetree-discuss,
john.stultz, linux-input
In-Reply-To: <1373902598-10062-1-git-send-email-mathieu.poirier@linaro.org>
On 07/15/2013 10:36 AM, mathieu.poirier@linaro.org wrote:
> From: "Mathieu J. Poirier" <mathieu.poirier@linaro.org>
>
> Adding a simple device tree binding for the specification of key sequences.
> Definition of the keys found in the sequence are located in
> 'include/uapi/linux/input.h'.
>
> For the sysrq driver, holding the sequence of keys down for a specific amount of time
> will reset the system.
>
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
> changes for v4:
> - Moved seach of reset sequence nodes to a single call.
> ---
> .../devicetree/bindings/input/input-reset.txt | 34 ++++++++++++++
> drivers/tty/sysrq.c | 54 ++++++++++++++++++++++
> 2 files changed, 88 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/input/input-reset.txt
>
> diff --git a/Documentation/devicetree/bindings/input/input-reset.txt b/Documentation/devicetree/bindings/input/input-reset.txt
> new file mode 100644
> index 0000000..79504af
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/input-reset.txt
> @@ -0,0 +1,34 @@
> +Input: sysrq reset sequence
> +
> +A simple binding to represent a set of keys as described in
> +include/uapi/linux/input.h. This is to communicate a
> +sequence of keys to the sysrq driver. Upon holding the keys
> +for a specified amount of time (if specified) the system is
> +sync'ed and reset.
> +
> +Key sequences are global to the system but all the keys in a
> +set must be coming from the same input device.
> +
> +The /chosen node should contain a 'linux,sysrq-reset-seq' child
> +node to define a set of keys.
> +
> +Required property:
> +sysrq-reset-seq: array of keycodes
> +
> +Optional property:
> +timeout-ms: duration keys must be pressed together in microseconds
milliseconds (ms) or microseconds (us)?
> +before generating a sysrq
> +
> +Example:
> +
> + chosen {
> + linux,sysrq-reset-seq {
> + keyset = <0x03
> + 0x04
> + 0x0a>;
> + timeout-ms = <3000>;
> + };
> + };
> +
> +Would represent KEY_2, KEY_3 and KEY_9.
> +
> diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
> index b51c154..01a8729 100644
> --- a/drivers/tty/sysrq.c
> +++ b/drivers/tty/sysrq.c
> @@ -44,6 +44,7 @@
> #include <linux/uaccess.h>
> #include <linux/moduleparam.h>
> #include <linux/jiffies.h>
> +#include <linux/of.h>
>
> #include <asm/ptrace.h>
> #include <asm/irq_regs.h>
> @@ -671,6 +672,50 @@ static void sysrq_detect_reset_sequence(struct sysrq_state *state,
> }
> }
>
> +static void sysrq_of_get_keyreset_config(void)
> +{
> + unsigned short key;
> + struct device_node *np;
> + const struct property *prop;
> + const __be32 *val;
> + int count, i;
> +
> + np = of_find_node_by_path("/chosen/linux,sysrq-reset-seq");
> + if (!np) {
> + pr_debug("No sysrq node found");
> + goto out;
> + }
> +
> + prop = of_find_property(np, "keyset", NULL);
> + if (!prop || !prop->value) {
> + pr_err("Invalid input keyset");
> + goto out;
> + }
> +
> + count = prop->length / sizeof(u32);
> + val = prop->value;
None of the existing helpers to retrieve property arrays doesn't work here?
> +
> + if (count > SYSRQ_KEY_RESET_MAX)
> + count = SYSRQ_KEY_RESET_MAX;
> +
> + /* reset in case a __weak definition was present */
> + sysrq_reset_seq_len = 0;
> +
> + for (i = 0; i < count; i++) {
> + key = (unsigned short)be32_to_cpup(val++);
> + if (key == KEY_RESERVED || key > KEY_MAX)
> + break;
> +
> + sysrq_reset_seq[sysrq_reset_seq_len++] = key;
> + }
> +
> + /* get reset timeout if any */
> + of_property_read_u32(np, "timeout-ms", &sysrq_reset_downtime_ms);
> +
> + out:
> + ;
> +}
> +
> static void sysrq_reinject_alt_sysrq(struct work_struct *work)
> {
> struct sysrq_state *sysrq =
> @@ -688,6 +733,7 @@ static void sysrq_reinject_alt_sysrq(struct work_struct *work)
> input_inject_event(handle, EV_KEY, KEY_SYSRQ, 1);
> input_inject_event(handle, EV_SYN, SYN_REPORT, 1);
>
> +
spurious ws change
> input_inject_event(handle, EV_KEY, KEY_SYSRQ, 0);
> input_inject_event(handle, EV_KEY, alt_code, 0);
> input_inject_event(handle, EV_SYN, SYN_REPORT, 1);
> @@ -903,6 +949,7 @@ static inline void sysrq_register_handler(void)
> int error;
> int i;
>
> + /* first check if a __weak interface was instantiated */
> for (i = 0; i < ARRAY_SIZE(sysrq_reset_seq); i++) {
> key = platform_sysrq_reset_seq[i];
> if (key == KEY_RESERVED || key > KEY_MAX)
> @@ -911,6 +958,13 @@ static inline void sysrq_register_handler(void)
> sysrq_reset_seq[sysrq_reset_seq_len++] = key;
> }
>
> + /*
> + * DT configuration takes precedence over anything
> + * that would have been defined via the __weak
> + * interface
> + */
> + sysrq_of_get_keyreset_config();
> +
> error = input_register_handler(&sysrq_handler);
> if (error)
> pr_err("Failed to register input handler, error %d", error);
>
^ permalink raw reply
* Re: [RFC 0/8] HID: Transport Driver Cleanup
From: Benjamin Tissoires @ 2013-07-15 18:55 UTC (permalink / raw)
To: David Herrmann; +Cc: linux-input, Jiri Kosina, Henrik Rydberg, Oliver Neukum
In-Reply-To: <1373908217-16748-1-git-send-email-dh.herrmann@gmail.com>
Hi David,
On Mon, Jul 15, 2013 at 7:10 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> 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
First, thank you very much for this work. Especially the patch #7. I
did not read it entirely though, but it looks great.
I won't have the brain in a good state this evening to do an entire
review right now, but here are a few preliminary comments:
>
> 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
I am not 100% sure GET/SET_REPORT are synchronous. At least, they
ensure for the usbhid module some kind of PM negotiation which allows
communication.
> 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.
I'm afraid no (for the 3 points). See dcd9006b1b053c7 in Linus' tree.
hid_hw_request uses usbhid_submit_report() which is very different
from ->hid_output_raw_report() or ->hid_get_raw_report() thanks to all
the glue around the USB communication.
>
> 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?).
To me (I will double check later):
- an INPUT report is read only, and send spontaneous events.
- an OUTPUT report is write only.
- a FEATURE is read/write but does not send spontaneous events
Then, the device firmware may allow you to do other things, but I
doubt there is any sense to write something on an INPUT report for
example.
> 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!
I can help. Actually, on I2C, there is no ctrl and intr channels. You
just send plain reports from both sides, and the device can notify the
host from an INPUT report thanks to an interrupt line (external to the
actual I2C communication). Anyway, I'll implement and test the I2C
stuffs if you need.
>
>
> 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.
Yes, I am very interested. Before cleaning it up, I think we already
have a lot to discuss (at least, I have some points :-P ).
Again, thanks for this work.
Cheers,
Benjamin
>
> 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
>
^ permalink raw reply
* [Regression][v3.10][v3.11] Revert "HID: Fix logitech-dj: missing Unifying device issue"
From: Joseph Salisbury @ 2013-07-15 17:26 UTC (permalink / raw)
To: adlr, nlopezcasad; +Cc: jkosina, linux-input, linux-kernel
Hi Andrew,
A bug was opened against the Ubuntu kernel[0]. It was found that
reverting the following commit resolved this bug:
commit 8af6c08830b1ae114d1a8b548b1f8b056e068887
Author: Andrew de los Reyes <adlr@chromium.org>
Date: Mon Feb 18 09:20:23 2013 -0800
Revert "HID: Fix logitech-dj: missing Unifying device issue"
The regression was introduced as of v3.10-rc1. Commit 8af6c08 reverts
commit 5962640, so it seems commit 5962640 may still be needed.
I see that you are the author of this patch, so I wanted to run this by
you. I was thinking of requesting a revert for v3.11, but I wanted to
get your feedback first.
Thanks,
Joe
[0] http://pad.lv/1194649
^ permalink raw reply
* [RFC 8/8] HID: implement new transport-driver callbacks
From: David Herrmann @ 2013-07-15 17:10 UTC (permalink / raw)
To: linux-input
Cc: Jiri Kosina, Benjamin Tissoires, Henrik Rydberg, Oliver Neukum,
David Herrmann
In-Reply-To: <1373908217-16748-1-git-send-email-dh.herrmann@gmail.com>
->raw_request() and ->output_report() implement the SET/GET_REPORT and raw
intr OUTPUT requests for all transport drivers. Once these are
implemented, we can remove the old get_raw_report() and
output_raw_report() per-device callbacks.
Note that the different device drivers expect different semantics for each
call. The new helpers are properly documented and the semantics are known.
We now need to carefully convert each driver to use the correct callback
and then remove the old helpers.
UHID isn't converted, yet. I need to invest how user-space currently
handles UHID_OUTPUT to see whether it's a SET_REPORT or intr-OUTPUT
report.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
drivers/hid/uhid.c | 27 +++++++++++++
drivers/hid/usbhid/hid-core.c | 77 ++++++++++++++++++++++++++++++++++++
include/linux/hid.h | 8 +++-
net/bluetooth/hidp/core.c | 90 +++++++++++++++++++++++++++++++++++++++++++
4 files changed, 201 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index f53f2d5..a344172 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -244,12 +244,39 @@ static int uhid_hid_output_raw(struct hid_device *hid, __u8 *buf, size_t count,
return count;
}
+static int uhid_hid_output_report(struct hid_device *hid, __u8 *buf,
+ size_t count)
+{
+ struct uhid_device *uhid = hid->driver_data;
+ unsigned long flags;
+ struct uhid_event *ev;
+
+ if (count < 1 || count > UHID_DATA_MAX)
+ return -EINVAL;
+
+ ev = kzalloc(sizeof(*ev), GFP_KERNEL);
+ if (!ev)
+ return -ENOMEM;
+
+ ev->type = UHID_OUTPUT;
+ ev->u.output.size = count;
+ ev->u.output.rtype = UHID_OUTPUT_REPORT;
+ memcpy(ev->u.output.data, buf, count);
+
+ spin_lock_irqsave(&uhid->qlock, flags);
+ uhid_queue(uhid, ev);
+ spin_unlock_irqrestore(&uhid->qlock, flags);
+
+ return count;
+}
+
static struct hid_ll_driver uhid_hid_driver = {
.start = uhid_hid_start,
.stop = uhid_hid_stop,
.open = uhid_hid_open,
.close = uhid_hid_close,
.parse = uhid_hid_parse,
+ .output_report = uhid_hid_output_report,
};
#ifdef CONFIG_COMPAT
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 8c32357..6666345 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -880,6 +880,37 @@ static int usbhid_get_raw_report(struct hid_device *hid,
return ret;
}
+static int usbhid_set_raw_report(struct hid_device *hid, unsigned int reportnum,
+ __u8 *buf, size_t count, unsigned char rtype)
+{
+ struct usbhid_device *usbhid = hid->driver_data;
+ struct usb_device *dev = hid_to_usb_dev(hid);
+ struct usb_interface *intf = usbhid->intf;
+ struct usb_host_interface *interface = intf->cur_altsetting;
+ int ret, skipped_report_id = 0;
+
+ /* Byte 0 is the report number. Report data starts at byte 1.*/
+ buf[0] = reportnum;
+ if (buf[0] == 0x0) {
+ /* Don't send the Report ID */
+ buf++;
+ count--;
+ skipped_report_id = 1;
+ }
+
+ ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
+ HID_REQ_SET_REPORT,
+ USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+ ((rtype + 1) << 8) | reportnum,
+ interface->desc.bInterfaceNumber, buf, count,
+ USB_CTRL_SET_TIMEOUT);
+ /* count also the report id, if this was a numbered report. */
+ if (ret > 0 && skipped_report_id)
+ ret++;
+
+ return ret;
+}
+
static int usbhid_output_raw_report(struct hid_device *hid, __u8 *buf, size_t count,
unsigned char report_type)
{
@@ -932,6 +963,36 @@ static int usbhid_output_raw_report(struct hid_device *hid, __u8 *buf, size_t co
return ret;
}
+static int usbhid_output_report(struct hid_device *hid, __u8 *buf, size_t count)
+{
+ struct usbhid_device *usbhid = hid->driver_data;
+ struct usb_device *dev = hid_to_usb_dev(hid);
+ int actual_length, skipped_report_id = 0, ret;
+
+ if (!usbhid->urbout)
+ return -EIO;
+
+ if (buf[0] == 0x0) {
+ /* Don't send the Report ID */
+ buf++;
+ count--;
+ skipped_report_id = 1;
+ }
+
+ ret = usb_interrupt_msg(dev, usbhid->urbout->pipe,
+ buf, count, &actual_length,
+ USB_CTRL_SET_TIMEOUT);
+ /* return the number of bytes transferred */
+ if (ret == 0) {
+ ret = actual_length;
+ /* count also the report id */
+ if (skipped_report_id)
+ ret++;
+ }
+
+ return ret;
+}
+
static void usbhid_restart_queues(struct usbhid_device *usbhid)
{
if (usbhid->urbout && !test_bit(HID_OUT_RUNNING, &usbhid->iofl))
@@ -1196,6 +1257,20 @@ static void usbhid_request(struct hid_device *hid, struct hid_report *rep, int r
}
}
+static int usbhid_raw_request(struct hid_device *hid, unsigned char reportnum,
+ __u8 *buf, size_t len, unsigned char rtype,
+ int reqtype)
+{
+ switch (reqtype) {
+ case HID_REQ_GET_REPORT:
+ return usbhid_get_raw_report(hid, reportnum, buf, len, rtype);
+ case HID_REQ_SET_REPORT:
+ return usbhid_set_raw_report(hid, reportnum, buf, len, rtype);
+ default:
+ return -EIO;
+ }
+}
+
static int usbhid_idle(struct hid_device *hid, int report, int idle,
int reqtype)
{
@@ -1219,6 +1294,8 @@ static struct hid_ll_driver usb_hid_driver = {
.power = usbhid_power,
.request = usbhid_request,
.wait = usbhid_wait_io,
+ .raw_request = usbhid_raw_request,
+ .output_report = usbhid_output_report,
.idle = usbhid_idle,
};
diff --git a/include/linux/hid.h b/include/linux/hid.h
index ea4b828..c0bf05f 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -693,8 +693,14 @@ struct hid_ll_driver {
struct hid_report *report, int reqtype);
int (*wait)(struct hid_device *hdev);
- int (*idle)(struct hid_device *hdev, int report, int idle, int reqtype);
+ int (*raw_request) (struct hid_device *hdev, unsigned char reportnum,
+ __u8 *buf, size_t len, unsigned char rtype,
+ int reqtype);
+
+ int (*output_report) (struct hid_device *hdev, __u8 *buf, size_t len);
+
+ int (*idle)(struct hid_device *hdev, int report, int idle, int reqtype);
};
#define PM_HINT_FULLON 1<<5
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 46c6a14..754d797 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -329,6 +329,71 @@ err:
return ret;
}
+static int hidp_set_raw_report(struct hid_device *hid, unsigned char reportnum,
+ unsigned char *data, size_t count,
+ unsigned char report_type)
+{
+ struct hidp_session *session = hid->driver_data;
+ int ret;
+
+ switch (report_type) {
+ case HID_FEATURE_REPORT:
+ report_type = HIDP_TRANS_SET_REPORT | HIDP_DATA_RTYPE_FEATURE;
+ break;
+ case HID_INPUT_REPORT:
+ report_type = HIDP_TRANS_SET_REPORT | HIDP_DATA_RTYPE_INPUT;
+ break;
+ case HID_OUTPUT_REPORT:
+ report_type = HIDP_TRANS_SET_REPORT | HIDP_DATA_RTYPE_OUPUT;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ if (mutex_lock_interruptible(&session->report_mutex))
+ return -ERESTARTSYS;
+
+ /* Set up our wait, and send the report request to the device. */
+ data[0] = reportnum;
+ set_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags);
+ ret = hidp_send_ctrl_message(session, report_type, data, count);
+ if (ret)
+ goto err;
+
+ /* Wait for the ACK from the device. */
+ while (test_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags) &&
+ !atomic_read(&session->terminate)) {
+ int res;
+
+ res = wait_event_interruptible_timeout(session->report_queue,
+ !test_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags)
+ || atomic_read(&session->terminate),
+ 10*HZ);
+ if (res == 0) {
+ /* timeout */
+ ret = -EIO;
+ goto err;
+ }
+ if (res < 0) {
+ /* signal */
+ ret = -ERESTARTSYS;
+ goto err;
+ }
+ }
+
+ if (!session->output_report_success) {
+ ret = -EIO;
+ goto err;
+ }
+
+ ret = count;
+
+err:
+ clear_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags);
+ mutex_unlock(&session->report_mutex);
+ return ret;
+}
+
static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data, size_t count,
unsigned char report_type)
{
@@ -387,6 +452,29 @@ err:
return ret;
}
+static int hidp_raw_request(struct hid_device *hid, unsigned char reportnum,
+ __u8 *buf, size_t len, unsigned char rtype,
+ int reqtype)
+{
+ switch (reqtype) {
+ case HID_REQ_GET_REPORT:
+ return hidp_get_raw_report(hid, reportnum, buf, len, rtype);
+ case HID_REQ_SET_REPORT:
+ return hidp_set_raw_report(hid, reportnum, buf, len, rtype);
+ default:
+ return -EIO;
+ }
+}
+
+static int hidp_output_report(struct hid_device *hid, __u8 *data, size_t count)
+{
+ struct hidp_session *session = hid->driver_data;
+
+ return hidp_send_intr_message(session,
+ HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT,
+ data, count);
+}
+
static void hidp_idle_timeout(unsigned long arg)
{
struct hidp_session *session = (struct hidp_session *) arg;
@@ -717,6 +805,8 @@ static struct hid_ll_driver hidp_hid_driver = {
.stop = hidp_stop,
.open = hidp_open,
.close = hidp_close,
+ .raw_request = hidp_raw_request,
+ .output_report = hidp_output_report,
};
/* This function sets up the hid device. It does not add it
--
1.8.3.2
^ permalink raw reply related
* [RFC 7/8] HID: add transport driver documentation
From: David Herrmann @ 2013-07-15 17:10 UTC (permalink / raw)
To: linux-input
Cc: Jiri Kosina, Benjamin Tissoires, Henrik Rydberg, Oliver Neukum,
David Herrmann
In-Reply-To: <1373908217-16748-1-git-send-email-dh.herrmann@gmail.com>
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 <dh.herrmann@gmail.com>
---
Documentation/hid/hid-transport.txt | 299 ++++++++++++++++++++++++++++++++++++
1 file changed, 299 insertions(+)
create mode 100644 Documentation/hid/hid-transport.txt
diff --git a/Documentation/hid/hid-transport.txt b/Documentation/hid/hid-transport.txt
new file mode 100644
index 0000000..034f11d
--- /dev/null
+++ b/Documentation/hid/hid-transport.txt
@@ -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.
+
+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.
+
+ +-----------+ +-----------+ +-----------+ +-----------+
+ | 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:
+ 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.
+
+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.
+
+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
+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
+ 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.
+
+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
+ explicit requests. Devices can choose to send data continously or only on
+ 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.
+ Hosts may choose to send output reports either continously or only on change.
+ - 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.
+ 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
+ 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.
+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).
+
+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.
+ 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.
+ GET_REPORT requests can be sent for any of the 3 report types and shall
+ return the current report state of the device.
+ - 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.
+ 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:
+
+ - GET/SET_IDLE: Only used by USB-HID. Do not implement!
+ - GET/SET_PROTOCOL: Not used by HID core. Do not implement!
+
+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;
+ hid->bus = <device-bus>;
+ hid->vendor = <device-vendor>;
+ hid->product = <device-product>;
+ hid->version = <device-version>;
+ hid->country = <device-country>;
+ hid->dev.parent = <pointer-to-parent-device>;
+ 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:
+
+ 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.
+ 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().
+
+ - 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,
+ 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.
+
+ - 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!
+
+ - 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.
+
+ - 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
+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 <dh.herrmann@gmail.com>
--
1.8.3.2
^ permalink raw reply related
* [RFC 6/8] HID: uhid: use generic hidinput_input_event()
From: David Herrmann @ 2013-07-15 17:10 UTC (permalink / raw)
To: linux-input
Cc: Jiri Kosina, Benjamin Tissoires, Henrik Rydberg, Oliver Neukum,
David Herrmann
In-Reply-To: <1373908217-16748-1-git-send-email-dh.herrmann@gmail.com>
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 <dh.herrmann@gmail.com>
---
Documentation/hid/uhid.txt | 4 +++-
drivers/hid/uhid.c | 25 -------------------------
include/uapi/linux/uhid.h | 4 +++-
3 files changed, 6 insertions(+), 27 deletions(-)
diff --git a/Documentation/hid/uhid.txt b/Documentation/hid/uhid.txt
index 3c74121..dc35a2b 100644
--- a/Documentation/hid/uhid.txt
+++ b/Documentation/hid/uhid.txt
@@ -149,11 +149,13 @@ needs. Only UHID_OUTPUT and UHID_OUTPUT_EV have payloads.
is of type "struct uhid_data_req".
This may be received even though you haven't received UHID_OPEN, yet.
- UHID_OUTPUT_EV:
+ UHID_OUTPUT_EV (obsolete):
Same as UHID_OUTPUT but this contains a "struct input_event" as payload. This
is called for force-feedback, LED or similar events which are received through
an input device by the HID subsystem. You should convert this into raw reports
and send them to your device similar to events of type UHID_OUTPUT.
+ This is no longer sent by newer kernels. Instead, HID core converts it into a
+ raw output report and sends it via UHID_OUTPUT.
UHID_FEATURE:
This event is sent if the kernel driver wants to perform a feature request as
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index fc307e0..f53f2d5 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -116,30 +116,6 @@ static void uhid_hid_close(struct hid_device *hid)
uhid_queue_event(uhid, UHID_CLOSE);
}
-static int uhid_hid_input(struct input_dev *input, unsigned int type,
- unsigned int code, int value)
-{
- struct hid_device *hid = input_get_drvdata(input);
- struct uhid_device *uhid = hid->driver_data;
- unsigned long flags;
- struct uhid_event *ev;
-
- ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
- if (!ev)
- return -ENOMEM;
-
- ev->type = UHID_OUTPUT_EV;
- ev->u.output_ev.type = type;
- ev->u.output_ev.code = code;
- ev->u.output_ev.value = value;
-
- spin_lock_irqsave(&uhid->qlock, flags);
- uhid_queue(uhid, ev);
- spin_unlock_irqrestore(&uhid->qlock, flags);
-
- return 0;
-}
-
static int uhid_hid_parse(struct hid_device *hid)
{
struct uhid_device *uhid = hid->driver_data;
@@ -273,7 +249,6 @@ static struct hid_ll_driver uhid_hid_driver = {
.stop = uhid_hid_stop,
.open = uhid_hid_open,
.close = uhid_hid_close,
- .hidinput_input_event = uhid_hid_input,
.parse = uhid_hid_parse,
};
diff --git a/include/uapi/linux/uhid.h b/include/uapi/linux/uhid.h
index e9ed951..414b74b 100644
--- a/include/uapi/linux/uhid.h
+++ b/include/uapi/linux/uhid.h
@@ -30,7 +30,7 @@ enum uhid_event_type {
UHID_OPEN,
UHID_CLOSE,
UHID_OUTPUT,
- UHID_OUTPUT_EV,
+ UHID_OUTPUT_EV, /* obsolete! */
UHID_INPUT,
UHID_FEATURE,
UHID_FEATURE_ANSWER,
@@ -69,6 +69,8 @@ struct uhid_output_req {
__u8 rtype;
} __attribute__((__packed__));
+/* Obsolete! Newer kernels will no longer send these events but instead convert
+ * it into raw output reports via UHID_OUTPUT. */
struct uhid_output_ev_req {
__u16 type;
__u16 code;
--
1.8.3.2
^ permalink raw reply related
* [RFC 5/8] HID: i2c: use generic hidinput_input_event()
From: David Herrmann @ 2013-07-15 17:10 UTC (permalink / raw)
To: linux-input
Cc: Jiri Kosina, Benjamin Tissoires, Henrik Rydberg, Oliver Neukum,
David Herrmann
In-Reply-To: <1373908217-16748-1-git-send-email-dh.herrmann@gmail.com>
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 <dh.herrmann@gmail.com>
---
drivers/hid/i2c-hid/i2c-hid.c | 24 ------------------------
1 file changed, 24 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 879b0ed..db2253b 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -756,29 +756,6 @@ static int i2c_hid_power(struct hid_device *hid, int lvl)
return ret;
}
-static int i2c_hid_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;
-
- offset = hidinput_find_field(hid, type, code, &field);
-
- if (offset == -1) {
- hid_warn(dev, "event field not found\n");
- return -1;
- }
-
- return hid_set_field(field, offset, value);
-}
-
static struct hid_ll_driver i2c_hid_ll_driver = {
.parse = i2c_hid_parse,
.start = i2c_hid_start,
@@ -787,7 +764,6 @@ static struct hid_ll_driver i2c_hid_ll_driver = {
.close = i2c_hid_close,
.power = i2c_hid_power,
.request = i2c_hid_request,
- .hidinput_input_event = i2c_hid_hidinput_input_event,
};
static int i2c_hid_init_irq(struct i2c_client *client)
--
1.8.3.2
^ permalink raw reply related
* [RFC 4/8] HID: usbhid: use generic hidinput_input_event()
From: David Herrmann @ 2013-07-15 17:10 UTC (permalink / raw)
To: linux-input
Cc: Jiri Kosina, Benjamin Tissoires, Henrik Rydberg, Oliver Neukum,
David Herrmann
In-Reply-To: <1373908217-16748-1-git-send-email-dh.herrmann@gmail.com>
HID core provides the same functionality as we do, so drop the custom
hidinput_input_event() handler.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
drivers/hid/usbhid/hid-core.c | 74 ++-----------------------------------------
drivers/hid/usbhid/usbhid.h | 3 --
2 files changed, 3 insertions(+), 74 deletions(-)
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 62b5131..8c32357 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -649,72 +649,6 @@ static void usbhid_submit_report(struct hid_device *hid, struct hid_report *repo
spin_unlock_irqrestore(&usbhid->lock, flags);
}
-/* Workqueue routine to send requests to change LEDs */
-static void hid_led(struct work_struct *work)
-{
- struct usbhid_device *usbhid =
- container_of(work, struct usbhid_device, led_work);
- struct hid_device *hid = usbhid->hid;
- struct hid_field *field;
- unsigned long flags;
-
- field = hidinput_get_led_field(hid);
- if (!field) {
- hid_warn(hid, "LED event field not found\n");
- 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 hid_led() worker is spawned
- * for every SET-LED request so the following hid_led() worker will send
- * the correct value, guaranteed!
- */
-
- spin_lock_irqsave(&usbhid->lock, flags);
- if (!test_bit(HID_DISCONNECTED, &usbhid->iofl)) {
- usbhid->ledcount = hidinput_count_leds(hid);
- hid_dbg(usbhid->hid, "New ledcount = %u\n", usbhid->ledcount);
- __usbhid_submit_report(hid, field->report, USB_DIR_OUT);
- }
- spin_unlock_irqrestore(&usbhid->lock, flags);
-}
-
-static int usb_hidinput_input_event(struct input_dev *dev, unsigned int type, unsigned int code, int value)
-{
- struct hid_device *hid = input_get_drvdata(dev);
- struct usbhid_device *usbhid = hid->driver_data;
- 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);
-
- /*
- * Defer performing requested LED action.
- * This is more likely gather all LED changes into a single URB.
- */
- schedule_work(&usbhid->led_work);
-
- return 0;
-}
-
static int usbhid_wait_io(struct hid_device *hid)
{
struct usbhid_device *usbhid = hid->driver_data;
@@ -1283,7 +1217,6 @@ static struct hid_ll_driver usb_hid_driver = {
.open = usbhid_open,
.close = usbhid_close,
.power = usbhid_power,
- .hidinput_input_event = usb_hidinput_input_event,
.request = usbhid_request,
.wait = usbhid_wait_io,
.idle = usbhid_idle,
@@ -1377,8 +1310,6 @@ static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id *
setup_timer(&usbhid->io_retry, hid_retry_timeout, (unsigned long) hid);
spin_lock_init(&usbhid->lock);
- INIT_WORK(&usbhid->led_work, hid_led);
-
ret = hid_add_device(hid);
if (ret) {
if (ret != -ENODEV)
@@ -1411,7 +1342,6 @@ static void hid_cancel_delayed_stuff(struct usbhid_device *usbhid)
{
del_timer_sync(&usbhid->io_retry);
cancel_work_sync(&usbhid->reset_work);
- cancel_work_sync(&usbhid->led_work);
}
static void hid_cease_io(struct usbhid_device *usbhid)
@@ -1531,15 +1461,17 @@ static int hid_suspend(struct usb_interface *intf, pm_message_t message)
struct usbhid_device *usbhid = hid->driver_data;
int status = 0;
bool driver_suspended = false;
+ unsigned int ledcount;
if (PMSG_IS_AUTO(message)) {
+ ledcount = hidinput_count_leds(hid);
spin_lock_irq(&usbhid->lock); /* Sync with error handler */
if (!test_bit(HID_RESET_PENDING, &usbhid->iofl)
&& !test_bit(HID_CLEAR_HALT, &usbhid->iofl)
&& !test_bit(HID_OUT_RUNNING, &usbhid->iofl)
&& !test_bit(HID_CTRL_RUNNING, &usbhid->iofl)
&& !test_bit(HID_KEYS_PRESSED, &usbhid->iofl)
- && (!usbhid->ledcount || ignoreled))
+ && (!ledcount || ignoreled))
{
set_bit(HID_SUSPENDED, &usbhid->iofl);
spin_unlock_irq(&usbhid->lock);
diff --git a/drivers/hid/usbhid/usbhid.h b/drivers/hid/usbhid/usbhid.h
index dbb6af6..f633c24 100644
--- a/drivers/hid/usbhid/usbhid.h
+++ b/drivers/hid/usbhid/usbhid.h
@@ -92,9 +92,6 @@ struct usbhid_device {
unsigned int retry_delay; /* Delay length in ms */
struct work_struct reset_work; /* Task context for resets */
wait_queue_head_t wait; /* For sleeping */
- int ledcount; /* counting the number of active leds */
-
- struct work_struct led_work; /* Task context for setting LEDs */
};
#define hid_to_usb_dev(hid_dev) \
--
1.8.3.2
^ permalink raw reply related
* [RFC 3/8] HID: input: generic hidinput_input_event handler
From: David Herrmann @ 2013-07-15 17:10 UTC (permalink / raw)
To: linux-input
Cc: Jiri Kosina, Benjamin Tissoires, Henrik Rydberg, Oliver Neukum,
David Herrmann
In-Reply-To: <1373908217-16748-1-git-send-email-dh.herrmann@gmail.com>
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 <dh.herrmann@gmail.com>
---
drivers/hid/hid-input.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++-
include/linux/hid.h | 1 +
2 files changed, 80 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 7480799..308eee8 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -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);
+}
+
+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);
@@ -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;
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++) {
@@ -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);
}
EXPORT_SYMBOL_GPL(hidinput_disconnect);
diff --git a/include/linux/hid.h b/include/linux/hid.h
index b8058c5..ea4b828 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -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
^ permalink raw reply related
* [RFC 2/8] HID: usbhid: update LED fields unlocked
From: David Herrmann @ 2013-07-15 17:10 UTC (permalink / raw)
To: linux-input
Cc: Jiri Kosina, Benjamin Tissoires, Henrik Rydberg, Oliver Neukum,
David Herrmann
In-Reply-To: <1373908217-16748-1-git-send-email-dh.herrmann@gmail.com>
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 <dh.herrmann@gmail.com>
---
drivers/hid/usbhid/hid-core.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 5482bf4..62b5131 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -664,6 +664,19 @@ static void hid_led(struct work_struct *work)
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 hid_led() worker is spawned
+ * for every SET-LED request so the following hid_led() worker will send
+ * the correct value, guaranteed!
+ */
+
spin_lock_irqsave(&usbhid->lock, flags);
if (!test_bit(HID_DISCONNECTED, &usbhid->iofl)) {
usbhid->ledcount = hidinput_count_leds(hid);
@@ -678,7 +691,6 @@ static int usb_hidinput_input_event(struct input_dev *dev, unsigned int type, un
struct hid_device *hid = input_get_drvdata(dev);
struct usbhid_device *usbhid = hid->driver_data;
struct hid_field *field;
- unsigned long flags;
int offset;
if (type == EV_FF)
@@ -692,9 +704,7 @@ static int usb_hidinput_input_event(struct input_dev *dev, unsigned int type, un
return -1;
}
- spin_lock_irqsave(&usbhid->lock, flags);
hid_set_field(field, offset, value);
- spin_unlock_irqrestore(&usbhid->lock, flags);
/*
* Defer performing requested LED action.
--
1.8.3.2
^ permalink raw reply related
* [RFC 1/8] HID: usbhid: make usbhid_set_leds() static
From: David Herrmann @ 2013-07-15 17:10 UTC (permalink / raw)
To: linux-input
Cc: Jiri Kosina, Benjamin Tissoires, Henrik Rydberg, Oliver Neukum,
David Herrmann
In-Reply-To: <1373908217-16748-1-git-send-email-dh.herrmann@gmail.com>
usbhid_set_leds() is only used inside of usbhid/hid-core.c so no need to
export it.
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
drivers/hid/usbhid/hid-core.c | 3 +--
include/linux/hid.h | 1 -
2 files changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 9941828..5482bf4 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -857,7 +857,7 @@ static int hid_find_field_early(struct hid_device *hid, unsigned int page,
return -1;
}
-void usbhid_set_leds(struct hid_device *hid)
+static void usbhid_set_leds(struct hid_device *hid)
{
struct hid_field *field;
int offset;
@@ -867,7 +867,6 @@ void usbhid_set_leds(struct hid_device *hid)
usbhid_submit_report(hid, field->report, USB_DIR_OUT);
}
}
-EXPORT_SYMBOL_GPL(usbhid_set_leds);
/*
* Traverse the supplied list of reports and find the longest
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 0c48991..b8058c5 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -989,7 +989,6 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size,
u32 usbhid_lookup_quirk(const u16 idVendor, const u16 idProduct);
int usbhid_quirks_init(char **quirks_param);
void usbhid_quirks_exit(void);
-void usbhid_set_leds(struct hid_device *hid);
#ifdef CONFIG_HID_PID
int hid_pidff_init(struct hid_device *hid);
--
1.8.3.2
^ permalink raw reply related
* [RFC 0/8] HID: Transport Driver Cleanup
From: David Herrmann @ 2013-07-15 17:10 UTC (permalink / raw)
To: linux-input
Cc: Jiri Kosina, Benjamin Tissoires, Henrik Rydberg, Oliver Neukum,
David Herrmann
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
^ permalink raw reply
* [PATCH v4] Input: sysrq - DT binding for key sequence
From: mathieu.poirier @ 2013-07-15 15:36 UTC (permalink / raw)
To: grant.likely, dmitry.torokhov
Cc: devicetree-discuss, john.stultz, linux-input, kernel-team,
mathieu.poirier
From: "Mathieu J. Poirier" <mathieu.poirier@linaro.org>
Adding a simple device tree binding for the specification of key sequences.
Definition of the keys found in the sequence are located in
'include/uapi/linux/input.h'.
For the sysrq driver, holding the sequence of keys down for a specific amount of time
will reset the system.
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
changes for v4:
- Moved seach of reset sequence nodes to a single call.
---
.../devicetree/bindings/input/input-reset.txt | 34 ++++++++++++++
drivers/tty/sysrq.c | 54 ++++++++++++++++++++++
2 files changed, 88 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/input-reset.txt
diff --git a/Documentation/devicetree/bindings/input/input-reset.txt b/Documentation/devicetree/bindings/input/input-reset.txt
new file mode 100644
index 0000000..79504af
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/input-reset.txt
@@ -0,0 +1,34 @@
+Input: sysrq reset sequence
+
+A simple binding to represent a set of keys as described in
+include/uapi/linux/input.h. This is to communicate a
+sequence of keys to the sysrq driver. Upon holding the keys
+for a specified amount of time (if specified) the system is
+sync'ed and reset.
+
+Key sequences are global to the system but all the keys in a
+set must be coming from the same input device.
+
+The /chosen node should contain a 'linux,sysrq-reset-seq' child
+node to define a set of keys.
+
+Required property:
+sysrq-reset-seq: array of keycodes
+
+Optional property:
+timeout-ms: duration keys must be pressed together in microseconds
+before generating a sysrq
+
+Example:
+
+ chosen {
+ linux,sysrq-reset-seq {
+ keyset = <0x03
+ 0x04
+ 0x0a>;
+ timeout-ms = <3000>;
+ };
+ };
+
+Would represent KEY_2, KEY_3 and KEY_9.
+
diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
index b51c154..01a8729 100644
--- a/drivers/tty/sysrq.c
+++ b/drivers/tty/sysrq.c
@@ -44,6 +44,7 @@
#include <linux/uaccess.h>
#include <linux/moduleparam.h>
#include <linux/jiffies.h>
+#include <linux/of.h>
#include <asm/ptrace.h>
#include <asm/irq_regs.h>
@@ -671,6 +672,50 @@ static void sysrq_detect_reset_sequence(struct sysrq_state *state,
}
}
+static void sysrq_of_get_keyreset_config(void)
+{
+ unsigned short key;
+ struct device_node *np;
+ const struct property *prop;
+ const __be32 *val;
+ int count, i;
+
+ np = of_find_node_by_path("/chosen/linux,sysrq-reset-seq");
+ if (!np) {
+ pr_debug("No sysrq node found");
+ goto out;
+ }
+
+ prop = of_find_property(np, "keyset", NULL);
+ if (!prop || !prop->value) {
+ pr_err("Invalid input keyset");
+ goto out;
+ }
+
+ count = prop->length / sizeof(u32);
+ val = prop->value;
+
+ if (count > SYSRQ_KEY_RESET_MAX)
+ count = SYSRQ_KEY_RESET_MAX;
+
+ /* reset in case a __weak definition was present */
+ sysrq_reset_seq_len = 0;
+
+ for (i = 0; i < count; i++) {
+ key = (unsigned short)be32_to_cpup(val++);
+ if (key == KEY_RESERVED || key > KEY_MAX)
+ break;
+
+ sysrq_reset_seq[sysrq_reset_seq_len++] = key;
+ }
+
+ /* get reset timeout if any */
+ of_property_read_u32(np, "timeout-ms", &sysrq_reset_downtime_ms);
+
+ out:
+ ;
+}
+
static void sysrq_reinject_alt_sysrq(struct work_struct *work)
{
struct sysrq_state *sysrq =
@@ -688,6 +733,7 @@ static void sysrq_reinject_alt_sysrq(struct work_struct *work)
input_inject_event(handle, EV_KEY, KEY_SYSRQ, 1);
input_inject_event(handle, EV_SYN, SYN_REPORT, 1);
+
input_inject_event(handle, EV_KEY, KEY_SYSRQ, 0);
input_inject_event(handle, EV_KEY, alt_code, 0);
input_inject_event(handle, EV_SYN, SYN_REPORT, 1);
@@ -903,6 +949,7 @@ static inline void sysrq_register_handler(void)
int error;
int i;
+ /* first check if a __weak interface was instantiated */
for (i = 0; i < ARRAY_SIZE(sysrq_reset_seq); i++) {
key = platform_sysrq_reset_seq[i];
if (key == KEY_RESERVED || key > KEY_MAX)
@@ -911,6 +958,13 @@ static inline void sysrq_register_handler(void)
sysrq_reset_seq[sysrq_reset_seq_len++] = key;
}
+ /*
+ * DT configuration takes precedence over anything
+ * that would have been defined via the __weak
+ * interface
+ */
+ sysrq_of_get_keyreset_config();
+
error = input_register_handler(&sysrq_handler);
if (error)
pr_err("Failed to register input handler, error %d", error);
--
1.8.1.2
^ permalink raw reply related
* [PATCH] typo fixes (coordiante -> coordinate) in am335x
From: Zubair Lutfullah @ 2013-07-15 15:25 UTC (permalink / raw)
To: sameo; +Cc: linux-doc, linux-kernel, linux-omap, linux-arm-kernel,
linux-input
Did a grep for coordiante and replaced them all
with coordinate.
This applies to the mfd-next tree.
Signed-off-by: Zubair Lutfullah <zubair.lutfullah@gmail.com>
---
.../bindings/input/touchscreen/ti-tsc-adc.txt | 4 ++--
arch/arm/boot/dts/am335x-evm.dts | 2 +-
drivers/input/touchscreen/ti_am335x_tsc.c | 2 +-
drivers/mfd/ti_am335x_tscadc.c | 2 +-
4 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
index 491c97b..3e22aec 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
@@ -6,7 +6,7 @@ Required properties:
ti,wires: Wires refer to application modes i.e. 4/5/8 wire touchscreen
support on the platform.
ti,x-plate-resistance: X plate resistance
- ti,coordiante-readouts: The sequencer supports a total of 16
+ ti,coordinate-readouts: The sequencer supports a total of 16
programmable steps each step is used to
read a single coordinate. A single
readout is enough but multiple reads can
@@ -34,7 +34,7 @@ Example:
tsc {
ti,wires = <4>;
ti,x-plate-resistance = <200>;
- ti,coordiante-readouts = <5>;
+ ti,coordinate-readouts = <5>;
ti,wire-config = <0x00 0x11 0x22 0x33>;
};
diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
index 0fa4c7f..e50b781 100644
--- a/arch/arm/boot/dts/am335x-evm.dts
+++ b/arch/arm/boot/dts/am335x-evm.dts
@@ -250,7 +250,7 @@
tsc {
ti,wires = <4>;
ti,x-plate-resistance = <200>;
- ti,coordiante-readouts = <5>;
+ ti,coordinate-readouts = <5>;
ti,wire-config = <0x00 0x11 0x22 0x33>;
};
diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index 0e9f02a..6422f65 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -348,7 +348,7 @@ static int titsc_parse_dt(struct platform_device *pdev,
if (err < 0)
return err;
- err = of_property_read_u32(node, "ti,coordiante-readouts",
+ err = of_property_read_u32(node, "ti,coordinate-readouts",
&ts_dev->coordinate_readouts);
if (err < 0)
return err;
diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index b003a16..dd31e23 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -106,7 +106,7 @@ static int ti_tscadc_probe(struct platform_device *pdev)
node = of_get_child_by_name(pdev->dev.of_node, "tsc");
of_property_read_u32(node, "ti,wires", &tsc_wires);
- of_property_read_u32(node, "ti,coordiante-readouts", &readouts);
+ of_property_read_u32(node, "ti,coordinate-readouts", &readouts);
node = of_get_child_by_name(pdev->dev.of_node, "adc");
of_property_for_each_u32(node, "ti,adc-channels", prop, cur, val) {
--
1.7.9.5
^ permalink raw reply related
* [PATCH v2 1/2] input/uinput: support abs resolution
From: Benjamin Tissoires @ 2013-07-15 13:37 UTC (permalink / raw)
To: Dmitry Torokhov, Benjamin Tissoires, Peter Hutterer, linux-input,
linux-kernel
From: Peter Hutterer <peter.hutterer@who-t.net>
Input resolution has been added in kernel v2.6.31 with commit
ec20a022aa24fc63. However, uinput was never updated since, meaning that
devices cannot be emulated in full. User-space processes that rely on the
resolution to be accurate need this.
Resolution is added in struct uinput_user_dev in a way it won't break
current programs relying on the old implementation.
Bump the UINPUT_VERSION number to 4 to aid detection of this in user-space.
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
Sorry for that, I mixed up the "From:" field.
This v2 contains the right From in the first patch , and this is the only
difference...
Cheers,
Benjamin
drivers/input/misc/uinput.c | 17 ++++++++++++-----
include/linux/uinput.h | 2 ++
include/uapi/linux/uinput.h | 3 +++
3 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index a0a4bba..7d518b4 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -20,6 +20,8 @@
* Author: Aristeu Sergio Rozanski Filho <aris@cathedrallabs.org>
*
* Changes/Revisions:
+ * 0.4 12/07/2013 (Peter Hutterer <peter.hutterer@redhat.com>)
+ * - update uinput_user_dev struct to allow abs resolution
* 0.3 09/04/2006 (Anssi Hannula <anssi.hannula@gmail.com>)
* - updated ff support for the changes in kernel interface
* - added MODULE_VERSION
@@ -364,9 +366,9 @@ static int uinput_setup_device(struct uinput_device *udev,
struct input_dev *dev;
int i;
int retval;
+ size_t size;
- if (count != sizeof(struct uinput_user_dev))
- return -EINVAL;
+ size = min_t(size_t, count, sizeof(struct uinput_user_dev));
if (!udev->dev) {
retval = uinput_allocate_device(udev);
@@ -376,9 +378,13 @@ static int uinput_setup_device(struct uinput_device *udev,
dev = udev->dev;
- user_dev = memdup_user(buffer, sizeof(struct uinput_user_dev));
- if (IS_ERR(user_dev))
- return PTR_ERR(user_dev);
+ user_dev = kzalloc(sizeof(struct uinput_user_dev), GFP_KERNEL);
+ if (!user_dev)
+ return -ENOMEM;
+ if (copy_from_user(user_dev, buffer, size)) {
+ retval = -EFAULT;
+ goto exit;
+ }
udev->ff_effects_max = user_dev->ff_effects_max;
@@ -406,6 +412,7 @@ static int uinput_setup_device(struct uinput_device *udev,
input_abs_set_min(dev, i, user_dev->absmin[i]);
input_abs_set_fuzz(dev, i, user_dev->absfuzz[i]);
input_abs_set_flat(dev, i, user_dev->absflat[i]);
+ input_abs_set_res(dev, i, user_dev->absres[i]);
}
/* check if absmin/absmax/absfuzz/absflat are filled as
diff --git a/include/linux/uinput.h b/include/linux/uinput.h
index 0a4487d..6291a22 100644
--- a/include/linux/uinput.h
+++ b/include/linux/uinput.h
@@ -20,6 +20,8 @@
* Author: Aristeu Sergio Rozanski Filho <aris@cathedrallabs.org>
*
* Changes/Revisions:
+ * 0.4 12/07/2013 (Peter Hutterer <peter.hutterer@redhat.com>)
+ * - update uinput_user_dev struct to allow abs resolution
* 0.3 24/05/2006 (Anssi Hannula <anssi.hannulagmail.com>)
* - update ff support for the changes in kernel interface
* - add UINPUT_VERSION
diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
index fe46431..f6a393b 100644
--- a/include/uapi/linux/uinput.h
+++ b/include/uapi/linux/uinput.h
@@ -20,6 +20,8 @@
* Author: Aristeu Sergio Rozanski Filho <aris@cathedrallabs.org>
*
* Changes/Revisions:
+ * 0.4 12/07/2013 (Peter Hutterer <peter.hutterer@redhat.com>)
+ * - update uinput_user_dev struct to allow abs resolution
* 0.3 24/05/2006 (Anssi Hannula <anssi.hannulagmail.com>)
* - update ff support for the changes in kernel interface
* - add UINPUT_VERSION
@@ -133,5 +135,6 @@ struct uinput_user_dev {
__s32 absmin[ABS_CNT];
__s32 absfuzz[ABS_CNT];
__s32 absflat[ABS_CNT];
+ __s32 absres[ABS_CNT];
};
#endif /* _UAPI__UINPUT_H_ */
--
1.8.3.1
^ permalink raw reply related
* [PATCH v2 2/2] input/uinput: add UI_GET_SYSPATH ioctl to retrieve the sysfs path
From: Benjamin Tissoires @ 2013-07-15 13:37 UTC (permalink / raw)
To: Dmitry Torokhov, Benjamin Tissoires, Peter Hutterer, linux-input,
linux-kernel
Cc: Benjamin Tissoires
In-Reply-To: <1373895429-8846-1-git-send-email-benjamin.tissoires@redhat.com>
Evemu [1] uses uinput to replay devices traces it has recorded. However,
the way evemu uses uinput is slightly different from how uinput is
supposed to be used.
Evemu creates the device node through uinput, bu inject events through
the input device node directly (and skipping the uinput node).
Currently, evemu relies on an heuristic to guess which input node was
created. The problem is that is heuristic is subjected to races between
different uinput devices or even with physical devices. Having a way
to retrieve the sysfs path allows us to find the event node without
having to rely on this heuristic.
[1] http://www.freedesktop.org/wiki/Evemu/
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
drivers/input/misc/uinput.c | 37 ++++++++++++++++++++++++++++++++++++-
include/linux/uinput.h | 1 +
include/uapi/linux/uinput.h | 3 +++
3 files changed, 40 insertions(+), 1 deletion(-)
diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index 7d518b4..49a9f7d 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -22,6 +22,7 @@
* Changes/Revisions:
* 0.4 12/07/2013 (Peter Hutterer <peter.hutterer@redhat.com>)
* - update uinput_user_dev struct to allow abs resolution
+ * - add UI_GET_SYSPATH ioctl
* 0.3 09/04/2006 (Anssi Hannula <anssi.hannula@gmail.com>)
* - updated ff support for the changes in kernel interface
* - added MODULE_VERSION
@@ -667,6 +668,21 @@ static int uinput_ff_upload_from_user(const char __user *buffer,
__ret; \
})
+static int uinput_str_to_user(const char *str, unsigned int maxlen,
+ void __user *p)
+{
+ int len;
+
+ if (!str)
+ return -ENOENT;
+
+ len = strlen(str) + 1;
+ if (len > maxlen)
+ len = maxlen;
+
+ return copy_to_user(p, str, len) ? -EFAULT : len;
+}
+
static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
unsigned long arg, void __user *p)
{
@@ -676,6 +692,8 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
struct uinput_ff_erase ff_erase;
struct uinput_request *req;
char *phys;
+ const char *path;
+ unsigned int size;
retval = mutex_lock_interruptible(&udev->mutex);
if (retval)
@@ -828,7 +846,24 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
break;
default:
- retval = -EINVAL;
+ retval = -EAGAIN;
+ }
+
+ if (retval == -EAGAIN) {
+ size = _IOC_SIZE(cmd);
+
+ /* Now check variable-length commands */
+ switch (cmd & ~IOCSIZE_MASK) {
+ case UI_GET_SYSPATH(0):
+ path = kobject_get_path(&udev->dev->dev.kobj,
+ GFP_KERNEL);
+ retval = uinput_str_to_user(path, size, p);
+ kfree(path);
+ break;
+
+ default:
+ retval = -EINVAL;
+ }
}
out:
diff --git a/include/linux/uinput.h b/include/linux/uinput.h
index 6291a22..64fab81 100644
--- a/include/linux/uinput.h
+++ b/include/linux/uinput.h
@@ -22,6 +22,7 @@
* Changes/Revisions:
* 0.4 12/07/2013 (Peter Hutterer <peter.hutterer@redhat.com>)
* - update uinput_user_dev struct to allow abs resolution
+ * - add UI_GET_SYSPATH ioctl
* 0.3 24/05/2006 (Anssi Hannula <anssi.hannulagmail.com>)
* - update ff support for the changes in kernel interface
* - add UINPUT_VERSION
diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
index f6a393b..d826409 100644
--- a/include/uapi/linux/uinput.h
+++ b/include/uapi/linux/uinput.h
@@ -22,6 +22,7 @@
* Changes/Revisions:
* 0.4 12/07/2013 (Peter Hutterer <peter.hutterer@redhat.com>)
* - update uinput_user_dev struct to allow abs resolution
+ * - add UI_GET_SYSPATH ioctl
* 0.3 24/05/2006 (Anssi Hannula <anssi.hannulagmail.com>)
* - update ff support for the changes in kernel interface
* - add UINPUT_VERSION
@@ -75,6 +76,8 @@ struct uinput_ff_erase {
#define UI_BEGIN_FF_ERASE _IOWR(UINPUT_IOCTL_BASE, 202, struct uinput_ff_erase)
#define UI_END_FF_ERASE _IOW(UINPUT_IOCTL_BASE, 203, struct uinput_ff_erase)
+#define UI_GET_SYSPATH(len) _IOC(_IOC_READ, UINPUT_IOCTL_BASE, 300, len)
+
/*
* To write a force-feedback-capable driver, the upload_effect
* and erase_effect callbacks in input_dev must be implemented.
--
1.8.3.1
^ permalink raw reply related
* [PATCH 2/2] input/uinput: add UI_GET_SYSPATH ioctl to retrieve the sysfs path
From: Benjamin Tissoires @ 2013-07-15 13:14 UTC (permalink / raw)
To: Dmitry Torokhov, Benjamin Tissoires, Peter Hutterer, linux-input,
linux-kernel
Cc: Benjamin Tissoires
Evemu [1] uses uinput to replay devices traces it has recorded. However,
the way evemu uses uinput is slightly different from how uinput is
supposed to be used.
Evemu creates the device node through uinput, bu inject events through
the input device node directly (and skipping the uinput node).
Currently, evemu relies on an heuristic to guess which input node was
created. The problem is that is heuristic is subjected to races between
different uinput devices or even with physical devices. Having a way
to retrieve the sysfs path allows us to find the event node without
having to rely on this heuristic.
[1] http://www.freedesktop.org/wiki/Evemu/
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
drivers/input/misc/uinput.c | 37 ++++++++++++++++++++++++++++++++++++-
include/linux/uinput.h | 1 +
include/uapi/linux/uinput.h | 3 +++
3 files changed, 40 insertions(+), 1 deletion(-)
diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index 7d518b4..49a9f7d 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -22,6 +22,7 @@
* Changes/Revisions:
* 0.4 12/07/2013 (Peter Hutterer <peter.hutterer@redhat.com>)
* - update uinput_user_dev struct to allow abs resolution
+ * - add UI_GET_SYSPATH ioctl
* 0.3 09/04/2006 (Anssi Hannula <anssi.hannula@gmail.com>)
* - updated ff support for the changes in kernel interface
* - added MODULE_VERSION
@@ -667,6 +668,21 @@ static int uinput_ff_upload_from_user(const char __user *buffer,
__ret; \
})
+static int uinput_str_to_user(const char *str, unsigned int maxlen,
+ void __user *p)
+{
+ int len;
+
+ if (!str)
+ return -ENOENT;
+
+ len = strlen(str) + 1;
+ if (len > maxlen)
+ len = maxlen;
+
+ return copy_to_user(p, str, len) ? -EFAULT : len;
+}
+
static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
unsigned long arg, void __user *p)
{
@@ -676,6 +692,8 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
struct uinput_ff_erase ff_erase;
struct uinput_request *req;
char *phys;
+ const char *path;
+ unsigned int size;
retval = mutex_lock_interruptible(&udev->mutex);
if (retval)
@@ -828,7 +846,24 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
break;
default:
- retval = -EINVAL;
+ retval = -EAGAIN;
+ }
+
+ if (retval == -EAGAIN) {
+ size = _IOC_SIZE(cmd);
+
+ /* Now check variable-length commands */
+ switch (cmd & ~IOCSIZE_MASK) {
+ case UI_GET_SYSPATH(0):
+ path = kobject_get_path(&udev->dev->dev.kobj,
+ GFP_KERNEL);
+ retval = uinput_str_to_user(path, size, p);
+ kfree(path);
+ break;
+
+ default:
+ retval = -EINVAL;
+ }
}
out:
diff --git a/include/linux/uinput.h b/include/linux/uinput.h
index 6291a22..64fab81 100644
--- a/include/linux/uinput.h
+++ b/include/linux/uinput.h
@@ -22,6 +22,7 @@
* Changes/Revisions:
* 0.4 12/07/2013 (Peter Hutterer <peter.hutterer@redhat.com>)
* - update uinput_user_dev struct to allow abs resolution
+ * - add UI_GET_SYSPATH ioctl
* 0.3 24/05/2006 (Anssi Hannula <anssi.hannulagmail.com>)
* - update ff support for the changes in kernel interface
* - add UINPUT_VERSION
diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
index f6a393b..d826409 100644
--- a/include/uapi/linux/uinput.h
+++ b/include/uapi/linux/uinput.h
@@ -22,6 +22,7 @@
* Changes/Revisions:
* 0.4 12/07/2013 (Peter Hutterer <peter.hutterer@redhat.com>)
* - update uinput_user_dev struct to allow abs resolution
+ * - add UI_GET_SYSPATH ioctl
* 0.3 24/05/2006 (Anssi Hannula <anssi.hannulagmail.com>)
* - update ff support for the changes in kernel interface
* - add UINPUT_VERSION
@@ -75,6 +76,8 @@ struct uinput_ff_erase {
#define UI_BEGIN_FF_ERASE _IOWR(UINPUT_IOCTL_BASE, 202, struct uinput_ff_erase)
#define UI_END_FF_ERASE _IOW(UINPUT_IOCTL_BASE, 203, struct uinput_ff_erase)
+#define UI_GET_SYSPATH(len) _IOC(_IOC_READ, UINPUT_IOCTL_BASE, 300, len)
+
/*
* To write a force-feedback-capable driver, the upload_effect
* and erase_effect callbacks in input_dev must be implemented.
--
1.8.3.1
^ permalink raw reply related
* [PATCH 1/2] input/uinput: support abs resolution
From: Benjamin Tissoires @ 2013-07-15 13:12 UTC (permalink / raw)
To: Dmitry Torokhov, Benjamin Tissoires, linux-input, linux-kernel
Cc: Benjamin Tissoires, Peter Hutterer
Input resolution has been added in kernel v2.6.31 with commit
ec20a022aa24fc63. However, uinput was never updated since, meaning that
devices cannot be emulated in full. User-space processes that rely on the
resolution to be accurate need this.
Resolution is added in struct uinput_user_dev in a way it won't break
current programs relying on the old implementation.
Bump the UINPUT_VERSION number to 4 to aid detection of this in user-space.
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
drivers/input/misc/uinput.c | 17 ++++++++++++-----
include/linux/uinput.h | 2 ++
include/uapi/linux/uinput.h | 3 +++
3 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index a0a4bba..7d518b4 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -20,6 +20,8 @@
* Author: Aristeu Sergio Rozanski Filho <aris@cathedrallabs.org>
*
* Changes/Revisions:
+ * 0.4 12/07/2013 (Peter Hutterer <peter.hutterer@redhat.com>)
+ * - update uinput_user_dev struct to allow abs resolution
* 0.3 09/04/2006 (Anssi Hannula <anssi.hannula@gmail.com>)
* - updated ff support for the changes in kernel interface
* - added MODULE_VERSION
@@ -364,9 +366,9 @@ static int uinput_setup_device(struct uinput_device *udev,
struct input_dev *dev;
int i;
int retval;
+ size_t size;
- if (count != sizeof(struct uinput_user_dev))
- return -EINVAL;
+ size = min_t(size_t, count, sizeof(struct uinput_user_dev));
if (!udev->dev) {
retval = uinput_allocate_device(udev);
@@ -376,9 +378,13 @@ static int uinput_setup_device(struct uinput_device *udev,
dev = udev->dev;
- user_dev = memdup_user(buffer, sizeof(struct uinput_user_dev));
- if (IS_ERR(user_dev))
- return PTR_ERR(user_dev);
+ user_dev = kzalloc(sizeof(struct uinput_user_dev), GFP_KERNEL);
+ if (!user_dev)
+ return -ENOMEM;
+ if (copy_from_user(user_dev, buffer, size)) {
+ retval = -EFAULT;
+ goto exit;
+ }
udev->ff_effects_max = user_dev->ff_effects_max;
@@ -406,6 +412,7 @@ static int uinput_setup_device(struct uinput_device *udev,
input_abs_set_min(dev, i, user_dev->absmin[i]);
input_abs_set_fuzz(dev, i, user_dev->absfuzz[i]);
input_abs_set_flat(dev, i, user_dev->absflat[i]);
+ input_abs_set_res(dev, i, user_dev->absres[i]);
}
/* check if absmin/absmax/absfuzz/absflat are filled as
diff --git a/include/linux/uinput.h b/include/linux/uinput.h
index 0a4487d..6291a22 100644
--- a/include/linux/uinput.h
+++ b/include/linux/uinput.h
@@ -20,6 +20,8 @@
* Author: Aristeu Sergio Rozanski Filho <aris@cathedrallabs.org>
*
* Changes/Revisions:
+ * 0.4 12/07/2013 (Peter Hutterer <peter.hutterer@redhat.com>)
+ * - update uinput_user_dev struct to allow abs resolution
* 0.3 24/05/2006 (Anssi Hannula <anssi.hannulagmail.com>)
* - update ff support for the changes in kernel interface
* - add UINPUT_VERSION
diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
index fe46431..f6a393b 100644
--- a/include/uapi/linux/uinput.h
+++ b/include/uapi/linux/uinput.h
@@ -20,6 +20,8 @@
* Author: Aristeu Sergio Rozanski Filho <aris@cathedrallabs.org>
*
* Changes/Revisions:
+ * 0.4 12/07/2013 (Peter Hutterer <peter.hutterer@redhat.com>)
+ * - update uinput_user_dev struct to allow abs resolution
* 0.3 24/05/2006 (Anssi Hannula <anssi.hannulagmail.com>)
* - update ff support for the changes in kernel interface
* - add UINPUT_VERSION
@@ -133,5 +135,6 @@ struct uinput_user_dev {
__s32 absmin[ABS_CNT];
__s32 absfuzz[ABS_CNT];
__s32 absflat[ABS_CNT];
+ __s32 absres[ABS_CNT];
};
#endif /* _UAPI__UINPUT_H_ */
--
1.8.3.1
^ permalink raw reply related
* Re: joydev bug with playstation USB adapter
From: Renato @ 2013-07-15 12:27 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On Sat, 2 Feb 2013 01:11:17 Jiri Kosina wrote
>On Sat, 2 Feb 2013, renato wrote:
>
>> Hi, I have two playstation USB adpters, one of them with this [1]
>> "lsusb -v" (the other I don't have access to ATM) and they both give
>> me the following buggy behaviour: the range of the sticks is limited
>> - i.e. value 32767 is reached when the stick is only about half-way
>> through its run, leaving a big dead zone after that. This happens on
>> all axes and recalibrating with "jscal -c" didn't help.
>>
>> I'm on an up-to-date Archlinux, kernel 3.7.5, and have tried various
>> kernel versions down to 3.0.1 with same results.
>>
>> Is there any other info I can provide?
>
>[ adding Anssi to CC ]
>
>Hi renato,
>
>there is a driver for this particular device (hid-pl), written by
>Anssi Hannula.
>
>So before I'll be digging into this any further -- Anssi, does this
>limited range sound familiar to you when it comes to 0x0810/0x001
>device?
>
>Thanks.
>
>>
>> thanks,
>> renato
>>
>>
>> [1]
>> Bus 002 Device 002: ID 0810:0001 Personal Communication Systems, Inc.
>> Dual PSX A daptor
>> Device Descriptor:
>> bLength 18
>> bDescriptorType 1
>> bcdUSB 1.00
>> bDeviceClass 0 (Defined at Interface level)
>> bDeviceSubClass 0
>> bDeviceProtocol 0
>> bMaxPacketSize0 8
>> idVendor 0x0810 Personal Communication Systems, Inc.
>> idProduct 0x0001 Dual PSX Adaptor
>> bcdDevice 1.06
>> iManufacturer 0
>> iProduct 2 Twin USB Joystick
>> iSerial 0
>> bNumConfigurations 1
>> Configuration Descriptor:
>> bLength 9
>> bDescriptorType 2
>> wTotalLength 34
>> bNumInterfaces 1
>> bConfigurationValue 1
>> iConfiguration 0
>> bmAttributes 0x80
>> (Bus Powered)
>> MaxPower 500mA
>> Interface Descriptor:
>> bLength 9
>> bDescriptorType 4
>> bInterfaceNumber 0
>> bAlternateSetting 0
>> bNumEndpoints 1
>> bInterfaceClass 3 Human Interface Device
>> bInterfaceSubClass 0 No Subclass
>> bInterfaceProtocol 0 None
>> iInterface 0
>> HID Device Descriptor:
>> bLength 9
>> bDescriptorType 33
>> bcdHID 1.10
>> bCountryCode 33 US
>> bNumDescriptors 1
>> bDescriptorType 34 Report
>> wDescriptorLength 202
>> Report Descriptors:
>> ** UNAVAILABLE **
>> Endpoint Descriptor:
>> bLength 7
>> bDescriptorType 5
>> bEndpointAddress 0x81 EP 1 IN
>> bmAttributes 3
>> Transfer Type Interrupt
>> Synch Type None
>> Usage Type Data
>> wMaxPacketSize 0x0008 1x 8 bytes
>> bInterval 10
>> Device Status: 0x0000
>> (Bus Powered)
>>
>
>--
>Jiri Kosina
>SUSE Labs
>--
Hi, so any news on this? I still have a limited range on my gamepads'
analog sticks, with kernel 3.9.9.
I woul've CCed Anssi Hannula as was suggested by you but I couln't find
his mail.
cheers,
renato
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.20 (GNU/Linux)
iQEcBAEBAgAGBQJR4+qsAAoJEBz6xFdttjrfBmwH/2ihT+nKdkmFuKnT5JR0Z9Ue
f2UdToUwy8V940Cg39v3HgVL9ZZO+vvuzK1Qg/mYgvtjd1UqmOKCOJFEdZdiLlz7
Ke1FHP+7dBY/c5tEK96psMiwXJJpJTZJvfynTv0jGgLtJ2EH+1xj2QY9n9HWYLgw
inNxWwq1uKxaXt6LD+ZWwaVdKTJ8Os+x+OmlDhSIaPhW31mBqJmgCoVI6FCyupxT
n021Zse3+ytj03odQ68Ocgyl3wkmjR/LxFYXJdwkB1mSkiYg5nseJ8UWqv2AyXCF
JfiKDigznDUKLyMVRfoplgl33qIa2K4iHt7X1W3EreQnugMRQyw9ul3YWwYH4cw=
=I4J2
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: [PATCH v2] HID: kye: Add report fixup for Genius Gx Imperator Keyboard
From: Jiri Kosina @ 2013-07-15 8:27 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Honza Brazdil, linux-input, linux-kernel
In-Reply-To: <1373875938-10165-1-git-send-email-benjamin.tissoires@redhat.com>
On Mon, 15 Jul 2013, Benjamin Tissoires wrote:
> Genius Gx Imperator Keyboard presents the same problem in its report
> descriptors than Genius Gila Gaming Mouse.
> Use the same fixup for both.
>
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=928561
>
> Reported-and-tested-by: Honza Brazdil <jbrazdil@redhat.com>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>
> with the proper blacklist in hid-core this time.
Applied, thanks.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox