| Message ID | 20210311233142.7900-5-logang@deltatee.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Add support to dma_map_sg for P2PDMA | expand |
On Thu, Mar 11, 2021 at 04:31:34PM -0700, Logan Gunthorpe wrote: > Introduce pci_p2pdma_should_map_bus() which is meant to be called by > DMA map functions to determine how to map a given p2pdma page. > > pci_p2pdma_bus_offset() is also added to allow callers to get the bus > offset if they need to map the bus address. > > Signed-off-by: Logan Gunthorpe <logang@deltatee.com> > --- > drivers/pci/p2pdma.c | 50 ++++++++++++++++++++++++++++++++++++++ > include/linux/pci-p2pdma.h | 11 +++++++++ > 2 files changed, 61 insertions(+) > > diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c > index 7f6836732bce..66d16b7eb668 100644 > --- a/drivers/pci/p2pdma.c > +++ b/drivers/pci/p2pdma.c > @@ -912,6 +912,56 @@ void pci_p2pdma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg, > } > EXPORT_SYMBOL_GPL(pci_p2pdma_unmap_sg_attrs); > > +/** > + * pci_p2pdma_bus_offset - returns the bus offset for a given page > + * @page: page to get the offset for > + * > + * Must be passed a PCI p2pdma page. > + */ > +u64 pci_p2pdma_bus_offset(struct page *page) > +{ > + struct pci_p2pdma_pagemap *p2p_pgmap = to_p2p_pgmap(page->pgmap); > + > + WARN_ON(!is_pci_p2pdma_page(page)); Shouldn't this check be before the to_p2p_pgmap() call? And I've been told not to introduce WARN_ON's. Should this be? if (!is_pci_p2pdma_page(page)) return -1; ??? Ira
On Thu, Mar 11, 2021 at 04:31:34PM -0700, Logan Gunthorpe wrote: > Introduce pci_p2pdma_should_map_bus() which is meant to be called by ^^^^^^^^^^^^^^^^^^^^^^^^^ pci_p2pdma_dma_map_type() ??? FWIW I find this name confusing with pci_p2pdma_map_type() and looking at the implementation I'm not clear why pci_p2pdma_dma_map_type() needs to exist? Ira [snip] > + > +/** > + * pci_p2pdma_dma_map_type - determine if a DMA mapping should use the > + * bus address, be mapped normally or fail > + * @dev: device doing the DMA request > + * @pgmap: dev_pagemap structure for the mapping > + * > + * Returns: > + * 1 - if the page should be mapped with a bus address, > + * 0 - if the page should be mapped normally through an IOMMU mapping or > + * physical address; or > + * -1 - if the device should not map the pages and an error should be > + * returned > + */ > +int pci_p2pdma_dma_map_type(struct device *dev, struct dev_pagemap *pgmap) > +{ > + struct pci_p2pdma_pagemap *p2p_pgmap = to_p2p_pgmap(pgmap); > + struct pci_dev *client; > + > + if (!dev_is_pci(dev)) > + return -1; > + > + client = to_pci_dev(dev); > + > + switch (pci_p2pdma_map_type(p2p_pgmap->provider, client)) { > + case PCI_P2PDMA_MAP_THRU_HOST_BRIDGE: > + return 0; > + case PCI_P2PDMA_MAP_BUS_ADDR: > + return 1; > + default: > + return -1; > + } > +} > +EXPORT_SYMBOL_GPL(pci_p2pdma_dma_map_type); I guess the main point here is to export this to the DMA layer? Ira
On 2021-03-12 6:38 p.m., Ira Weiny wrote: > On Thu, Mar 11, 2021 at 04:31:34PM -0700, Logan Gunthorpe wrote: >> Introduce pci_p2pdma_should_map_bus() which is meant to be called by >> DMA map functions to determine how to map a given p2pdma page. >> >> pci_p2pdma_bus_offset() is also added to allow callers to get the bus >> offset if they need to map the bus address. >> >> Signed-off-by: Logan Gunthorpe <logang@deltatee.com> >> --- >> drivers/pci/p2pdma.c | 50 ++++++++++++++++++++++++++++++++++++++ >> include/linux/pci-p2pdma.h | 11 +++++++++ >> 2 files changed, 61 insertions(+) >> >> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c >> index 7f6836732bce..66d16b7eb668 100644 >> --- a/drivers/pci/p2pdma.c >> +++ b/drivers/pci/p2pdma.c >> @@ -912,6 +912,56 @@ void pci_p2pdma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg, >> } >> EXPORT_SYMBOL_GPL(pci_p2pdma_unmap_sg_attrs); >> >> +/** >> + * pci_p2pdma_bus_offset - returns the bus offset for a given page >> + * @page: page to get the offset for >> + * >> + * Must be passed a PCI p2pdma page. >> + */ >> +u64 pci_p2pdma_bus_offset(struct page *page) >> +{ >> + struct pci_p2pdma_pagemap *p2p_pgmap = to_p2p_pgmap(page->pgmap); >> + >> + WARN_ON(!is_pci_p2pdma_page(page)); > > Shouldn't this check be before the to_p2p_pgmap() call? The to_p2p_pgmap() call is just doing pointer arithmetic, so strictly speaking it doesn't need to be before. We just can't access p2p_pgmap until it has been checked. > And I've been told not > to introduce WARN_ON's. Should this be? > > if (!is_pci_p2pdma_page(page)) > return -1; In this case the WARN_ON is just to guard against misuse of the function. It should never happen unless a developer changes the code in a way that is incorrect. So I think that's the correct use of WARN_ON. Though I might change it to WARN and return, that seems safer. Logan
On 2021-03-12 7:32 p.m., Ira Weiny wrote: > On Thu, Mar 11, 2021 at 04:31:34PM -0700, Logan Gunthorpe wrote: >> Introduce pci_p2pdma_should_map_bus() which is meant to be called by > ^^^^^^^^^^^^^^^^^^^^^^^^^ > pci_p2pdma_dma_map_type() ??? > > FWIW I find this name confusing with pci_p2pdma_map_type() and looking at the > implementation I'm not clear why pci_p2pdma_dma_map_type() needs to exist? Yeah, there are subtle differences in prototype. But yes, they can probably be combined. Will do for future postings. >> + * pci_p2pdma_dma_map_type - determine if a DMA mapping should use the >> + * bus address, be mapped normally or fail >> + * @dev: device doing the DMA request >> + * @pgmap: dev_pagemap structure for the mapping >> + * >> + * Returns: >> + * 1 - if the page should be mapped with a bus address, >> + * 0 - if the page should be mapped normally through an IOMMU mapping or >> + * physical address; or >> + * -1 - if the device should not map the pages and an error should be >> + * returned >> + */ >> +int pci_p2pdma_dma_map_type(struct device *dev, struct dev_pagemap *pgmap) >> +{ >> + struct pci_p2pdma_pagemap *p2p_pgmap = to_p2p_pgmap(pgmap); >> + struct pci_dev *client; >> + >> + if (!dev_is_pci(dev)) >> + return -1; >> + >> + client = to_pci_dev(dev); >> + >> + switch (pci_p2pdma_map_type(p2p_pgmap->provider, client)) { >> + case PCI_P2PDMA_MAP_THRU_HOST_BRIDGE: >> + return 0; >> + case PCI_P2PDMA_MAP_BUS_ADDR: >> + return 1; >> + default: >> + return -1; >> + } >> +} >> +EXPORT_SYMBOL_GPL(pci_p2pdma_dma_map_type); > > I guess the main point here is to export this to the DMA layer? Yes, that's correct. Logan
On Thu, Mar 11, 2021 at 04:31:34PM -0700, Logan Gunthorpe wrote: > Introduce pci_p2pdma_should_map_bus() which is meant to be called by > DMA map functions to determine how to map a given p2pdma page. > > pci_p2pdma_bus_offset() is also added to allow callers to get the bus > offset if they need to map the bus address. > > Signed-off-by: Logan Gunthorpe <logang@deltatee.com> > --- > drivers/pci/p2pdma.c | 50 ++++++++++++++++++++++++++++++++++++++ > include/linux/pci-p2pdma.h | 11 +++++++++ > 2 files changed, 61 insertions(+) > > diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c > index 7f6836732bce..66d16b7eb668 100644 > --- a/drivers/pci/p2pdma.c > +++ b/drivers/pci/p2pdma.c > @@ -912,6 +912,56 @@ void pci_p2pdma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg, > } > EXPORT_SYMBOL_GPL(pci_p2pdma_unmap_sg_attrs); > > +/** > + * pci_p2pdma_bus_offset - returns the bus offset for a given page > + * @page: page to get the offset for > + * > + * Must be passed a PCI p2pdma page. > + */ > +u64 pci_p2pdma_bus_offset(struct page *page) > +{ > + struct pci_p2pdma_pagemap *p2p_pgmap = to_p2p_pgmap(page->pgmap); > + > + WARN_ON(!is_pci_p2pdma_page(page)); > + > + return p2p_pgmap->bus_offset; > +} > +EXPORT_SYMBOL_GPL(pci_p2pdma_bus_offset); I don't see why this would be exported. > +EXPORT_SYMBOL_GPL(pci_p2pdma_dma_map_type); Same here. Also the two helpers don't really look very related. I suspect you really want to move the p2p handling bits currenly added to kernel/dma/direct.c into drivers/pci/p2pdma.c instead, as that will allow direct access to the pci_p2pdma_pagemap and should thus simplify things quite a bit.
On Mon, Mar 15, 2021 at 10:27:08AM -0600, Logan Gunthorpe wrote: > In this case the WARN_ON is just to guard against misuse of the > function. It should never happen unless a developer changes the code in > a way that is incorrect. So I think that's the correct use of WARN_ON. > Though I might change it to WARN and return, that seems safer. Right, WARN_ON and return is the right pattern for an assertion that must never happen: if (WARN_ON(foo)) return -1 Linus wants assertions like this to be able to recover. People runing the 'panic on warn' mode want the kernel to stop if it detects an internal malfunction. Jason
Am 24.03.21 um 18:21 schrieb Jason Gunthorpe: > On Mon, Mar 15, 2021 at 10:27:08AM -0600, Logan Gunthorpe wrote: > >> In this case the WARN_ON is just to guard against misuse of the >> function. It should never happen unless a developer changes the code in >> a way that is incorrect. So I think that's the correct use of WARN_ON. >> Though I might change it to WARN and return, that seems safer. > Right, WARN_ON and return is the right pattern for an assertion that > must never happen: > > if (WARN_ON(foo)) > return -1 > > Linus wants assertions like this to be able to recover. People runing > the 'panic on warn' mode want the kernel to stop if it detects an > internal malfunction. The only justification I can see for a "panic on warn" is to prevent further data loss or warn early about a crash. We only use a BUG_ON() when the alternative would be to corrupt something. Christian. > > Jason
diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c index 7f6836732bce..66d16b7eb668 100644 --- a/drivers/pci/p2pdma.c +++ b/drivers/pci/p2pdma.c @@ -912,6 +912,56 @@ void pci_p2pdma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg, } EXPORT_SYMBOL_GPL(pci_p2pdma_unmap_sg_attrs); +/** + * pci_p2pdma_bus_offset - returns the bus offset for a given page + * @page: page to get the offset for + * + * Must be passed a PCI p2pdma page. + */ +u64 pci_p2pdma_bus_offset(struct page *page) +{ + struct pci_p2pdma_pagemap *p2p_pgmap = to_p2p_pgmap(page->pgmap); + + WARN_ON(!is_pci_p2pdma_page(page)); + + return p2p_pgmap->bus_offset; +} +EXPORT_SYMBOL_GPL(pci_p2pdma_bus_offset); + +/** + * pci_p2pdma_dma_map_type - determine if a DMA mapping should use the + * bus address, be mapped normally or fail + * @dev: device doing the DMA request + * @pgmap: dev_pagemap structure for the mapping + * + * Returns: + * 1 - if the page should be mapped with a bus address, + * 0 - if the page should be mapped normally through an IOMMU mapping or + * physical address; or + * -1 - if the device should not map the pages and an error should be + * returned + */ +int pci_p2pdma_dma_map_type(struct device *dev, struct dev_pagemap *pgmap) +{ + struct pci_p2pdma_pagemap *p2p_pgmap = to_p2p_pgmap(pgmap); + struct pci_dev *client; + + if (!dev_is_pci(dev)) + return -1; + + client = to_pci_dev(dev); + + switch (pci_p2pdma_map_type(p2p_pgmap->provider, client)) { + case PCI_P2PDMA_MAP_THRU_HOST_BRIDGE: + return 0; + case PCI_P2PDMA_MAP_BUS_ADDR: + return 1; + default: + return -1; + } +} +EXPORT_SYMBOL_GPL(pci_p2pdma_dma_map_type); + /** * pci_p2pdma_enable_store - parse a configfs/sysfs attribute store * to enable p2pdma diff --git a/include/linux/pci-p2pdma.h b/include/linux/pci-p2pdma.h index 8318a97c9c61..3d55ad19f54c 100644 --- a/include/linux/pci-p2pdma.h +++ b/include/linux/pci-p2pdma.h @@ -34,6 +34,8 @@ int pci_p2pdma_map_sg_attrs(struct device *dev, struct scatterlist *sg, int nents, enum dma_data_direction dir, unsigned long attrs); void pci_p2pdma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg, int nents, enum dma_data_direction dir, unsigned long attrs); +u64 pci_p2pdma_bus_offset(struct page *page); +int pci_p2pdma_dma_map_type(struct device *dev, struct dev_pagemap *pgmap); int pci_p2pdma_enable_store(const char *page, struct pci_dev **p2p_dev, bool *use_p2pdma); ssize_t pci_p2pdma_enable_show(char *page, struct pci_dev *p2p_dev, @@ -83,6 +85,15 @@ static inline void pci_p2pmem_free_sgl(struct pci_dev *pdev, static inline void pci_p2pmem_publish(struct pci_dev *pdev, bool publish) { } +static inline u64 pci_p2pdma_bus_offset(struct page *page) +{ + return -1; +} +static inline int pci_p2pdma_dma_map_type(struct device *dev, + struct dev_pagemap *pgmap) +{ + return -1; +} static inline int pci_p2pdma_map_sg_attrs(struct device *dev, struct scatterlist *sg, int nents, enum dma_data_direction dir, unsigned long attrs)
Introduce pci_p2pdma_should_map_bus() which is meant to be called by DMA map functions to determine how to map a given p2pdma page. pci_p2pdma_bus_offset() is also added to allow callers to get the bus offset if they need to map the bus address. Signed-off-by: Logan Gunthorpe <logang@deltatee.com> --- drivers/pci/p2pdma.c | 50 ++++++++++++++++++++++++++++++++++++++ include/linux/pci-p2pdma.h | 11 +++++++++ 2 files changed, 61 insertions(+)