* [PATCH v2] media: stk1160: Avoid stack-allocated buffer for control URBs
@ 2014-04-17 12:28 Ezequiel Garcia
2014-04-25 21:51 ` Ezequiel Garcia
2014-05-09 10:34 ` Hans Verkuil
0 siblings, 2 replies; 7+ messages in thread
From: Ezequiel Garcia @ 2014-04-17 12:28 UTC (permalink / raw)
To: linux-media; +Cc: Hans Verkuil, Ezequiel Garcia, Alan Stern
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(-)
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] media: stk1160: Avoid stack-allocated buffer for control URBs
2014-04-17 12:28 [PATCH v2] media: stk1160: Avoid stack-allocated buffer for control URBs Ezequiel Garcia
@ 2014-04-25 21:51 ` Ezequiel Garcia
2014-04-28 16:42 ` Sander Eikelenboom
2014-05-09 10:34 ` Hans Verkuil
1 sibling, 1 reply; 7+ messages in thread
From: Ezequiel Garcia @ 2014-04-25 21:51 UTC (permalink / raw)
To: Sander Eikelenboom, linux-media; +Cc: Hans Verkuil, Alan Stern
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
--
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] media: stk1160: Avoid stack-allocated buffer for control URBs
2014-04-25 21:51 ` Ezequiel Garcia
@ 2014-04-28 16:42 ` Sander Eikelenboom
0 siblings, 0 replies; 7+ messages in thread
From: Sander Eikelenboom @ 2014-04-28 16:42 UTC (permalink / raw)
To: Ezequiel Garcia; +Cc: linux-media, Hans Verkuil, Alan Stern
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] media: stk1160: Avoid stack-allocated buffer for control URBs
2014-04-17 12:28 [PATCH v2] media: stk1160: Avoid stack-allocated buffer for control URBs Ezequiel Garcia
2014-04-25 21:51 ` Ezequiel Garcia
@ 2014-05-09 10:34 ` Hans Verkuil
2014-05-09 13:07 ` Ezequiel Garcia
2014-05-17 12:21 ` Ezequiel Garcia
1 sibling, 2 replies; 7+ messages in thread
From: Hans Verkuil @ 2014-05-09 10:34 UTC (permalink / raw)
To: Ezequiel Garcia, linux-media; +Cc: Alan Stern
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 */
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] media: stk1160: Avoid stack-allocated buffer for control URBs
2014-05-09 10:34 ` Hans Verkuil
@ 2014-05-09 13:07 ` Ezequiel Garcia
2014-05-17 12:21 ` Ezequiel Garcia
1 sibling, 0 replies; 7+ messages in thread
From: Ezequiel Garcia @ 2014-05-09 13:07 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, Alan Stern
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.
--
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] media: stk1160: Avoid stack-allocated buffer for control URBs
2014-05-09 10:34 ` Hans Verkuil
2014-05-09 13:07 ` Ezequiel Garcia
@ 2014-05-17 12:21 ` Ezequiel Garcia
2014-05-23 10:38 ` Hans Verkuil
1 sibling, 1 reply; 7+ messages in thread
From: Ezequiel Garcia @ 2014-05-17 12:21 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, Alan Stern
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!
--
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] media: stk1160: Avoid stack-allocated buffer for control URBs
2014-05-17 12:21 ` Ezequiel Garcia
@ 2014-05-23 10:38 ` Hans Verkuil
0 siblings, 0 replies; 7+ messages in thread
From: Hans Verkuil @ 2014-05-23 10:38 UTC (permalink / raw)
To: Ezequiel Garcia; +Cc: linux-media, Alan Stern
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-05-23 10:38 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-04-17 12:28 [PATCH v2] media: stk1160: Avoid stack-allocated buffer for control URBs Ezequiel Garcia
2014-04-25 21:51 ` Ezequiel Garcia
2014-04-28 16:42 ` Sander Eikelenboom
2014-05-09 10:34 ` Hans Verkuil
2014-05-09 13:07 ` Ezequiel Garcia
2014-05-17 12:21 ` Ezequiel Garcia
2014-05-23 10:38 ` Hans Verkuil
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).