| Message ID | 1267366082-15248-9-git-send-email-albert_herranz@yahoo.es (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Sun, 28 Feb 2010, Albert Herranz 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 flag is useful to overcome some esoteric memory access restrictions > found in some platforms. > For example, the Nintendo Wii video game console is a NOT_COHERENT_CACHE > platform that is unable to safely perform non-32 bit uncached writes > to RAM because the byte enables are not connected to the bus. > Thus, in that platform, "coherent" DMA buffers cannot be directly used > by the kernel code unless it guarantees that all write accesses > to said buffers are done in 32 bit chunks (which is not the case in the > USB subsystem). > > To avoid this unwanted behaviour HCD_NO_COHERENT_MEM can be enabled at > the HCD controller, causing USB buffer allocations to be satisfied from > normal kernel memory. In this case, the USB stack will make sure that > the buffers get properly mapped/unmapped for DMA transfers using the DMA > streaming mapping API. > > Note that other parts of the USB stack may also use coherent memory, > like for example the hardware descriptors used in the EHCI controllers. > This needs to be checked and addressed separately. In the EHCI example, > hardware descriptors are accessed in 32-bit (or 64-bit) chunks, causing > no problems in the described scenario. > --- a/drivers/usb/core/hcd.c > +++ b/drivers/usb/core/hcd.c > @@ -1260,6 +1260,34 @@ static void hcd_free_coherent(struct usb_bus *bus, dma_addr_t *dma_handle, > *dma_handle = 0; > } > > +static int urb_needs_setup_dma_map(struct usb_hcd *hcd, struct urb *urb) > +{ > + return !(urb->transfer_flags & URB_NO_SETUP_DMA_MAP) || > + ((hcd->driver->flags & HCD_NO_COHERENT_MEM) && > + urb->setup_dma == ~(dma_addr_t)0); > +} > + > +static int urb_needs_setup_dma_unmap(struct usb_hcd *hcd, struct urb *urb) > +{ > + return !(urb->transfer_flags & URB_NO_SETUP_DMA_MAP) || > + ((hcd->driver->flags & HCD_NO_COHERENT_MEM) && > + urb->setup_dma && urb->setup_dma != ~(dma_addr_t)0); > +} > + > +static int urb_needs_transfer_dma_map(struct usb_hcd *hcd, struct urb *urb) > +{ > + return !(urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP) || > + ((hcd->driver->flags & HCD_NO_COHERENT_MEM) && > + urb->transfer_dma == ~(dma_addr_t)0); > +} > + > +static int urb_needs_transfer_dma_unmap(struct usb_hcd *hcd, struct urb *urb) > +{ > + return !(urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP) || > + ((hcd->driver->flags & HCD_NO_COHERENT_MEM) && > + urb->transfer_dma && urb->transfer_dma != ~(dma_addr_t)0); > +} > + These functions would be a lot easier to understand if they were expanded into multiple test and return statements, rather than squeezing all the Boolean manipulations into single expressions. (Not to mention the fact that other developement is going to make them even more complicated than they are now...) Also, I can't help thinking that the corresponding *_map() and *_unmap() routines are so similar, it ought to be possible to combine them. The only difference is a check for a NULL DMA address, and it's not clear to me why it is present. It's also not clear why the test for a DMA address of all ones is present. Maybe they both can be removed. Alan Stern
Alan Stern wrote: >> --- a/drivers/usb/core/hcd.c >> +++ b/drivers/usb/core/hcd.c >> @@ -1260,6 +1260,34 @@ static void hcd_free_coherent(struct usb_bus *bus, dma_addr_t *dma_handle, >> *dma_handle = 0; >> } >> >> +static int urb_needs_setup_dma_map(struct usb_hcd *hcd, struct urb *urb) >> +{ >> + return !(urb->transfer_flags & URB_NO_SETUP_DMA_MAP) || >> + ((hcd->driver->flags & HCD_NO_COHERENT_MEM) && >> + urb->setup_dma == ~(dma_addr_t)0); >> +} >> + >> +static int urb_needs_setup_dma_unmap(struct usb_hcd *hcd, struct urb *urb) >> +{ >> + return !(urb->transfer_flags & URB_NO_SETUP_DMA_MAP) || >> + ((hcd->driver->flags & HCD_NO_COHERENT_MEM) && >> + urb->setup_dma && urb->setup_dma != ~(dma_addr_t)0); >> +} >> + >> +static int urb_needs_transfer_dma_map(struct usb_hcd *hcd, struct urb *urb) >> +{ >> + return !(urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP) || >> + ((hcd->driver->flags & HCD_NO_COHERENT_MEM) && >> + urb->transfer_dma == ~(dma_addr_t)0); >> +} >> + >> +static int urb_needs_transfer_dma_unmap(struct usb_hcd *hcd, struct urb *urb) >> +{ >> + return !(urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP) || >> + ((hcd->driver->flags & HCD_NO_COHERENT_MEM) && >> + urb->transfer_dma && urb->transfer_dma != ~(dma_addr_t)0); >> +} >> + > > These functions would be a lot easier to understand if they were > expanded into multiple test and return statements, rather than > squeezing all the Boolean manipulations into single expressions. (Not > to mention the fact that other developement is going to make them even > more complicated than they are now...) > Yes, agreed. I'll enhance that, thanks. > Also, I can't help thinking that the corresponding *_map() and > *_unmap() routines are so similar, it ought to be possible to combine > them. The only difference is a check for a NULL DMA address, and it's > not clear to me why it is present. It's also not clear why the test > for a DMA address of all ones is present. Maybe they both can be > removed. > I think too that I can simplify that logic. I added those checks in a defensive way seeking robustness while I familiarize with the USB stack innards. So far, those cases are just avoiding mappings when urb_needs_transfer_dma_map()/urb_needs_transfer_dma_unmap() are called with urb->transfer_buffer == 0 and urb->transfer_dma == 0. I guess that those cases are related to scatterlist-based urb requests. What should be the correct way to check if a urb has already been scatter/gather-mapped? The final logic would be something like: - map if URB_NO_TRANSFER_DMA_MAP is cleared - otherwise (URB_TRANSFER_NO_DMA_MAP is set so) map if HCD_NO_COHERENT_MEM is set _and_ it's not a scatter/gather request (as that should have been mapped already by usb_buffer_map_sg()) Am I on the right path? > Alan Stern > Thanks, Albert
On Mon, 1 Mar 2010, Albert Herranz wrote: > > Also, I can't help thinking that the corresponding *_map() and > > *_unmap() routines are so similar, it ought to be possible to combine > > them. The only difference is a check for a NULL DMA address, and it's > > not clear to me why it is present. It's also not clear why the test > > for a DMA address of all ones is present. Maybe they both can be > > removed. > > > > I think too that I can simplify that logic. > I added those checks in a defensive way seeking robustness while I > familiarize with the USB stack innards. So far, those cases are just > avoiding mappings when > urb_needs_transfer_dma_map()/urb_needs_transfer_dma_unmap() are > called with urb->transfer_buffer == 0 and urb->transfer_dma == 0. > > I guess that those cases are related to scatterlist-based urb requests. > What should be the correct way to check if a urb has already been > scatter/gather-mapped? If urb->num_sgs > 0 then urb has been s-g mapped. Although we don't currently check for it, quite a few URBs have transfer_buffer_length == 0 (a number of control requests are like this, for example) so they don't need a mapping either. > The final logic would be something like: > - map if URB_NO_TRANSFER_DMA_MAP is cleared > - otherwise (URB_TRANSFER_NO_DMA_MAP is set so) map if > HCD_NO_COHERENT_MEM is set _and_ it's not a scatter/gather request > (as that should have been mapped already by usb_buffer_map_sg()) > > Am I on the right path? More or less. I would do it somewhat differently: If URB_NO_TRANSFER_DMA_MAP is set then no map is needed. Otherwise if num_sgs > 0 then no map is needed. Otherwise if HCD_NO_COHERENT_MEM is set then use hcd_alloc_coherent(). Otherwise if transfer_buffer_length > 0 then use dma_map_single(). Similar logic is needed for the setup buffer mapping, but you can assume that control URBs never use scatter-gather so there's no need to check num_sgs (and there's no need to check the transfer length, since setup packets are always 8 bytes long). In fact, I think URB_NO_SETUP_DMA_MAP doesn't really offer any worthwhile advantages. (About the only place where multiple control requests are used in rapid succession is during firmware transfers, and those aren't time-constrained.) It is currently used in a few drivers, but we ought to be able to remove it without too much effort. That might make a good project. Alan Stern
Alan Stern wrote: > If urb->num_sgs > 0 then urb has been s-g mapped. Although we don't > currently check for it, quite a few URBs have transfer_buffer_length == > 0 (a number of control requests are like this, for example) so they > don't need a mapping either. > Ok, I'll use urb->num_sgs > 0 to check for s-g mapped requests. >> The final logic would be something like: >> - map if URB_NO_TRANSFER_DMA_MAP is cleared >> - otherwise (URB_TRANSFER_NO_DMA_MAP is set so) map if >> HCD_NO_COHERENT_MEM is set _and_ it's not a scatter/gather request >> (as that should have been mapped already by usb_buffer_map_sg()) >> >> Am I on the right path? > > More or less. I would do it somewhat differently: > > If URB_NO_TRANSFER_DMA_MAP is set then no map is needed. > Otherwise if num_sgs > 0 then no map is needed. > Otherwise if HCD_NO_COHERENT_MEM is set then use > hcd_alloc_coherent(). > Otherwise if transfer_buffer_length > 0 then use > dma_map_single(). > I think that logic is not quite right. Remember that the final goal is to avoid allocating USB buffers from coherent memory (because the USB drivers access USB buffers without access restrictions but the platform I'm working on can't write to coherent memory unless it is done in 32-bit chunks). And we want to avoid bouncing at the USB layer too (that's what v1 did). The strategy so far is: - we have modified the USB buffer allocation routines hcd_buffer_alloc()/hcd_buffer_free() to always return normal kernel memory when HCD_NO_COHERENT_MEM is set on the host controller. - during map_urb_for_dma()/unmap_urb_for_dma() we need to make sure that those USB buffers are sync'ed, even if we are told USB_NO_{SETUP,TRANSFER}_DMA_MAP So the logic would be: If URB_NO_TRANSFER_DMA_MAP is _cleared_ then do the mapping - this case covers normal kernel memory used as a buffer and not already DMA mapped by a USB driver Otherwise if HCD_NO_COHERENT_MEM is set _and_ num_sgs == 0 _and_ transfer_buffer_length > 0 then do the mapping too - this case covers USB buffers allocated via usb_buffer_alloc() and marked URB_NO_TRANSFER_DMA_MAP by a USB driver, which are allocated from normal kernel memory when HCD_NO_COHERENT_MEM is set (we avoid bouncing buffers here too, at least if they sit already within MEM2 in the Wii, but anyway that's part of the platform DMA mapping code) s-g urbs do not need a mapping as they have already been mapped, marked URB_NO_TRANSFER_DMA_MAP and have num_sgs > 0 > Similar logic is needed for the setup buffer mapping, but you can > assume that control URBs never use scatter-gather so there's no need to > check num_sgs (and there's no need to check the transfer length, since > setup packets are always 8 bytes long). > > In fact, I think URB_NO_SETUP_DMA_MAP doesn't really offer any > worthwhile advantages. (About the only place where multiple control > requests are used in rapid succession is during firmware transfers, and > those aren't time-constrained.) It is currently used in a few > drivers, but we ought to be able to remove it without too much effort. > That might make a good project. > I'll leave that projects to others or, in any case, address it later :) > Alan Stern > Thanks, Albert
On Mon, 1 Mar 2010, Albert Herranz wrote: > >> Am I on the right path? > > > > More or less. I would do it somewhat differently: > > > > If URB_NO_TRANSFER_DMA_MAP is set then no map is needed. > > Otherwise if num_sgs > 0 then no map is needed. > > Otherwise if HCD_NO_COHERENT_MEM is set then use > > hcd_alloc_coherent(). > > Otherwise if transfer_buffer_length > 0 then use > > dma_map_single(). > > > > I think that logic is not quite right. > Remember that the final goal is to avoid allocating USB buffers from coherent memory (because the USB drivers access USB buffers without access restrictions but the platform I'm working on can't write to coherent memory unless it is done in 32-bit chunks). Actually the final goal is to make the mapping/unmapping algorithms clear and correct. One of the subgoals involves avoiding coherent USB buffers, but there are others as well (if you look back the through the linux-usb mailing list for the last few weeks you'll see a discussion about a controller which has to use PIO for control transfers but can use DMA for other types). > And we want to avoid bouncing at the USB layer too (that's what v1 did). > > The strategy so far is: > - we have modified the USB buffer allocation routines hcd_buffer_alloc()/hcd_buffer_free() to always return normal kernel memory when HCD_NO_COHERENT_MEM is set on the host controller. > - during map_urb_for_dma()/unmap_urb_for_dma() we need to make sure that those USB buffers are sync'ed, even if we are told USB_NO_{SETUP,TRANSFER}_DMA_MAP > > So the logic would be: > > If URB_NO_TRANSFER_DMA_MAP is _cleared_ then do the mapping No, that's wrong because it ignores the HCD_LOCAL_MEM flag. > - this case covers normal kernel memory used as a buffer and not already DMA mapped by a USB driver > > Otherwise if HCD_NO_COHERENT_MEM is set _and_ num_sgs == 0 _and_ transfer_buffer_length > 0 then do the mapping too > - this case covers USB buffers allocated via usb_buffer_alloc() and marked URB_NO_TRANSFER_DMA_MAP by a USB driver, which are allocated from normal kernel memory when HCD_NO_COHERENT_MEM is set (we avoid bouncing buffers here too, at least if they sit already within MEM2 in the Wii, but anyway that's part of the platform DMA mapping code) > > s-g urbs do not need a mapping as they have already been mapped, marked URB_NO_TRANSFER_DMA_MAP and have num_sgs > 0 Actually the test for transfer_buffer_length == 0 should be done first, since obviously no mapping is needed if there's no data. (And in fact the current code does do this; I was wrong earlier when I said it doesn't.) So let's make things a little easier by first testing the conditions under which no mapping is needed: 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 are both set (this avoids your HCD_NO_COHERENT_MEM case) then do nothing. The remaining cases all need mapping and/or bounce buffers: Otherwise if HCD_LOCAL_MEM is set then call hcd_alloc_coherent. Otherwise call dma_map_single. Finally, the unmapping tests can be simplified greatly if the kind of mapping is recorded in the URB flags. Alan Stern
Alan Stern wrote: > On Mon, 1 Mar 2010, Albert Herranz wrote: > >>>> Am I on the right path? >>> More or less. I would do it somewhat differently: >>> >>> If URB_NO_TRANSFER_DMA_MAP is set then no map is needed. >>> Otherwise if num_sgs > 0 then no map is needed. >>> Otherwise if HCD_NO_COHERENT_MEM is set then use >>> hcd_alloc_coherent(). >>> Otherwise if transfer_buffer_length > 0 then use >>> dma_map_single(). >>> >> I think that logic is not quite right. >> Remember that the final goal is to avoid allocating USB buffers from coherent memory (because the USB drivers access USB buffers without access restrictions but the platform I'm working on can't write to coherent memory unless it is done in 32-bit chunks). > > Actually the final goal is to make the mapping/unmapping algorithms > clear and correct. One of the subgoals involves avoiding coherent USB > buffers, but there are others as well (if you look back the through the > linux-usb mailing list for the last few weeks you'll see a discussion > about a controller which has to use PIO for control transfers but can > use DMA for other types). > Well, I was talking about our particular case here. I did not imply that we should forget about the other cases. >> And we want to avoid bouncing at the USB layer too (that's what v1 did). >> >> The strategy so far is: >> - we have modified the USB buffer allocation routines hcd_buffer_alloc()/hcd_buffer_free() to always return normal kernel memory when HCD_NO_COHERENT_MEM is set on the host controller. >> - during map_urb_for_dma()/unmap_urb_for_dma() we need to make sure that those USB buffers are sync'ed, even if we are told USB_NO_{SETUP,TRANSFER}_DMA_MAP >> >> So the logic would be: >> >> If URB_NO_TRANSFER_DMA_MAP is _cleared_ then do the mapping > > No, that's wrong because it ignores the HCD_LOCAL_MEM flag. > When I said "do the mapping" there I meant to do a dma_map_single() if self.uses_dma, else if HCD_LOCAL_MEM is set then do a hcd_alloc_coherent(). I should have been more clear on that. >> - this case covers normal kernel memory used as a buffer and not already DMA mapped by a USB driver >> >> Otherwise if HCD_NO_COHERENT_MEM is set _and_ num_sgs == 0 _and_ transfer_buffer_length > 0 then do the mapping too >> - this case covers USB buffers allocated via usb_buffer_alloc() and marked URB_NO_TRANSFER_DMA_MAP by a USB driver, which are allocated from normal kernel memory when HCD_NO_COHERENT_MEM is set (we avoid bouncing buffers here too, at least if they sit already within MEM2 in the Wii, but anyway that's part of the platform DMA mapping code) >> >> s-g urbs do not need a mapping as they have already been mapped, marked URB_NO_TRANSFER_DMA_MAP and have num_sgs > 0 > > Actually the test for transfer_buffer_length == 0 should be done first, > since obviously no mapping is needed if there's no data. (And in fact > the current code does do this; I was wrong earlier when I said it > doesn't.) > > So let's make things a little easier by first testing the conditions > under which no mapping is needed: > > 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 > are both set (this avoids your HCD_NO_COHERENT_MEM > case) then do nothing. > I see. This case would include the PIO case too (for which dma_handle is set to all 1s). So this assumes that transfer_dma should be set initially to 0 when allocating USB buffers for HCD_NO_COHERENT_MEM. > The remaining cases all need mapping and/or bounce buffers: > > Otherwise if HCD_LOCAL_MEM is set then call hcd_alloc_coherent. > Otherwise call dma_map_single. > > Finally, the unmapping tests can be simplified greatly if the kind of > mapping is recorded in the URB flags. > Good point. > Alan Stern > Thanks for your comments, Albert
On Mon, 1 Mar 2010, Albert Herranz wrote: > > 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 > > are both set (this avoids your HCD_NO_COHERENT_MEM > > case) then do nothing. > > > > I see. This case would include the PIO case too (for which dma_handle > is set to all 1s). The test above should be transfer_dma != ~0, not transfer_dma != 0, since ~0 means the DMA address isn't set. In fact I forgot to include the PIO case; it should be handled by changing the remaining tests as follows: Otherwise if hcd->self.uses_dma is set then If this URB doesn't require PIO then call dma_map_single Otherwise if HCD_LOCAL_MEM is set then call hcd_alloc_coherent Otherwise do nothing (PIO case). Currently "this URB doesn't require PIO" is always true, but in the future it won't be. > So this assumes that transfer_dma should be set initially to 0 when > allocating USB buffers for HCD_NO_COHERENT_MEM. No, it should be set to ~0, the same as when buffers are allocated for a PIO-based controller. Alan Stern
Alan Stern wrote: > On Mon, 1 Mar 2010, Albert Herranz wrote: > >>> 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 >>> are both set (this avoids your HCD_NO_COHERENT_MEM >>> case) then do nothing. >>> >> I see. This case would include the PIO case too (for which dma_handle >> is set to all 1s). > > The test above should be transfer_dma != ~0, not transfer_dma != 0, > since ~0 means the DMA address isn't set. In fact I forgot to > include the PIO case; it should be handled by changing the remaining > tests as follows: > > Otherwise if hcd->self.uses_dma is set then > If this URB doesn't require PIO then call dma_map_single > Otherwise if HCD_LOCAL_MEM is set then call hcd_alloc_coherent > Otherwise do nothing (PIO case). > > Currently "this URB doesn't require PIO" is always true, but in the > future it won't be. > Can this be currently tested? Should I make provisions for this check now too? >> So this assumes that transfer_dma should be set initially to 0 when >> allocating USB buffers for HCD_NO_COHERENT_MEM. > > No, it should be set to ~0, the same as when buffers are allocated for > a PIO-based controller. > This logic now resembles more the one in my v2 proposal although with different formal checks. I think I'll code and post another iteration of the 8/9 patch with your proposed checks and then we can continue further discussion on it. I'll try to add explanatory comments for each check. > Alan Stern > Thanks a lot for your input, Albert
On Tue, 2 Mar 2010, Albert Herranz wrote: > > Currently "this URB doesn't require PIO" is always true, but in the > > future it won't be. > > > > Can this be currently tested? Not yet, but soon. You can follow the gory details in this email thread: http://marc.info/?l=linux-usb&m=126477569707174&w=2 See especially this email: http://marc.info/?l=linux-usb&m=126630714002200&w=2 > Should I make provisions for this check now too? Don't worry about it. If you structure the code as I described, it will be easy to insert the check later on. Alan Stern
diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c index 3ba2fff..fede7a0 100644 --- a/drivers/usb/core/buffer.c +++ b/drivers/usb/core/buffer.c @@ -53,8 +53,9 @@ int hcd_buffer_create(struct usb_hcd *hcd) char name[16]; int i, size; - if (!hcd->self.controller->dma_mask && - !(hcd->driver->flags & HCD_LOCAL_MEM)) + if ((!hcd->self.controller->dma_mask && + !(hcd->driver->flags & HCD_LOCAL_MEM)) || + (hcd->driver->flags & HCD_NO_COHERENT_MEM)) return 0; for (i = 0; i < HCD_BUFFER_POOLS; i++) { @@ -109,8 +110,9 @@ void *hcd_buffer_alloc( int i; /* some USB hosts just use PIO */ - 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)) { *dma = ~(dma_addr_t) 0; return kmalloc(size, mem_flags); } @@ -135,8 +137,9 @@ void hcd_buffer_free( if (!addr) return; - 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; } diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c index 80995ef..c2596c5 100644 --- a/drivers/usb/core/hcd.c +++ b/drivers/usb/core/hcd.c @@ -1260,6 +1260,34 @@ static void hcd_free_coherent(struct usb_bus *bus, dma_addr_t *dma_handle, *dma_handle = 0; } +static int urb_needs_setup_dma_map(struct usb_hcd *hcd, struct urb *urb) +{ + return !(urb->transfer_flags & URB_NO_SETUP_DMA_MAP) || + ((hcd->driver->flags & HCD_NO_COHERENT_MEM) && + urb->setup_dma == ~(dma_addr_t)0); +} + +static int urb_needs_setup_dma_unmap(struct usb_hcd *hcd, struct urb *urb) +{ + return !(urb->transfer_flags & URB_NO_SETUP_DMA_MAP) || + ((hcd->driver->flags & HCD_NO_COHERENT_MEM) && + urb->setup_dma && urb->setup_dma != ~(dma_addr_t)0); +} + +static int urb_needs_transfer_dma_map(struct usb_hcd *hcd, struct urb *urb) +{ + return !(urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP) || + ((hcd->driver->flags & HCD_NO_COHERENT_MEM) && + urb->transfer_dma == ~(dma_addr_t)0); +} + +static int urb_needs_transfer_dma_unmap(struct usb_hcd *hcd, struct urb *urb) +{ + return !(urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP) || + ((hcd->driver->flags & HCD_NO_COHERENT_MEM) && + urb->transfer_dma && urb->transfer_dma != ~(dma_addr_t)0); +} + static int map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flags) { @@ -1275,7 +1303,7 @@ static int map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb, return 0; if (usb_endpoint_xfer_control(&urb->ep->desc) - && !(urb->transfer_flags & URB_NO_SETUP_DMA_MAP)) { + && urb_needs_setup_dma_map(hcd, urb)) { if (hcd->self.uses_dma) { urb->setup_dma = dma_map_single( hcd->self.controller, @@ -1296,7 +1324,7 @@ static int map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb, dir = usb_urb_dir_in(urb) ? DMA_FROM_DEVICE : DMA_TO_DEVICE; if (ret == 0 && urb->transfer_buffer_length != 0 - && !(urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP)) { + && urb_needs_transfer_dma_map(hcd, urb)) { if (hcd->self.uses_dma) { urb->transfer_dma = dma_map_single ( hcd->self.controller, @@ -1334,12 +1362,15 @@ static void unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb) return; if (usb_endpoint_xfer_control(&urb->ep->desc) - && !(urb->transfer_flags & URB_NO_SETUP_DMA_MAP)) { - if (hcd->self.uses_dma) + && urb_needs_setup_dma_unmap(hcd, urb)) { + if (hcd->self.uses_dma) { dma_unmap_single(hcd->self.controller, urb->setup_dma, sizeof(struct usb_ctrlrequest), DMA_TO_DEVICE); - else if (hcd->driver->flags & HCD_LOCAL_MEM) + if ((hcd->driver->flags & HCD_NO_COHERENT_MEM) && + (urb->transfer_flags & URB_NO_SETUP_DMA_MAP)) + urb->setup_dma = ~(dma_addr_t)0; + } else if (hcd->driver->flags & HCD_LOCAL_MEM) hcd_free_coherent(urb->dev->bus, &urb->setup_dma, (void **)&urb->setup_packet, sizeof(struct usb_ctrlrequest), @@ -1348,13 +1379,16 @@ static void unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb) dir = usb_urb_dir_in(urb) ? DMA_FROM_DEVICE : DMA_TO_DEVICE; if (urb->transfer_buffer_length != 0 - && !(urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP)) { - if (hcd->self.uses_dma) + && urb_needs_transfer_dma_unmap(hcd, urb)) { + if (hcd->self.uses_dma) { dma_unmap_single(hcd->self.controller, urb->transfer_dma, urb->transfer_buffer_length, dir); - else if (hcd->driver->flags & HCD_LOCAL_MEM) + if ((hcd->driver->flags & HCD_NO_COHERENT_MEM) && + (urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP)) + urb->transfer_dma = ~(dma_addr_t)0; + } else if (hcd->driver->flags & HCD_LOCAL_MEM) hcd_free_coherent(urb->dev->bus, &urb->transfer_dma, &urb->transfer_buffer, urb->transfer_buffer_length, diff --git a/drivers/usb/core/hcd.h b/drivers/usb/core/hcd.h index bbe2b92..cde42f3 100644 --- a/drivers/usb/core/hcd.h +++ b/drivers/usb/core/hcd.h @@ -183,12 +183,13 @@ struct hc_driver { irqreturn_t (*irq) (struct usb_hcd *hcd); int flags; -#define HCD_MEMORY 0x0001 /* HC regs use memory (else I/O) */ -#define HCD_LOCAL_MEM 0x0002 /* HC needs local memory */ -#define HCD_USB11 0x0010 /* USB 1.1 */ -#define HCD_USB2 0x0020 /* USB 2.0 */ -#define HCD_USB3 0x0040 /* USB 3.0 */ -#define HCD_MASK 0x0070 +#define HCD_MEMORY 0x0001 /* HC regs use memory (else I/O) */ +#define HCD_LOCAL_MEM 0x0002 /* HC needs local memory */ +#define HCD_NO_COHERENT_MEM 0x0004 /* HC avoids use of "coherent" mem */ +#define HCD_USB11 0x0010 /* USB 1.1 */ +#define HCD_USB2 0x0020 /* USB 2.0 */ +#define HCD_USB3 0x0040 /* USB 3.0 */ +#define HCD_MASK 0x0070 /* called to init HCD and root hub */ int (*reset) (struct usb_hcd *hcd);
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 flag is useful to overcome some esoteric memory access restrictions found in some platforms. For example, the Nintendo Wii video game console is a NOT_COHERENT_CACHE platform that is unable to safely perform non-32 bit uncached writes to RAM because the byte enables are not connected to the bus. Thus, in that platform, "coherent" DMA buffers cannot be directly used by the kernel code unless it guarantees that all write accesses to said buffers are done in 32 bit chunks (which is not the case in the USB subsystem). To avoid this unwanted behaviour HCD_NO_COHERENT_MEM can be enabled at the HCD controller, causing USB buffer allocations to be satisfied from normal kernel memory. In this case, the USB stack will make sure that the buffers get properly mapped/unmapped for DMA transfers using the DMA streaming mapping API. Note that other parts of the USB stack may also use coherent memory, like for example the hardware descriptors used in the EHCI controllers. This needs to be checked and addressed separately. In the EHCI example, hardware descriptors are accessed in 32-bit (or 64-bit) chunks, causing no problems in the described scenario. Signed-off-by: Albert Herranz <albert_herranz@yahoo.es> --- drivers/usb/core/buffer.c | 15 ++++++++----- drivers/usb/core/hcd.c | 50 +++++++++++++++++++++++++++++++++++++------- drivers/usb/core/hcd.h | 13 ++++++----- 3 files changed, 58 insertions(+), 20 deletions(-)