[PATCH v2] crypto: Remove duplicate code of SG helpers functions

[PATCH v2] crypto: Remove duplicate code of SG helpers functions

[LABBE Corentin]

Hello

This patch series try to remove some duplicate code of some SG helpers functions.
The first four patch replace custom functions by already in-tree helper functions.

The fourth add a new functions "sg_nents_len_chained" who is the same as
sg_nents_for_len with an additionnal arguments.

The last three patch use sg_nents_len_chained for removing custom functions.

Note that I do not own any of those hardware to test.

Regards

[PATCH v2 1/8] crypto: bfin: replace sg_count by sg_nents

[LABBE Corentin]

The sg_count function in bfin_crc.c is the same function as sg_nents.
Remove the duplicate code and use sg_nents() instead.

Signed-off-by: LABBE Corentin <clabbe.montjoie@gmail.com>
---
 drivers/crypto/bfin_crc.c | 25 +++----------------------
 1 file changed, 3 insertions(+), 22 deletions(-)

diff --git a/drivers/crypto/bfin_crc.c b/drivers/crypto/bfin_crc.c
index 2f0b333..95b7396 100644
--- a/drivers/crypto/bfin_crc.c
+++ b/drivers/crypto/bfin_crc.c
@@ -96,26 +96,6 @@ struct bfin_crypto_crc_ctx {
 	u32			key;
 };
 
-
-/*
- * derive number of elements in scatterlist
- */
-static int sg_count(struct scatterlist *sg_list)
-{
-	struct scatterlist *sg = sg_list;
-	int sg_nents = 1;
-
-	if (sg_list == NULL)
-		return 0;
-
-	while (!sg_is_last(sg)) {
-		sg_nents++;
-		sg = sg_next(sg);
-	}
-
-	return sg_nents;
-}
-
 /*
  * get element in scatter list by given index
  */
@@ -160,7 +140,7 @@ static int bfin_crypto_crc_init(struct ahash_request *req)
 	}
 	spin_unlock_bh(&crc_list.lock);
 
-	if (sg_count(req->src) > CRC_MAX_DMA_DESC) {
+	if (sg_nents(req->src) > CRC_MAX_DMA_DESC) {
 		dev_dbg(ctx->crc->dev, "init: requested sg list is too big > %d\n",
 			CRC_MAX_DMA_DESC);
 		return -EINVAL;
@@ -376,7 +356,8 @@ static int bfin_crypto_crc_handle_queue(struct bfin_crypto_crc *crc,
 			ctx->sg = req->src;
 
 		/* Chop crc buffer size to multiple of 32 bit */
-		nsg = ctx->sg_nents = sg_count(ctx->sg);
+		nsg = sg_nents(ctx->sg);
+		ctx->sg_nents = nsg;
 		ctx->sg_buflen = ctx->buflast_len + req->nbytes;
 		ctx->bufnext_len = ctx->sg_buflen % 4;
 		ctx->sg_buflen &= ~0x3;
-- 
2.4.6

[PATCH v2 2/8] crypto: amcc replace get_sg_count by sg_nents_for_len

[LABBE Corentin]

The get_sg_count function of amcc is the same as sg_nents_for_len from
lib/scatterlist.c

Signed-off-by: LABBE Corentin <clabbe.montjoie@gmail.com>
---
 drivers/crypto/amcc/crypto4xx_core.c | 22 +---------------------
 1 file changed, 1 insertion(+), 21 deletions(-)

diff --git a/drivers/crypto/amcc/crypto4xx_core.c b/drivers/crypto/amcc/crypto4xx_core.c
index 192a8fa..27288fc 100644
--- a/drivers/crypto/amcc/crypto4xx_core.c
+++ b/drivers/crypto/amcc/crypto4xx_core.c
@@ -740,26 +740,6 @@ void crypto4xx_return_pd(struct crypto4xx_device *dev,
 	pd_uinfo->state = PD_ENTRY_FREE;
 }
 
-/*
- * derive number of elements in scatterlist
- * Shamlessly copy from talitos.c
- */
-static int get_sg_count(struct scatterlist *sg_list, int nbytes)
-{
-	struct scatterlist *sg = sg_list;
-	int sg_nents = 0;
-
-	while (nbytes) {
-		sg_nents++;
-		if (sg->length > nbytes)
-			break;
-		nbytes -= sg->length;
-		sg = sg_next(sg);
-	}
-
-	return sg_nents;
-}
-
 static u32 get_next_gd(u32 current)
 {
 	if (current != PPC4XX_LAST_GD)
@@ -800,7 +780,7 @@ u32 crypto4xx_build_pd(struct crypto_async_request *req,
 	u32 gd_idx = 0;
 
 	/* figure how many gd is needed */
-	num_gd = get_sg_count(src, datalen);
+	num_gd = sg_nents_for_len(src, datalen);
 	if (num_gd == 1)
 		num_gd = 0;
 
-- 
2.4.6

[PATCH v2 3/8] crypto: sahara: replace sahara_sg_length with sg_nents_for_len

[LABBE Corentin]

The sahara_sg_length function of the sahara driver is the same
as sg_nents_for_len from lib/scatterlist.c

Signed-off-by: LABBE Corentin <clabbe.montjoie@gmail.com>
---
 drivers/crypto/sahara.c | 30 +++---------------------------
 1 file changed, 3 insertions(+), 27 deletions(-)

diff --git a/drivers/crypto/sahara.c b/drivers/crypto/sahara.c
index 820dc3a..cea2411 100644
--- a/drivers/crypto/sahara.c
+++ b/drivers/crypto/sahara.c
@@ -274,30 +274,6 @@ static u32 sahara_aes_data_link_hdr(struct sahara_dev *dev)
 			SAHARA_HDR_CHA_SKHA | SAHARA_HDR_PARITY_BIT;
 }
 
-static int sahara_sg_length(struct scatterlist *sg,
-			    unsigned int total)
-{
-	int sg_nb;
-	unsigned int len;
-	struct scatterlist *sg_list;
-
-	sg_nb = 0;
-	sg_list = sg;
-
-	while (total) {
-		len = min(sg_list->length, total);
-
-		sg_nb++;
-		total -= len;
-
-		sg_list = sg_next(sg_list);
-		if (!sg_list)
-			total = 0;
-	}
-
-	return sg_nb;
-}
-
 static char *sahara_err_src[16] = {
 	"No error",
 	"Header error",
@@ -502,8 +478,8 @@ static int sahara_hw_descriptor_create(struct sahara_dev *dev)
 		idx++;
 	}
 
-	dev->nb_in_sg = sahara_sg_length(dev->in_sg, dev->total);
-	dev->nb_out_sg = sahara_sg_length(dev->out_sg, dev->total);
+	dev->nb_in_sg = sg_nents_for_len(dev->in_sg, dev->total);
+	dev->nb_out_sg = sg_nents_for_len(dev->out_sg, dev->total);
 	if ((dev->nb_in_sg + dev->nb_out_sg) > SAHARA_MAX_HW_LINK) {
 		dev_err(dev->device, "not enough hw links (%d)\n",
 			dev->nb_in_sg + dev->nb_out_sg);
@@ -818,7 +794,7 @@ static int sahara_sha_hw_links_create(struct sahara_dev *dev,
 
 	dev->in_sg = rctx->in_sg;
 
-	dev->nb_in_sg = sahara_sg_length(dev->in_sg, rctx->total);
+	dev->nb_in_sg = sg_nents_for_len(dev->in_sg, rctx->total);
 	if ((dev->nb_in_sg) > SAHARA_MAX_HW_LINK) {
 		dev_err(dev->device, "not enough hw links (%d)\n",
 			dev->nb_in_sg + dev->nb_out_sg);
-- 
2.4.6

[PATCH v2 4/8] s390: replace zfcp_qdio_sbale_count by sg_nents

[LABBE Corentin]

The zfcp_qdio_sbale_count function do the same work than sg_nents().
So replace it by sg_nents() for removing duplicate code.

Signed-off-by: LABBE Corentin <clabbe.montjoie@gmail.com>
---
 drivers/s390/scsi/zfcp_fsf.c  |  3 +--
 drivers/s390/scsi/zfcp_qdio.h | 15 ---------------
 2 files changed, 1 insertion(+), 17 deletions(-)

diff --git a/drivers/s390/scsi/zfcp_fsf.c b/drivers/s390/scsi/zfcp_fsf.c
index 522a633..edc137a 100644
--- a/drivers/s390/scsi/zfcp_fsf.c
+++ b/drivers/s390/scsi/zfcp_fsf.c
@@ -985,8 +985,7 @@ static int zfcp_fsf_setup_ct_els_sbals(struct zfcp_fsf_req *req,
 		if (zfcp_qdio_sbals_from_sg(qdio, &req->qdio_req, sg_resp))
 			return -EIO;
 
-		zfcp_qdio_set_data_div(qdio, &req->qdio_req,
-					zfcp_qdio_sbale_count(sg_req));
+		zfcp_qdio_set_data_div(qdio, &req->qdio_req, sg_nents(sg_req));
 		zfcp_qdio_set_sbale_last(qdio, &req->qdio_req);
 		zfcp_qdio_set_scount(qdio, &req->qdio_req);
 		return 0;
diff --git a/drivers/s390/scsi/zfcp_qdio.h b/drivers/s390/scsi/zfcp_qdio.h
index 497cd37..85cdb82 100644
--- a/drivers/s390/scsi/zfcp_qdio.h
+++ b/drivers/s390/scsi/zfcp_qdio.h
@@ -225,21 +225,6 @@ void zfcp_qdio_set_data_div(struct zfcp_qdio *qdio,
 }
 
 /**
- * zfcp_qdio_sbale_count - count sbale used
- * @sg: pointer to struct scatterlist
- */
-static inline
-unsigned int zfcp_qdio_sbale_count(struct scatterlist *sg)
-{
-	unsigned int count = 0;
-
-	for (; sg; sg = sg_next(sg))
-		count++;
-
-	return count;
-}
-
-/**
  * zfcp_qdio_real_bytes - count bytes used
  * @sg: pointer to struct scatterlist
  */
-- 
2.4.6

Re: [PATCH v2 4/8] s390: replace zfcp_qdio_sbale_count by sg_nents

[Steffen Maier]

Thanks, looks good.

I've added it to my queue for sending zfcp patches upstream next time 
(might take a while).

On 09/18/2015 02:57 PM, LABBE Corentin wrote:
> The zfcp_qdio_sbale_count function do the same work than sg_nents().
> So replace it by sg_nents() for removing duplicate code.
>
> Signed-off-by: LABBE Corentin <clabbe.montjoie@gmail.com>
> ---
>   drivers/s390/scsi/zfcp_fsf.c  |  3 +--
>   drivers/s390/scsi/zfcp_qdio.h | 15 ---------------
>   2 files changed, 1 insertion(+), 17 deletions(-)
>
> diff --git a/drivers/s390/scsi/zfcp_fsf.c b/drivers/s390/scsi/zfcp_fsf.c
> index 522a633..edc137a 100644
> --- a/drivers/s390/scsi/zfcp_fsf.c
> +++ b/drivers/s390/scsi/zfcp_fsf.c
> @@ -985,8 +985,7 @@ static int zfcp_fsf_setup_ct_els_sbals(struct zfcp_fsf_req *req,
>   		if (zfcp_qdio_sbals_from_sg(qdio, &req->qdio_req, sg_resp))
>   			return -EIO;
>
> -		zfcp_qdio_set_data_div(qdio, &req->qdio_req,
> -					zfcp_qdio_sbale_count(sg_req));
> +		zfcp_qdio_set_data_div(qdio, &req->qdio_req, sg_nents(sg_req));
>   		zfcp_qdio_set_sbale_last(qdio, &req->qdio_req);
>   		zfcp_qdio_set_scount(qdio, &req->qdio_req);
>   		return 0;
> diff --git a/drivers/s390/scsi/zfcp_qdio.h b/drivers/s390/scsi/zfcp_qdio.h
> index 497cd37..85cdb82 100644
> --- a/drivers/s390/scsi/zfcp_qdio.h
> +++ b/drivers/s390/scsi/zfcp_qdio.h
> @@ -225,21 +225,6 @@ void zfcp_qdio_set_data_div(struct zfcp_qdio *qdio,
>   }
>
>   /**
> - * zfcp_qdio_sbale_count - count sbale used
> - * @sg: pointer to struct scatterlist
> - */
> -static inline
> -unsigned int zfcp_qdio_sbale_count(struct scatterlist *sg)
> -{
> -	unsigned int count = 0;
> -
> -	for (; sg; sg = sg_next(sg))
> -		count++;
> -
> -	return count;
> -}
> -
> -/**
>    * zfcp_qdio_real_bytes - count bytes used
>    * @sg: pointer to struct scatterlist
>    */
>

-- 
Mit freundlichen Grüßen / Kind regards
Steffen Maier

Linux on z Systems Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

[PATCH v2 5/8] lib: introduce sg_nents_len_chained

[LABBE Corentin]

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)
+			*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
-- 
2.4.6

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 <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
>

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

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

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 <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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,

[PATCH v2 6/8] crypto: talitos: replace sg_count with sg_nents_len_chained

[LABBE Corentin]

The talitos driver use a modified version of sg_nents_for_len
called sg_count.
This function is now availlable in lib/scatterlist.c

Replace sg_count by sg_nents_len_chained

Signed-off-by: LABBE Corentin <clabbe.montjoie@gmail.com>
---
 drivers/crypto/talitos.c | 42 ++++++++++++------------------------------
 1 file changed, 12 insertions(+), 30 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 3b20a1b..9156272 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -1234,26 +1234,6 @@ static int ipsec_esp(struct talitos_edesc *edesc, struct aead_request *areq,
 }
 
 /*
- * derive number of elements in scatterlist
- */
-static int sg_count(struct scatterlist *sg_list, int nbytes, bool *chained)
-{
-	struct scatterlist *sg = sg_list;
-	int sg_nents = 0;
-
-	*chained = false;
-	while (nbytes > 0 && sg) {
-		sg_nents++;
-		nbytes -= sg->length;
-		if (!sg_is_last(sg) && (sg + 1)->length == 0)
-			*chained = true;
-		sg = sg_next(sg);
-	}
-
-	return sg_nents;
-}
-
-/*
  * allocate and map the extended descriptor
  */
 static struct talitos_edesc *talitos_edesc_alloc(struct device *dev,
@@ -1287,18 +1267,19 @@ static struct talitos_edesc *talitos_edesc_alloc(struct device *dev,
 		iv_dma = dma_map_single(dev, iv, ivsize, DMA_TO_DEVICE);
 
 	if (!dst || dst == src) {
-		src_nents = sg_count(src, assoclen + cryptlen + authsize,
-				     &src_chained);
+		src_nents = sg_nents_len_chained(src,
+						 assoclen + cryptlen + authsize,
+						 &src_chained);
 		src_nents = (src_nents == 1) ? 0 : src_nents;
 		dst_nents = dst ? src_nents : 0;
 	} else { /* dst && dst != src*/
-		src_nents = sg_count(src, assoclen + cryptlen +
-					  (encrypt ? 0 : authsize),
-				     &src_chained);
+		src_nents = sg_nents_len_chained(src, assoclen + cryptlen +
+						 (encrypt ? 0 : authsize),
+						 &src_chained);
 		src_nents = (src_nents == 1) ? 0 : src_nents;
-		dst_nents = sg_count(dst, assoclen + cryptlen +
-					  (encrypt ? authsize : 0),
-				     &dst_chained);
+		dst_nents = sg_nents_len_chained(dst, assoclen + cryptlen +
+						 (encrypt ? authsize : 0),
+						 &dst_chained);
 		dst_nents = (dst_nents == 1) ? 0 : dst_nents;
 	}
 
@@ -1902,7 +1883,8 @@ static int ahash_process_req(struct ahash_request *areq, unsigned int nbytes)
 	if (!req_ctx->last && (nbytes + req_ctx->nbuf <= blocksize)) {
 		/* Buffer up to one whole block */
 		sg_copy_to_buffer(areq->src,
-				  sg_count(areq->src, nbytes, &chained),
+				  sg_nents_len_chained(areq->src,
+						       nbytes, &chained),
 				  req_ctx->buf + req_ctx->nbuf, nbytes);
 		req_ctx->nbuf += nbytes;
 		return 0;
@@ -1935,7 +1917,7 @@ static int ahash_process_req(struct ahash_request *areq, unsigned int nbytes)
 		req_ctx->psrc = areq->src;
 
 	if (to_hash_later) {
-		int nents = sg_count(areq->src, nbytes, &chained);
+		int nents = sg_nents_len_chained(areq->src, nbytes, &chained);
 		sg_pcopy_to_buffer(areq->src, nents,
 				      req_ctx->bufnext,
 				      to_hash_later,
-- 
2.4.6

[PATCH v2 7/8] crypto: qce: replace qce_countsg with sg_nents_len_chained

[LABBE Corentin]

The qce driver use a modified version of sg_nents_for_len
called qce_countsg.
This function is now availlable in lib/scatterlist.c

Replace qce_countsg by sg_nents_len_chained

Signed-off-by: LABBE Corentin <clabbe.montjoie@gmail.com>
---
 drivers/crypto/qce/ablkcipher.c |  8 ++++----
 drivers/crypto/qce/dma.c        | 19 -------------------
 drivers/crypto/qce/dma.h        |  1 -
 drivers/crypto/qce/sha.c        |  4 ++--
 4 files changed, 6 insertions(+), 26 deletions(-)

diff --git a/drivers/crypto/qce/ablkcipher.c b/drivers/crypto/qce/ablkcipher.c
index ad592de..215bbd2 100644
--- a/drivers/crypto/qce/ablkcipher.c
+++ b/drivers/crypto/qce/ablkcipher.c
@@ -80,11 +80,11 @@ qce_ablkcipher_async_req_handle(struct crypto_async_request *async_req)
 	dir_src = diff_dst ? DMA_TO_DEVICE : DMA_BIDIRECTIONAL;
 	dir_dst = diff_dst ? DMA_FROM_DEVICE : DMA_BIDIRECTIONAL;
 
-	rctx->src_nents = qce_countsg(req->src, req->nbytes,
-				      &rctx->src_chained);
+	rctx->src_nents = sg_nents_len_chained(req->src, req->nbytes,
+					       &rctx->src_chained);
 	if (diff_dst) {
-		rctx->dst_nents = qce_countsg(req->dst, req->nbytes,
-					      &rctx->dst_chained);
+		rctx->dst_nents = sg_nents_len_chained(req->dst, req->nbytes,
+						       &rctx->dst_chained);
 	} else {
 		rctx->dst_nents = rctx->src_nents;
 		rctx->dst_chained = rctx->src_chained;
diff --git a/drivers/crypto/qce/dma.c b/drivers/crypto/qce/dma.c
index 378cb76..c18aaca 100644
--- a/drivers/crypto/qce/dma.c
+++ b/drivers/crypto/qce/dma.c
@@ -87,25 +87,6 @@ void qce_unmapsg(struct device *dev, struct scatterlist *sg, int nents,
 		dma_unmap_sg(dev, sg, nents, dir);
 }
 
-int qce_countsg(struct scatterlist *sglist, int nbytes, bool *chained)
-{
-	struct scatterlist *sg = sglist;
-	int nents = 0;
-
-	if (chained)
-		*chained = false;
-
-	while (nbytes > 0 && sg) {
-		nents++;
-		nbytes -= sg->length;
-		if (!sg_is_last(sg) && (sg + 1)->length == 0 && chained)
-			*chained = true;
-		sg = sg_next(sg);
-	}
-
-	return nents;
-}
-
 struct scatterlist *
 qce_sgtable_add(struct sg_table *sgt, struct scatterlist *new_sgl)
 {
diff --git a/drivers/crypto/qce/dma.h b/drivers/crypto/qce/dma.h
index 65bedb8..4653e2d8 100644
--- a/drivers/crypto/qce/dma.h
+++ b/drivers/crypto/qce/dma.h
@@ -49,7 +49,6 @@ int qce_dma_prep_sgs(struct qce_dma_data *dma, struct scatterlist *sg_in,
 		     dma_async_tx_callback cb, void *cb_param);
 void qce_dma_issue_pending(struct qce_dma_data *dma);
 int qce_dma_terminate_all(struct qce_dma_data *dma);
-int qce_countsg(struct scatterlist *sg_list, int nbytes, bool *chained);
 void qce_unmapsg(struct device *dev, struct scatterlist *sg, int nents,
 		 enum dma_data_direction dir, bool chained);
 int qce_mapsg(struct device *dev, struct scatterlist *sg, int nents,
diff --git a/drivers/crypto/qce/sha.c b/drivers/crypto/qce/sha.c
index be2f504..69c9495 100644
--- a/drivers/crypto/qce/sha.c
+++ b/drivers/crypto/qce/sha.c
@@ -92,8 +92,8 @@ static int qce_ahash_async_req_handle(struct crypto_async_request *async_req)
 		rctx->authklen = AES_KEYSIZE_128;
 	}
 
-	rctx->src_nents = qce_countsg(req->src, req->nbytes,
-				      &rctx->src_chained);
+	rctx->src_nents = sg_nents_len_chained(req->src, req->nbytes,
+					       &rctx->src_chained);
 	ret = qce_mapsg(qce->dev, req->src, rctx->src_nents, DMA_TO_DEVICE,
 			rctx->src_chained);
 	if (ret < 0)
-- 
2.4.6

[PATCH v2 8/8] crypto: caam: replace __sg_count with sg_nents_len_chained

[LABBE Corentin]

The caam driver use a modified version of sg_nents_for_len
called __sg_count.
This function is now availlable in lib/scatterlist.c

Replace __sg_count by sg_nents_len_chained

Signed-off-by: LABBE Corentin <clabbe.montjoie@gmail.com>
---
 drivers/crypto/caam/caamhash.c   | 14 ++++++++------
 drivers/crypto/caam/sg_sw_sec4.h | 20 +-------------------
 2 files changed, 9 insertions(+), 25 deletions(-)

diff --git a/drivers/crypto/caam/caamhash.c b/drivers/crypto/caam/caamhash.c
index 94433b9..116af3e 100644
--- a/drivers/crypto/caam/caamhash.c
+++ b/drivers/crypto/caam/caamhash.c
@@ -797,8 +797,9 @@ static int ahash_update_ctx(struct ahash_request *req)
 	to_hash = in_len - *next_buflen;
 
 	if (to_hash) {
-		src_nents = __sg_count(req->src, req->nbytes - (*next_buflen),
-				       &chained);
+		src_nents = sg_nents_len_chained(req->src,
+						 req->nbytes - (*next_buflen),
+						 &chained);
 		sec4_sg_src_index = 1 + (*buflen ? 1 : 0);
 		sec4_sg_bytes = (sec4_sg_src_index + src_nents) *
 				 sizeof(struct sec4_sg_entry);
@@ -1000,7 +1001,7 @@ static int ahash_finup_ctx(struct ahash_request *req)
 	int ret = 0;
 	int sh_len;
 
-	src_nents = __sg_count(req->src, req->nbytes, &chained);
+	src_nents = sg_nents_len_chained(req->src, req->nbytes, &chained);
 	sec4_sg_src_index = 1 + (buflen ? 1 : 0);
 	sec4_sg_bytes = (sec4_sg_src_index + src_nents) *
 			 sizeof(struct sec4_sg_entry);
@@ -1236,8 +1237,9 @@ static int ahash_update_no_ctx(struct ahash_request *req)
 	to_hash = in_len - *next_buflen;
 
 	if (to_hash) {
-		src_nents = __sg_count(req->src, req->nbytes - (*next_buflen),
-				       &chained);
+		src_nents = sg_nents_len_chained(req->src,
+						 req->nbytes - (*next_buflen),
+						 &chained);
 		sec4_sg_bytes = (1 + src_nents) *
 				sizeof(struct sec4_sg_entry);
 
@@ -1347,7 +1349,7 @@ static int ahash_finup_no_ctx(struct ahash_request *req)
 	int sh_len;
 	int ret = 0;
 
-	src_nents = __sg_count(req->src, req->nbytes, &chained);
+	src_nents = sg_nents_len_chained(req->src, req->nbytes, &chained);
 	sec4_sg_src_index = 2;
 	sec4_sg_bytes = (sec4_sg_src_index + src_nents) *
 			 sizeof(struct sec4_sg_entry);
diff --git a/drivers/crypto/caam/sg_sw_sec4.h b/drivers/crypto/caam/sg_sw_sec4.h
index 18cd6d1..0575a8e 100644
--- a/drivers/crypto/caam/sg_sw_sec4.h
+++ b/drivers/crypto/caam/sg_sw_sec4.h
@@ -69,29 +69,11 @@ static inline struct sec4_sg_entry *sg_to_sec4_sg_len(
 	return sec4_sg_ptr - 1;
 }
 
-/* count number of elements in scatterlist */
-static inline int __sg_count(struct scatterlist *sg_list, int nbytes,
-			     bool *chained)
-{
-	struct scatterlist *sg = sg_list;
-	int sg_nents = 0;
-
-	while (nbytes > 0) {
-		sg_nents++;
-		nbytes -= sg->length;
-		if (!sg_is_last(sg) && (sg + 1)->length == 0)
-			*chained = true;
-		sg = sg_next(sg);
-	}
-
-	return sg_nents;
-}
-
 /* derive number of elements in scatterlist, but return 0 for 1 */
 static inline int sg_count(struct scatterlist *sg_list, int nbytes,
 			     bool *chained)
 {
-	int sg_nents = __sg_count(sg_list, nbytes, chained);
+	int sg_nents = sg_nents_len_chained(sg_list, nbytes, chained);
 
 	if (likely(sg_nents == 1))
 		return 0;
-- 
2.4.6

Re: [PATCH v2] crypto: Remove duplicate code of SG helpers functions

[Herbert Xu]

On Fri, Sep 18, 2015 at 02:57:08PM +0200, LABBE Corentin wrote:
> Hello
> 
> This patch series try to remove some duplicate code of some SG helpers functions.
> The first four patch replace custom functions by already in-tree helper functions.

Patches 1-3 applied.  Patch 4 should go through the s390 tree.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt