Hi Alan, Alan Stern wrote: >> The HCD_NO_COHERENT_MEM USB host controller driver flag can be enabled >> to instruct the USB stack to avoid allocating coherent memory for USB >> buffers. > >This patch appears to be based on your earlier work, not on the current >development kernel. > This patch is the second part of a 2 patch set based on top of v2.6.33. The patchset was sent as: [RFC PATCH v3 1/2] USB: refactor unmap_urb_for_dma/map_urb_for_dma http://marc.info/?l=linux-usb&m=126756887220876 [RFC PATCH v3 2/2] USB: add HCD_NO_COHERENT_MEM host controller driver flag http://marc.info/?l=linux-usb&m=126756887220879 The first patch, which basically refactors code, tries to cover these concerns from v2: - expand urb_needs_*_dma_map() into multiple test and return statements - record the mappings in the URB flags and use that info to do the right unmappings The second patch, implements the HCD_NO_COHERENT_MEM support, and tries to cover these concerns: - use urb->num_sgs > 0 to check for s-g requests - don't check for s-g requests on control requests - use the following revised logic for mapping transfer buffer (similar for setup_packet): If transfer_buffer_length is 0 then do nothing. Otherwise if num_sgs > 0 then do nothing. Otherwise if URB_NO_TRANSFER_DMA_MAP and transfer_dma != ~0 (this avoids your HCD_NO_COHERENT_MEM case) then do nothing. Otherwise if hcd->self.uses_dma is set then call dma_map_single Otherwise if HCD_LOCAL_MEM is set then call hcd_alloc_coherent Otherwise do nothing (PIO case). >> - if (!bus->controller->dma_mask && >> - !(hcd->driver->flags & HCD_LOCAL_MEM)) { >> + if ((!bus->controller->dma_mask && >> + !(hcd->driver->flags & HCD_LOCAL_MEM)) || >> + (hcd->driver->flags & HCD_NO_COHERENT_MEM)) { >> kfree(addr); >> return; >> } > >These three tests should be encapsulated in a single subroutine. > Ok, I'll fix that in a following v4 patch. >> --- a/drivers/usb/core/hcd.c >> +++ b/drivers/usb/core/hcd.c >> @@ -1267,6 +1267,16 @@ static void unmap_urb_setup_packet(struct usb_hcd *hcd, struct urb *urb) >> dma_unmap_single(hcd->self.controller, urb->setup_dma, >> sizeof(struct usb_ctrlrequest), >> DMA_TO_DEVICE); >> + /* >> + * For the HCD_NO_COHERENT_MEM case we need to reset >> + * setup_dma to its original value if the buffer was >> + * allocated via usb_buffer_alloc(). >> + * This is necessary to force a new streaming DMA mapping >> + * when the same buffer is used again as the setup packet. > >This is a little fragile. I think it would be better to move the test >for HCD_NO_COHERENT_MEM into the submission path, by combining it with >the URB_NO_SETUP_DMA_MAP test. Evidently this case has to be covered >either during submission or during giveback, and doing it during >submission would be cleaner. > >The same goes for URB_NO_TRANSFER_DMA_SETUP, of course. > Why exactly is this a little fragile? Doing the reset of {transfer,setup}_dma on unmap time has benefits from doing it on map time. I'll try to illustrate with an example. A USB driver that submits several times the same "coherent" USB buffer allocated via usb_buffer_alloc() assumes that the DMA address returned by usb_buffer_alloc() is valid during the life of the buffer, and can be used as is by the USB driver. When we do the reset of transfer_dma on unmap, which is my proposal, we have the following (pseudo-code) scenario: /* * HCD_NO_COHERENT_MEM set. * Strategy A. * transfer_dma reset on unmap */ urb = usb_alloc_urb(0, GFP_KERNEL); buf = usb_buffer_alloc(dev, bufsz, GFP_KERNEL, &urb->transfer_dma); /* transfer_dma == ~0 */ urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP; /* 1st transfer, HCD_NO_COHERENT_MEM && transfer_dma == ~0 -> needs map true */ usb_submit_urb(urb, GFP_KERNEL); /* transfer_dma reset on unmap */ /* transfer_dma == ~0 */ /* 2nd transfer, HCD_NO_COHERENT_MEM && transfer_dma == ~0 -> needs map true */ usb_submit_urb(urb, GFP_KERNEL); /* transfer_dma == ~0 */ /* ... etc */ In this case the value of urb->transfer_dma is always preserved from the point of view of the USB driver, and its value changes only _within_ usb_submit_urb() time. On the contrary, when we do the reset of transfer_dma on map, we have the following (pseudo-code) scenario with the current logic: /* * HCD_NO_COHERENT_MEM set. * Strategy B. * transfer_dma reset on map */ urb = usb_alloc_urb(0, GFP_KERNEL); buf = usb_buffer_alloc(dev, bufsz, GFP_KERNEL, &urb->transfer_dma); /* transfer_dma == ~0 */ urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP; /* 1st transfer, HCD_NO_COHERENT_MEM + transfer_dma == ~0 -> needs map true */ usb_submit_urb(urb, GFP_KERNEL); /* this works, the kernel memory was properly mapped for the transfer */ /* transfer_dma _not_ reset on unmap, has now a different value */ /* transfer_dma != 0 && transfer_dma != ~0 */ /* * No way to distinguish now if transfer_dma really needs mapping or not, * unless you _assume_ that it must be unconditionally reset to ~0 if * HCD_NO_COHERENT_MEM is set. * This is more fragile IMHO because if, for some reason, a DMA handle for * a coherent buffer or a valid existing mapping is passed in * transfer_dma then we will be doing the wrong thing. * * On the other hand, when we do the reset of transfer_dma at unmap time * we are absolutely sure that transfer_dma came in with value ~0 when we are * doing the unmap of a URB_NO_TRANSFER_DMA_MAP transfer in the HCD_NO_COHERENT_MEM case. */ /* 2nd transfer, HCD_NO_COHERENT_MEM && transfer_dma != ~0 -> needs map false! */ usb_submit_urb(urb, GFP_KERNEL); /* this fails! the kernel memory was not properly mapped for the transfer! */ /* transfer_dma != ~0 */ In this case we will have a different transfer_dma value visible for the USB driver depending on if we are doing the first transfer of the URB after its allocation or a subsequent transfer. Also it is hard (and may fail to do it right) for the USB stack to determine for the subsequent transfers if the transfer_dma value is then a real mapping or not. This problem is not present in the "reset-on-unmap" strategy. I hope this clarifies a little bit the logic behind that decision. Thanks, Albert -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
