Paulius Zaleckas wrote: > 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@xxxxxxxxxxxxxxxx> >> --- >> >> 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, > Thank you for your review and comments. I'll wait a bit to see what other comments are offered, but I have implemented all your changes. Larry -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
