| Message ID | 1442581036-23789-6-git-send-email-clabbe.montjoie@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
On 09/18/2015 07:57 AM, LABBE Corentin wrote: > Some driver use a modified version of sg_nents_for_len with an > additional parameter bool *chained for knowing if the scatterlist is > chained or not. > > So, for removing duplicate code, add sg_nents_len_chained in > lib/scatterlist.c > > Signed-off-by: LABBE Corentin <clabbe.montjoie@gmail.com> > --- > include/linux/scatterlist.h | 1 + > lib/scatterlist.c | 40 ++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 41 insertions(+) > > diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h > index 556ec1e..594cdb0 100644 > --- a/include/linux/scatterlist.h > +++ b/include/linux/scatterlist.h > @@ -243,6 +243,7 @@ static inline void *sg_virt(struct scatterlist *sg) > > int sg_nents(struct scatterlist *sg); > int sg_nents_for_len(struct scatterlist *sg, u64 len); > +int sg_nents_len_chained(struct scatterlist *sg, u64 len, bool *chained); > struct scatterlist *sg_next(struct scatterlist *); > struct scatterlist *sg_last(struct scatterlist *s, unsigned int); > void sg_init_table(struct scatterlist *, unsigned int); > diff --git a/lib/scatterlist.c b/lib/scatterlist.c > index bafa993..070e396 100644 > --- a/lib/scatterlist.c > +++ b/lib/scatterlist.c > @@ -90,6 +90,46 @@ int sg_nents_for_len(struct scatterlist *sg, u64 len) > EXPORT_SYMBOL(sg_nents_for_len); > > /** > + * sg_nents_len_chained - return total count of entries in scatterlist > + * needed to satisfy the supplied length > + * @sg: The scatterlist > + * @len: The total required length > + * @chained A pointer where to store if SG is chained or not > + * > + * Description: > + * Determines the number of entries in sg that are required to meet > + * the supplied length, taking into account chaining as well > + * If the scatterlist is chained, set *chained to true. > + * > + * Returns: > + * the number of sg entries needed, negative error on failure > + * > + **/ > +int sg_nents_len_chained(struct scatterlist *sg, u64 len, bool *chained) > +{ > + int nents; > + u64 total; > + > + if (chained) > + *chained = false; > + > + if (!len) > + return 0; > + > + for (nents = 0, total = 0; sg; sg = sg_next(sg)) { > + nents++; > + total += sg->length; > + if (!sg_is_last(sg) && (sg + 1)->length == 0 && chained) Wouldn't it be better to use the sg_is_chain macro to determine if the the entry is chained instead of checking the length? Thanks, Tom > + *chained = true; > + if (total >= len) > + return nents; > + } > + > + return -EINVAL; > +} > +EXPORT_SYMBOL(sg_nents_len_chained); > + > +/** > * sg_last - return the last scatterlist entry in a list > * @sgl: First entry in the scatterlist > * @nents: Number of entries in the scatterlist > -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/18/2015 08:57 AM, LABBE Corentin wrote: > + for (nents = 0, total = 0; sg; sg = sg_next(sg)) { > + nents++; > + total += sg->length; > + if (!sg_is_last(sg) && (sg + 1)->length == 0 && chained) > + *chained = true; > + if (total >= len) > + return nents; > + } > + > (resending with fixed formatting; Thunderbird seems braindamaged lately) It seems to me like the check for total >= len should be above the check for chaining. The way the code is now, it will return chained = true if the first "unneeded" sg vector is a chain, which does not make intuitive sense. But why do drivers even need this at all? Here is a typical usage: int qce_mapsg(struct device *dev, struct scatterlist *sg, int nents, enum dma_data_direction dir, bool chained) { int err; if (chained) { while (sg) { err = dma_map_sg(dev, sg, 1, dir); if (!err) return -EFAULT; sg = sg_next(sg); } } else { err = dma_map_sg(dev, sg, nents, dir); if (!err) return -EFAULT; } return nents; } Here is another: static int talitos_map_sg(struct device *dev, struct scatterlist *sg, unsigned int nents, enum dma_data_direction dir, bool chained) { if (unlikely(chained)) while (sg) { dma_map_sg(dev, sg, 1, dir); sg = sg_next(sg); } else dma_map_sg(dev, sg, nents, dir); return nents; } Can anyone clarify why you can't just use dma_map_sg(dev, sg, nents, dir) always? It should be able to handle chained scatterlists just fine. If the check for chaining is a just workaround for some problem in dma_map_sg(), maybe it would be better to fix dma_map_sg() instead, which would eliminate the need for sg_nents_len_chained() and all these buggy workarounds (e.g. if chained is true, qce_mapsg() can leave the DMA list partially mapped when it returns -EFAULT, and talitos_map_sg() doesn't even check for errors). One problem that I see is that sg_last() in scatterlist.c has a "BUG_ON(!sg_is_last(ret));" if CONFIG_DEBUG_SG is enabled, and using a smaller-than-original nents (as returned by sg_nents_len_chained()) with the same scatterlist will trigger that bug. But that should be true regardless of whether chaining is used or not. For example, talitos.c calls sg_last() in a way that can trigger that bug. For anyone willing to dig further, these are the first two commits that introduce code like this: 4de9d0b547b9 "crypto: talitos - Add ablkcipher algorithms" (2009) 643b39b031f5 "crypto: caam - chaining support" (2012) (CC'ing the original authors) Tony Battersby Cybernetics -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/18/2015 12:19 PM, Tony Battersby wrote: > But why do drivers even need this at all? Here is a typical usage: > > int qce_mapsg(struct device *dev, struct scatterlist *sg, int nents, > enum dma_data_direction dir, bool chained) > { > int err; > > if (chained) { > while (sg) { > err = dma_map_sg(dev, sg, 1, dir); > if (!err) > return -EFAULT; > sg = sg_next(sg); > } > } else { > err = dma_map_sg(dev, sg, nents, dir); > if (!err) > return -EFAULT; > } > > return nents; > } > > Here is another: > > static int talitos_map_sg(struct device *dev, struct scatterlist *sg, > unsigned int nents, enum dma_data_direction dir, > bool chained) > { > if (unlikely(chained)) > while (sg) { > dma_map_sg(dev, sg, 1, dir); > sg = sg_next(sg); > } > else > dma_map_sg(dev, sg, nents, dir); > return nents; > } > > Can anyone clarify why you can't just use dma_map_sg(dev, sg, nents, > dir) always? It should be able to handle chained scatterlists just fine. After further investigation, it looks like this was a remnant of scatterwalk_sg_next() which was removed by commit 5be4d4c94b1f ("crypto: replace scatterwalk_sg_next with sg_next"). Apparently crypto scatterlists used to be chained differently than normal scatterlists, so functions like dma_map_sg() would not work on a chained crypto scatterlist, but that is no longer the case. So instead of adding a new function sg_nents_len_chained(), a better cleanup would be: 1) replace the driver-specific functions with calls to sg_nents_for_len() 2) get rid of the "chained" variables 3) always call dma_map_sg()/dma_unmap_sg() for the entire scatterlist regardless of whether or not the scatterlist is chained Would someone more familiar with the crypto API please confirm that my suggestions are correct? Tony Battersby Cybernetics -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Sep 18, 2015 at 05:25:47PM -0400, Tony Battersby wrote: > > So instead of adding a new function sg_nents_len_chained(), a better > cleanup would be: > 1) replace the driver-specific functions with calls to sg_nents_for_len() > 2) get rid of the "chained" variables > 3) always call dma_map_sg()/dma_unmap_sg() for the entire scatterlist > regardless of whether or not the scatterlist is chained > > Would someone more familiar with the crypto API please confirm that my > suggestions are correct? Yes I think you're absolutely right Tony. Corentin, could you please take this opportunity to clean up those drivers so that they simply use dma_map_sg a single time rather than over and over again for chained SG lists? You only have to redo patches 5-8. Thanks,
On Mon, Sep 21, 2015 at 10:19:17PM +0800, Herbert Xu wrote: > On Fri, Sep 18, 2015 at 05:25:47PM -0400, Tony Battersby wrote: > > > > So instead of adding a new function sg_nents_len_chained(), a better > > cleanup would be: > > 1) replace the driver-specific functions with calls to sg_nents_for_len() > > 2) get rid of the "chained" variables > > 3) always call dma_map_sg()/dma_unmap_sg() for the entire scatterlist > > regardless of whether or not the scatterlist is chained > > > > Would someone more familiar with the crypto API please confirm that my > > suggestions are correct? > > Yes I think you're absolutely right Tony. Corentin, could you > please take this opportunity to clean up those drivers so that > they simply use dma_map_sg a single time rather than over and > over again for chained SG lists? Yes I will > > You only have to redo patches 5-8. > > Thanks, -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h index 556ec1e..594cdb0 100644 --- a/include/linux/scatterlist.h +++ b/include/linux/scatterlist.h @@ -243,6 +243,7 @@ static inline void *sg_virt(struct scatterlist *sg) int sg_nents(struct scatterlist *sg); int sg_nents_for_len(struct scatterlist *sg, u64 len); +int sg_nents_len_chained(struct scatterlist *sg, u64 len, bool *chained); struct scatterlist *sg_next(struct scatterlist *); struct scatterlist *sg_last(struct scatterlist *s, unsigned int); void sg_init_table(struct scatterlist *, unsigned int); diff --git a/lib/scatterlist.c b/lib/scatterlist.c index bafa993..070e396 100644 --- a/lib/scatterlist.c +++ b/lib/scatterlist.c @@ -90,6 +90,46 @@ int sg_nents_for_len(struct scatterlist *sg, u64 len) EXPORT_SYMBOL(sg_nents_for_len); /** + * sg_nents_len_chained - return total count of entries in scatterlist + * needed to satisfy the supplied length + * @sg: The scatterlist + * @len: The total required length + * @chained A pointer where to store if SG is chained or not + * + * Description: + * Determines the number of entries in sg that are required to meet + * the supplied length, taking into account chaining as well + * If the scatterlist is chained, set *chained to true. + * + * Returns: + * the number of sg entries needed, negative error on failure + * + **/ +int sg_nents_len_chained(struct scatterlist *sg, u64 len, bool *chained) +{ + int nents; + u64 total; + + if (chained) + *chained = false; + + if (!len) + return 0; + + for (nents = 0, total = 0; sg; sg = sg_next(sg)) { + nents++; + total += sg->length; + if (!sg_is_last(sg) && (sg + 1)->length == 0 && chained) + *chained = true; + if (total >= len) + return nents; + } + + return -EINVAL; +} +EXPORT_SYMBOL(sg_nents_len_chained); + +/** * sg_last - return the last scatterlist entry in a list * @sgl: First entry in the scatterlist * @nents: Number of entries in the scatterlist
Some driver use a modified version of sg_nents_for_len with an additional parameter bool *chained for knowing if the scatterlist is chained or not. So, for removing duplicate code, add sg_nents_len_chained in lib/scatterlist.c Signed-off-by: LABBE Corentin <clabbe.montjoie@gmail.com> --- include/linux/scatterlist.h | 1 + lib/scatterlist.c | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+)