| Message ID | 1397737700-1081-1-git-send-email-ezequiel.garcia@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 */
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(-)