[PATCH 0/4] HID: debugfs rework

[PATCH 0/4] HID: debugfs rework

[Benjamin Tissoires]

Hi Jiri,

This is a small rework of the HID debugfs.
I encountered a problem with multitouch devices: they have too much usages to
fit into the fixed size output buffer of 512.
So I digg a little, and end up with those 4 patches.

Cheers,
Benjamin

Benjamin Tissoires (4):
  HID: debug: break out hid_dump_report() into hid-debug
  HID: core: break out hid_find_max_report() in hid-core
  HID: debug: empty output queue on read
  HID: debug: allocate the output buffer with an estimate

 drivers/hid/hid-core.c        | 84 +++++++++++++++++++++++++++++-----------
 drivers/hid/hid-debug.c       | 89 +++++++++++++++++++++++++++++++++----------
 drivers/hid/i2c-hid/i2c-hid.c | 15 ++------
 drivers/hid/usbhid/hid-core.c | 16 --------
 include/linux/hid-debug.h     |  7 +++-
 include/linux/hid.h           |  5 +++
 6 files changed, 143 insertions(+), 73 deletions(-)

-- 
1.8.1.4

[PATCH 1/4] HID: debug: break out hid_dump_report() into hid-debug

[Benjamin Tissoires]

No semantic changes, but hid_dump_report should be in hid-debug.c, not
in hid-core.c

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-core.c    | 25 ++-----------------------
 drivers/hid/hid-debug.c   | 30 ++++++++++++++++++++++++++++++
 include/linux/hid-debug.h |  6 ++++--
 3 files changed, 36 insertions(+), 25 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 1129f49..34c53fc 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1259,8 +1259,6 @@ int hid_input_report(struct hid_device *hid, int type, u8 *data, int size, int i
 	struct hid_report_enum *report_enum;
 	struct hid_driver *hdrv;
 	struct hid_report *report;
-	char *buf;
-	unsigned int i;
 	int ret = 0;
 
 	if (!hid)
@@ -1283,28 +1281,9 @@ int hid_input_report(struct hid_device *hid, int type, u8 *data, int size, int i
 	}
 
 	/* Avoid unnecessary overhead if debugfs is disabled */
-	if (list_empty(&hid->debug_list))
-		goto nomem;
-
-	buf = kmalloc(sizeof(char) * HID_DEBUG_BUFSIZE, GFP_ATOMIC);
-
-	if (!buf)
-		goto nomem;
-
-	/* dump the report */
-	snprintf(buf, HID_DEBUG_BUFSIZE - 1,
-			"\nreport (size %u) (%snumbered) = ", size, report_enum->numbered ? "" : "un");
-	hid_debug_event(hid, buf);
-
-	for (i = 0; i < size; i++) {
-		snprintf(buf, HID_DEBUG_BUFSIZE - 1,
-				" %02x", data[i]);
-		hid_debug_event(hid, buf);
-	}
-	hid_debug_event(hid, "\n");
-	kfree(buf);
+	if (!list_empty(&hid->debug_list))
+		hid_dump_report(hid, type, data, size);
 
-nomem:
 	report = hid_get_report(report_enum, data);
 
 	if (!report) {
diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index 933fff0..094cbcf 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -591,6 +591,36 @@ void hid_debug_event(struct hid_device *hdev, char *buf)
 }
 EXPORT_SYMBOL_GPL(hid_debug_event);
 
+void hid_dump_report(struct hid_device *hid, int type, u8 *data,
+		int size)
+{
+	struct hid_report_enum *report_enum;
+	char *buf;
+	unsigned int i;
+
+	buf = kmalloc(sizeof(char) * HID_DEBUG_BUFSIZE, GFP_ATOMIC);
+
+	if (!buf)
+		return;
+
+	report_enum = hid->report_enum + type;
+
+	/* dump the report */
+	snprintf(buf, HID_DEBUG_BUFSIZE - 1,
+			"\nreport (size %u) (%snumbered) = ", size,
+			report_enum->numbered ? "" : "un");
+	hid_debug_event(hid, buf);
+
+	for (i = 0; i < size; i++) {
+		snprintf(buf, HID_DEBUG_BUFSIZE - 1,
+				" %02x", data[i]);
+		hid_debug_event(hid, buf);
+	}
+	hid_debug_event(hid, "\n");
+	kfree(buf);
+}
+EXPORT_SYMBOL_GPL(hid_dump_report);
+
 void hid_dump_input(struct hid_device *hdev, struct hid_usage *usage, __s32 value)
 {
 	char *buf;
diff --git a/include/linux/hid-debug.h b/include/linux/hid-debug.h
index 53744fa..8663f21 100644
--- a/include/linux/hid-debug.h
+++ b/include/linux/hid-debug.h
@@ -22,11 +22,12 @@
  *
  */
 
-#define HID_DEBUG_BUFSIZE 512
-
 #ifdef CONFIG_DEBUG_FS
 
+#define HID_DEBUG_BUFSIZE 512
+
 void hid_dump_input(struct hid_device *, struct hid_usage *, __s32);
+void hid_dump_report(struct hid_device *, int , u8 *, int);
 void hid_dump_device(struct hid_device *, struct seq_file *);
 void hid_dump_field(struct hid_field *, int, struct seq_file *);
 char *hid_resolv_usage(unsigned, struct seq_file *);
@@ -50,6 +51,7 @@ struct hid_debug_list {
 #else
 
 #define hid_dump_input(a,b,c)		do { } while (0)
+#define hid_dump_report(a,b,c,d)	do { } while (0)
 #define hid_dump_device(a,b)		do { } while (0)
 #define hid_dump_field(a,b,c)		do { } while (0)
 #define hid_resolv_usage(a,b)		do { } while (0)
-- 
1.8.1.4

[PATCH 2/4] HID: core: break out hid_find_max_report() in hid-core

[Benjamin Tissoires]

hid_find_max_report() was used both in usbhid and i2c-hid.
Put it in core to avoid code duplication.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-core.c        | 29 +++++++++++++++++++++++++++++
 drivers/hid/i2c-hid/i2c-hid.c | 15 +++------------
 drivers/hid/usbhid/hid-core.c | 16 ----------------
 include/linux/hid.h           |  2 ++
 4 files changed, 34 insertions(+), 28 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 34c53fc..25d7903 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1145,6 +1145,35 @@ void hid_output_report(struct hid_report *report, __u8 *data)
 EXPORT_SYMBOL_GPL(hid_output_report);
 
 /*
+ * Return the length (in bytes) of a report
+ */
+int hid_get_report_length(struct hid_report *report)
+{
+	return report->size ? ((report->size - 1) >> 3) + 1 +
+		report->device->report_enum[report->type].numbered : 0;
+}
+EXPORT_SYMBOL_GPL(hid_get_report_length);
+
+/*
+ * Traverse the supplied list of reports and find the longest
+ */
+void hid_find_max_report(struct hid_device *hid, unsigned int type,
+		unsigned int *max)
+{
+	struct hid_report *report;
+	unsigned int size;
+
+	/* We should not rely on wMaxInputLength, as some devices may set it to
+	 * a wrong length. */
+	list_for_each_entry(report, &hid->report_enum[type].report_list, list) {
+		size = hid_get_report_length(report);
+		if (*max < size)
+			*max = size;
+	}
+}
+EXPORT_SYMBOL_GPL(hid_find_max_report);
+
+/*
  * Set a field value. The report this field belongs to has to be
  * created and transferred to the device, to set this value in the
  * device.
diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 2b1799a..5aa1dc0 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -390,8 +390,7 @@ static irqreturn_t i2c_hid_irq(int irq, void *dev_id)
 
 static int i2c_hid_get_report_length(struct hid_report *report)
 {
-	return ((report->size - 1) >> 3) + 1 +
-		report->device->report_enum[report->type].numbered + 2;
+	return hid_get_report_length(report) + 2;
 }
 
 static void i2c_hid_init_report(struct hid_report *report, u8 *buffer,
@@ -456,16 +455,8 @@ static void i2c_hid_init_reports(struct hid_device *hid)
 static void i2c_hid_find_max_report(struct hid_device *hid, unsigned int type,
 		unsigned int *max)
 {
-	struct hid_report *report;
-	unsigned int size;
-
-	/* We should not rely on wMaxInputLength, as some devices may set it to
-	 * a wrong length. */
-	list_for_each_entry(report, &hid->report_enum[type].report_list, list) {
-		size = i2c_hid_get_report_length(report);
-		if (*max < size)
-			*max = size;
-	}
+	hid_find_max_report(hid, type, max);
+	*max += 2;
 }
 
 static void i2c_hid_free_buffers(struct i2c_hid *ihid)
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index effcd3d..5cf7ddb 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -869,22 +869,6 @@ void usbhid_set_leds(struct hid_device *hid)
 }
 EXPORT_SYMBOL_GPL(usbhid_set_leds);
 
-/*
- * Traverse the supplied list of reports and find the longest
- */
-static void hid_find_max_report(struct hid_device *hid, unsigned int type,
-		unsigned int *max)
-{
-	struct hid_report *report;
-	unsigned int size;
-
-	list_for_each_entry(report, &hid->report_enum[type].report_list, list) {
-		size = ((report->size - 1) >> 3) + 1 + hid->report_enum[type].numbered;
-		if (*max < size)
-			*max = size;
-	}
-}
-
 static int hid_alloc_buffers(struct usb_device *dev, struct hid_device *hid)
 {
 	struct usbhid_device *usbhid = hid->driver_data;
diff --git a/include/linux/hid.h b/include/linux/hid.h
index b7b5ff2..9b6c71c 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -736,6 +736,8 @@ extern void hidinput_report_event(struct hid_device *hid, struct hid_report *rep
 extern int hidinput_connect(struct hid_device *hid, unsigned int force);
 extern void hidinput_disconnect(struct hid_device *);
 
+int hid_get_report_length(struct hid_report *);
+void hid_find_max_report(struct hid_device *, unsigned int, unsigned int *);
 int hid_set_field(struct hid_field *, unsigned, __s32);
 int hid_input_report(struct hid_device *, int type, u8 *, int, int);
 int hidinput_find_field(struct hid_device *hid, unsigned int type, unsigned int code, struct hid_field **field);
-- 
1.8.1.4

[PATCH 3/4] HID: debug: empty output queue on read

[Benjamin Tissoires]

If an event occurs while the hid debugfs is forwarding events, list->tail
is updated during copy_to_user().

Remove the gotos and use a regular while-loop to empty the queue.

Second benefit, it checks that we are not writing more than count bytes
to the user-space output buffer.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-debug.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index 094cbcf..1dc8104 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -1000,6 +1000,7 @@ static ssize_t hid_debug_events_read(struct file *file, char __user *buffer,
 		size_t count, loff_t *ppos)
 {
 	struct hid_debug_list *list = file->private_data;
+	char *buf_head;
 	int ret = 0, len;
 	DECLARE_WAITQUEUE(wait, current);
 
@@ -1039,28 +1040,22 @@ static ssize_t hid_debug_events_read(struct file *file, char __user *buffer,
 			goto out;
 
 		/* pass the ringbuffer contents to userspace */
-copy_rest:
-		if (list->tail == list->head)
-			goto out;
-		if (list->tail > list->head) {
-			len = list->tail - list->head;
+		while (list->tail != list->head && ret < count) {
+			buf_head = &list->hid_debug_buf[list->head];
 
-			if (copy_to_user(buffer + ret, &list->hid_debug_buf[list->head], len)) {
-				ret = -EFAULT;
-				goto out;
-			}
-			ret += len;
-			list->head += len;
-		} else {
-			len = HID_DEBUG_BUFSIZE - list->head;
+			if (list->tail > list->head)
+				len = list->tail - list->head;
+			else
+				len = HID_DEBUG_BUFSIZE - list->head;
+
+			len = min(count - ret, len);
 
-			if (copy_to_user(buffer, &list->hid_debug_buf[list->head], len)) {
+			if (copy_to_user(buffer + ret, buf_head, len)) {
 				ret = -EFAULT;
 				goto out;
 			}
-			list->head = 0;
 			ret += len;
-			goto copy_rest;
+			list->head = (list->head + len) % HID_DEBUG_BUFSIZE;
 		}
 
 	}
-- 
1.8.1.4

[PATCH 4/4] HID: debug: allocate the output buffer with an estimate

[Benjamin Tissoires]

Many multitouch device reports a lot of usage in their reports.
The size of the report can be quite big, and as we are dumping both
the report and the parsing in plain text format, the chosen size of
512 is much of the time not big enough.

For instance, one Elan 04f3:0732 gives a report of size 116 and an usage
count of 92. The result is that the ring buffer is not big enough to
contain the whole output, giving a partial debug information.

This estimate gives:
- 512 for a regular keyboard
- 524 for a regular mouse
- 2648 for Elan 04f3:0732

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Conflicts:
	drivers/hid/hid-debug.c
---
 drivers/hid/hid-core.c    | 30 ++++++++++++++++++++++++++++++
 drivers/hid/hid-debug.c   | 36 ++++++++++++++++++++++++++++++------
 include/linux/hid-debug.h |  1 +
 include/linux/hid.h       |  3 +++
 4 files changed, 64 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 25d7903..3569ce8 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1174,6 +1174,36 @@ void hid_find_max_report(struct hid_device *hid, unsigned int type,
 EXPORT_SYMBOL_GPL(hid_find_max_report);
 
 /*
+ * Return the count of different usages in a report
+ */
+int hid_get_report_count(struct hid_report *report)
+{
+	int n = 0;
+	int i;
+	for (i = 0; i < report->maxfield; i++)
+		n += report->field[i]->report_count;
+	return n;
+}
+EXPORT_SYMBOL_GPL(hid_get_report_count);
+
+/*
+ * Traverse the supplied list of reports and find the longest usage count
+ */
+void hid_find_max_report_count(struct hid_device *hid, unsigned int type,
+		unsigned int *max)
+{
+	struct hid_report *report;
+	unsigned int size;
+
+	list_for_each_entry(report, &hid->report_enum[type].report_list, list) {
+		size = hid_get_report_count(report);
+		if (*max < size)
+			*max = size;
+	}
+}
+EXPORT_SYMBOL_GPL(hid_find_max_report_count);
+
+/*
  * Set a field value. The report this field belongs to has to be
  * created and transferred to the device, to set this value in the
  * device.
diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index 1dc8104..cc2e81e 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -582,9 +582,9 @@ void hid_debug_event(struct hid_device *hdev, char *buf)
 
 	list_for_each_entry(list, &hdev->debug_list, node) {
 		for (i = 0; i < strlen(buf); i++)
-			list->hid_debug_buf[(list->tail + i) % HID_DEBUG_BUFSIZE] =
+			list->hid_debug_buf[(list->tail + i) % list->size] =
 				buf[i];
-		list->tail = (list->tail + i) % HID_DEBUG_BUFSIZE;
+		list->tail = (list->tail + i) % list->size;
         }
 
 	wake_up_interruptible(&hdev->debug_wait);
@@ -971,6 +971,25 @@ static int hid_debug_rdesc_open(struct inode *inode, struct file *file)
 	return single_open(file, hid_debug_rdesc_show, inode->i_private);
 }
 
+static int hid_debug_estimate_buffer_size(struct hid_device *hdev)
+{
+	int max_report_size = 0;
+	int max_report_count = 0;
+	int estimate;
+
+	hid_find_max_report(hdev, HID_INPUT_REPORT, &max_report_size);
+	hid_find_max_report_count(hdev, HID_INPUT_REPORT, &max_report_count);
+
+	/*
+	 * We need enough space to:
+	 * - dump the report (max_report_size * 3)
+	 * - dump each usage in a human reading style. 25 columns seem enough.
+	 */
+	estimate = max_report_size * 3 + max_report_count * 25;
+
+	return max(estimate, HID_DEBUG_BUFSIZE);
+}
+
 static int hid_debug_events_open(struct inode *inode, struct file *file)
 {
 	int err = 0;
@@ -981,12 +1000,17 @@ static int hid_debug_events_open(struct inode *inode, struct file *file)
 		goto out;
 	}
 
-	if (!(list->hid_debug_buf = kzalloc(sizeof(char) * HID_DEBUG_BUFSIZE, GFP_KERNEL))) {
+	list->hdev = (struct hid_device *) inode->i_private;
+	list->size = hid_debug_estimate_buffer_size(list->hdev);
+
+	list->hid_debug_buf = kzalloc(sizeof(char) * list->size, GFP_KERNEL);
+
+	if (!list->hid_debug_buf) {
 		err = -ENOMEM;
 		kfree(list);
 		goto out;
 	}
-	list->hdev = (struct hid_device *) inode->i_private;
+
 	file->private_data = list;
 	mutex_init(&list->read_mutex);
 
@@ -1046,7 +1070,7 @@ static ssize_t hid_debug_events_read(struct file *file, char __user *buffer,
 			if (list->tail > list->head)
 				len = list->tail - list->head;
 			else
-				len = HID_DEBUG_BUFSIZE - list->head;
+				len = list->size - list->head;
 
 			len = min(count - ret, len);
 
@@ -1055,7 +1079,7 @@ static ssize_t hid_debug_events_read(struct file *file, char __user *buffer,
 				goto out;
 			}
 			ret += len;
-			list->head = (list->head + len) % HID_DEBUG_BUFSIZE;
+			list->head = (list->head + len) % list->size;
 		}
 
 	}
diff --git a/include/linux/hid-debug.h b/include/linux/hid-debug.h
index 8663f21..404d5e1 100644
--- a/include/linux/hid-debug.h
+++ b/include/linux/hid-debug.h
@@ -42,6 +42,7 @@ struct hid_debug_list {
 	char *hid_debug_buf;
 	int head;
 	int tail;
+	int size;
 	struct fasync_struct *fasync;
 	struct hid_device *hdev;
 	struct list_head node;
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 9b6c71c..cd75732 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -738,6 +738,9 @@ extern void hidinput_disconnect(struct hid_device *);
 
 int hid_get_report_length(struct hid_report *);
 void hid_find_max_report(struct hid_device *, unsigned int, unsigned int *);
+int hid_get_report_count(struct hid_report *);
+void hid_find_max_report_count(struct hid_device *, unsigned int,
+	unsigned int *);
 int hid_set_field(struct hid_field *, unsigned, __s32);
 int hid_input_report(struct hid_device *, int type, u8 *, int, int);
 int hidinput_find_field(struct hid_device *hid, unsigned int type, unsigned int code, struct hid_field **field);
-- 
1.8.1.4

Re: [PATCH 0/4] HID: debugfs rework

[Jiri Kosina]

On Wed, 17 Apr 2013, Benjamin Tissoires wrote:

> Hi Jiri,
> 
> This is a small rework of the HID debugfs.
> I encountered a problem with multitouch devices: they have too much usages to
> fit into the fixed size output buffer of 512.
> So I digg a little, and end up with those 4 patches.

Hi Benjamin,

thanks, I will look into it and see whether I would be able to apply it 
still for 3.10 merge window.

I also have a locking fix for HID-debugfs which I am going to apply 
shortly, but I am travelling this week, so I am in a bit degraded mode.

For reference, locking fix below.



From: Jiri Kosina <jkosina@suse.cz>
Subject: [PATCH] HID: protect hid_debug_list

Accesses to hid_device->hid_debug_list are not serialized properly, which
could result in SMP concurrency issues when HID debugfs events are accessesed
by multiple userspace processess.

Serialize all the list operations by a mutex.

Reported-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
 drivers/hid/hid-core.c  | 1 +
 drivers/hid/hid-debug.c | 6 ++++++
 include/linux/hid.h     | 1 +
 3 files changed, 8 insertions(+)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 1129f49..e3b7123 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -2347,6 +2347,7 @@ struct hid_device *hid_allocate_device(void)
 
 	init_waitqueue_head(&hdev->debug_wait);
 	INIT_LIST_HEAD(&hdev->debug_list);
+	mutex_init(&hdev->debug_list_lock);
 	sema_init(&hdev->driver_lock, 1);
 	sema_init(&hdev->driver_input_lock, 1);
 
diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index 933fff0..3337261 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -580,12 +580,14 @@ void hid_debug_event(struct hid_device *hdev, char *buf)
 	int i;
 	struct hid_debug_list *list;
 
+	mutex_lock(&hdev->debug_list_lock);
 	list_for_each_entry(list, &hdev->debug_list, node) {
 		for (i = 0; i < strlen(buf); i++)
 			list->hid_debug_buf[(list->tail + i) % HID_DEBUG_BUFSIZE] =
 				buf[i];
 		list->tail = (list->tail + i) % HID_DEBUG_BUFSIZE;
         }
+	mutex_unlock(&hdev->debug_list_lock);
 
 	wake_up_interruptible(&hdev->debug_wait);
 }
@@ -960,7 +962,9 @@ static int hid_debug_events_open(struct inode *inode, struct file *file)
 	file->private_data = list;
 	mutex_init(&list->read_mutex);
 
+	mutex_lock(&list->hdev->debug_list_lock);
 	list_add_tail(&list->node, &list->hdev->debug_list);
+	mutex_unlock(&list->hdev->debug_list_lock);
 
 out:
 	return err;
@@ -1055,7 +1059,9 @@ static int hid_debug_events_release(struct inode *inode, struct file *file)
 {
 	struct hid_debug_list *list = file->private_data;
 
+	mutex_lock(&list->hdev->debug_list_lock);
 	list_del(&list->node);
+	mutex_unlock(&list->hdev->debug_list_lock);
 	kfree(list->hid_debug_buf);
 	kfree(list);
 
diff --git a/include/linux/hid.h b/include/linux/hid.h
index b7b5ff2..af1b86d 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -515,6 +515,7 @@ struct hid_device {							/* device report descriptor */
 	struct dentry *debug_rdesc;
 	struct dentry *debug_events;
 	struct list_head debug_list;
+	struct mutex debug_list_lock;
 	wait_queue_head_t debug_wait;
 };
 

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 0/4] HID: debugfs rework

[Benjamin Tissoires]

On 04/17/2013 07:44 PM, Jiri Kosina wrote:
> On Wed, 17 Apr 2013, Benjamin Tissoires wrote:
> 
>> Hi Jiri,
>>
>> This is a small rework of the HID debugfs.
>> I encountered a problem with multitouch devices: they have too much usages to
>> fit into the fixed size output buffer of 512.
>> So I digg a little, and end up with those 4 patches.
> 
> Hi Benjamin,
> 
> thanks, I will look into it and see whether I would be able to apply it 
> still for 3.10 merge window.

Thanks. I think patches 1 and 2 of this series are pretty straightforward.
Patches 3 and 4 will maybe require a little bit more attention.

I don't mind if it's postponed to 3.11 (given the long time this has
been broken for devices with big reports).
I just need to access HID debugfs for hid-replay when hidraw does not
send anything. But as I'm also willing to use hid-replay with current
kernels, I still need a way to use the best option (hidraw or hid
debugfs) for current kernels.

> 
> I also have a locking fix for HID-debugfs which I am going to apply 
> shortly, but I am travelling this week, so I am in a bit degraded mode.
> 
> For reference, locking fix below.
> 
> 
> 
> From: Jiri Kosina <jkosina@suse.cz>
> Subject: [PATCH] HID: protect hid_debug_list
> 
> Accesses to hid_device->hid_debug_list are not serialized properly, which
> could result in SMP concurrency issues when HID debugfs events are accessesed

s/accessesed/accessed ?

> by multiple userspace processess.

s/processess/processes ?

> 
> Serialize all the list operations by a mutex.
> 
> Reported-by: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Jiri Kosina <jkosina@suse.cz>

Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

I also have a patch regarding forcing hidraw output even if raw_event
returns > 0, but I'll send it over a new thread. This thread will start
to be quite complicate to follow otherwise... :)

Cheers,
Benjamin

Re: [PATCH 3/4] HID: debug: empty output queue on read

[Jiri Kosina]

On Wed, 17 Apr 2013, Benjamin Tissoires wrote:

> If an event occurs while the hid debugfs is forwarding events, list->tail
> is updated during copy_to_user().
> 
> Remove the gotos and use a regular while-loop to empty the queue.
> 
> Second benefit, it checks that we are not writing more than count bytes
> to the user-space output buffer.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  drivers/hid/hid-debug.c | 27 +++++++++++----------------
>  1 file changed, 11 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
> index 094cbcf..1dc8104 100644
> --- a/drivers/hid/hid-debug.c
> +++ b/drivers/hid/hid-debug.c
> @@ -1000,6 +1000,7 @@ static ssize_t hid_debug_events_read(struct file *file, char __user *buffer,
>  		size_t count, loff_t *ppos)
>  {
>  	struct hid_debug_list *list = file->private_data;
> +	char *buf_head;
>  	int ret = 0, len;
>  	DECLARE_WAITQUEUE(wait, current);
>  
> @@ -1039,28 +1040,22 @@ static ssize_t hid_debug_events_read(struct file *file, char __user *buffer,
>  			goto out;
>  
>  		/* pass the ringbuffer contents to userspace */
> -copy_rest:
> -		if (list->tail == list->head)
> -			goto out;
> -		if (list->tail > list->head) {
> -			len = list->tail - list->head;
> +		while (list->tail != list->head && ret < count) {
> +			buf_head = &list->hid_debug_buf[list->head];
>  
> -			if (copy_to_user(buffer + ret, &list->hid_debug_buf[list->head], len)) {
> -				ret = -EFAULT;
> -				goto out;
> -			}
> -			ret += len;
> -			list->head += len;
> -		} else {
> -			len = HID_DEBUG_BUFSIZE - list->head;
> +			if (list->tail > list->head)
> +				len = list->tail - list->head;
> +			else
> +				len = HID_DEBUG_BUFSIZE - list->head;
> +
> +			len = min(count - ret, len);

This triggers a type checking warning in min(), and it seems to be correct 
on this one (mixing size_t with signed int).

drivers/hid/hid-debug.c:1079: warning: comparison of distinct pointer types lacks a cast

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 1/4] HID: debug: break out hid_dump_report() into hid-debug

[Jiri Kosina]

On Wed, 17 Apr 2013, Benjamin Tissoires wrote:

> No semantic changes, but hid_dump_report should be in hid-debug.c, not
> in hid-core.c
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

I have applied this one independently on the rest.

-- 
Jiri Kosina
SUSE Labs