| Message ID | 20210408170123.8788-6-logang@deltatee.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add new DMA mapping operation for P2PDMA | expand |
On Thu, Apr 08, 2021 at 11:01:12AM -0600, Logan Gunthorpe wrote: > dma_map_sg() either returns a positive number indicating the number > of entries mapped or zero indicating that resources were not available > to create the mapping. When zero is returned, it is always safe to retry > the mapping later once resources have been freed. > > Once P2PDMA pages are mixed into the SGL there may be pages that may > never be successfully mapped with a given device because that device may > not actually be able to access those pages. Thus, multiple error > conditions will need to be distinguished to determine weather a retry > is safe. > > Introduce dma_map_sg_p2pdma[_attrs]() with a different calling > convention from dma_map_sg(). The function will return a positive > integer on success or a negative errno on failure. > > ENOMEM will be used to indicate a resource failure and EREMOTEIO to > indicate that a P2PDMA page is not mappable. > > The __DMA_ATTR_PCI_P2PDMA attribute is introduced to inform the lower > level implementations that P2PDMA pages are allowed and to warn if a > caller introduces them into the regular dma_map_sg() interface. So this new API is all about being able to return an error code because auditing the old API is basically terrifying? OK, but why name everything new P2PDMA? It seems nicer to give this some generic name and have some general program to gradually deprecate normal non-error-capable dma_map_sg() ? I think that will raise less questions when subsystem people see the changes, as I was wondering why RW was being moved to use what looked like a p2pdma only API. dma_map_sg_or_err() would have been clearer The flag is also clearer as to the purpose if it is named __DMA_ATTR_ERROR_ALLOWED Jason
On Thu, Apr 08, 2021 at 11:01:12AM -0600, Logan Gunthorpe wrote: > +/* > + * dma_maps_sg_attrs returns 0 on error and > 0 on success. > + * It should never return a value < 0. > + */ Also it is weird a function that can't return 0 is returning an int type > +int dma_map_sg_attrs(struct device *dev, struct scatterlist *sg, int nents, > + enum dma_data_direction dir, unsigned long attrs) > +{ > + int ents; > + > + ents = __dma_map_sg_attrs(dev, sg, nents, dir, attrs); > BUG_ON(ents < 0); if (WARN_ON(ents < 0)) return 0; instead of bug on? Also, I see only 8 users of this function. How about just fix them all to support negative returns and use this as the p2p API instead of adding new API? Add the opposite logic flag, 'DMA_ATTRS_NO_ERROR' and pass it through the other api entry callers that can't handle it? Jason
On 2021-04-27 1:22 p.m., Jason Gunthorpe wrote: > On Thu, Apr 08, 2021 at 11:01:12AM -0600, Logan Gunthorpe wrote: >> dma_map_sg() either returns a positive number indicating the number >> of entries mapped or zero indicating that resources were not available >> to create the mapping. When zero is returned, it is always safe to retry >> the mapping later once resources have been freed. >> >> Once P2PDMA pages are mixed into the SGL there may be pages that may >> never be successfully mapped with a given device because that device may >> not actually be able to access those pages. Thus, multiple error >> conditions will need to be distinguished to determine weather a retry >> is safe. >> >> Introduce dma_map_sg_p2pdma[_attrs]() with a different calling >> convention from dma_map_sg(). The function will return a positive >> integer on success or a negative errno on failure. >> >> ENOMEM will be used to indicate a resource failure and EREMOTEIO to >> indicate that a P2PDMA page is not mappable. >> >> The __DMA_ATTR_PCI_P2PDMA attribute is introduced to inform the lower >> level implementations that P2PDMA pages are allowed and to warn if a >> caller introduces them into the regular dma_map_sg() interface. > > So this new API is all about being able to return an error code > because auditing the old API is basically terrifying? > > OK, but why name everything new P2PDMA? It seems nicer to give this > some generic name and have some general program to gradually deprecate > normal non-error-capable dma_map_sg() ? > > I think that will raise less questions when subsystem people see the > changes, as I was wondering why RW was being moved to use what looked > like a p2pdma only API. > > dma_map_sg_or_err() would have been clearer > > The flag is also clearer as to the purpose if it is named > __DMA_ATTR_ERROR_ALLOWED I'm not opposed to these names. I can use them for v2 if there are no other opinions. Logan
On 2021-04-27 1:31 p.m., Jason Gunthorpe wrote: > On Thu, Apr 08, 2021 at 11:01:12AM -0600, Logan Gunthorpe wrote: >> +/* >> + * dma_maps_sg_attrs returns 0 on error and > 0 on success. >> + * It should never return a value < 0. >> + */ > > Also it is weird a function that can't return 0 is returning an int type Yes, Christoph mentioned in the last series that this should probably change to an unsigned but I wasn't really sure if that change should be a part of the P2PDMA series. >> +int dma_map_sg_attrs(struct device *dev, struct scatterlist *sg, int nents, >> + enum dma_data_direction dir, unsigned long attrs) >> +{ >> + int ents; >> + >> + ents = __dma_map_sg_attrs(dev, sg, nents, dir, attrs); >> BUG_ON(ents < 0); > > if (WARN_ON(ents < 0)) > return 0; > > instead of bug on? It was BUG_ON in the original code. So I felt I should leave it. > Also, I see only 8 users of this function. How about just fix them all > to support negative returns and use this as the p2p API instead of > adding new API? Well there might be 8 users of dma_map_sg_attrs() but there are a very large number of dma_map_sg(). Seems odd to me to single out the first as requiring these changes, but leave the latter. > Add the opposite logic flag, 'DMA_ATTRS_NO_ERROR' and pass it through > the other api entry callers that can't handle it? I'm not that opposed to this. But it will make this series a fair bit longer to change the 8 map_sg_attrs() usages. Logan
On Tue, Apr 27, 2021 at 04:55:45PM -0600, Logan Gunthorpe wrote: > > Also, I see only 8 users of this function. How about just fix them all > > to support negative returns and use this as the p2p API instead of > > adding new API? > > Well there might be 8 users of dma_map_sg_attrs() but there are a very > large number of dma_map_sg(). Seems odd to me to single out the first as > requiring these changes, but leave the latter. At a high level I'm OK with it. dma_map_sg_attrs() is the extra extended version of dma_map_sg(), it already has a different signature, a different return code is not out of the question. dma_map_sg() is just the simple easy to use interface that can't do advanced stuff. > I'm not that opposed to this. But it will make this series a fair bit > longer to change the 8 map_sg_attrs() usages. Yes, but the result seems much nicer to not grow the DMA API further. Jason
On 4/8/21 10:01 AM, Logan Gunthorpe wrote: > dma_map_sg() either returns a positive number indicating the number > of entries mapped or zero indicating that resources were not available > to create the mapping. When zero is returned, it is always safe to retry > the mapping later once resources have been freed. > > Once P2PDMA pages are mixed into the SGL there may be pages that may > never be successfully mapped with a given device because that device may > not actually be able to access those pages. Thus, multiple error > conditions will need to be distinguished to determine weather a retry > is safe. > > Introduce dma_map_sg_p2pdma[_attrs]() with a different calling > convention from dma_map_sg(). The function will return a positive > integer on success or a negative errno on failure. > > ENOMEM will be used to indicate a resource failure and EREMOTEIO to > indicate that a P2PDMA page is not mappable. > > The __DMA_ATTR_PCI_P2PDMA attribute is introduced to inform the lower > level implementations that P2PDMA pages are allowed and to warn if a > caller introduces them into the regular dma_map_sg() interface. > > Signed-off-by: Logan Gunthorpe <logang@deltatee.com> > --- > include/linux/dma-mapping.h | 15 +++++++++++ > kernel/dma/mapping.c | 52 ++++++++++++++++++++++++++++++++----- > 2 files changed, 61 insertions(+), 6 deletions(-) > > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h > index 2a984cb4d1e0..50b8f586cf59 100644 > --- a/include/linux/dma-mapping.h > +++ b/include/linux/dma-mapping.h > @@ -60,6 +60,12 @@ > * at least read-only at lesser-privileged levels). > */ > #define DMA_ATTR_PRIVILEGED (1UL << 9) > +/* > + * __DMA_ATTR_PCI_P2PDMA: This should not be used directly, use > + * dma_map_sg_p2pdma() instead. Used internally to indicate that the > + * caller is using the dma_map_sg_p2pdma() interface. > + */ > +#define __DMA_ATTR_PCI_P2PDMA (1UL << 10) > As mentioned near the top of this file, Documentation/core-api/dma-attributes.rst also needs to be updated, for this new item. > /* > * A dma_addr_t can hold any valid DMA or bus address for the platform. It can > @@ -107,6 +113,8 @@ void dma_unmap_page_attrs(struct device *dev, dma_addr_t addr, size_t size, > enum dma_data_direction dir, unsigned long attrs); > int dma_map_sg_attrs(struct device *dev, struct scatterlist *sg, int nents, > enum dma_data_direction dir, unsigned long attrs); > +int dma_map_sg_p2pdma_attrs(struct device *dev, struct scatterlist *sg, > + int nents, enum dma_data_direction dir, unsigned long attrs); > void dma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg, > int nents, enum dma_data_direction dir, > unsigned long attrs); > @@ -160,6 +168,12 @@ static inline int dma_map_sg_attrs(struct device *dev, struct scatterlist *sg, > { > return 0; > } > +static inline int dma_map_sg_p2pdma_attrs(struct device *dev, > + struct scatterlist *sg, int nents, enum dma_data_direction dir, > + unsigned long attrs) > +{ > + return 0; > +} > static inline void dma_unmap_sg_attrs(struct device *dev, > struct scatterlist *sg, int nents, enum dma_data_direction dir, > unsigned long attrs) > @@ -392,6 +406,7 @@ static inline void dma_sync_sgtable_for_device(struct device *dev, > #define dma_map_single(d, a, s, r) dma_map_single_attrs(d, a, s, r, 0) > #define dma_unmap_single(d, a, s, r) dma_unmap_single_attrs(d, a, s, r, 0) > #define dma_map_sg(d, s, n, r) dma_map_sg_attrs(d, s, n, r, 0) > +#define dma_map_sg_p2pdma(d, s, n, r) dma_map_sg_p2pdma_attrs(d, s, n, r, 0) This hunk is fine, of course. But, about pre-existing issues: note to self, or to anyone: send a patch to turn these into inline functions. The macro redirection here is not adding value, but it does make things just a little bit worse. > #define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0) > #define dma_map_page(d, p, o, s, r) dma_map_page_attrs(d, p, o, s, r, 0) > #define dma_unmap_page(d, a, s, r) dma_unmap_page_attrs(d, a, s, r, 0) > diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c > index b6a633679933..923089c4267b 100644 > --- a/kernel/dma/mapping.c > +++ b/kernel/dma/mapping.c > @@ -177,12 +177,8 @@ void dma_unmap_page_attrs(struct device *dev, dma_addr_t addr, size_t size, > } > EXPORT_SYMBOL(dma_unmap_page_attrs); > > -/* > - * dma_maps_sg_attrs returns 0 on error and > 0 on success. > - * It should never return a value < 0. > - */ It would be better to leave the comment in place, given the non-standard return values. However, looking around here, it would be better if we go with the standard -ERRNO for error, and >0 for sucess. There are pre-existing BUG_ON() and WARN_ON_ONCE() items that are partly an attempt to compensate for not being able to return proper -ERRNO codes. For example, this: BUG_ON(!valid_dma_direction(dir)); ...arguably should be more like this: if(WARN_ON_ONCE(!valid_dma_direction(dir))) return -EINVAL; > -int dma_map_sg_attrs(struct device *dev, struct scatterlist *sg, int nents, > - enum dma_data_direction dir, unsigned long attrs) > +static int __dma_map_sg_attrs(struct device *dev, struct scatterlist *sg, > + int nents, enum dma_data_direction dir, unsigned long attrs) > { > const struct dma_map_ops *ops = get_dma_ops(dev); > int ents; > @@ -197,6 +193,20 @@ int dma_map_sg_attrs(struct device *dev, struct scatterlist *sg, int nents, > ents = dma_direct_map_sg(dev, sg, nents, dir, attrs); > else > ents = ops->map_sg(dev, sg, nents, dir, attrs); > + > + return ents; > +} > + > +/* > + * dma_maps_sg_attrs returns 0 on error and > 0 on success. > + * It should never return a value < 0. > + */ > +int dma_map_sg_attrs(struct device *dev, struct scatterlist *sg, int nents, > + enum dma_data_direction dir, unsigned long attrs) > +{ > + int ents; Pre-existing note, feel free to ignore: the ents and nents in the same routines together, are way too close to the each other in naming. Maybe using "requested_nents", or "nents_arg", for the incoming value, would help. > + > + ents = __dma_map_sg_attrs(dev, sg, nents, dir, attrs); > BUG_ON(ents < 0); > debug_dma_map_sg(dev, sg, nents, ents, dir); > > @@ -204,6 +214,36 @@ int dma_map_sg_attrs(struct device *dev, struct scatterlist *sg, int nents, > } > EXPORT_SYMBOL(dma_map_sg_attrs); > > +/* > + * like dma_map_sg_attrs, but returns a negative errno on error (and > 0 > + * on success). This function must be used if PCI P2PDMA pages might > + * be in the scatterlist. Let's turn this into a kernel doc comment block, seeing as how it clearly wants to be--you're almost there already. You've even reinvented @Return, below. :) > + * > + * On error this function may return: > + * -ENOMEM indicating that there was not enough resources available and > + * the transfer may be retried later > + * -EREMOTEIO indicating that P2PDMA pages were included but cannot > + * be mapped by the specified device, retries will always fail > + * > + * The scatterlist should be unmapped with the regular dma_unmap_sg[_attrs](). How about: "The scatterlist should be unmapped via dma_unmap_sg[_attrs]()." > + */ > +int dma_map_sg_p2pdma_attrs(struct device *dev, struct scatterlist *sg, > + int nents, enum dma_data_direction dir, unsigned long attrs) > +{ > + int ents; > + > + ents = __dma_map_sg_attrs(dev, sg, nents, dir, > + attrs | __DMA_ATTR_PCI_P2PDMA); > + if (!ents) > + ents = -ENOMEM; > + > + if (ents > 0) > + debug_dma_map_sg(dev, sg, nents, ents, dir); > + > + return ents; > +} > +EXPORT_SYMBOL_GPL(dma_map_sg_p2pdma_attrs); > + > void dma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg, > int nents, enum dma_data_direction dir, > unsigned long attrs) > thanks,
On 2021-05-02 3:23 p.m., John Hubbard wrote: > On 4/8/21 10:01 AM, Logan Gunthorpe wrote: >> dma_map_sg() either returns a positive number indicating the number >> of entries mapped or zero indicating that resources were not available >> to create the mapping. When zero is returned, it is always safe to retry >> the mapping later once resources have been freed. >> >> Once P2PDMA pages are mixed into the SGL there may be pages that may >> never be successfully mapped with a given device because that device may >> not actually be able to access those pages. Thus, multiple error >> conditions will need to be distinguished to determine weather a retry >> is safe. >> >> Introduce dma_map_sg_p2pdma[_attrs]() with a different calling >> convention from dma_map_sg(). The function will return a positive >> integer on success or a negative errno on failure. >> >> ENOMEM will be used to indicate a resource failure and EREMOTEIO to >> indicate that a P2PDMA page is not mappable. >> >> The __DMA_ATTR_PCI_P2PDMA attribute is introduced to inform the lower >> level implementations that P2PDMA pages are allowed and to warn if a >> caller introduces them into the regular dma_map_sg() interface. >> >> Signed-off-by: Logan Gunthorpe <logang@deltatee.com> >> --- >> include/linux/dma-mapping.h | 15 +++++++++++ >> kernel/dma/mapping.c | 52 ++++++++++++++++++++++++++++++++----- >> 2 files changed, 61 insertions(+), 6 deletions(-) >> >> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h >> index 2a984cb4d1e0..50b8f586cf59 100644 >> --- a/include/linux/dma-mapping.h >> +++ b/include/linux/dma-mapping.h >> @@ -60,6 +60,12 @@ >> * at least read-only at lesser-privileged levels). >> */ >> #define DMA_ATTR_PRIVILEGED (1UL << 9) >> +/* >> + * __DMA_ATTR_PCI_P2PDMA: This should not be used directly, use >> + * dma_map_sg_p2pdma() instead. Used internally to indicate that the >> + * caller is using the dma_map_sg_p2pdma() interface. >> + */ >> +#define __DMA_ATTR_PCI_P2PDMA (1UL << 10) >> > > As mentioned near the top of this file, > Documentation/core-api/dma-attributes.rst also needs to be updated, for > this new item. As this attribute is not meant to be used by anyone outside the dma functions, I don't think it should be documented here. (That's why it has a double underscource prefix). >> /* >> * A dma_addr_t can hold any valid DMA or bus address for the platform. It can >> @@ -107,6 +113,8 @@ void dma_unmap_page_attrs(struct device *dev, dma_addr_t addr, size_t size, >> enum dma_data_direction dir, unsigned long attrs); >> int dma_map_sg_attrs(struct device *dev, struct scatterlist *sg, int nents, >> enum dma_data_direction dir, unsigned long attrs); >> +int dma_map_sg_p2pdma_attrs(struct device *dev, struct scatterlist *sg, >> + int nents, enum dma_data_direction dir, unsigned long attrs); >> void dma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg, >> int nents, enum dma_data_direction dir, >> unsigned long attrs); >> @@ -160,6 +168,12 @@ static inline int dma_map_sg_attrs(struct device *dev, struct scatterlist *sg, >> { >> return 0; >> } >> +static inline int dma_map_sg_p2pdma_attrs(struct device *dev, >> + struct scatterlist *sg, int nents, enum dma_data_direction dir, >> + unsigned long attrs) >> +{ >> + return 0; >> +} >> static inline void dma_unmap_sg_attrs(struct device *dev, >> struct scatterlist *sg, int nents, enum dma_data_direction dir, >> unsigned long attrs) >> @@ -392,6 +406,7 @@ static inline void dma_sync_sgtable_for_device(struct device *dev, >> #define dma_map_single(d, a, s, r) dma_map_single_attrs(d, a, s, r, 0) >> #define dma_unmap_single(d, a, s, r) dma_unmap_single_attrs(d, a, s, r, 0) >> #define dma_map_sg(d, s, n, r) dma_map_sg_attrs(d, s, n, r, 0) >> +#define dma_map_sg_p2pdma(d, s, n, r) dma_map_sg_p2pdma_attrs(d, s, n, r, 0) > > This hunk is fine, of course. > > But, about pre-existing issues: note to self, or to anyone: send a patch to turn > these into inline functions. The macro redirection here is not adding value, but > it does make things just a little bit worse. > > >> #define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0) >> #define dma_map_page(d, p, o, s, r) dma_map_page_attrs(d, p, o, s, r, 0) >> #define dma_unmap_page(d, a, s, r) dma_unmap_page_attrs(d, a, s, r, 0) >> diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c >> index b6a633679933..923089c4267b 100644 >> --- a/kernel/dma/mapping.c >> +++ b/kernel/dma/mapping.c >> @@ -177,12 +177,8 @@ void dma_unmap_page_attrs(struct device *dev, dma_addr_t addr, size_t size, >> } >> EXPORT_SYMBOL(dma_unmap_page_attrs); >> >> -/* >> - * dma_maps_sg_attrs returns 0 on error and > 0 on success. >> - * It should never return a value < 0. >> - */ > > It would be better to leave the comment in place, given the non-standard > return values. However, looking around here, it would be better if we go > with the standard -ERRNO for error, and >0 for sucess. The comment is actually left in place. The diff just makes it look like it was removed. It is added back lower down in the diff. > There are pre-existing BUG_ON() and WARN_ON_ONCE() items that are partly > an attempt to compensate for not being able to return proper -ERRNO > codes. For example, this: > > BUG_ON(!valid_dma_direction(dir)); > > ...arguably should be more like this: > > if(WARN_ON_ONCE(!valid_dma_direction(dir))) > return -EINVAL; Yes, but you'll have to see the discussion in the RFC. The complaint was that the calling convention for dma_map_sg() is not expected to return anything other than 0 or the number of entries mapped. It can't return a negative error code. That's why BUG_ON(ents < 0) is in the existing code. That's also why this series introduces the new dma_map_sg_p2pdma() function. (Though, Jason has made some suggestions to further change this). > >> -int dma_map_sg_attrs(struct device *dev, struct scatterlist *sg, int nents, >> - enum dma_data_direction dir, unsigned long attrs) >> +static int __dma_map_sg_attrs(struct device *dev, struct scatterlist *sg, >> + int nents, enum dma_data_direction dir, unsigned long attrs) >> { >> const struct dma_map_ops *ops = get_dma_ops(dev); >> int ents; >> @@ -197,6 +193,20 @@ int dma_map_sg_attrs(struct device *dev, struct scatterlist *sg, int nents, >> ents = dma_direct_map_sg(dev, sg, nents, dir, attrs); >> else >> ents = ops->map_sg(dev, sg, nents, dir, attrs); >> + >> + return ents; >> +} >> + >> +/* >> + * dma_maps_sg_attrs returns 0 on error and > 0 on success. >> + * It should never return a value < 0. >> + */ >> +int dma_map_sg_attrs(struct device *dev, struct scatterlist *sg, int nents, >> + enum dma_data_direction dir, unsigned long attrs) >> +{ >> + int ents; > > Pre-existing note, feel free to ignore: the ents and nents in the same > routines together, are way too close to the each other in naming. Maybe > using "requested_nents", or "nents_arg", for the incoming value, would > help. Ok, will change. >> + >> + ents = __dma_map_sg_attrs(dev, sg, nents, dir, attrs); >> BUG_ON(ents < 0); >> debug_dma_map_sg(dev, sg, nents, ents, dir); >> >> @@ -204,6 +214,36 @@ int dma_map_sg_attrs(struct device *dev, struct scatterlist *sg, int nents, >> } >> EXPORT_SYMBOL(dma_map_sg_attrs); >> >> +/* >> + * like dma_map_sg_attrs, but returns a negative errno on error (and > 0 >> + * on success). This function must be used if PCI P2PDMA pages might >> + * be in the scatterlist. > > Let's turn this into a kernel doc comment block, seeing as how it clearly > wants to be--you're almost there already. You've even reinvented @Return, > below. :) Just trying to follow the convention in this file. But I can make it a kernel doc. >> + * >> + * On error this function may return: >> + * -ENOMEM indicating that there was not enough resources available and >> + * the transfer may be retried later >> + * -EREMOTEIO indicating that P2PDMA pages were included but cannot >> + * be mapped by the specified device, retries will always fail >> + * >> + * The scatterlist should be unmapped with the regular dma_unmap_sg[_attrs](). > > How about: > > "The scatterlist should be unmapped via dma_unmap_sg[_attrs]()." Ok Logan
On Tue, Apr 27, 2021 at 08:01:13PM -0300, Jason Gunthorpe wrote: > At a high level I'm OK with it. dma_map_sg_attrs() is the extra > extended version of dma_map_sg(), it already has a different > signature, a different return code is not out of the question. > > dma_map_sg() is just the simple easy to use interface that can't do > advanced stuff. > > > I'm not that opposed to this. But it will make this series a fair bit > > longer to change the 8 map_sg_attrs() usages. > > Yes, but the result seems much nicer to not grow the DMA API further. We already have a mapping function that can return errors: dma_map_sgtable. I think it might make more sense to piggy back on that, as the sg_table abstraction is pretty useful basically everywhere that we deal with scatterlists anyway. In the hopefully no too long run I plan to get rid of scatterlists in at least NVMe and other high performance devices anyway.
On 2021-05-03 12:28 p.m., Christoph Hellwig wrote: > On Tue, Apr 27, 2021 at 08:01:13PM -0300, Jason Gunthorpe wrote: >> At a high level I'm OK with it. dma_map_sg_attrs() is the extra >> extended version of dma_map_sg(), it already has a different >> signature, a different return code is not out of the question. >> >> dma_map_sg() is just the simple easy to use interface that can't do >> advanced stuff. >> >>> I'm not that opposed to this. But it will make this series a fair bit >>> longer to change the 8 map_sg_attrs() usages. >> >> Yes, but the result seems much nicer to not grow the DMA API further. > > We already have a mapping function that can return errors: > dma_map_sgtable. > > I think it might make more sense to piggy back on that, as the sg_table > abstraction is pretty useful basically everywhere that we deal with > scatterlists anyway. Oh, I didn't even realize that existed. I'll use dma_map_sgtable() for v2. Thanks, Logan
On 4/8/21 1:01 PM, Logan Gunthorpe wrote: > dma_map_sg() either returns a positive number indicating the number > of entries mapped or zero indicating that resources were not available > to create the mapping. When zero is returned, it is always safe to retry > the mapping later once resources have been freed. > > Once P2PDMA pages are mixed into the SGL there may be pages that may > never be successfully mapped with a given device because that device may > not actually be able to access those pages. Thus, multiple error > conditions will need to be distinguished to determine weather a retry s/weather/whether/ > is safe. > > Introduce dma_map_sg_p2pdma[_attrs]() with a different calling > convention from dma_map_sg(). The function will return a positive > integer on success or a negative errno on failure. > > ENOMEM will be used to indicate a resource failure and EREMOTEIO to > indicate that a P2PDMA page is not mappable. > > The __DMA_ATTR_PCI_P2PDMA attribute is introduced to inform the lower > level implementations that P2PDMA pages are allowed and to warn if a > caller introduces them into the regular dma_map_sg() interface. > > Signed-off-by: Logan Gunthorpe <logang@deltatee.com> John caught any other comment I had (and more). -dd > --- > include/linux/dma-mapping.h | 15 +++++++++++ > kernel/dma/mapping.c | 52 ++++++++++++++++++++++++++++++++----- > 2 files changed, 61 insertions(+), 6 deletions(-) > > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h > index 2a984cb4d1e0..50b8f586cf59 100644 > --- a/include/linux/dma-mapping.h > +++ b/include/linux/dma-mapping.h > @@ -60,6 +60,12 @@ > * at least read-only at lesser-privileged levels). > */ > #define DMA_ATTR_PRIVILEGED (1UL << 9) > +/* > + * __DMA_ATTR_PCI_P2PDMA: This should not be used directly, use > + * dma_map_sg_p2pdma() instead. Used internally to indicate that the > + * caller is using the dma_map_sg_p2pdma() interface. > + */ > +#define __DMA_ATTR_PCI_P2PDMA (1UL << 10) > > /* > * A dma_addr_t can hold any valid DMA or bus address for the platform. It can > @@ -107,6 +113,8 @@ void dma_unmap_page_attrs(struct device *dev, dma_addr_t addr, size_t size, > enum dma_data_direction dir, unsigned long attrs); > int dma_map_sg_attrs(struct device *dev, struct scatterlist *sg, int nents, > enum dma_data_direction dir, unsigned long attrs); > +int dma_map_sg_p2pdma_attrs(struct device *dev, struct scatterlist *sg, > + int nents, enum dma_data_direction dir, unsigned long attrs); > void dma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg, > int nents, enum dma_data_direction dir, > unsigned long attrs); > @@ -160,6 +168,12 @@ static inline int dma_map_sg_attrs(struct device *dev, struct scatterlist *sg, > { > return 0; > } > +static inline int dma_map_sg_p2pdma_attrs(struct device *dev, > + struct scatterlist *sg, int nents, enum dma_data_direction dir, > + unsigned long attrs) > +{ > + return 0; > +} > static inline void dma_unmap_sg_attrs(struct device *dev, > struct scatterlist *sg, int nents, enum dma_data_direction dir, > unsigned long attrs) > @@ -392,6 +406,7 @@ static inline void dma_sync_sgtable_for_device(struct device *dev, > #define dma_map_single(d, a, s, r) dma_map_single_attrs(d, a, s, r, 0) > #define dma_unmap_single(d, a, s, r) dma_unmap_single_attrs(d, a, s, r, 0) > #define dma_map_sg(d, s, n, r) dma_map_sg_attrs(d, s, n, r, 0) > +#define dma_map_sg_p2pdma(d, s, n, r) dma_map_sg_p2pdma_attrs(d, s, n, r, 0) > #define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0) > #define dma_map_page(d, p, o, s, r) dma_map_page_attrs(d, p, o, s, r, 0) > #define dma_unmap_page(d, a, s, r) dma_unmap_page_attrs(d, a, s, r, 0) > diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c > index b6a633679933..923089c4267b 100644 > --- a/kernel/dma/mapping.c > +++ b/kernel/dma/mapping.c > @@ -177,12 +177,8 @@ void dma_unmap_page_attrs(struct device *dev, dma_addr_t addr, size_t size, > } > EXPORT_SYMBOL(dma_unmap_page_attrs); > > -/* > - * dma_maps_sg_attrs returns 0 on error and > 0 on success. > - * It should never return a value < 0. > - */ > -int dma_map_sg_attrs(struct device *dev, struct scatterlist *sg, int nents, > - enum dma_data_direction dir, unsigned long attrs) > +static int __dma_map_sg_attrs(struct device *dev, struct scatterlist *sg, > + int nents, enum dma_data_direction dir, unsigned long attrs) > { > const struct dma_map_ops *ops = get_dma_ops(dev); > int ents; > @@ -197,6 +193,20 @@ int dma_map_sg_attrs(struct device *dev, struct scatterlist *sg, int nents, > ents = dma_direct_map_sg(dev, sg, nents, dir, attrs); > else > ents = ops->map_sg(dev, sg, nents, dir, attrs); > + > + return ents; > +} > + > +/* > + * dma_maps_sg_attrs returns 0 on error and > 0 on success. > + * It should never return a value < 0. > + */ > +int dma_map_sg_attrs(struct device *dev, struct scatterlist *sg, int nents, > + enum dma_data_direction dir, unsigned long attrs) > +{ > + int ents; > + > + ents = __dma_map_sg_attrs(dev, sg, nents, dir, attrs); > BUG_ON(ents < 0); > debug_dma_map_sg(dev, sg, nents, ents, dir); > > @@ -204,6 +214,36 @@ int dma_map_sg_attrs(struct device *dev, struct scatterlist *sg, int nents, > } > EXPORT_SYMBOL(dma_map_sg_attrs); > > +/* > + * like dma_map_sg_attrs, but returns a negative errno on error (and > 0 > + * on success). This function must be used if PCI P2PDMA pages might > + * be in the scatterlist. > + * > + * On error this function may return: > + * -ENOMEM indicating that there was not enough resources available and > + * the transfer may be retried later > + * -EREMOTEIO indicating that P2PDMA pages were included but cannot > + * be mapped by the specified device, retries will always fail > + * > + * The scatterlist should be unmapped with the regular dma_unmap_sg[_attrs](). > + */ > +int dma_map_sg_p2pdma_attrs(struct device *dev, struct scatterlist *sg, > + int nents, enum dma_data_direction dir, unsigned long attrs) > +{ > + int ents; > + > + ents = __dma_map_sg_attrs(dev, sg, nents, dir, > + attrs | __DMA_ATTR_PCI_P2PDMA); > + if (!ents) > + ents = -ENOMEM; > + > + if (ents > 0) > + debug_dma_map_sg(dev, sg, nents, ents, dir); > + > + return ents; > +} > +EXPORT_SYMBOL_GPL(dma_map_sg_p2pdma_attrs); > + > void dma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg, > int nents, enum dma_data_direction dir, > unsigned long attrs)
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h index 2a984cb4d1e0..50b8f586cf59 100644 --- a/include/linux/dma-mapping.h +++ b/include/linux/dma-mapping.h @@ -60,6 +60,12 @@ * at least read-only at lesser-privileged levels). */ #define DMA_ATTR_PRIVILEGED (1UL << 9) +/* + * __DMA_ATTR_PCI_P2PDMA: This should not be used directly, use + * dma_map_sg_p2pdma() instead. Used internally to indicate that the + * caller is using the dma_map_sg_p2pdma() interface. + */ +#define __DMA_ATTR_PCI_P2PDMA (1UL << 10) /* * A dma_addr_t can hold any valid DMA or bus address for the platform. It can @@ -107,6 +113,8 @@ void dma_unmap_page_attrs(struct device *dev, dma_addr_t addr, size_t size, enum dma_data_direction dir, unsigned long attrs); int dma_map_sg_attrs(struct device *dev, struct scatterlist *sg, int nents, enum dma_data_direction dir, unsigned long attrs); +int dma_map_sg_p2pdma_attrs(struct device *dev, struct scatterlist *sg, + int nents, enum dma_data_direction dir, unsigned long attrs); void dma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg, int nents, enum dma_data_direction dir, unsigned long attrs); @@ -160,6 +168,12 @@ static inline int dma_map_sg_attrs(struct device *dev, struct scatterlist *sg, { return 0; } +static inline int dma_map_sg_p2pdma_attrs(struct device *dev, + struct scatterlist *sg, int nents, enum dma_data_direction dir, + unsigned long attrs) +{ + return 0; +} static inline void dma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg, int nents, enum dma_data_direction dir, unsigned long attrs) @@ -392,6 +406,7 @@ static inline void dma_sync_sgtable_for_device(struct device *dev, #define dma_map_single(d, a, s, r) dma_map_single_attrs(d, a, s, r, 0) #define dma_unmap_single(d, a, s, r) dma_unmap_single_attrs(d, a, s, r, 0) #define dma_map_sg(d, s, n, r) dma_map_sg_attrs(d, s, n, r, 0) +#define dma_map_sg_p2pdma(d, s, n, r) dma_map_sg_p2pdma_attrs(d, s, n, r, 0) #define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0) #define dma_map_page(d, p, o, s, r) dma_map_page_attrs(d, p, o, s, r, 0) #define dma_unmap_page(d, a, s, r) dma_unmap_page_attrs(d, a, s, r, 0) diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c index b6a633679933..923089c4267b 100644 --- a/kernel/dma/mapping.c +++ b/kernel/dma/mapping.c @@ -177,12 +177,8 @@ void dma_unmap_page_attrs(struct device *dev, dma_addr_t addr, size_t size, } EXPORT_SYMBOL(dma_unmap_page_attrs); -/* - * dma_maps_sg_attrs returns 0 on error and > 0 on success. - * It should never return a value < 0. - */ -int dma_map_sg_attrs(struct device *dev, struct scatterlist *sg, int nents, - enum dma_data_direction dir, unsigned long attrs) +static int __dma_map_sg_attrs(struct device *dev, struct scatterlist *sg, + int nents, enum dma_data_direction dir, unsigned long attrs) { const struct dma_map_ops *ops = get_dma_ops(dev); int ents; @@ -197,6 +193,20 @@ int dma_map_sg_attrs(struct device *dev, struct scatterlist *sg, int nents, ents = dma_direct_map_sg(dev, sg, nents, dir, attrs); else ents = ops->map_sg(dev, sg, nents, dir, attrs); + + return ents; +} + +/* + * dma_maps_sg_attrs returns 0 on error and > 0 on success. + * It should never return a value < 0. + */ +int dma_map_sg_attrs(struct device *dev, struct scatterlist *sg, int nents, + enum dma_data_direction dir, unsigned long attrs) +{ + int ents; + + ents = __dma_map_sg_attrs(dev, sg, nents, dir, attrs); BUG_ON(ents < 0); debug_dma_map_sg(dev, sg, nents, ents, dir); @@ -204,6 +214,36 @@ int dma_map_sg_attrs(struct device *dev, struct scatterlist *sg, int nents, } EXPORT_SYMBOL(dma_map_sg_attrs); +/* + * like dma_map_sg_attrs, but returns a negative errno on error (and > 0 + * on success). This function must be used if PCI P2PDMA pages might + * be in the scatterlist. + * + * On error this function may return: + * -ENOMEM indicating that there was not enough resources available and + * the transfer may be retried later + * -EREMOTEIO indicating that P2PDMA pages were included but cannot + * be mapped by the specified device, retries will always fail + * + * The scatterlist should be unmapped with the regular dma_unmap_sg[_attrs](). + */ +int dma_map_sg_p2pdma_attrs(struct device *dev, struct scatterlist *sg, + int nents, enum dma_data_direction dir, unsigned long attrs) +{ + int ents; + + ents = __dma_map_sg_attrs(dev, sg, nents, dir, + attrs | __DMA_ATTR_PCI_P2PDMA); + if (!ents) + ents = -ENOMEM; + + if (ents > 0) + debug_dma_map_sg(dev, sg, nents, ents, dir); + + return ents; +} +EXPORT_SYMBOL_GPL(dma_map_sg_p2pdma_attrs); + void dma_unmap_sg_attrs(struct device *dev, struct scatterlist *sg, int nents, enum dma_data_direction dir, unsigned long attrs)
dma_map_sg() either returns a positive number indicating the number of entries mapped or zero indicating that resources were not available to create the mapping. When zero is returned, it is always safe to retry the mapping later once resources have been freed. Once P2PDMA pages are mixed into the SGL there may be pages that may never be successfully mapped with a given device because that device may not actually be able to access those pages. Thus, multiple error conditions will need to be distinguished to determine weather a retry is safe. Introduce dma_map_sg_p2pdma[_attrs]() with a different calling convention from dma_map_sg(). The function will return a positive integer on success or a negative errno on failure. ENOMEM will be used to indicate a resource failure and EREMOTEIO to indicate that a P2PDMA page is not mappable. The __DMA_ATTR_PCI_P2PDMA attribute is introduced to inform the lower level implementations that P2PDMA pages are allowed and to warn if a caller introduces them into the regular dma_map_sg() interface. Signed-off-by: Logan Gunthorpe <logang@deltatee.com> --- include/linux/dma-mapping.h | 15 +++++++++++ kernel/dma/mapping.c | 52 ++++++++++++++++++++++++++++++++----- 2 files changed, 61 insertions(+), 6 deletions(-)