* [PATCH 2/2] staging: comedi: drivers: usbduxfast.c: fix for DMA buffers on stack
@ 2013-02-22 18:07 Kumar Amit Mehta
2013-02-22 18:58 ` walter harms
0 siblings, 1 reply; 6+ messages in thread
From: Kumar Amit Mehta @ 2013-02-22 18:07 UTC (permalink / raw)
To: abbotti; +Cc: fmhess, gregkh, hsweeten, devel, linux-kernel, kernel-janitors
fix for instances of DMA buffer on stack(being passed to usb_control_msg) for
the USB-DUXfast Board driver.
Signed-off-by: Kumar Amit Mehta <gmate.amit@gmail.com>
---
drivers/staging/comedi/drivers/usbduxfast.c | 30 ++++++++++++++++-----------
1 file changed, 18 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/comedi/drivers/usbduxfast.c b/drivers/staging/comedi/drivers/usbduxfast.c
index 4bf5dd0..1ba0e3d 100644
--- a/drivers/staging/comedi/drivers/usbduxfast.c
+++ b/drivers/staging/comedi/drivers/usbduxfast.c
@@ -436,10 +436,14 @@ static void usbduxfastsub_ai_Irq(struct urb *urb)
static int usbduxfastsub_start(struct usbduxfastsub_s *udfs)
{
int ret;
- unsigned char local_transfer_buffer[16];
+ unsigned char *local_transfer_buffer;
+
+ local_transfer_buffer = kmalloc(1, GFP_KERNEL);
+ if (!local_transfer_buffer)
+ return -ENOMEM;
/* 7f92 to zero */
- local_transfer_buffer[0] = 0;
+ *local_transfer_buffer = 0;
/* bRequest, "Firmware" */
ret = usb_control_msg(udfs->usbdev, usb_sndctrlpipe(udfs->usbdev, 0),
USBDUXFASTSUB_FIRMWARE,
@@ -450,22 +454,25 @@ static int usbduxfastsub_start(struct usbduxfastsub_s *udfs)
local_transfer_buffer,
1, /* Length */
EZTIMEOUT); /* Timeout */
- if (ret < 0) {
+ if (ret < 0)
dev_err(&udfs->interface->dev,
"control msg failed (start)\n");
- return ret;
- }
- return 0;
+ kfree(local_transfer_buffer);
+ return ret;
}
static int usbduxfastsub_stop(struct usbduxfastsub_s *udfs)
{
int ret;
- unsigned char local_transfer_buffer[16];
+ unsigned char *local_transfer_buffer;
+
+ local_transfer_buffer = kmalloc(1, GFP_KERNEL);
+ if (!local_transfer_buffer)
+ return -ENOMEM;
/* 7f92 to one */
- local_transfer_buffer[0] = 1;
+ *local_transfer_buffer = 1;
/* bRequest, "Firmware" */
ret = usb_control_msg(udfs->usbdev, usb_sndctrlpipe(udfs->usbdev, 0),
USBDUXFASTSUB_FIRMWARE,
@@ -474,13 +481,12 @@ static int usbduxfastsub_stop(struct usbduxfastsub_s *udfs)
0x0000, /* Index */
local_transfer_buffer, 1, /* Length */
EZTIMEOUT); /* Timeout */
- if (ret < 0) {
+ if (ret < 0)
dev_err(&udfs->interface->dev,
"control msg failed (stop)\n");
- return ret;
- }
- return 0;
+ kfree(local_transfer_buffer);
+ return ret;
}
static int usbduxfastsub_upload(struct usbduxfastsub_s *udfs,
--
1.7.9.5
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 2/2] staging: comedi: drivers: usbduxfast.c: fix for DMA buffers on stack
2013-02-22 18:07 [PATCH 2/2] staging: comedi: drivers: usbduxfast.c: fix for DMA buffers on stack Kumar Amit Mehta
@ 2013-02-22 18:58 ` walter harms
2013-02-22 19:06 ` Greg KH
0 siblings, 1 reply; 6+ messages in thread
From: walter harms @ 2013-02-22 18:58 UTC (permalink / raw)
To: Kumar Amit Mehta
Cc: abbotti, fmhess, gregkh, hsweeten, devel, linux-kernel,
kernel-janitors
Am 22.02.2013 19:07, schrieb Kumar Amit Mehta:
> fix for instances of DMA buffer on stack(being passed to usb_control_msg) for
> the USB-DUXfast Board driver.
>
> Signed-off-by: Kumar Amit Mehta <gmate.amit@gmail.com>
> ---
> drivers/staging/comedi/drivers/usbduxfast.c | 30 ++++++++++++++++-----------
> 1 file changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/usbduxfast.c b/drivers/staging/comedi/drivers/usbduxfast.c
> index 4bf5dd0..1ba0e3d 100644
> --- a/drivers/staging/comedi/drivers/usbduxfast.c
> +++ b/drivers/staging/comedi/drivers/usbduxfast.c
> @@ -436,10 +436,14 @@ static void usbduxfastsub_ai_Irq(struct urb *urb)
> static int usbduxfastsub_start(struct usbduxfastsub_s *udfs)
> {
> int ret;
> - unsigned char local_transfer_buffer[16];
> + unsigned char *local_transfer_buffer;
> +
> + local_transfer_buffer = kmalloc(1, GFP_KERNEL);
> + if (!local_transfer_buffer)
> + return -ENOMEM;
>
> /* 7f92 to zero */
> - local_transfer_buffer[0] = 0;
> + *local_transfer_buffer = 0;
> /* bRequest, "Firmware" */
> ret = usb_control_msg(udfs->usbdev, usb_sndctrlpipe(udfs->usbdev, 0),
> USBDUXFASTSUB_FIRMWARE,
> @@ -450,22 +454,25 @@ static int usbduxfastsub_start(struct usbduxfastsub_s *udfs)
> local_transfer_buffer,
> 1, /* Length */
> EZTIMEOUT); /* Timeout */
> - if (ret < 0) {
> + if (ret < 0)
> dev_err(&udfs->interface->dev,
> "control msg failed (start)\n");
> - return ret;
> - }
>
> - return 0;
> + kfree(local_transfer_buffer);
> + return ret;
> }
>
> static int usbduxfastsub_stop(struct usbduxfastsub_s *udfs)
> {
> int ret;
> - unsigned char local_transfer_buffer[16];
> + unsigned char *local_transfer_buffer;
> +
> + local_transfer_buffer = kmalloc(1, GFP_KERNEL);
> + if (!local_transfer_buffer)
> + return -ENOMEM;
>
> /* 7f92 to one */
> - local_transfer_buffer[0] = 1;
> + *local_transfer_buffer = 1;
> /* bRequest, "Firmware" */
> ret = usb_control_msg(udfs->usbdev, usb_sndctrlpipe(udfs->usbdev, 0),
> USBDUXFASTSUB_FIRMWARE,
> @@ -474,13 +481,12 @@ static int usbduxfastsub_stop(struct usbduxfastsub_s *udfs)
> 0x0000, /* Index */
> local_transfer_buffer, 1, /* Length */
> EZTIMEOUT); /* Timeout */
> - if (ret < 0) {
> + if (ret < 0)
> dev_err(&udfs->interface->dev,
> "control msg failed (stop)\n");
> - return ret;
> - }
>
> - return 0;
> + kfree(local_transfer_buffer);
> + return ret;
> }
>
> static int usbduxfastsub_upload(struct usbduxfastsub_s *udfs,
mmh, it seems a bit overheat to alloc 1 byte.
Could one of the driver maintainers please comment
is that realy need ? or is it possible to pass one byte
in a register ? (aka char/int) without allocating ?
re,
wh
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 2/2] staging: comedi: drivers: usbduxfast.c: fix for DMA buffers on stack
2013-02-22 18:58 ` walter harms
@ 2013-02-22 19:06 ` Greg KH
2013-02-23 15:59 ` walter harms
0 siblings, 1 reply; 6+ messages in thread
From: Greg KH @ 2013-02-22 19:06 UTC (permalink / raw)
To: walter harms
Cc: Kumar Amit Mehta, devel, fmhess, kernel-janitors, linux-kernel,
abbotti
On Fri, Feb 22, 2013 at 07:58:35PM +0100, walter harms wrote:
>
>
> Am 22.02.2013 19:07, schrieb Kumar Amit Mehta:
> > fix for instances of DMA buffer on stack(being passed to usb_control_msg) for
> > the USB-DUXfast Board driver.
> >
> > Signed-off-by: Kumar Amit Mehta <gmate.amit@gmail.com>
> > ---
> > drivers/staging/comedi/drivers/usbduxfast.c | 30 ++++++++++++++++-----------
> > 1 file changed, 18 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/staging/comedi/drivers/usbduxfast.c b/drivers/staging/comedi/drivers/usbduxfast.c
> > index 4bf5dd0..1ba0e3d 100644
> > --- a/drivers/staging/comedi/drivers/usbduxfast.c
> > +++ b/drivers/staging/comedi/drivers/usbduxfast.c
> > @@ -436,10 +436,14 @@ static void usbduxfastsub_ai_Irq(struct urb *urb)
> > static int usbduxfastsub_start(struct usbduxfastsub_s *udfs)
> > {
> > int ret;
> > - unsigned char local_transfer_buffer[16];
> > + unsigned char *local_transfer_buffer;
> > +
> > + local_transfer_buffer = kmalloc(1, GFP_KERNEL);
> > + if (!local_transfer_buffer)
> > + return -ENOMEM;
> >
> > /* 7f92 to zero */
> > - local_transfer_buffer[0] = 0;
> > + *local_transfer_buffer = 0;
> > /* bRequest, "Firmware" */
> > ret = usb_control_msg(udfs->usbdev, usb_sndctrlpipe(udfs->usbdev, 0),
> > USBDUXFASTSUB_FIRMWARE,
> > @@ -450,22 +454,25 @@ static int usbduxfastsub_start(struct usbduxfastsub_s *udfs)
> > local_transfer_buffer,
> > 1, /* Length */
> > EZTIMEOUT); /* Timeout */
> > - if (ret < 0) {
> > + if (ret < 0)
> > dev_err(&udfs->interface->dev,
> > "control msg failed (start)\n");
> > - return ret;
> > - }
> >
> > - return 0;
> > + kfree(local_transfer_buffer);
> > + return ret;
> > }
> >
> > static int usbduxfastsub_stop(struct usbduxfastsub_s *udfs)
> > {
> > int ret;
> > - unsigned char local_transfer_buffer[16];
> > + unsigned char *local_transfer_buffer;
> > +
> > + local_transfer_buffer = kmalloc(1, GFP_KERNEL);
> > + if (!local_transfer_buffer)
> > + return -ENOMEM;
> >
> > /* 7f92 to one */
> > - local_transfer_buffer[0] = 1;
> > + *local_transfer_buffer = 1;
> > /* bRequest, "Firmware" */
> > ret = usb_control_msg(udfs->usbdev, usb_sndctrlpipe(udfs->usbdev, 0),
> > USBDUXFASTSUB_FIRMWARE,
> > @@ -474,13 +481,12 @@ static int usbduxfastsub_stop(struct usbduxfastsub_s *udfs)
> > 0x0000, /* Index */
> > local_transfer_buffer, 1, /* Length */
> > EZTIMEOUT); /* Timeout */
> > - if (ret < 0) {
> > + if (ret < 0)
> > dev_err(&udfs->interface->dev,
> > "control msg failed (stop)\n");
> > - return ret;
> > - }
> >
> > - return 0;
> > + kfree(local_transfer_buffer);
> > + return ret;
> > }
> >
> > static int usbduxfastsub_upload(struct usbduxfastsub_s *udfs,
>
> mmh, it seems a bit overheat to alloc 1 byte.
> Could one of the driver maintainers please comment
> is that realy need ?
Yes it is needed.
> or is it possible to pass one byte
> in a register ? (aka char/int) without allocating ?
Nope, the USB host controllers must be able to DMA to this memory
location, so you have to allocate it dynamically, sorry.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 2/2] staging: comedi: drivers: usbduxfast.c: fix for DMA buffers on stack
2013-02-22 19:06 ` Greg KH
@ 2013-02-23 15:59 ` walter harms
2013-02-23 17:34 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: walter harms @ 2013-02-23 15:59 UTC (permalink / raw)
To: Greg KH; +Cc: Kumar Amit Mehta, devel, kernel-janitors, linux-kernel
Am 22.02.2013 20:06, schrieb Greg KH:
> On Fri, Feb 22, 2013 at 07:58:35PM +0100, walter harms wrote:
>>
>>
>> Am 22.02.2013 19:07, schrieb Kumar Amit Mehta:
>>> fix for instances of DMA buffer on stack(being passed to usb_control_msg) for
>>> the USB-DUXfast Board driver.
>>>
>>> Signed-off-by: Kumar Amit Mehta <gmate.amit@gmail.com>
>>> ---
>>> drivers/staging/comedi/drivers/usbduxfast.c | 30 ++++++++++++++++-----------
>>> 1 file changed, 18 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/staging/comedi/drivers/usbduxfast.c b/drivers/staging/comedi/drivers/usbduxfast.c
>>> index 4bf5dd0..1ba0e3d 100644
>>> --- a/drivers/staging/comedi/drivers/usbduxfast.c
>>> +++ b/drivers/staging/comedi/drivers/usbduxfast.c
>>> @@ -436,10 +436,14 @@ static void usbduxfastsub_ai_Irq(struct urb *urb)
>>> static int usbduxfastsub_start(struct usbduxfastsub_s *udfs)
>>> {
>>> int ret;
>>> - unsigned char local_transfer_buffer[16];
>>> + unsigned char *local_transfer_buffer;
>>> +
>>> + local_transfer_buffer = kmalloc(1, GFP_KERNEL);
>>> + if (!local_transfer_buffer)
>>> + return -ENOMEM;
>>>
>>> /* 7f92 to zero */
>>> - local_transfer_buffer[0] = 0;
>>> + *local_transfer_buffer = 0;
>>> /* bRequest, "Firmware" */
>>> ret = usb_control_msg(udfs->usbdev, usb_sndctrlpipe(udfs->usbdev, 0),
>>> USBDUXFASTSUB_FIRMWARE,
>>> @@ -450,22 +454,25 @@ static int usbduxfastsub_start(struct usbduxfastsub_s *udfs)
>>> local_transfer_buffer,
>>> 1, /* Length */
>>> EZTIMEOUT); /* Timeout */
>>> - if (ret < 0) {
>>> + if (ret < 0)
>>> dev_err(&udfs->interface->dev,
>>> "control msg failed (start)\n");
>>> - return ret;
>>> - }
>>>
>>> - return 0;
>>> + kfree(local_transfer_buffer);
>>> + return ret;
>>> }
>>>
>>> static int usbduxfastsub_stop(struct usbduxfastsub_s *udfs)
>>> {
>>> int ret;
>>> - unsigned char local_transfer_buffer[16];
>>> + unsigned char *local_transfer_buffer;
>>> +
>>> + local_transfer_buffer = kmalloc(1, GFP_KERNEL);
>>> + if (!local_transfer_buffer)
>>> + return -ENOMEM;
>>>
>>> /* 7f92 to one */
>>> - local_transfer_buffer[0] = 1;
>>> + *local_transfer_buffer = 1;
>>> /* bRequest, "Firmware" */
>>> ret = usb_control_msg(udfs->usbdev, usb_sndctrlpipe(udfs->usbdev, 0),
>>> USBDUXFASTSUB_FIRMWARE,
>>> @@ -474,13 +481,12 @@ static int usbduxfastsub_stop(struct usbduxfastsub_s *udfs)
>>> 0x0000, /* Index */
>>> local_transfer_buffer, 1, /* Length */
>>> EZTIMEOUT); /* Timeout */
>>> - if (ret < 0) {
>>> + if (ret < 0)
>>> dev_err(&udfs->interface->dev,
>>> "control msg failed (stop)\n");
>>> - return ret;
>>> - }
>>>
>>> - return 0;
>>> + kfree(local_transfer_buffer);
>>> + return ret;
>>> }
>>>
>>> static int usbduxfastsub_upload(struct usbduxfastsub_s *udfs,
>>
>> mmh, it seems a bit overheat to alloc 1 byte.
>> Could one of the driver maintainers please comment
>> is that realy need ?
>
> Yes it is needed.
>
>> or is it possible to pass one byte
>> in a register ? (aka char/int) without allocating ?
>
> Nope, the USB host controllers must be able to DMA to this memory
> location, so you have to allocate it dynamically, sorry.
>
> thanks,
thx for clarification.
@Kumar Amit Mehta:
Would you mind to add this as comment ? Allocating one byte does not
look clever so maybe will come up with the idea of changing that.
re,
wh
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 2/2] staging: comedi: drivers: usbduxfast.c: fix for DMA buffers on stack
2013-02-23 15:59 ` walter harms
@ 2013-02-23 17:34 ` Dan Carpenter
2013-02-23 18:13 ` walter harms
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2013-02-23 17:34 UTC (permalink / raw)
To: walter harms
Cc: Greg KH, Kumar Amit Mehta, devel, kernel-janitors, linux-kernel
On Sat, Feb 23, 2013 at 04:59:43PM +0100, walter harms wrote:
> >> or is it possible to pass one byte
> >> in a register ? (aka char/int) without allocating ?
> >
> > Nope, the USB host controllers must be able to DMA to this memory
> > location, so you have to allocate it dynamically, sorry.
> >
> > thanks,
>
> thx for clarification.
>
> @Kumar Amit Mehta:
> Would you mind to add this as comment ? Allocating one byte does not
> look clever so maybe will come up with the idea of changing that.
>
That can't happen. The reason is already recorded in the git
history. Greg and Ian know that DMA to stack memory doesn't work.
Most maintainers know about that. If someone changed it back then
Fengguang would send an automatic email about it as soon as it was
committed to a public git tree.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] staging: comedi: drivers: usbduxfast.c: fix for DMA buffers on stack
2013-02-23 17:34 ` Dan Carpenter
@ 2013-02-23 18:13 ` walter harms
0 siblings, 0 replies; 6+ messages in thread
From: walter harms @ 2013-02-23 18:13 UTC (permalink / raw)
To: Dan Carpenter
Cc: Greg KH, Kumar Amit Mehta, devel, kernel-janitors, linux-kernel
Am 23.02.2013 18:34, schrieb Dan Carpenter:
> On Sat, Feb 23, 2013 at 04:59:43PM +0100, walter harms wrote:
>>>> or is it possible to pass one byte
>>>> in a register ? (aka char/int) without allocating ?
>>>
>>> Nope, the USB host controllers must be able to DMA to this memory
>>> location, so you have to allocate it dynamically, sorry.
>>>
>>> thanks,
>>
>> thx for clarification.
>>
>> @Kumar Amit Mehta:
>> Would you mind to add this as comment ? Allocating one byte does not
>> look clever so maybe will come up with the idea of changing that.
>>
>
> That can't happen. The reason is already recorded in the git
> history. Greg and Ian know that DMA to stack memory doesn't work.
> Most maintainers know about that. If someone changed it back then
> Fengguang would send an automatic email about it as soon as it was
> committed to a public git tree.
>
I am not sure about that we talk about the same thing, i am thinking
about the "casual "reader" discovering a one-byte-malloc.
He will not check the githistory, he will thing "WTF there are doing here ?"
I will not make things more complicated, IMHO an malloc of one byte
is a strange thing and strange things should be commented in code.
If the maintainer says "no" i will survive.
re,
wh
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-02-23 18:13 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-02-22 18:07 [PATCH 2/2] staging: comedi: drivers: usbduxfast.c: fix for DMA buffers on stack Kumar Amit Mehta
2013-02-22 18:58 ` walter harms
2013-02-22 19:06 ` Greg KH
2013-02-23 15:59 ` walter harms
2013-02-23 17:34 ` Dan Carpenter
2013-02-23 18:13 ` walter harms
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).