Re: [RFC] remove the ->mapping_error method from dma_map_ops
On Fri, Nov 09, 2018 at 03:12:34PM -0800, David Miller wrote: > But patch #2 on the other hand, not so much. > > I hate seeing values returned by reference, it adds cost especially > on cpus where all argments and return values fit in registers (we end > up forcing a stack slot and memory references). > > And we don't need it here. > > DMA addresses are like pointers, and therefore we can return errors and > valid success values in the same dma_addr_t just fine. PTR_ERR() --> > DMA_ERR(), > IS_PTR_ERR() --> IS_DMA_ERR, etc. In the end this is an inline function, so with a decently smart compiler the generated code shouldn't change too much. The big problem that prompted me to come up with this patch is that not handling failure from dma_map* in a swiotlb setup can lead to grave data corruption, and we have no easy way to force error checking on these return values. I've added a few of the static typechecking suspect if they have a better idea on how to make the return value of dma_map_single/pages in a way that we get warnings if dma_mapping_error isn't called on them. But I can't really think of a good way. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC] remove the ->mapping_error method from dma_map_ops
From: Christoph Hellwig Date: Fri, 9 Nov 2018 09:46:30 +0100 > Error reporting for the dma_map_single and dma_map_page operations is > currently a mess. Both APIs directly return the dma_addr_t to be used for > the DMA, with a magic error escape that is specific to the instance and > checked by another method provided. This has a few downsides: > > - the error check is easily forgotten and a __must_check marker doesn't >help as the value always is consumed anyway > - the error checking requires another indirect call, which have gotten >incredibly expensive > - a lot of code is wasted on implementing these methods > > The historical reason for this is that people thought DMA mappings would > not fail when the API was created, which sounds like a really bad > assumption in retrospective, and then we tried to cram error handling > onto it later on. > > There basically are two variants: the error code is 0 because the > implementation will never return 0 as a valid DMA address, or the error > code is all-F as the implementation won't ever return an address that > high. The old AMD GART is the only one not falling into these two camps > as it picks sort of a relative zero relative to where it is mapped. > > The 0 return doesn't work for a lot of IOMMUs that start allocating > bus space from address zero, so we can't consolidate on that, but I think > we can move everyone to all-Fs, which the patch here does. The reason > for that is that there is only one way to ever get this address: by > doing a 1-byte long, 1-byte aligned mapping, but all our mappings > are not only longer but generally aligned, and the mappings have to > keep at least the basic alignment. Please try to poke holes into this > idea as it would simplify our logic a lot, and also allow to change > at least the not so commonly used yet dma_map_single_attrs and > dma_map_page_attrs to actually return an error code and further improve > the error handling flow in the drivers. I have no problem with patch #1, it looks great. But patch #2 on the other hand, not so much. I hate seeing values returned by reference, it adds cost especially on cpus where all argments and return values fit in registers (we end up forcing a stack slot and memory references). And we don't need it here. DMA addresses are like pointers, and therefore we can return errors and valid success values in the same dma_addr_t just fine. PTR_ERR() --> DMA_ERR(), IS_PTR_ERR() --> IS_DMA_ERR, etc. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[RFC] remove the ->mapping_error method from dma_map_ops
Error reporting for the dma_map_single and dma_map_page operations is currently a mess. Both APIs directly return the dma_addr_t to be used for the DMA, with a magic error escape that is specific to the instance and checked by another method provided. This has a few downsides: - the error check is easily forgotten and a __must_check marker doesn't help as the value always is consumed anyway - the error checking requires another indirect call, which have gotten incredibly expensive - a lot of code is wasted on implementing these methods The historical reason for this is that people thought DMA mappings would not fail when the API was created, which sounds like a really bad assumption in retrospective, and then we tried to cram error handling onto it later on. There basically are two variants: the error code is 0 because the implementation will never return 0 as a valid DMA address, or the error code is all-F as the implementation won't ever return an address that high. The old AMD GART is the only one not falling into these two camps as it picks sort of a relative zero relative to where it is mapped. The 0 return doesn't work for a lot of IOMMUs that start allocating bus space from address zero, so we can't consolidate on that, but I think we can move everyone to all-Fs, which the patch here does. The reason for that is that there is only one way to ever get this address: by doing a 1-byte long, 1-byte aligned mapping, but all our mappings are not only longer but generally aligned, and the mappings have to keep at least the basic alignment. Please try to poke holes into this idea as it would simplify our logic a lot, and also allow to change at least the not so commonly used yet dma_map_single_attrs and dma_map_page_attrs to actually return an error code and further improve the error handling flow in the drivers. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
