Re: [RFC] usb: Check results of dma_map_single

[Paulius Zaleckas <paulius.zaleckas@gmail.com>]

On 11/04/2009 07:48 AM, Larry Finger wrote:
> At http://marc.info/?l=linux-wireless&m=125695331205062&w=2, a problem
> with DMA buffer processing was corrected for the libertas driver. Because
> routine usb_fill_bulk_urb() does not check that DMA is possible when
> dma_map_single() is called, this condition was not detected until the buffer
> was unmapped. By this time memory corruption had occurred.
>
> The situation is fixed by testing the returned DMA address. If not a legal
> address, a WARN_ON(1) is executed to provide traceback and the error is
> returned.
>
> Signed-off-by: Larry Finger<Larry.Finger-tQ5ms3gMjBLk1uMJSBkQmQ@public.gmane.org>
> ---
>
> Index: linux-2.6/drivers/usb/core/hcd.c
> ===================================================================
> --- linux-2.6.orig/drivers/usb/core/hcd.c
> +++ linux-2.6/drivers/usb/core/hcd.c
> @@ -1281,6 +1281,13 @@ static int map_urb_for_dma(struct usb_hc
>   					urb->setup_packet,
>   					sizeof(struct usb_ctrlrequest),
>   					DMA_TO_DEVICE);
> +			ret = dma_mapping_error(hcd->self.controller,
> +						urb->setup_dma);
> +			/* warn if DMA mapping failed */
> +			if (ret) {
> +				WARN_ON(1);
> +				return ret;
> +			}

First of all you forgot to add { } around everything under if statement.

I don't think WARN_ON is needed...
dma_mapping_error under most architectures return 0 or 1 so it would be
better to make some real error value. EAGAIN seems to be proper error,
since documentation says that driver should try again later.

I would write this error handler like this:

if (dma_mapping_error(hcd->self.controller, urb->setup_dma))
	ret = -EAGAIN;

>   		else if (hcd->driver->flags&  HCD_LOCAL_MEM)
>   			ret = hcd_alloc_coherent(
>   					urb->dev->bus, mem_flags,
> @@ -1299,6 +1306,13 @@ static int map_urb_for_dma(struct usb_hc
>   					urb->transfer_buffer,
>   					urb->transfer_buffer_length,
>   					dir);
> +			ret = dma_mapping_error(hcd->self.controller,
> +						urb->transfer_dma);
> +			/* warn if DMA mapping failed */
> +			if (ret) {
> +				WARN_ON(1);
> +				return ret;
> +			}

ditto

>   		else if (hcd->driver->flags&  HCD_LOCAL_MEM) {
>   			ret = hcd_alloc_coherent(
>   					urb->dev->bus, mem_flags,