Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752074Ab2KIJ7g (ORCPT ); Fri, 9 Nov 2012 04:59:36 -0500 Received: from mail-lb0-f174.google.com ([209.85.217.174]:61233 "EHLO mail-lb0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751073Ab2KIJ7e (ORCPT ); Fri, 9 Nov 2012 04:59:34 -0500 MIME-Version: 1.0 In-Reply-To: References: <1350333533-13366-1-git-send-email-benjamin.tissoires@gmail.com> <507D182E.9010902@suse.cz> Date: Fri, 9 Nov 2012 04:59:32 -0500 Message-ID: Subject: Re: [PATCH v2] i2c-hid: introduce HID over i2c specification implementation From: Benjamin Tissoires To: Jiri Kosina Cc: Jiri Slaby , Dmitry Torokhov , Stephane Chatty , fabien.andre@gmail.com, scott.liu@emc.com.tw, Jean Delvare , JJ Ding , Shubhrajyoti Datta , linux-i2c@vger.kernel.org, linux-input@vger.kernel.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2547 Lines: 79 Hi Jiri, On Fri, Nov 9, 2012 at 4:49 AM, Jiri Kosina wrote: > On Tue, 16 Oct 2012, Benjamin Tissoires wrote: > >> >> --- /dev/null >> >> +++ b/drivers/hid/i2c-hid/i2c-hid.c >> > ... >> >> +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? yes sure. It was not on my top priority list since I started at Red Hat. Moreover, I've been reported a bug yesterday with the set_report command. I'll need to do a few more tests before sending the v3. If I send it by the end of the day or at the beginning of next week, will it have a chance to land into 3.8? Cheers, Benjamin > > Thanks, > > -- > Jiri Kosina > SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/