[RFC v2 0/4] uhid LED input events handling

Hi all,

This patchset implements the support for handling LED input events in uhid
driver.

As requested in the previous RFC, it was introduced a new helper in hid-input.c
to carry out a generic LED input events handling (hidinput_led_output_report).
The helper is used by uhid driver.

The helper can also be used in i2c-hid driver since it does a very simply
handling of LED events too. I have no i2c device to test the i2c-hid patch,
so it requires some testing before applying it.

As long as the helper relies on device's .hid_output_raw_report() and
hid-logitech-dj driver doesn't properly implement this callback, the helper is
not used in hid-logitech-dj driver.

Also, this helper is not used in usbhid driver as long as it does a more
sophisticated handling than what hidinput_led_output_report helper does. For
further information about usbhid LEDs handling see comments in the patchset [1].

This patchset is rebased on Jiri's for-next branch.

Best regards,

Andre Guedes

[1] - http://marc.info/?l=linux-usb&m=132013970108623&w=2


Andre Guedes (4):
  HID: Add generic handler for LED input events
  HID: uhid: Handle LED input events
  HID: uhid: Use GFP_ATOMIC in uhid_hid_output_raw
  HID: i2c-hid: LED input events handling

 drivers/hid/hid-input.c       | 51 +++++++++++++++++++++++++++++++++++++++++++
 drivers/hid/i2c-hid/i2c-hid.c | 11 +---------
 drivers/hid/uhid.c            | 34 +++++++++++++++++++----------
 include/linux/hid.h           |  2 ++
 4 files changed, 76 insertions(+), 22 deletions(-)

-- 
1.8.0.1

This patch adds the helper hidinput_led_output_report to handle LED
input events.

This helper implements a generic handler for LED input events. It
basically sets the proper field, builds the output report and call
hid_output_raw_report callback to send the raw report to the device.

Signed-off-by: Andre Guedes <redacted>
---
 drivers/hid/hid-input.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/hid.h     |  2 ++
 2 files changed, 53 insertions(+)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 21b196c..957f510 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c

@@ -1300,3 +1300,54 @@ void hidinput_disconnect(struct hid_device *hid)
 }
 EXPORT_SYMBOL_GPL(hidinput_disconnect);
 
+/*
+ * hidinput_led_output_report - Generic handler for LED input events
+ *
+ * @hid: hid device
+ * @code: input event code
+ * @value: input event value
+ *
+ * This function implements a generic handler for LED input events. It
+ * sets the proper LED field, builds the output report and sends it to
+ * device.
+ *
+ * This helper relies on device's .hid_output_raw_report() to send
+ * reports to the device.
+ */
+int hidinput_led_output_report(struct hid_device *hid, unsigned int code,
+				    int value)
+{
+	int offset;
+	struct hid_field *field;
+	struct hid_report *report;
+	u8 *buf;
+	int len;
+	int ret;
+
+	if (!hid->hid_output_raw_report)
+		return -ENOTSUPP;
+
+	offset = hidinput_find_field(hid, EV_LED, code, &field);
+	if (offset == -1)
+		return -ENOENT;
+
+	hid_set_field(field, offset, value);
+
+	report = field->report;
+
+	len = ((report->size - 1) >> 3) + 1 + (report->id > 0);
+
+	buf = kzalloc(len, GFP_ATOMIC);
+	if (!buf)
+		return -ENOMEM;
+
+	hid_output_report(report, buf);
+
+	ret = hid->hid_output_raw_report(hid, buf, len, HID_OUTPUT_REPORT);
+
+	kfree(buf);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(hidinput_led_output_report);
+

diff --git a/include/linux/hid.h b/include/linux/hid.h
index d2c42dd..bd02df3 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h

@@ -701,6 +701,8 @@ extern void hidinput_hid_event(struct hid_device *, struct hid_field *, struct h
 extern void hidinput_report_event(struct hid_device *hid, struct hid_report *report);
 extern int hidinput_connect(struct hid_device *hid, unsigned int force);
 extern void hidinput_disconnect(struct hid_device *);
+extern int hidinput_led_output_report(struct hid_device *hid,
+					unsigned int code, int value);
 
 int hid_set_field(struct hid_field *, unsigned, __s32);
 int hid_input_report(struct hid_device *, int type, u8 *, int, int);

-- 
1.8.0.1

This patch adds support for handling LED input events in uhid driver.
Others input events are sent to userspace as UHID_OUTPUT_EV events.

Signed-off-by: Andre Guedes <redacted>
---
 drivers/hid/uhid.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index 714cd8c..a5cf905 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c

@@ -122,21 +122,31 @@ static int uhid_hid_input(struct input_dev *input, unsigned int type,
 	struct uhid_device *uhid = hid->driver_data;
 	unsigned long flags;
 	struct uhid_event *ev;
+	int ret;
 
-	ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
-	if (!ev)
-		return -ENOMEM;
+	switch (type) {
+	case EV_LED:
+		ret = hidinput_led_output_report(hid, code, value);
+		break;
 
-	ev->type = UHID_OUTPUT_EV;
-	ev->u.output_ev.type = type;
-	ev->u.output_ev.code = code;
-	ev->u.output_ev.value = value;
+	default:
+		ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
+		if (!ev)
+			return -ENOMEM;
 
-	spin_lock_irqsave(&uhid->qlock, flags);
-	uhid_queue(uhid, ev);
-	spin_unlock_irqrestore(&uhid->qlock, flags);
+		ev->type = UHID_OUTPUT_EV;
+		ev->u.output_ev.type = type;
+		ev->u.output_ev.code = code;
+		ev->u.output_ev.value = value;
 
-	return 0;
+		spin_lock_irqsave(&uhid->qlock, flags);
+		uhid_queue(uhid, ev);
+		spin_unlock_irqrestore(&uhid->qlock, flags);
+
+		ret = 0;
+	}
+
+	return ret;
 }
 
 static int uhid_hid_parse(struct hid_device *hid)

-- 
1.8.0.1

We should use GFP_ATOMIC in uhid_hid_output_raw as long as it may
be called in atomic section.

Signed-off-by: Andre Guedes <redacted>
---
 drivers/hid/uhid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index a5cf905..ef74fc6 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c

@@ -261,7 +261,7 @@ static int uhid_hid_output_raw(struct hid_device *hid, __u8 *buf, size_t count,
 	if (count < 1 || count > UHID_DATA_MAX)
 		return -EINVAL;
 
-	ev = kzalloc(sizeof(*ev), GFP_KERNEL);
+	ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
 	if (!ev)
 		return -ENOMEM;
 

-- 
1.8.0.1

This patch removes code for handling LED input events from i2c_hid_
hidinput_input_event and call hidinput_led_output_report helper
instead.

In consequence, i2c-hid driver is now sending LED output
reports to device.

Signed-off-by: Andre Guedes <redacted>
---
 drivers/hid/i2c-hid/i2c-hid.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 67ab5b7..e4722d3 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c

@@ -701,8 +701,6 @@ 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);

@@ -710,14 +708,7 @@ static int i2c_hid_hidinput_input_event(struct input_dev *dev,
 	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;
-	}
-
-	hid_set_field(field, offset, value);
+	hidinput_led_output_report(hid, code, value);
 
 	return 0;
 }

-- 
1.8.0.1

Ping.

On Sat, Dec 1, 2012 at 4:07 PM, Andre Guedes [off-list ref] wrote:

Hi all,

This patchset implements the support for handling LED input events in uhid
driver.

As requested in the previous RFC, it was introduced a new helper in hid-input.c
to carry out a generic LED input events handling (hidinput_led_output_report).
The helper is used by uhid driver.

The helper can also be used in i2c-hid driver since it does a very simply
handling of LED events too. I have no i2c device to test the i2c-hid patch,
so it requires some testing before applying it.

As long as the helper relies on device's .hid_output_raw_report() and
hid-logitech-dj driver doesn't properly implement this callback, the helper is
not used in hid-logitech-dj driver.

Also, this helper is not used in usbhid driver as long as it does a more
sophisticated handling than what hidinput_led_output_report helper does. For
further information about usbhid LEDs handling see comments in the patchset [1].

This patchset is rebased on Jiri's for-next branch.

Best regards,

Andre Guedes

[1] - http://marc.info/?l=linux-usb&m=132013970108623&w=2


Andre Guedes (4):
  HID: Add generic handler for LED input events
  HID: uhid: Handle LED input events
  HID: uhid: Use GFP_ATOMIC in uhid_hid_output_raw
  HID: i2c-hid: LED input events handling

 drivers/hid/hid-input.c       | 51 +++++++++++++++++++++++++++++++++++++++++++
 drivers/hid/i2c-hid/i2c-hid.c | 11 +---------
 drivers/hid/uhid.c            | 34 +++++++++++++++++++----------
 include/linux/hid.h           |  2 ++
 4 files changed, 76 insertions(+), 22 deletions(-)

--
1.8.0.1

Hi Andre

On Mon, Dec 17, 2012 at 2:28 PM, Andre Guedes
[off-list ref] wrote:

Ping.

On Sat, Dec 1, 2012 at 4:07 PM, Andre Guedes [off-list ref] wrote:

quoted

Hi all,

This patchset implements the support for handling LED input events in uhid
driver.

As requested in the previous RFC, it was introduced a new helper in hid-input.c
to carry out a generic LED input events handling (hidinput_led_output_report).
The helper is used by uhid driver.

The helper can also be used in i2c-hid driver since it does a very simply
handling of LED events too. I have no i2c device to test the i2c-hid patch,
so it requires some testing before applying it.

As long as the helper relies on device's .hid_output_raw_report() and
hid-logitech-dj driver doesn't properly implement this callback, the helper is
not used in hid-logitech-dj driver.

Also, this helper is not used in usbhid driver as long as it does a more
sophisticated handling than what hidinput_led_output_report helper does. For
further information about usbhid LEDs handling see comments in the patchset [1].

I actually don't like the idea to call hid_output_raw_report() from
atomic-contexts. Using a work-queue would allow to use your callback
from usbhid and hidp. Furthermore, you wouldn't need the GFP_ATOMIC
patch for uhid.
Also, please first apply the GFP_ATOMIC patch and after that fix uhid
to use hid_output_raw_report(). We want fully bisectable trees.

I am actually ok with all the uhid patches, but Jiri needs to ack the
hid-core patches. Maybe he is ok with it, but I'd like to see all
ll_drivers using this callback. Because the current solution doesn't
seem any better than just fixing uhid to use it's own implementation.
Also please elaborate why hidp and usbhid cannot use this callback.

Sorry for the delay, but I think this will have to wait until next
year or at least after the holidays. At least I am not very often
available during December.
Regards
David

Hi David,

On Thu, Dec 20, 2012 at 4:43 PM, David Herrmann
[off-list ref] wrote:

Hi Andre

On Mon, Dec 17, 2012 at 2:28 PM, Andre Guedes
[off-list ref] wrote:

quoted

Ping.

On Sat, Dec 1, 2012 at 4:07 PM, Andre Guedes [off-list ref] wrote:

quoted

Hi all,

This patchset implements the support for handling LED input events in uhid
driver.

As requested in the previous RFC, it was introduced a new helper in hid-input.c
to carry out a generic LED input events handling (hidinput_led_output_report).
The helper is used by uhid driver.

The helper can also be used in i2c-hid driver since it does a very simply
handling of LED events too. I have no i2c device to test the i2c-hid patch,
so it requires some testing before applying it.

As long as the helper relies on device's .hid_output_raw_report() and
hid-logitech-dj driver doesn't properly implement this callback, the helper is
not used in hid-logitech-dj driver.

Also, this helper is not used in usbhid driver as long as it does a more
sophisticated handling than what hidinput_led_output_report helper does. For
further information about usbhid LEDs handling see comments in the patchset [1].

I actually don't like the idea to call hid_output_raw_report() from
atomic-contexts. Using a work-queue would allow to use your callback
from usbhid and hidp. Furthermore, you wouldn't need the GFP_ATOMIC
patch for uhid.

I agree with you about calling hid_output_raw_report from atomic
sections. I'll re-write hidinput_led_output_report and use a work.

I am actually ok with all the uhid patches, but Jiri needs to ack the
hid-core patches. Maybe he is ok with it, but I'd like to see all
ll_drivers using this callback. Because the current solution doesn't
seem any better than just fixing uhid to use it's own implementation.
Also please elaborate why hidp and usbhid cannot use this callback.

Usbhid uses a more sophisticated approach to handle LED input events.
When a LED input event occurs it only sets the "field" and schedule a
work to carry out sending the report to the device. This way, it is
more likely to gather all LED changes into a single URB. This prevents
a race condition when trying to suspend a laptop with an attached USB
keyboard with both NumLock and CapsLock LEDs on. The race condition is
explained in details in [1, 2, 3].

In hidp, hidinput_led_output_report runs in atomic section, but hidp
.hid_output_raw_report() callback may sleep.

Anyway, with the workqueue approach, I believe we'll be able to use
the helper in all ll_drivers.

As you are ok with uhid changes, I was wondering if we could take an
incremental approach here. First we would simply fix the uhid driver
by handling LED events the same way others ll_drivers do. This way, we
would get uhid sending output reports to userspace and HID over GATT
working fine. Meanwhile, I'll keep working on this refactoring and
testing the ll_drivers. As soon as it is well-tested, I send a new
patchset.

Are you fine with this approach? If you agree, I can send the uhid
patch right away.

Best regards,

Andre

[1] - http://marc.info/?l=linux-usb&m=132013970108623&w=2
[2] - http://marc.info/?l=linux-usb&m=132013966708616&w=2
[3] - http://marc.info/?l=linux-usb&m=132013968208618&w=2