Re: [PATCH v2] i2c-hid: introduce HID over i2c specification implementation

On Tue, 16 Oct 2012, Benjamin Tissoires wrote:

quoted

quoted

--- /dev/null
+++ b/drivers/hid/i2c-hid/i2c-hid.c

...

quoted

+static int i2c_hid_alloc_buffers(struct i2c_hid *ihid)
+{
+     /* the worst case is computed from the set_report command with a
+      * reportID > 15 and the maximum report length */
+     int args_len = sizeof(__u8) + /* optional ReportID byte */
+                    sizeof(__u16) + /* data register */
+                    sizeof(__u16) + /* size of the report */
+                    ihid->bufsize; /* report */
+
+     ihid->inbuf = kzalloc(ihid->bufsize, GFP_KERNEL);
+
+     if (!ihid->inbuf)
+             return -ENOMEM;
+
+     ihid->argsbuf = kzalloc(args_len, GFP_KERNEL);
+
+     if (!ihid->argsbuf) {
+             kfree(ihid->inbuf);
+             return -ENOMEM;
+     }
+
+     ihid->cmdbuf = kzalloc(sizeof(union command) + args_len, GFP_KERNEL);
+
+     if (!ihid->cmdbuf) {
+             kfree(ihid->inbuf);
+             kfree(ihid->argsbuf);
+             return -ENOMEM;
+     }
+
+     return 0;

If this is called from hid_start and some of the latter allocation fails
here, you free the buffers appropriately. However if another start
occurs (e.g. by loading another module for that particular device), it
will crash, as the buffers will remain unallocated because at this point
ihid->bufsize == old_bufsize. You should set ihid->bufsize back to
old_bufsize if i2c_hid_alloc_buffers fails and also set the pointers to
NULL here.

well spotted, thanks.
I'll change it in v3 then.

Benjamin,

just a quick check -- are you planning on submitting v3 eventually?

Thanks,

-- 
Jiri Kosina
SUSE Labs