| Message ID | 1384265520-6833-2-git-send-email-stefano.stabellini@eu.citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Nov 12, 2013 at 02:12:00PM +0000, Stefano Stabellini wrote: > Many ARM devices do not set the dma_mask correctly today. > As a consequence dma_capable fails for them regardless of the address > passed to it. Wouldn't the DMA API debug warn of bad usage. > In xen_swiotlb_map_page we currently use dma_capable to check if the > address returned by the swiotlb is good for dma for the device. Right.. > However the check would often fail even if the address is correct. .. and they will fail b/c the device hasn't set its dma mask so we use the platform default right? What is the platform default? > Considering that we know that the swiotlb buffer has a low address, > skip the check. I am not following that sentence. Could you please explain to me how the SWIOTLB buffer low address guarantees that we don't need the check? > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> What are the drivers that are busted. Does DMA API debug flag them? Thanks! > --- > drivers/xen/swiotlb-xen.c | 7 ------- > 1 files changed, 0 insertions(+), 7 deletions(-) > > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c > index 1eac073..543e30b 100644 > --- a/drivers/xen/swiotlb-xen.c > +++ b/drivers/xen/swiotlb-xen.c > @@ -402,13 +402,6 @@ dma_addr_t xen_swiotlb_map_page(struct device *dev, struct page *page, > map & ~PAGE_MASK, size, dir, attrs); > dev_addr = xen_phys_to_bus(map); > > - /* > - * Ensure that the address returned is DMA'ble > - */ > - if (!dma_capable(dev, dev_addr, size)) { > - swiotlb_tbl_unmap_single(dev, map, size, dir); > - dev_addr = 0; > - } > return dev_addr; > } > EXPORT_SYMBOL_GPL(xen_swiotlb_map_page); > -- > 1.7.2.5 > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
On Tue, Nov 12, 2013 at 09:48:32AM -0500, Konrad Rzeszutek Wilk wrote: > On Tue, Nov 12, 2013 at 02:12:00PM +0000, Stefano Stabellini wrote: > > Many ARM devices do not set the dma_mask correctly today. > > As a consequence dma_capable fails for them regardless of the address > > passed to it. > > Wouldn't the DMA API debug warn of bad usage. No. The problem is that dev->dma_mask is NULL, and that's generally interpreted as "this device doesn't do DMA". ARM drivers have been particularly bad in respect of getting this right: there's an overwhelming problem here of "I don't need to do that so I'm not going to do it" rather than taking the attitude of "it's good practice to do X because it will save me hastles in the future". I'll admit that even I'm guilty of the former in this regard to some extent: I've not added the necessary dma_set_mask/dma_set_coherent_mask() calls to the various drivers because they haven't been a concern. That was wrong, because the DMA API technically requires it. However, not having a DMA mask set when the device is created is something I'm not guilty of - I always ensured with the old board files that a DMA capabable device had a DMA mask set and those which weren't capable didn't: this causes the DMA API to behave correctly and report according to the way the device was declared whether it is DMA capable or not. Unfortunately, others decided (especially post DT) that the solution to this was to have the driver code - which could be modular - do things like this: static u64 mask = blah; foo_probe(dev) { if (!dev->dma_mask) dev->dma_mask = &mask; } which works nicely until you reload the module. I strongly suggest _not_ working around this problem in subsystem code but raising bug reports against the buggy drivers. All drivers without exception using DMA should have something like this pattern: foo_probe(dev) { int rc; rc = dma_set_mask(dev, DMA_BIT_MASK(bits)); if (rc) return rc; dma_set_coherent_mask(dev, DMA_BIT_MASK(bits)); ... } The first call is required if the driver uses dma_map_*, the second call is required if it makes use of coherent allocations and can be of the above form if it's previously used dma_set_mask with the same mask. Otherwise, the return value of dma_set_coherent_mask() must be checked. Of course, dma_set_mask() will fail if dev->dma_mask is NULL - and that's something which must be fixed where the device was created, not by the driver.
Russell gave a great explanation of the issue so I am just going to limit myself to answering to: On Tue, 12 Nov 2013, Konrad Rzeszutek Wilk wrote: > > Considering that we know that the swiotlb buffer has a low address, > > skip the check. > > I am not following that sentence. Could you please explain to me > how the SWIOTLB buffer low address guarantees that we don't need > the check? xen_swiotlb_fixup makes sure that the swiotlb buffer is lower than 4GB, probably lower than 3GB, by passing dma_bits to xen_create_contiguous_region. This meets the requirements of most devices out there. In fact we are not even running this check under the same conditions in swiotlb_map_sg_attrs. I admit that it is possible to come up with a scenario where the check would be useful, but it is far easier to come up with scenarios where not only is unneeded but it is even harmful. Alternatively (without Rob's "of: set dma_mask to point to coherent_dma_mask") Linux 3.13 is going to fail to get the network running on Midway. It is going to avoid fs mounting failures just because we don't do the same check in swiotlb_map_sg_attrs. FYI given that Rob's patch is probably going upstream soon anyway, I don't feel so strongly about this.
On 11/12/2013 11:27 AM, Stefano Stabellini wrote: > Russell gave a great explanation of the issue so I am just going to > limit myself to answering to: > > On Tue, 12 Nov 2013, Konrad Rzeszutek Wilk wrote: >>> Considering that we know that the swiotlb buffer has a low address, >>> skip the check. >> >> I am not following that sentence. Could you please explain to me >> how the SWIOTLB buffer low address guarantees that we don't need >> the check? > > xen_swiotlb_fixup makes sure that the swiotlb buffer is lower than 4GB, > probably lower than 3GB, by passing dma_bits to > xen_create_contiguous_region. > This meets the requirements of most devices out there. > In fact we are not even running this check under the same conditions in > swiotlb_map_sg_attrs. > I admit that it is possible to come up with a scenario where the check > would be useful, but it is far easier to come up with scenarios where > not only is unneeded but it is even harmful. > > Alternatively (without Rob's "of: set dma_mask to point to > coherent_dma_mask") Linux 3.13 is going to fail to get the network > running on Midway. It is going to avoid fs mounting failures just > because we don't do the same check in swiotlb_map_sg_attrs. > > FYI given that Rob's patch is probably going upstream soon anyway, I > don't feel so strongly about this. It is in Linus' tree now. Rob
On Tue, 12 Nov 2013, Rob Herring wrote: > On 11/12/2013 11:27 AM, Stefano Stabellini wrote: > > Russell gave a great explanation of the issue so I am just going to > > limit myself to answering to: > > > > On Tue, 12 Nov 2013, Konrad Rzeszutek Wilk wrote: > >>> Considering that we know that the swiotlb buffer has a low address, > >>> skip the check. > >> > >> I am not following that sentence. Could you please explain to me > >> how the SWIOTLB buffer low address guarantees that we don't need > >> the check? > > > > xen_swiotlb_fixup makes sure that the swiotlb buffer is lower than 4GB, > > probably lower than 3GB, by passing dma_bits to > > xen_create_contiguous_region. > > This meets the requirements of most devices out there. > > In fact we are not even running this check under the same conditions in > > swiotlb_map_sg_attrs. > > I admit that it is possible to come up with a scenario where the check > > would be useful, but it is far easier to come up with scenarios where > > not only is unneeded but it is even harmful. > > > > Alternatively (without Rob's "of: set dma_mask to point to > > coherent_dma_mask") Linux 3.13 is going to fail to get the network > > running on Midway. It is going to avoid fs mounting failures just > > because we don't do the same check in swiotlb_map_sg_attrs. > > > > FYI given that Rob's patch is probably going upstream soon anyway, I > > don't feel so strongly about this. > > It is in Linus' tree now. OK, I'll drop this patch then.
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c index 1eac073..543e30b 100644 --- a/drivers/xen/swiotlb-xen.c +++ b/drivers/xen/swiotlb-xen.c @@ -402,13 +402,6 @@ dma_addr_t xen_swiotlb_map_page(struct device *dev, struct page *page, map & ~PAGE_MASK, size, dir, attrs); dev_addr = xen_phys_to_bus(map); - /* - * Ensure that the address returned is DMA'ble - */ - if (!dma_capable(dev, dev_addr, size)) { - swiotlb_tbl_unmap_single(dev, map, size, dir); - dev_addr = 0; - } return dev_addr; } EXPORT_SYMBOL_GPL(xen_swiotlb_map_page);
Many ARM devices do not set the dma_mask correctly today. As a consequence dma_capable fails for them regardless of the address passed to it. In xen_swiotlb_map_page we currently use dma_capable to check if the address returned by the swiotlb is good for dma for the device. However the check would often fail even if the address is correct. Considering that we know that the swiotlb buffer has a low address, skip the check. Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> --- drivers/xen/swiotlb-xen.c | 7 ------- 1 files changed, 0 insertions(+), 7 deletions(-)