Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1227843 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v2] crypto: Remove duplicate code of SG helpers functions LABBE Corentin <clabbe.montjoie@gmail.com> - 2015-09-18 15:00 +0200
    [PATCH v2 4/8] s390: replace zfcp_qdio_sbale_count by sg_nents LABBE Corentin <clabbe.montjoie@gmail.com> - 2015-09-18 15:00 +0200
    [PATCH v2 5/8] lib: introduce sg_nents_len_chained LABBE Corentin <clabbe.montjoie@gmail.com> - 2015-09-18 15:00 +0200
      Re: [PATCH v2 5/8] lib: introduce sg_nents_len_chained Tony Battersby <tonyb@cybernetics.com> - 2015-09-18 18:20 +0200
        Re: [PATCH v2 5/8] lib: introduce sg_nents_len_chained Tony Battersby <tonyb@cybernetics.com> - 2015-09-18 23:30 +0200
          Re: [PATCH v2 5/8] lib: introduce sg_nents_len_chained Herbert Xu <herbert@gondor.apana.org.au> - 2015-09-21 16:30 +0200
            Re: [PATCH v2 5/8] lib: introduce sg_nents_len_chained LABBE Corentin <clabbe.montjoie@gmail.com> - 2015-09-21 17:10 +0200
    [PATCH v2 3/8] crypto: sahara: replace sahara_sg_length with sg_nents_for_len LABBE Corentin <clabbe.montjoie@gmail.com> - 2015-09-18 15:00 +0200
    Re: [PATCH v2] crypto: Remove duplicate code of SG helpers functions Herbert Xu <herbert@gondor.apana.org.au> - 2015-09-21 17:10 +0200

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1227845 — [PATCH v2 5/8] lib: introduce sg_nents_len_chained

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1229456

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web