[PATCH] hid: avoid '\0' in hid debugfs events file

[PATCH] hid: avoid '\0' in hid debugfs events file

[Bruno Prémont]

When dumping /sys/kernel/debug/hid/$device/events '\0' characters show up
(invisible if cat to console but shown by less or while looking at a dump
file).
These are due to hid_debug_event() adding strlen()+1 bytes to the ring
buffer (e.g. including the trailing '\0').
Any roll-over causes a '\0' as well as hid_debug_event() handles the ring
buffers with HID_DEBUG_BUFSIZE-1 size while hid_debug_events_read() handles
it with full HID_DEBUG_BUFSIZE size.

Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
---
Note:
The ring buffer overflow case (when tail crosses head) seems to be
suboptimal at best.
Would there be a good way to mark those cases so reader can know where
data got lost. (though this might not be easy keeping the lockless
design)

diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index 6abd036..2602695 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -564,10 +564,10 @@ void hid_debug_event(struct hid_device *hdev, char *buf)
 	struct hid_debug_list *list;
 
 	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 - 1)] =
+		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 - 1);
+		list->tail = (list->tail + i) % HID_DEBUG_BUFSIZE;
         }
 }
 EXPORT_SYMBOL_GPL(hid_debug_event);
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] hid: avoid '\0' in hid debugfs events file

[Jiri Kosina]

On Mon, 15 Mar 2010, Bruno Prémont wrote:

> When dumping /sys/kernel/debug/hid/$device/events '\0' characters show up
> (invisible if cat to console but shown by less or while looking at a dump
> file).
> These are due to hid_debug_event() adding strlen()+1 bytes to the ring
> buffer (e.g. including the trailing '\0').
> Any roll-over causes a '\0' as well as hid_debug_event() handles the ring
> buffers with HID_DEBUG_BUFSIZE-1 size while hid_debug_events_read() handles
> it with full HID_DEBUG_BUFSIZE size.

Applied, thanks.

> Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
> ---
> Note:
> The ring buffer overflow case (when tail crosses head) seems to be
> suboptimal at best.
> Would there be a good way to mark those cases so reader can know where
> data got lost. (though this might not be easy keeping the lockless
> design)

I agree that there might be better implementation. The circular buffer is 
merely the same to what we use in other userspace interfaces already (see 
original hiddev, for example). Plus this is only debugging interface, and 
with SIZE being 512, it's very unlikely that events will get lost.

But I don't think the lockless design is that important here, so if you 
are motivated to rewrite it to something better, I wouldn't object.

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] hid: avoid '\0' in hid debugfs events file

[Bruno Prémont]

On Tue, 16 March 2010 Jiri Kosina <jkosina@suse.cz> wrote:
> On Mon, 15 Mar 2010, Bruno Prémont wrote:
> > The ring buffer overflow case (when tail crosses head) seems to be
> > suboptimal at best.
> > Would there be a good way to mark those cases so reader can know where
> > data got lost. (though this might not be easy keeping the lockless
> > design)
> 
> I agree that there might be better implementation. The circular buffer is 
> merely the same to what we use in other userspace interfaces already (see 
> original hiddev, for example). Plus this is only debugging interface, and 
> with SIZE being 512, it's very unlikely that events will get lost.

Well, 512 is not that large, considering that a single event may well
fill half of it (e.g. 64byte HID event * 3 for hex dump, adding decoded
event to it). For basic keyboard events it's definitely sufficient.

In my case I was seeing overflow with outbound reports I dumped there
too, where it's pretty common to have more than a single report sent
back to device in a row.
Sure it's just an aid for debugging so loosing data is not an issue,
just having a hard time to understand output when things got lost would
be nice to avoid.

> But I don't think the lockless design is that important here, so if you 
> are motivated to rewrite it to something better, I wouldn't object.

The easy way to keep output consistent would be to stop appending new
data when the ring buffer is full. (and maybe adding a single '\0' or
equivalent just before head if more data has to be written than space is
available so there is an overflow marker).
This way reader would not risk reading old data a second time.

Dropping old data looks more complicated to get race-free or it would
have to be a bigger change (with new locking).


The optimal case would be to have a single ring-buffer implementation
for use by anyone needing it (I don't know if such one exists nor
how many ring-buffers are being used around the kernel)...
This is for a later time when I'm happy with my PicoLCD driver!

Bruno
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html