Patchwork [Precise] USB: Fix 'bad dma' problem on WDM device disconnect

login
register
mail settings
Submitter Ming Lei
Date Dec. 12, 2012, 3:04 p.m.
Message ID <1355324652-20305-1-git-send-email-ming.lei@canonical.com>
Download mbox | patch
Permalink /patch/205558/
State New
Headers show

Comments

Ming Lei - Dec. 12, 2012, 3:04 p.m.
From: Robert Lukassen <Robert.Lukassen@tomtom.com>

In the WDM class driver a disconnect event leads to calls to
usb_free_coherent to put back two USB DMA buffers allocated earlier.
The call to usb_free_coherent uses a different size parameter
(desc->wMaxCommand) than the corresponding call to usb_alloc_coherent
(desc->bMaxPacketSize0).

When a disconnect event occurs, this leads to 'bad dma' complaints
from usb core because the USB DMA buffer is being pushed back to the
'buffer-2048' pool from which it has not been allocated.

This patch against the most recent linux-2.6 kernel ensures that the
parameters used by usb_alloc_coherent & usb_free_coherent calls in
cdc-wdm.c match.

Looks this patch on stable tree is missed in precise release.

BugLink: https://bugs.launchpad.net/bugs/1081879

Signed-off-by: Robert Lukassen <robert.lukassen@tomtom.com>
Cc: stable <stable@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/usb/class/cdc-wdm.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Brad Figg - Dec. 12, 2012, 4:24 p.m.
On 12/12/2012 07:04 AM, Ming Lei wrote:
> From: Robert Lukassen <Robert.Lukassen@tomtom.com>
> 
> In the WDM class driver a disconnect event leads to calls to
> usb_free_coherent to put back two USB DMA buffers allocated earlier.
> The call to usb_free_coherent uses a different size parameter
> (desc->wMaxCommand) than the corresponding call to usb_alloc_coherent
> (desc->bMaxPacketSize0).
> 
> When a disconnect event occurs, this leads to 'bad dma' complaints
> from usb core because the USB DMA buffer is being pushed back to the
> 'buffer-2048' pool from which it has not been allocated.
> 
> This patch against the most recent linux-2.6 kernel ensures that the
> parameters used by usb_alloc_coherent & usb_free_coherent calls in
> cdc-wdm.c match.
> 
> Looks this patch on stable tree is missed in precise release.
> 
> BugLink: https://bugs.launchpad.net/bugs/1081879
> 
> Signed-off-by: Robert Lukassen <robert.lukassen@tomtom.com>
> Cc: stable <stable@kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  drivers/usb/class/cdc-wdm.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
> index 47085e5..a97c018 100644
> --- a/drivers/usb/class/cdc-wdm.c
> +++ b/drivers/usb/class/cdc-wdm.c
> @@ -281,7 +281,7 @@ static void cleanup(struct wdm_device *desc)
>  			  desc->sbuf,
>  			  desc->validity->transfer_dma);
>  	usb_free_coherent(interface_to_usbdev(desc->intf),
> -			  desc->wMaxCommand,
> +			  desc->bMaxPacketSize0,
>  			  desc->inbuf,
>  			  desc->response->transfer_dma);
>  	kfree(desc->orq);
> 

Looks reasonable.
Colin King - Dec. 12, 2012, 4:31 p.m.
On 12/12/12 15:04, Ming Lei wrote:
> From: Robert Lukassen <Robert.Lukassen@tomtom.com>
>
> In the WDM class driver a disconnect event leads to calls to
> usb_free_coherent to put back two USB DMA buffers allocated earlier.
> The call to usb_free_coherent uses a different size parameter
> (desc->wMaxCommand) than the corresponding call to usb_alloc_coherent
> (desc->bMaxPacketSize0).
>
> When a disconnect event occurs, this leads to 'bad dma' complaints
> from usb core because the USB DMA buffer is being pushed back to the
> 'buffer-2048' pool from which it has not been allocated.
>
> This patch against the most recent linux-2.6 kernel ensures that the
> parameters used by usb_alloc_coherent & usb_free_coherent calls in
> cdc-wdm.c match.
>
> Looks this patch on stable tree is missed in precise release.
>
> BugLink: https://bugs.launchpad.net/bugs/1081879
>
> Signed-off-by: Robert Lukassen <robert.lukassen@tomtom.com>
> Cc: stable <stable@kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>   drivers/usb/class/cdc-wdm.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
> index 47085e5..a97c018 100644
> --- a/drivers/usb/class/cdc-wdm.c
> +++ b/drivers/usb/class/cdc-wdm.c
> @@ -281,7 +281,7 @@ static void cleanup(struct wdm_device *desc)
>   			  desc->sbuf,
>   			  desc->validity->transfer_dma);
>   	usb_free_coherent(interface_to_usbdev(desc->intf),
> -			  desc->wMaxCommand,
> +			  desc->bMaxPacketSize0,
>   			  desc->inbuf,
>   			  desc->response->transfer_dma);
>   	kfree(desc->orq);
>

I'm happy to take this.

Acked-by: Colin Ian King <colin.king@canonical.com>
Leann Ogasawara - Dec. 12, 2012, 5:29 p.m.
Hi Ming,

I'm a little confused here.  This patch was included upstream as of 2.6.39:

~/linux$ git show 878b753e32ca765cd346a5d3038d630178ec78ff
commit 878b753e32ca765cd346a5d3038d630178ec78ff
Author: Robert Lukassen <Robert.Lukassen@tomtom.com>
Date:   Wed Mar 16 12:13:34 2011 +0100

    USB: Fix 'bad dma' problem on WDM device disconnect

~/linux$ git describe --contains 878b753e32ca765cd346a5d3038d630178ec78ff
v2.6.39-rc1~85^2~3

Thus, this patch is already available in Precise by default.  Am I
missing something here?  Have you tested with a stock Precise kernel?

Thanks,
Leann

On 12/12/2012 07:04 AM, Ming Lei wrote:
> From: Robert Lukassen <Robert.Lukassen@tomtom.com>
>
> In the WDM class driver a disconnect event leads to calls to
> usb_free_coherent to put back two USB DMA buffers allocated earlier.
> The call to usb_free_coherent uses a different size parameter
> (desc->wMaxCommand) than the corresponding call to usb_alloc_coherent
> (desc->bMaxPacketSize0).
>
> When a disconnect event occurs, this leads to 'bad dma' complaints
> from usb core because the USB DMA buffer is being pushed back to the
> 'buffer-2048' pool from which it has not been allocated.
>
> This patch against the most recent linux-2.6 kernel ensures that the
> parameters used by usb_alloc_coherent & usb_free_coherent calls in
> cdc-wdm.c match.
>
> Looks this patch on stable tree is missed in precise release.
>
> BugLink: https://bugs.launchpad.net/bugs/1081879
>
> Signed-off-by: Robert Lukassen <robert.lukassen@tomtom.com>
> Cc: stable <stable@kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  drivers/usb/class/cdc-wdm.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
> index 47085e5..a97c018 100644
> --- a/drivers/usb/class/cdc-wdm.c
> +++ b/drivers/usb/class/cdc-wdm.c
> @@ -281,7 +281,7 @@ static void cleanup(struct wdm_device *desc)
>  			  desc->sbuf,
>  			  desc->validity->transfer_dma);
>  	usb_free_coherent(interface_to_usbdev(desc->intf),
> -			  desc->wMaxCommand,
> +			  desc->bMaxPacketSize0,
>  			  desc->inbuf,
>  			  desc->response->transfer_dma);
>  	kfree(desc->orq);
Ming Lei - Dec. 13, 2012, 4:33 a.m.
Hi Leann,

On Thu, Dec 13, 2012 at 1:29 AM, Leann Ogasawara
<leann.ogasawara@canonical.com> wrote:
> Hi Ming,
>
> I'm a little confused here.  This patch was included upstream as of 2.6.39:
>
> ~/linux$ git show 878b753e32ca765cd346a5d3038d630178ec78ff
> commit 878b753e32ca765cd346a5d3038d630178ec78ff
> Author: Robert Lukassen <Robert.Lukassen@tomtom.com>
> Date:   Wed Mar 16 12:13:34 2011 +0100
>
>     USB: Fix 'bad dma' problem on WDM device disconnect
>
> ~/linux$ git describe --contains 878b753e32ca765cd346a5d3038d630178ec78ff
> v2.6.39-rc1~85^2~3
>
> Thus, this patch is already available in Precise by default.  Am I
> missing something here?  Have you tested with a stock Precise kernel?

You are correct, looks I miss that, and the root cause is the below commit,
which is a partial fix, so we still need the patch to avoid the 'bad dma' and
coherent buffer leak. The patch isn't needed for upstream kernel, so need to
push it to stable tree first or you can apply it directly on precise?

commit 7a55df4694e1758a7a1c3b1432e36fca0c859d6a
Author: Bjørn Mork <bjorn@mork.no>
Date:   Mon Jan 16 15:11:59 2012 +0100

    USB: cdc-wdm: better allocate a buffer that is at least as big as
we tell the USB core


Thanks,
--
Ming Lei
Leann Ogasawara - Dec. 13, 2012, 2:34 p.m.
On 12/12/2012 08:33 PM, Ming Lei wrote:
> Hi Leann,
>
> On Thu, Dec 13, 2012 at 1:29 AM, Leann Ogasawara
> <leann.ogasawara@canonical.com> wrote:
>> Hi Ming,
>>
>> I'm a little confused here.  This patch was included upstream as of 2.6.39:
>>
>> ~/linux$ git show 878b753e32ca765cd346a5d3038d630178ec78ff
>> commit 878b753e32ca765cd346a5d3038d630178ec78ff
>> Author: Robert Lukassen <Robert.Lukassen@tomtom.com>
>> Date:   Wed Mar 16 12:13:34 2011 +0100
>>
>>     USB: Fix 'bad dma' problem on WDM device disconnect
>>
>> ~/linux$ git describe --contains 878b753e32ca765cd346a5d3038d630178ec78ff
>> v2.6.39-rc1~85^2~3
>>
>> Thus, this patch is already available in Precise by default.  Am I
>> missing something here?  Have you tested with a stock Precise kernel?
> You are correct, looks I miss that, and the root cause is the below commit,
> which is a partial fix, so we still need the patch to avoid the 'bad dma' and
> coherent buffer leak. The patch isn't needed for upstream kernel, so need to
> push it to stable tree first or you can apply it directly on precise?
>
> commit 7a55df4694e1758a7a1c3b1432e36fca0c859d6a
> Author: Bjørn Mork <bjorn@mork.no>
> Date:   Mon Jan 16 15:11:59 2012 +0100
>
>     USB: cdc-wdm: better allocate a buffer that is at least as big as
> we tell the USB core

I'm still a little confused... The above patch is also already in
Precise.  We pulled it in via upstream stable.

~/ubuntu-precise$ git describe --contains
7a55df4694e1758a7a1c3b1432e36fca0c859d6a
v3.2.3~23

Thanks,
Leann
Ming Lei - Dec. 13, 2012, 3:10 p.m.
On Thu, Dec 13, 2012 at 10:34 PM, Leann Ogasawara
<leann.ogasawara@canonical.com> wrote:
>> commit 7a55df4694e1758a7a1c3b1432e36fca0c859d6a
>> Author: Bjørn Mork <bjorn@mork.no>
>> Date:   Mon Jan 16 15:11:59 2012 +0100
>>
>>     USB: cdc-wdm: better allocate a buffer that is at least as big as
>> we tell the USB core
>
> I'm still a little confused... The above patch is also already in
> Precise.  We pulled it in via upstream stable.

Yes, that is just the problem because it is the patch which causes
the 'bad dma' error in LP1081879 and coherent memory leak.

The patch should have done the change in both allocation and free,
but it only did it in allocation.

Thanks,
--
Ming Lei

Patch

diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index 47085e5..a97c018 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -281,7 +281,7 @@  static void cleanup(struct wdm_device *desc)
 			  desc->sbuf,
 			  desc->validity->transfer_dma);
 	usb_free_coherent(interface_to_usbdev(desc->intf),
-			  desc->wMaxCommand,
+			  desc->bMaxPacketSize0,
 			  desc->inbuf,
 			  desc->response->transfer_dma);
 	kfree(desc->orq);