Re: [PATCH v2 5/8] lib: introduce sg_nents_len_chained

[Herbert Xu]

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,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/8] lib: introduce sg_nents_len_chained

[LABBE Corentin]

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/8] lib: introduce sg_nents_len_chained

[Tony Battersby]

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/8] lib: introduce sg_nents_len_chained

[Tom Lendacky]

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 
---
  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
+ * @chainedA 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/8] lib: introduce sg_nents_len_chained

[Tony Battersby]

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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html