Message ID | 1397737700-1081-1-git-send-email-ezequiel.garcia@free-electrons.com (mailing list archive) |
---|---|
State | Accepted, archived |
Delegated to: | Hans Verkuil |
Headers |
Received: from mail.tu-berlin.de ([130.149.7.33]) by www.linuxtv.org with esmtp (Exim 4.72) (envelope-from <linux-media-owner@vger.kernel.org>) id 1WalS4-0001LW-UP; Thu, 17 Apr 2014 14:30:00 +0200 X-tubIT-Incoming-IP: 209.132.180.67 Received: from vger.kernel.org ([209.132.180.67]) by mail.tu-berlin.de (exim-4.72/mailfrontend-8) with esmtp id 1WalS1-0005UX-le; Thu, 17 Apr 2014 14:30:00 +0200 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753639AbaDQM3X (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Thu, 17 Apr 2014 08:29:23 -0400 Received: from top.free-electrons.com ([176.31.233.9]:55479 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751335AbaDQM3W (ORCPT <rfc822;linux-media@vger.kernel.org>); Thu, 17 Apr 2014 08:29:22 -0400 Received: by mail.free-electrons.com (Postfix, from userid 106) id 1BFEE7D9; Thu, 17 Apr 2014 14:29:23 +0200 (CEST) X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on mail.free-electrons.com X-Spam-Level: X-Spam-Status: No, score=-1.0 required=5.0 tests=ALL_TRUSTED,SHORTCIRCUIT shortcircuit=ham autolearn=disabled version=3.3.2 Received: from localhost.localdomain (unknown [190.2.108.30]) by mail.free-electrons.com (Postfix) with ESMTPSA id 6932E669; Thu, 17 Apr 2014 14:29:21 +0200 (CEST) From: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> To: <linux-media@vger.kernel.org> Cc: Hans Verkuil <hverkuil@xs4all.nl>, Ezequiel Garcia <ezequiel.garcia@free-electrons.com>, Alan Stern <stern@rowland.harvard.edu> Subject: [PATCH v2] media: stk1160: Avoid stack-allocated buffer for control URBs Date: Thu, 17 Apr 2014 09:28:20 -0300 Message-Id: <1397737700-1081-1-git-send-email-ezequiel.garcia@free-electrons.com> X-Mailer: git-send-email 1.9.1 Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-PMX-Version: 6.0.0.2142326, Antispam-Engine: 2.7.2.2107409, Antispam-Data: 2014.4.17.122119 X-PMX-Spam: Gauge=IIIIIIII, Probability=8%, Report=' MULTIPLE_RCPTS 0.1, HTML_00_01 0.05, HTML_00_10 0.05, BODYTEXTP_SIZE_3000_LESS 0, BODY_SIZE_2000_2999 0, BODY_SIZE_5000_LESS 0, BODY_SIZE_7000_LESS 0, URI_ENDS_IN_HTML 0, __ANY_URI 0, __CP_URI_IN_BODY 0, __HAS_FROM 0, __HAS_MSGID 0, __HAS_X_MAILER 0, __HAS_X_MAILING_LIST 0, __MIME_TEXT_ONLY 0, __MULTIPLE_RCPTS_CC_X2 0, __SANE_MSGID 0, __SUBJ_ALPHA_END 0, __TO_MALFORMED_2 0, __TO_NO_NAME 0, __URI_NO_WWW 0, __URI_NS ' |
Commit Message
Ezequiel Garcia
April 17, 2014, 12:28 p.m. UTC
Currently stk1160_read_reg() uses a stack-allocated char to get the
read control value. This is wrong because usb_control_msg() requires
a kmalloc-ed buffer.
This commit fixes such issue by kmalloc'ating a 1-byte buffer to receive
the read value.
While here, let's remove the urb_buf array which was meant for a similar
purpose, but never really used.
Cc: Alan Stern <stern@rowland.harvard.edu>
Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
drivers/media/usb/stk1160/stk1160-core.c | 10 +++++++++-
drivers/media/usb/stk1160/stk1160.h | 1 -
2 files changed, 9 insertions(+), 2 deletions(-)
Comments
On Apr 17, Ezequiel Garcia wrote: > Currently stk1160_read_reg() uses a stack-allocated char to get the > read control value. This is wrong because usb_control_msg() requires > a kmalloc-ed buffer. > > This commit fixes such issue by kmalloc'ating a 1-byte buffer to receive > the read value. > > While here, let's remove the urb_buf array which was meant for a similar > purpose, but never really used. > > Cc: Alan Stern <stern@rowland.harvard.edu> > Reported-by: Sander Eikelenboom <linux@eikelenboom.it> Ouch, I forgot to Cc Sander! Sander, Here's the patch: https://patchwork.linuxtv.org/patch/23660/ Maybe you can give it a ride and confirm it fixes the warning over there? Also, have you observed any serious issues caused by this or just the DMA API debug warning? > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> > --- > drivers/media/usb/stk1160/stk1160-core.c | 10 +++++++++- > drivers/media/usb/stk1160/stk1160.h | 1 - > 2 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/usb/stk1160/stk1160-core.c b/drivers/media/usb/stk1160/stk1160-core.c > index 34a26e0..03504dc 100644 > --- a/drivers/media/usb/stk1160/stk1160-core.c > +++ b/drivers/media/usb/stk1160/stk1160-core.c > @@ -67,17 +67,25 @@ int stk1160_read_reg(struct stk1160 *dev, u16 reg, u8 *value) > { > int ret; > int pipe = usb_rcvctrlpipe(dev->udev, 0); > + u8 *buf; > > *value = 0; > + > + buf = kmalloc(sizeof(u8), GFP_KERNEL); > + if (!buf) > + return -ENOMEM; > ret = usb_control_msg(dev->udev, pipe, 0x00, > USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE, > - 0x00, reg, value, sizeof(u8), HZ); > + 0x00, reg, buf, sizeof(u8), HZ); > if (ret < 0) { > stk1160_err("read failed on reg 0x%x (%d)\n", > reg, ret); > + kfree(buf); > return ret; > } > > + *value = *buf; > + kfree(buf); > return 0; > } > > diff --git a/drivers/media/usb/stk1160/stk1160.h b/drivers/media/usb/stk1160/stk1160.h > index 05b05b1..abdea48 100644 > --- a/drivers/media/usb/stk1160/stk1160.h > +++ b/drivers/media/usb/stk1160/stk1160.h > @@ -143,7 +143,6 @@ struct stk1160 { > int num_alt; > > struct stk1160_isoc_ctl isoc_ctl; > - char urb_buf[255]; /* urb control msg buffer */ > > /* frame properties */ > int width; /* current frame width */ > -- > 1.9.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Friday, April 25, 2014, 11:51:53 PM, you wrote: > On Apr 17, Ezequiel Garcia wrote: >> Currently stk1160_read_reg() uses a stack-allocated char to get the >> read control value. This is wrong because usb_control_msg() requires >> a kmalloc-ed buffer. >> >> This commit fixes such issue by kmalloc'ating a 1-byte buffer to receive >> the read value. >> >> While here, let's remove the urb_buf array which was meant for a similar >> purpose, but never really used. >> >> Cc: Alan Stern <stern@rowland.harvard.edu> >> Reported-by: Sander Eikelenboom <linux@eikelenboom.it> > Ouch, I forgot to Cc Sander! > Sander, Here's the patch: > https://patchwork.linuxtv.org/patch/23660/ > Maybe you can give it a ride and confirm it fixes the warning over there? > Also, have you observed any serious issues caused by this or just the > DMA API debug warning? Hi Ezequiel, Just tested with your v2 patch for a while and haven't seen the warning again :-) I don't remember for certain if it gave any serious issues .. since i have been running with you v1 patch now for a while and it's on a test machine i use infrequently. -- Sander >> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> >> --- >> drivers/media/usb/stk1160/stk1160-core.c | 10 +++++++++- >> drivers/media/usb/stk1160/stk1160.h | 1 - >> 2 files changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/media/usb/stk1160/stk1160-core.c b/drivers/media/usb/stk1160/stk1160-core.c >> index 34a26e0..03504dc 100644 >> --- a/drivers/media/usb/stk1160/stk1160-core.c >> +++ b/drivers/media/usb/stk1160/stk1160-core.c >> @@ -67,17 +67,25 @@ int stk1160_read_reg(struct stk1160 *dev, u16 reg, u8 *value) >> { >> int ret; >> int pipe = usb_rcvctrlpipe(dev->udev, 0); >> + u8 *buf; >> >> *value = 0; >> + >> + buf = kmalloc(sizeof(u8), GFP_KERNEL); >> + if (!buf) >> + return -ENOMEM; >> ret = usb_control_msg(dev->udev, pipe, 0x00, >> USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE, >> - 0x00, reg, value, sizeof(u8), HZ); >> + 0x00, reg, buf, sizeof(u8), HZ); >> if (ret < 0) { >> stk1160_err("read failed on reg 0x%x (%d)\n", >> reg, ret); >> + kfree(buf); >> return ret; >> } >> >> + *value = *buf; >> + kfree(buf); >> return 0; >> } >> >> diff --git a/drivers/media/usb/stk1160/stk1160.h b/drivers/media/usb/stk1160/stk1160.h >> index 05b05b1..abdea48 100644 >> --- a/drivers/media/usb/stk1160/stk1160.h >> +++ b/drivers/media/usb/stk1160/stk1160.h >> @@ -143,7 +143,6 @@ struct stk1160 { >> int num_alt; >> >> struct stk1160_isoc_ctl isoc_ctl; >> - char urb_buf[255]; /* urb control msg buffer */ >> >> /* frame properties */ >> int width; /* current frame width */ >> -- >> 1.9.1 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-media" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Ezequiel, On 04/17/2014 02:28 PM, Ezequiel Garcia wrote: > Currently stk1160_read_reg() uses a stack-allocated char to get the > read control value. This is wrong because usb_control_msg() requires > a kmalloc-ed buffer. > > This commit fixes such issue by kmalloc'ating a 1-byte buffer to receive > the read value. > > While here, let's remove the urb_buf array which was meant for a similar > purpose, but never really used. Rather than allocating and freeing a buffer for every read_reg I would allocate this buffer in the probe function. That way this allocation is done only once. Regards, Hans > > Cc: Alan Stern <stern@rowland.harvard.edu> > Reported-by: Sander Eikelenboom <linux@eikelenboom.it> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> > --- > drivers/media/usb/stk1160/stk1160-core.c | 10 +++++++++- > drivers/media/usb/stk1160/stk1160.h | 1 - > 2 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/usb/stk1160/stk1160-core.c b/drivers/media/usb/stk1160/stk1160-core.c > index 34a26e0..03504dc 100644 > --- a/drivers/media/usb/stk1160/stk1160-core.c > +++ b/drivers/media/usb/stk1160/stk1160-core.c > @@ -67,17 +67,25 @@ int stk1160_read_reg(struct stk1160 *dev, u16 reg, u8 *value) > { > int ret; > int pipe = usb_rcvctrlpipe(dev->udev, 0); > + u8 *buf; > > *value = 0; > + > + buf = kmalloc(sizeof(u8), GFP_KERNEL); > + if (!buf) > + return -ENOMEM; > ret = usb_control_msg(dev->udev, pipe, 0x00, > USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE, > - 0x00, reg, value, sizeof(u8), HZ); > + 0x00, reg, buf, sizeof(u8), HZ); > if (ret < 0) { > stk1160_err("read failed on reg 0x%x (%d)\n", > reg, ret); > + kfree(buf); > return ret; > } > > + *value = *buf; > + kfree(buf); > return 0; > } > > diff --git a/drivers/media/usb/stk1160/stk1160.h b/drivers/media/usb/stk1160/stk1160.h > index 05b05b1..abdea48 100644 > --- a/drivers/media/usb/stk1160/stk1160.h > +++ b/drivers/media/usb/stk1160/stk1160.h > @@ -143,7 +143,6 @@ struct stk1160 { > int num_alt; > > struct stk1160_isoc_ctl isoc_ctl; > - char urb_buf[255]; /* urb control msg buffer */ > > /* frame properties */ > int width; /* current frame width */ > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09 May 12:34 PM, Hans Verkuil wrote: > Hi Ezequiel, > > On 04/17/2014 02:28 PM, Ezequiel Garcia wrote: > > Currently stk1160_read_reg() uses a stack-allocated char to get the > > read control value. This is wrong because usb_control_msg() requires > > a kmalloc-ed buffer. > > > > This commit fixes such issue by kmalloc'ating a 1-byte buffer to receive > > the read value. > > > > While here, let's remove the urb_buf array which was meant for a similar > > purpose, but never really used. > > Rather than allocating and freeing a buffer for every read_reg I would allocate > this buffer in the probe function. > > That way this allocation is done only once. > I get your point. I just thought that since the control URBs are only used for changing the configuration parameters, and this path is scarcely taken, it wasn't a big deal to allocate it each time.
Hi Hans, On 09 May 12:34 PM, Hans Verkuil wrote: > On 04/17/2014 02:28 PM, Ezequiel Garcia wrote: > > Currently stk1160_read_reg() uses a stack-allocated char to get the > > read control value. This is wrong because usb_control_msg() requires > > a kmalloc-ed buffer. > > > > This commit fixes such issue by kmalloc'ating a 1-byte buffer to receive > > the read value. > > > > While here, let's remove the urb_buf array which was meant for a similar > > purpose, but never really used. > > Rather than allocating and freeing a buffer for every read_reg I would allocate > this buffer in the probe function. > > That way this allocation is done only once. > Hm... sorry for being so stubborn, but I've just noticed that having a shared buffer would require adding a spinlock to protect it, where the current proposal doesn't need it. Do you still think that's the right thing to do? Thanks!
On 05/17/2014 02:21 PM, Ezequiel Garcia wrote: > Hi Hans, > > On 09 May 12:34 PM, Hans Verkuil wrote: >> On 04/17/2014 02:28 PM, Ezequiel Garcia wrote: >>> Currently stk1160_read_reg() uses a stack-allocated char to get the >>> read control value. This is wrong because usb_control_msg() requires >>> a kmalloc-ed buffer. >>> >>> This commit fixes such issue by kmalloc'ating a 1-byte buffer to receive >>> the read value. >>> >>> While here, let's remove the urb_buf array which was meant for a similar >>> purpose, but never really used. >> >> Rather than allocating and freeing a buffer for every read_reg I would allocate >> this buffer in the probe function. >> >> That way this allocation is done only once. >> > > Hm... sorry for being so stubborn, but I've just noticed that having a > shared buffer would require adding a spinlock to protect it, where the current > proposal doesn't need it. > > Do you still think that's the right thing to do? I'm still not entirely happy, but I've decided to accept it. It's a bug and it needs to be fixed. Adding a mutex to protect the datastructure adds only more complexity and it not really worth the effort. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/usb/stk1160/stk1160-core.c b/drivers/media/usb/stk1160/stk1160-core.c index 34a26e0..03504dc 100644 --- a/drivers/media/usb/stk1160/stk1160-core.c +++ b/drivers/media/usb/stk1160/stk1160-core.c @@ -67,17 +67,25 @@ int stk1160_read_reg(struct stk1160 *dev, u16 reg, u8 *value) { int ret; int pipe = usb_rcvctrlpipe(dev->udev, 0); + u8 *buf; *value = 0; + + buf = kmalloc(sizeof(u8), GFP_KERNEL); + if (!buf) + return -ENOMEM; ret = usb_control_msg(dev->udev, pipe, 0x00, USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE, - 0x00, reg, value, sizeof(u8), HZ); + 0x00, reg, buf, sizeof(u8), HZ); if (ret < 0) { stk1160_err("read failed on reg 0x%x (%d)\n", reg, ret); + kfree(buf); return ret; } + *value = *buf; + kfree(buf); return 0; } diff --git a/drivers/media/usb/stk1160/stk1160.h b/drivers/media/usb/stk1160/stk1160.h index 05b05b1..abdea48 100644 --- a/drivers/media/usb/stk1160/stk1160.h +++ b/drivers/media/usb/stk1160/stk1160.h @@ -143,7 +143,6 @@ struct stk1160 { int num_alt; struct stk1160_isoc_ctl isoc_ctl; - char urb_buf[255]; /* urb control msg buffer */ /* frame properties */ int width; /* current frame width */