2011-03-20 21:51:36

by Florian Mickler

[permalink] [raw]
Subject: [PATCH 0/5] get rid of on-stack dma buffers (part1)

Hi Mauro!

These are the patches which got tested already and
should be good to go. [first batch of patches]

I have another batch with updated patches (dib0700, gp8psk, vp702x)
where I did some more extensive changes to use preallocated memory.
And a small update to the vp7045 patch.

Third batch are the patches to opera1, m920x, dw2102, friio,
a800 which I left as is, for the time beeing.
Regards,
Flo

Florian Mickler (5):
[media] ec168: get rid of on-stack dma buffers
[media] ce6230: get rid of on-stack dma buffer
[media] au6610: get rid of on-stack dma buffer
[media] lmedm04: correct indentation
[media] lmedm04: get rid of on-stack dma buffers

drivers/media/dvb/dvb-usb/au6610.c | 22 ++++++++++++++++------
drivers/media/dvb/dvb-usb/ce6230.c | 11 +++++++++--
drivers/media/dvb/dvb-usb/ec168.c | 18 +++++++++++++++---
drivers/media/dvb/dvb-usb/lmedm04.c | 35 +++++++++++++++++++++++------------
4 files changed, 63 insertions(+), 23 deletions(-)

--
1.7.4.1


2011-03-20 21:51:38

by Florian Mickler

[permalink] [raw]
Subject: [PATCH 5/5 v2] [media] lmedm04: get rid of on-stack dma buffers

usb_control_msg initiates (and waits for completion of) a dma transfer using
the supplied buffer. That buffer thus has to be seperately allocated on
the heap.

In lib/dma_debug.c the function check_for_stack even warns about it:
WARNING: at lib/dma-debug.c:866 check_for_stack

Tested-By: Malcolm Priestley <[email protected]>
Signed-off-by: Florian Mickler <[email protected]>

---

[v2: fix use after free as noted by Malcom]

drivers/media/dvb/dvb-usb/lmedm04.c | 19 +++++++++++++++----
1 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/media/dvb/dvb-usb/lmedm04.c b/drivers/media/dvb/dvb-usb/lmedm04.c
index 0a3e88f..8a79354 100644
--- a/drivers/media/dvb/dvb-usb/lmedm04.c
+++ b/drivers/media/dvb/dvb-usb/lmedm04.c
@@ -314,13 +314,19 @@ static int lme2510_int_read(struct dvb_usb_adapter *adap)
static int lme2510_return_status(struct usb_device *dev)
{
int ret = 0;
- u8 data[10] = {0};
+ u8 *data;
+
+ data = kzalloc(10, GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;

ret |= usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
0x06, 0x80, 0x0302, 0x00, data, 0x0006, 200);
info("Firmware Status: %x (%x)", ret , data[2]);

- return (ret < 0) ? -ENODEV : data[2];
+ ret = (ret < 0) ? -ENODEV : data[2];
+ kfree(data);
+ return ret;
}

static int lme2510_msg(struct dvb_usb_device *d,
@@ -603,7 +609,7 @@ static int lme2510_download_firmware(struct usb_device *dev,
const struct firmware *fw)
{
int ret = 0;
- u8 data[512] = {0};
+ u8 *data;
u16 j, wlen, len_in, start, end;
u8 packet_size, dlen, i;
u8 *fw_data;
@@ -611,6 +617,11 @@ static int lme2510_download_firmware(struct usb_device *dev,
packet_size = 0x31;
len_in = 1;

+ data = kzalloc(512, GFP_KERNEL);
+ if (!data) {
+ info("FRM Could not start Firmware Download (Buffer allocation failed)");
+ return -ENOMEM;
+ }

info("FRM Starting Firmware Download");

@@ -654,7 +665,7 @@ static int lme2510_download_firmware(struct usb_device *dev,
else
info("FRM Firmware Download Completed - Resetting Device");

-
+ kfree(data);
return (ret < 0) ? -ENODEV : 0;
}

--
1.7.4.1

2011-03-20 21:51:37

by Florian Mickler

[permalink] [raw]
Subject: [PATCH 1/5] [media] ec168: get rid of on-stack dma buffers

usb_control_msg initiates (and waits for completion of) a dma transfer using
the supplied buffer. That buffer thus has to be seperately allocated on
the heap.

In lib/dma_debug.c the function check_for_stack even warns about it:
WARNING: at lib/dma-debug.c:866 check_for_stack

Signed-off-by: Florian Mickler <[email protected]>
Acked-by: Antti Palosaari <[email protected]>
Reviewed-by: Antti Palosaari <[email protected]>
Tested-by: Antti Palosaari <[email protected]>
---
drivers/media/dvb/dvb-usb/ec168.c | 18 +++++++++++++++---
1 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/media/dvb/dvb-usb/ec168.c b/drivers/media/dvb/dvb-usb/ec168.c
index 52f5d4f..1ba3e5d 100644
--- a/drivers/media/dvb/dvb-usb/ec168.c
+++ b/drivers/media/dvb/dvb-usb/ec168.c
@@ -36,7 +36,9 @@ static int ec168_rw_udev(struct usb_device *udev, struct ec168_req *req)
int ret;
unsigned int pipe;
u8 request, requesttype;
- u8 buf[req->size];
+ u8 *buf;
+
+

switch (req->cmd) {
case DOWNLOAD_FIRMWARE:
@@ -72,6 +74,12 @@ static int ec168_rw_udev(struct usb_device *udev, struct ec168_req *req)
goto error;
}

+ buf = kmalloc(req->size, GFP_KERNEL);
+ if (!buf) {
+ ret = -ENOMEM;
+ goto error;
+ }
+
if (requesttype == (USB_TYPE_VENDOR | USB_DIR_OUT)) {
/* write */
memcpy(buf, req->data, req->size);
@@ -84,13 +92,13 @@ static int ec168_rw_udev(struct usb_device *udev, struct ec168_req *req)
msleep(1); /* avoid I2C errors */

ret = usb_control_msg(udev, pipe, request, requesttype, req->value,
- req->index, buf, sizeof(buf), EC168_USB_TIMEOUT);
+ req->index, buf, req->size, EC168_USB_TIMEOUT);

ec168_debug_dump(request, requesttype, req->value, req->index, buf,
req->size, deb_xfer);

if (ret < 0)
- goto error;
+ goto err_dealloc;
else
ret = 0;

@@ -98,7 +106,11 @@ static int ec168_rw_udev(struct usb_device *udev, struct ec168_req *req)
if (!ret && requesttype == (USB_TYPE_VENDOR | USB_DIR_IN))
memcpy(req->data, buf, req->size);

+ kfree(buf);
return ret;
+
+err_dealloc:
+ kfree(buf);
error:
deb_info("%s: failed:%d\n", __func__, ret);
return ret;
--
1.7.4.1

2011-03-20 21:51:33

by Florian Mickler

[permalink] [raw]
Subject: [PATCH 4/5] [media] lmedm04: correct indentation

This should not change anything except whitespace.

Signed-off-by: Florian Mickler <[email protected]>
---
drivers/media/dvb/dvb-usb/lmedm04.c | 16 ++++++++--------
1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/media/dvb/dvb-usb/lmedm04.c b/drivers/media/dvb/dvb-usb/lmedm04.c
index 9eea418..0a3e88f 100644
--- a/drivers/media/dvb/dvb-usb/lmedm04.c
+++ b/drivers/media/dvb/dvb-usb/lmedm04.c
@@ -626,15 +626,15 @@ static int lme2510_download_firmware(struct usb_device *dev,
data[0] = i | 0x80;
dlen = (u8)(end - j)-1;
}
- data[1] = dlen;
- memcpy(&data[2], fw_data, dlen+1);
- wlen = (u8) dlen + 4;
- data[wlen-1] = check_sum(fw_data, dlen+1);
- deb_info(1, "Data S=%02x:E=%02x CS= %02x", data[3],
+ data[1] = dlen;
+ memcpy(&data[2], fw_data, dlen+1);
+ wlen = (u8) dlen + 4;
+ data[wlen-1] = check_sum(fw_data, dlen+1);
+ deb_info(1, "Data S=%02x:E=%02x CS= %02x", data[3],
data[dlen+2], data[dlen+3]);
- ret |= lme2510_bulk_write(dev, data, wlen, 1);
- ret |= lme2510_bulk_read(dev, data, len_in , 1);
- ret |= (data[0] == 0x88) ? 0 : -1;
+ ret |= lme2510_bulk_write(dev, data, wlen, 1);
+ ret |= lme2510_bulk_read(dev, data, len_in , 1);
+ ret |= (data[0] == 0x88) ? 0 : -1;
}
}

--
1.7.4.1

2011-03-20 21:52:54

by Florian Mickler

[permalink] [raw]
Subject: [PATCH 3/5] [media] au6610: get rid of on-stack dma buffer

usb_control_msg initiates (and waits for completion of) a dma transfer using
the supplied buffer. That buffer thus has to be seperately allocated on
the heap.

In lib/dma_debug.c the function check_for_stack even warns about it:
WARNING: at lib/dma-debug.c:866 check_for_stack

Signed-off-by: Florian Mickler <[email protected]>
Acked-by: Antti Palosaari <[email protected]>
Reviewed-by: Antti Palosaari <[email protected]>
Tested-by: Antti Palosaari <[email protected]>
---
drivers/media/dvb/dvb-usb/au6610.c | 22 ++++++++++++++++------
1 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/media/dvb/dvb-usb/au6610.c b/drivers/media/dvb/dvb-usb/au6610.c
index eb34cc3..2351077 100644
--- a/drivers/media/dvb/dvb-usb/au6610.c
+++ b/drivers/media/dvb/dvb-usb/au6610.c
@@ -33,8 +33,16 @@ static int au6610_usb_msg(struct dvb_usb_device *d, u8 operation, u8 addr,
{
int ret;
u16 index;
- u8 usb_buf[6]; /* enough for all known requests,
- read returns 5 and write 6 bytes */
+ u8 *usb_buf;
+
+ /*
+ * allocate enough for all known requests,
+ * read returns 5 and write 6 bytes
+ */
+ usb_buf = kmalloc(6, GFP_KERNEL);
+ if (!usb_buf)
+ return -ENOMEM;
+
switch (wlen) {
case 1:
index = wbuf[0] << 8;
@@ -45,14 +53,15 @@ static int au6610_usb_msg(struct dvb_usb_device *d, u8 operation, u8 addr,
break;
default:
warn("wlen = %x, aborting.", wlen);
- return -EINVAL;
+ ret = -EINVAL;
+ goto error;
}

ret = usb_control_msg(d->udev, usb_rcvctrlpipe(d->udev, 0), operation,
USB_TYPE_VENDOR|USB_DIR_IN, addr << 1, index,
- usb_buf, sizeof(usb_buf), AU6610_USB_TIMEOUT);
+ usb_buf, 6, AU6610_USB_TIMEOUT);
if (ret < 0)
- return ret;
+ goto error;

switch (operation) {
case AU6610_REQ_I2C_READ:
@@ -60,7 +69,8 @@ static int au6610_usb_msg(struct dvb_usb_device *d, u8 operation, u8 addr,
/* requested value is always 5th byte in buffer */
rbuf[0] = usb_buf[4];
}
-
+error:
+ kfree(usb_buf);
return ret;
}

--
1.7.4.1

2011-03-20 21:51:35

by Florian Mickler

[permalink] [raw]
Subject: [PATCH 2/5] [media] ce6230: get rid of on-stack dma buffer

usb_control_msg initiates (and waits for completion of) a dma transfer using
the supplied buffer. That buffer thus has to be seperately allocated on
the heap.

In lib/dma_debug.c the function check_for_stack even warns about it:
WARNING: at lib/dma-debug.c:866 check_for_stack

Signed-off-by: Florian Mickler <[email protected]>
Acked-by: Antti Palosaari <[email protected]>
Reviewed-by: Antti Palosaari <[email protected]>
Tested-by: Antti Palosaari <[email protected]>
---
drivers/media/dvb/dvb-usb/ce6230.c | 11 +++++++++--
1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/media/dvb/dvb-usb/ce6230.c b/drivers/media/dvb/dvb-usb/ce6230.c
index 3df2045..6d1a304 100644
--- a/drivers/media/dvb/dvb-usb/ce6230.c
+++ b/drivers/media/dvb/dvb-usb/ce6230.c
@@ -39,7 +39,7 @@ static int ce6230_rw_udev(struct usb_device *udev, struct req_t *req)
u8 requesttype;
u16 value;
u16 index;
- u8 buf[req->data_len];
+ u8 *buf;

request = req->cmd;
value = req->value;
@@ -62,6 +62,12 @@ static int ce6230_rw_udev(struct usb_device *udev, struct req_t *req)
goto error;
}

+ buf = kmalloc(req->data_len, GFP_KERNEL);
+ if (!buf) {
+ ret = -ENOMEM;
+ goto error;
+ }
+
if (requesttype == (USB_TYPE_VENDOR | USB_DIR_OUT)) {
/* write */
memcpy(buf, req->data, req->data_len);
@@ -74,7 +80,7 @@ static int ce6230_rw_udev(struct usb_device *udev, struct req_t *req)
msleep(1); /* avoid I2C errors */

ret = usb_control_msg(udev, pipe, request, requesttype, value, index,
- buf, sizeof(buf), CE6230_USB_TIMEOUT);
+ buf, req->data_len, CE6230_USB_TIMEOUT);

ce6230_debug_dump(request, requesttype, value, index, buf,
req->data_len, deb_xfer);
@@ -88,6 +94,7 @@ static int ce6230_rw_udev(struct usb_device *udev, struct req_t *req)
if (!ret && requesttype == (USB_TYPE_VENDOR | USB_DIR_IN))
memcpy(req->data, buf, req->data_len);

+ kfree(buf);
error:
return ret;
}
--
1.7.4.1

2011-04-30 18:54:20

by Florian Mickler

[permalink] [raw]
Subject: Re: [PATCH 0/5] get rid of on-stack dma buffers (part1)

Hi Mauro!

I just saw that you picked up some patches of mine. What about these?
These are actually tested...

Regards,
Flo

On Sun, 20 Mar 2011 22:50:47 +0100
Florian Mickler <[email protected]> wrote:

> Hi Mauro!
>
> These are the patches which got tested already and
> should be good to go. [first batch of patches]
>
> I have another batch with updated patches (dib0700, gp8psk, vp702x)
> where I did some more extensive changes to use preallocated memory.
> And a small update to the vp7045 patch.
>
> Third batch are the patches to opera1, m920x, dw2102, friio,
> a800 which I left as is, for the time beeing.
> Regards,
> Flo
>
> Florian Mickler (5):
> [media] ec168: get rid of on-stack dma buffers
> [media] ce6230: get rid of on-stack dma buffer
> [media] au6610: get rid of on-stack dma buffer
> [media] lmedm04: correct indentation
> [media] lmedm04: get rid of on-stack dma buffers
>
> drivers/media/dvb/dvb-usb/au6610.c | 22 ++++++++++++++++------
> drivers/media/dvb/dvb-usb/ce6230.c | 11 +++++++++--
> drivers/media/dvb/dvb-usb/ec168.c | 18 +++++++++++++++---
> drivers/media/dvb/dvb-usb/lmedm04.c | 35 +++++++++++++++++++++++------------
> 4 files changed, 63 insertions(+), 23 deletions(-)
>

2011-04-30 22:31:09

by Mauro Carvalho Chehab

[permalink] [raw]
Subject: Re: [PATCH 0/5] get rid of on-stack dma buffers (part1)

Hi Florian,

Em 30-04-2011 15:54, Florian Mickler escreveu:
> Hi Mauro!
>
> I just saw that you picked up some patches of mine. What about these?
> These are actually tested...

I'm still in process of applying the pending patches. Due to patchwork.kernel.org
troubles (including the loss of about 270 patches from its SQL database only
recovered yesterday[1]), I have a long backlog. So, I'm gradually applying the remaing
stuff. It will take some time though, and it will depend on patchwork mood, but I intend
to spend some time during this weekend to minimize the backlog.


Cheers,
Mauro

[1] The recover lost the email's body/SOB, so I've wrote a script to use my email
queue to get the data, using patchwork just to mark what patches were already
processed. This increses the time I have to spend on each patch, as I need to run
a script to match the patchwork patch with the patch ID inside my email queue.

>
> Regards,
> Flo
>
> On Sun, 20 Mar 2011 22:50:47 +0100
> Florian Mickler <[email protected]> wrote:
>
>> Hi Mauro!
>>
>> These are the patches which got tested already and
>> should be good to go. [first batch of patches]
>>
>> I have another batch with updated patches (dib0700, gp8psk, vp702x)
>> where I did some more extensive changes to use preallocated memory.
>> And a small update to the vp7045 patch.
>>
>> Third batch are the patches to opera1, m920x, dw2102, friio,
>> a800 which I left as is, for the time beeing.
>> Regards,
>> Flo
>>
>> Florian Mickler (5):
>> [media] ec168: get rid of on-stack dma buffers
>> [media] ce6230: get rid of on-stack dma buffer
>> [media] au6610: get rid of on-stack dma buffer
>> [media] lmedm04: correct indentation
>> [media] lmedm04: get rid of on-stack dma buffers
>>
>> drivers/media/dvb/dvb-usb/au6610.c | 22 ++++++++++++++++------
>> drivers/media/dvb/dvb-usb/ce6230.c | 11 +++++++++--
>> drivers/media/dvb/dvb-usb/ec168.c | 18 +++++++++++++++---
>> drivers/media/dvb/dvb-usb/lmedm04.c | 35 +++++++++++++++++++++++------------
>> 4 files changed, 63 insertions(+), 23 deletions(-)
>>

2011-05-01 10:39:10

by Florian Mickler

[permalink] [raw]
Subject: Re: [PATCH 0/5] get rid of on-stack dma buffers (part1)

On Sat, 30 Apr 2011 19:30:52 -0300
Mauro Carvalho Chehab <[email protected]> wrote:

> Hi Florian,
>
> Em 30-04-2011 15:54, Florian Mickler escreveu:
> > Hi Mauro!
> >
> > I just saw that you picked up some patches of mine. What about these?
> > These are actually tested...
>
> I'm still in process of applying the pending patches. Due to patchwork.kernel.org
> troubles (including the loss of about 270 patches from its SQL database only
> recovered yesterday[1]), I have a long backlog. So, I'm gradually applying the remaing
> stuff. It will take some time though, and it will depend on patchwork mood, but I intend
> to spend some time during this weekend to minimize the backlog.
>
>
> Cheers,
> Mauro
>
> [1] The recover lost the email's body/SOB, so I've wrote a script to use my email
> queue to get the data, using patchwork just to mark what patches were already
> processed. This increses the time I have to spend on each patch, as I need to run
> a script to match the patchwork patch with the patch ID inside my email queue.
>

Ah ok, no time pressure over here.. just wanted to make sure
that these don't get lost.

Regards and thanks,
Flo

2011-05-01 15:59:51

by Mauro Carvalho Chehab

[permalink] [raw]
Subject: Re: [PATCH 0/5] get rid of on-stack dma buffers (part1)

Em 01-05-2011 07:38, Florian Mickler escreveu:
> On Sat, 30 Apr 2011 19:30:52 -0300
> Mauro Carvalho Chehab <[email protected]> wrote:
>
>> Hi Florian,
>>
>> Em 30-04-2011 15:54, Florian Mickler escreveu:
>>> Hi Mauro!
>>>
>>> I just saw that you picked up some patches of mine. What about these?
>>> These are actually tested...
>>
>> I'm still in process of applying the pending patches. Due to patchwork.kernel.org
>> troubles (including the loss of about 270 patches from its SQL database only
>> recovered yesterday[1]), I have a long backlog. So, I'm gradually applying the remaing
>> stuff. It will take some time though, and it will depend on patchwork mood, but I intend
>> to spend some time during this weekend to minimize the backlog.
>>
>>
>> Cheers,
>> Mauro
>>
>> [1] The recover lost the email's body/SOB, so I've wrote a script to use my email
>> queue to get the data, using patchwork just to mark what patches were already
>> processed. This increses the time I have to spend on each patch, as I need to run
>> a script to match the patchwork patch with the patch ID inside my email queue.
>>
>
> Ah ok, no time pressure over here.. just wanted to make sure
> that these don't get lost.

I think I've applied your series yesterday. Yet, patchwork is currently at bad mood, and it is
not allowing to mark your patches as applied (and the previous series as superseded).

I suspect that I'll need to completely abandon patchwork and work on a new way for handling it
that doesn't depend (nor will update) patchwork.kernel.org.

Mauro.