| Message ID | 20210513223203.5542-16-logang@deltatee.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Add new DMA mapping operation for P2PDMA | expand |
> + for_each_sg(sgl, sg, nents, i) { > + if (sg_is_dma_pci_p2pdma(sg)) { > + sg_dma_unmark_pci_p2pdma(sg); > + } else { Double space here. We also don't really need the curly braces to start with. > + struct pci_p2pdma_map_state p2pdma_state = {}; > + enum pci_p2pdma_map_type map; > struct scatterlist *sg; > + int i, ret; > > for_each_sg(sgl, sg, nents, i) { > + if (is_pci_p2pdma_page(sg_page(sg))) { > + map = pci_p2pdma_map_segment(&p2pdma_state, dev, sg); > + switch (map) { Why not just: switch (pci_p2pdma_map_segment(&p2pdma_state, dev, sg)) { (even better with a shorter name for p2pdma_state so that it all fits on a single line)? > + case PCI_P2PDMA_MAP_BUS_ADDR: > + continue; > + case PCI_P2PDMA_MAP_THRU_HOST_BRIDGE: > + /* > + * Mapping through host bridge should be > + * mapped normally, thus we do nothing > + * and continue below. > + */ I have a bit of a hard time parsing this comment. > + if (sg->dma_address == DMA_MAPPING_ERROR) { > + ret = -EINVAL; > goto out_unmap; > + } > sg_dma_len(sg) = sg->length; > } > > @@ -411,7 +443,7 @@ int dma_direct_map_sg(struct device *dev, struct scatterlist *sgl, int nents, > > out_unmap: > dma_direct_unmap_sg(dev, sgl, i, dir, attrs | DMA_ATTR_SKIP_CPU_SYNC); > - return -EINVAL; > + return ret; Maybe just initialize ret to -EINVAL at declaration time to simplify this a bit?
On 2021-05-14 7:57 a.m., Christoph Hellwig wrote: >> + for_each_sg(sgl, sg, nents, i) { >> + if (sg_is_dma_pci_p2pdma(sg)) { >> + sg_dma_unmark_pci_p2pdma(sg); >> + } else { > > Double space here. We also don't really need the curly braces to start > with. > >> + struct pci_p2pdma_map_state p2pdma_state = {}; >> + enum pci_p2pdma_map_type map; >> struct scatterlist *sg; >> + int i, ret; >> >> for_each_sg(sgl, sg, nents, i) { >> + if (is_pci_p2pdma_page(sg_page(sg))) { >> + map = pci_p2pdma_map_segment(&p2pdma_state, dev, sg); >> + switch (map) { > > Why not just: > > switch (pci_p2pdma_map_segment(&p2pdma_state, dev, > sg)) { > > (even better with a shorter name for p2pdma_state so that it all fits on > a single line)? > >> + case PCI_P2PDMA_MAP_BUS_ADDR: >> + continue; >> + case PCI_P2PDMA_MAP_THRU_HOST_BRIDGE: >> + /* >> + * Mapping through host bridge should be >> + * mapped normally, thus we do nothing >> + * and continue below. >> + */ > > I have a bit of a hard time parsing this comment. > >> + if (sg->dma_address == DMA_MAPPING_ERROR) { >> + ret = -EINVAL; >> goto out_unmap; >> + } >> sg_dma_len(sg) = sg->length; >> } >> >> @@ -411,7 +443,7 @@ int dma_direct_map_sg(struct device *dev, struct scatterlist *sgl, int nents, >> >> out_unmap: >> dma_direct_unmap_sg(dev, sgl, i, dir, attrs | DMA_ATTR_SKIP_CPU_SYNC); >> - return -EINVAL; >> + return ret; > > Maybe just initialize ret to -EINVAL at declaration time to simplify this > a bit? > All fair points, will fix in v3. Logan
diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c index 803ee9321170..567dac942e89 100644 --- a/kernel/dma/direct.c +++ b/kernel/dma/direct.c @@ -13,6 +13,7 @@ #include <linux/vmalloc.h> #include <linux/set_memory.h> #include <linux/slab.h> +#include <linux/pci-p2pdma.h> #include "direct.h" /* @@ -381,29 +382,60 @@ void dma_direct_sync_sg_for_cpu(struct device *dev, arch_sync_dma_for_cpu_all(); } +/* + * Unmaps segments, except for ones marked as pci_p2pdma which do not + * require any further action as they contain a bus address. + */ void dma_direct_unmap_sg(struct device *dev, struct scatterlist *sgl, int nents, enum dma_data_direction dir, unsigned long attrs) { struct scatterlist *sg; int i; - for_each_sg(sgl, sg, nents, i) - dma_direct_unmap_page(dev, sg->dma_address, sg_dma_len(sg), dir, - attrs); + for_each_sg(sgl, sg, nents, i) { + if (sg_is_dma_pci_p2pdma(sg)) { + sg_dma_unmark_pci_p2pdma(sg); + } else { + dma_direct_unmap_page(dev, sg->dma_address, + sg_dma_len(sg), dir, attrs); + } + } } #endif int dma_direct_map_sg(struct device *dev, struct scatterlist *sgl, int nents, enum dma_data_direction dir, unsigned long attrs) { - int i; + struct pci_p2pdma_map_state p2pdma_state = {}; + enum pci_p2pdma_map_type map; struct scatterlist *sg; + int i, ret; for_each_sg(sgl, sg, nents, i) { + if (is_pci_p2pdma_page(sg_page(sg))) { + map = pci_p2pdma_map_segment(&p2pdma_state, dev, sg); + switch (map) { + case PCI_P2PDMA_MAP_BUS_ADDR: + continue; + case PCI_P2PDMA_MAP_THRU_HOST_BRIDGE: + /* + * Mapping through host bridge should be + * mapped normally, thus we do nothing + * and continue below. + */ + break; + default: + ret = -EREMOTEIO; + goto out_unmap; + } + } + sg->dma_address = dma_direct_map_page(dev, sg_page(sg), sg->offset, sg->length, dir, attrs); - if (sg->dma_address == DMA_MAPPING_ERROR) + if (sg->dma_address == DMA_MAPPING_ERROR) { + ret = -EINVAL; goto out_unmap; + } sg_dma_len(sg) = sg->length; } @@ -411,7 +443,7 @@ int dma_direct_map_sg(struct device *dev, struct scatterlist *sgl, int nents, out_unmap: dma_direct_unmap_sg(dev, sgl, i, dir, attrs | DMA_ATTR_SKIP_CPU_SYNC); - return -EINVAL; + return ret; } dma_addr_t dma_direct_map_resource(struct device *dev, phys_addr_t paddr,
Add PCI P2PDMA support for dma_direct_map_sg() so that it can map PCI P2PDMA pages directly without a hack in the callers. This allows for heterogeneous SGLs that contain both P2PDMA and regular pages. A P2PDMA page may have three possible outcomes when being mapped: 1) If the data path between the two devices doesn't go through the root port, then it should be mapped with a PCI bus address 2) If the data path goes through the host bridge, it should be mapped normally, as though it were a CPU physical address 3) It is not possible for the two devices to communicate and thus the mapping operation should fail (and it will return -EREMOTEIO). SGL segments that contain PCI bus addresses are marked with sg_dma_mark_pci_p2pdma() and are ignored when unmapped. Signed-off-by: Logan Gunthorpe <logang@deltatee.com> --- kernel/dma/direct.c | 44 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 38 insertions(+), 6 deletions(-)