On 03/15/2011 10:43 AM, Florian Mickler wrote:
> 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
>
> Note: This change is tested to compile only, as I don't have the hardware.
>
> Signed-off-by: Florian Mickler<[email protected]>
This patch did not found from patchwork! Probably skipped due to broken
Cc at my contact. Please resend.
Anyhow, I tested and reviewed it.
Acked-by: Antti Palosaari <[email protected]>
Reviewed-by: Antti Palosaari <[email protected]>
Tested-by: Antti Palosaari <[email protected]>
[1] https://patchwork.kernel.org/project/linux-media/list/
Antti
> ---
> 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;
> }
>
--
http://palosaari.fi/
On Fri, 18 Mar 2011 18:34:58 +0200
Antti Palosaari <[email protected]> wrote:
> On 03/15/2011 10:43 AM, Florian Mickler wrote:
> > 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
> >
> > Note: This change is tested to compile only, as I don't have the hardware.
> >
> > Signed-off-by: Florian Mickler<[email protected]>
>
>
> This patch did not found from patchwork! Probably skipped due to broken
> Cc at my contact. Please resend.
>
> Anyhow, I tested and reviewed it.
>
> Acked-by: Antti Palosaari <[email protected]>
> Reviewed-by: Antti Palosaari <[email protected]>
> Tested-by: Antti Palosaari <[email protected]>
>
> [1] https://patchwork.kernel.org/project/linux-media/list/
>
> Antti
>
Yes, there was some broken adressing on my side. Sorry.
Thanks for review && test! I will resend (hopefully this weekend) the
series when I reviewed some of the other patches if it is
feasible/better to use prealocated memory as suggested by Mauro.
How often does au6610_usb_msg get called in normal operation? Should it
use preallocated memory?
Regards,
Flo
On 03/18/2011 11:27 PM, Florian Mickler wrote:
> On Fri, 18 Mar 2011 18:34:58 +0200
> Antti Palosaari<[email protected]> wrote:
>
>> On 03/15/2011 10:43 AM, Florian Mickler wrote:
>>> 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
>>>
>>> Note: This change is tested to compile only, as I don't have the hardware.
>>>
>>> Signed-off-by: Florian Mickler<[email protected]>
>>
>>
>> This patch did not found from patchwork! Probably skipped due to broken
>> Cc at my contact. Please resend.
>>
>> Anyhow, I tested and reviewed it.
>>
>> Acked-by: Antti Palosaari<[email protected]>
>> Reviewed-by: Antti Palosaari<[email protected]>
>> Tested-by: Antti Palosaari<[email protected]>
>>
>> [1] https://patchwork.kernel.org/project/linux-media/list/
>>
>> Antti
>>
>
> Yes, there was some broken adressing on my side. Sorry.
>
> Thanks for review&& test! I will resend (hopefully this weekend) the
> series when I reviewed some of the other patches if it is
> feasible/better to use prealocated memory as suggested by Mauro.
>
> How often does au6610_usb_msg get called in normal operation? Should it
> use preallocated memory?
It is called by demodulator and tuner drivers via I2C. One call per one
register access. Tuner driver is qt1010 and demod driver is zl10353.
When you perform tune to channel and device is in sleep there is maybe
100 or more calls. After channel is tuned as OK, application starts
calling only some signalling statistics from demod, it is usually only
few calls per sec.
On my experience I cannot say if it is wise to preallocate or not.
Anyhow, this same apply for all DVB USB drivers.
Antti
--
http://palosaari.fi/