[PATCH 1/2] crypto: s5p-sss - Minor coding cleanups

[PATCH 1/2] crypto: s5p-sss - Minor coding cleanups

[Krzysztof Kozlowski]

Remove unneeded inclusion of delay.h and get rid of indentation from
labels.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 drivers/crypto/s5p-sss.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
index 5f161a9777e3..60f835455a41 100644
--- a/drivers/crypto/s5p-sss.c
+++ b/drivers/crypto/s5p-sss.c
@@ -11,7 +11,6 @@
  *
  */
 
-#include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/module.h>
 #include <linux/init.h>
@@ -284,7 +283,7 @@ static int s5p_set_outdata(struct s5p_aes_dev *dev, struct scatterlist *sg)
 	dev->sg_dst = sg;
 	err = 0;
 
- exit:
+exit:
 	return err;
 }
 
@@ -310,7 +309,7 @@ static int s5p_set_indata(struct s5p_aes_dev *dev, struct scatterlist *sg)
 	dev->sg_src = sg;
 	err = 0;
 
- exit:
+exit:
 	return err;
 }
 
@@ -452,10 +451,10 @@ static void s5p_aes_crypt_start(struct s5p_aes_dev *dev, unsigned long mode)
 
 	return;
 
- outdata_error:
+outdata_error:
 	s5p_unset_indata(dev);
 
- indata_error:
+indata_error:
 	s5p_aes_complete(dev, err);
 	spin_unlock_irqrestore(&dev->lock, flags);
 }
@@ -506,7 +505,7 @@ static int s5p_aes_handle_req(struct s5p_aes_dev *dev,
 
 	tasklet_schedule(&dev->tasklet);
 
- exit:
+exit:
 	return err;
 }
 
@@ -705,7 +704,7 @@ static int s5p_aes_probe(struct platform_device *pdev)
 
 	return 0;
 
- err_algs:
+err_algs:
 	dev_err(dev, "can't register '%s': %d\n", algs[i].cra_name, err);
 
 	for (j = 0; j < i; j++)
@@ -713,7 +712,7 @@ static int s5p_aes_probe(struct platform_device *pdev)
 
 	tasklet_kill(&pdata->tasklet);
 
- err_irq:
+err_irq:
 	clk_disable_unprepare(pdata->clk);
 
 	s5p_dev = NULL;
-- 
2.1.4

[PATCH 2/2] crypto: s5p-sss - Handle unaligned buffers

[Krzysztof Kozlowski]

During crypto selftests on Odroid XU3 (Exynos5422) some of the
algorithms failed because of passing AES-block unaligned source and
destination buffers:

alg: skcipher: encryption failed on chunk test 1 for ecb-aes-s5p: ret=22

Handle such case by copying the buffers to a new aligned and contiguous
space.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 drivers/crypto/s5p-sss.c | 149 +++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 137 insertions(+), 12 deletions(-)

diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
index 60f835455a41..bac1f4593f98 100644
--- a/drivers/crypto/s5p-sss.c
+++ b/drivers/crypto/s5p-sss.c
@@ -28,6 +28,7 @@
 #include <crypto/algapi.h>
 #include <crypto/aes.h>
 #include <crypto/ctr.h>
+#include <crypto/scatterwalk.h>
 
 #define _SBF(s, v)                      ((v) << (s))
 #define _BIT(b)                         _SBF(b, 1)
@@ -185,6 +186,10 @@ struct s5p_aes_dev {
 	struct scatterlist         *sg_src;
 	struct scatterlist         *sg_dst;
 
+	/* In case of unaligned access: */
+	struct scatterlist         *sg_src_cpy;
+	struct scatterlist         *sg_dst_cpy;
+
 	struct tasklet_struct       tasklet;
 	struct crypto_queue         queue;
 	bool                        busy;
@@ -244,8 +249,45 @@ static void s5p_set_dma_outdata(struct s5p_aes_dev *dev, struct scatterlist *sg)
 	SSS_WRITE(dev, FCBTDMAL, sg_dma_len(sg));
 }
 
+static void s5p_free_sg_cpy(struct s5p_aes_dev *dev, struct scatterlist **sg)
+{
+	int len;
+
+	if (!*sg)
+		return;
+
+	len = ALIGN(dev->req->nbytes, AES_BLOCK_SIZE);
+	free_pages((unsigned long)sg_virt(*sg), get_order(len));
+
+	kfree(*sg);
+	*sg = NULL;
+}
+
+static void s5p_sg_copy_buf(void *buf, struct scatterlist *sg,
+			    unsigned int nbytes, int out)
+{
+	struct scatter_walk walk;
+
+	if (!nbytes)
+		return;
+
+	scatterwalk_start(&walk, sg);
+	scatterwalk_copychunks(buf, &walk, nbytes, out);
+	scatterwalk_done(&walk, out, 0);
+}
+
 static void s5p_aes_complete(struct s5p_aes_dev *dev, int err)
 {
+	if (dev->sg_dst_cpy) {
+		dev_dbg(dev->dev,
+			"Copying %d bytes of output data back to original place\n",
+			dev->req->nbytes);
+		s5p_sg_copy_buf(sg_virt(dev->sg_dst_cpy), dev->req->dst,
+				dev->req->nbytes, 1);
+	}
+	s5p_free_sg_cpy(dev, &dev->sg_src_cpy);
+	s5p_free_sg_cpy(dev, &dev->sg_dst_cpy);
+
 	/* holding a lock outside */
 	dev->req->base.complete(&dev->req->base, err);
 	dev->busy = false;
@@ -261,14 +303,36 @@ static void s5p_unset_indata(struct s5p_aes_dev *dev)
 	dma_unmap_sg(dev->dev, dev->sg_src, 1, DMA_TO_DEVICE);
 }
 
+static int s5p_make_sg_cpy(struct s5p_aes_dev *dev, struct scatterlist *src,
+			    struct scatterlist **dst)
+{
+	void *pages;
+	int len;
+
+	*dst = kmalloc(sizeof(**dst), GFP_ATOMIC);
+	if (!*dst)
+		return -ENOMEM;
+
+	len = ALIGN(dev->req->nbytes, AES_BLOCK_SIZE);
+	pages = (void *)__get_free_pages(GFP_ATOMIC, get_order(len));
+	if (!pages) {
+		kfree(*dst);
+		*dst = NULL;
+		return -ENOMEM;
+	}
+
+	s5p_sg_copy_buf(pages, src, dev->req->nbytes, 0);
+
+	sg_init_table(*dst, 1);
+	sg_set_buf(*dst, pages, len);
+
+	return 0;
+}
+
 static int s5p_set_outdata(struct s5p_aes_dev *dev, struct scatterlist *sg)
 {
 	int err;
 
-	if (!IS_ALIGNED(sg_dma_len(sg), AES_BLOCK_SIZE)) {
-		err = -EINVAL;
-		goto exit;
-	}
 	if (!sg_dma_len(sg)) {
 		err = -EINVAL;
 		goto exit;
@@ -291,10 +355,6 @@ static int s5p_set_indata(struct s5p_aes_dev *dev, struct scatterlist *sg)
 {
 	int err;
 
-	if (!IS_ALIGNED(sg_dma_len(sg), AES_BLOCK_SIZE)) {
-		err = -EINVAL;
-		goto exit;
-	}
 	if (!sg_dma_len(sg)) {
 		err = -EINVAL;
 		goto exit;
@@ -394,6 +454,70 @@ static void s5p_set_aes(struct s5p_aes_dev *dev,
 	memcpy_toio(keystart, key, keylen);
 }
 
+static bool s5p_is_sg_aligned(struct scatterlist *sg)
+{
+	do {
+		if (!IS_ALIGNED(sg_dma_len(sg), AES_BLOCK_SIZE))
+			return false;
+	} while (!sg_is_last(sg));
+
+	return true;
+}
+
+static int s5p_set_indata_start(struct s5p_aes_dev *dev,
+				struct ablkcipher_request *req)
+{
+	struct scatterlist *sg;
+	int err;
+
+	dev->sg_src_cpy = NULL;
+	sg = req->src;
+	if (!s5p_is_sg_aligned(sg)) {
+		dev_dbg(dev->dev,
+			"At least one unaligned source scatter list, making a copy\n");
+		err = s5p_make_sg_cpy(dev, sg, &dev->sg_src_cpy);
+		if (err)
+			return err;
+
+		sg = dev->sg_src_cpy;
+	}
+
+	err = s5p_set_indata(dev, sg);
+	if (err) {
+		s5p_free_sg_cpy(dev, &dev->sg_src_cpy);
+		return err;
+	}
+
+	return 0;
+}
+
+static int s5p_set_outdata_start(struct s5p_aes_dev *dev,
+				struct ablkcipher_request *req)
+{
+	struct scatterlist *sg;
+	int err;
+
+	dev->sg_dst_cpy = NULL;
+	sg = req->dst;
+	if (!s5p_is_sg_aligned(sg)) {
+		dev_dbg(dev->dev,
+			"At least one unaligned dest scatter list, making a copy\n");
+		err = s5p_make_sg_cpy(dev, sg, &dev->sg_dst_cpy);
+		if (err)
+			return err;
+
+		sg = dev->sg_dst_cpy;
+	}
+
+	err = s5p_set_outdata(dev, sg);
+	if (err) {
+		s5p_free_sg_cpy(dev, &dev->sg_dst_cpy);
+		return err;
+	}
+
+	return 0;
+}
+
 static void s5p_aes_crypt_start(struct s5p_aes_dev *dev, unsigned long mode)
 {
 	struct ablkcipher_request  *req = dev->req;
@@ -430,19 +554,19 @@ static void s5p_aes_crypt_start(struct s5p_aes_dev *dev, unsigned long mode)
 		  SSS_FCINTENCLR_BTDMAINTENCLR | SSS_FCINTENCLR_BRDMAINTENCLR);
 	SSS_WRITE(dev, FCFIFOCTRL, 0x00);
 
-	err = s5p_set_indata(dev, req->src);
+	err = s5p_set_indata_start(dev, req);
 	if (err)
 		goto indata_error;
 
-	err = s5p_set_outdata(dev, req->dst);
+	err = s5p_set_outdata_start(dev, req);
 	if (err)
 		goto outdata_error;
 
 	SSS_AES_WRITE(dev, AES_CONTROL, aes_control);
 	s5p_set_aes(dev, dev->ctx->aes_key, req->info, dev->ctx->keylen);
 
-	s5p_set_dma_indata(dev,  req->src);
-	s5p_set_dma_outdata(dev, req->dst);
+	s5p_set_dma_indata(dev,  dev->sg_src);
+	s5p_set_dma_outdata(dev, dev->sg_dst);
 
 	SSS_WRITE(dev, FCINTENSET,
 		  SSS_FCINTENSET_BTDMAINTENSET | SSS_FCINTENSET_BRDMAINTENSET);
@@ -452,6 +576,7 @@ static void s5p_aes_crypt_start(struct s5p_aes_dev *dev, unsigned long mode)
 	return;
 
 outdata_error:
+	s5p_free_sg_cpy(dev, &dev->sg_src_cpy);
 	s5p_unset_indata(dev);
 
 indata_error:
-- 
2.1.4

Re: [PATCH 2/2] crypto: s5p-sss - Handle unaligned buffers

[Vladimir Zapolskiy]

Hi Krzysztof,

On 06.03.2016 12:17, Krzysztof Kozlowski wrote:
> During crypto selftests on Odroid XU3 (Exynos5422) some of the
> algorithms failed because of passing AES-block unaligned source and
> destination buffers:

excuse my ignorance what are the crypto selftests you reference? Are they
run-time self tests run by crypto manager on algorithm registration?

> alg: skcipher: encryption failed on chunk test 1 for ecb-aes-s5p: ret=22
> 
> Handle such case by copying the buffers to a new aligned and contiguous
> space.

I'm not quite convinced that a particular crypto accelerator driver
is the right place for this change, at least I don't see in the change
anything S5P-SSS specific, but it might be good to add the change.

Briefly looking at other drivers similarly atmel-* and omap-* have
slow path fallbacks, ./rockchip/rk3288_crypto_ablkcipher.c fails like
s5p-sss etc.

Anyway since it is a valid improvement, please feel free to add my

Acked-by: Vladimir Zapolskiy <vz@mleia.com>

> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
>  drivers/crypto/s5p-sss.c | 149 +++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 137 insertions(+), 12 deletions(-)
> 

Re: [PATCH 2/2] crypto: s5p-sss - Handle unaligned buffers

[Krzysztof Kozlowski]

On 07.03.2016 10:28, Vladimir Zapolskiy wrote:
> Hi Krzysztof,
> 
> On 06.03.2016 12:17, Krzysztof Kozlowski wrote:
>> During crypto selftests on Odroid XU3 (Exynos5422) some of the
>> algorithms failed because of passing AES-block unaligned source and
>> destination buffers:
> 
> excuse my ignorance what are the crypto selftests you reference? Are they
> run-time self tests run by crypto manager on algorithm registration?

Yes, these tests. Disabled by CRYPTO_MANAGER_DISABLE_TESTS.

> 
>> alg: skcipher: encryption failed on chunk test 1 for ecb-aes-s5p: ret=22
>>
>> Handle such case by copying the buffers to a new aligned and contiguous
>> space.
> 
> I'm not quite convinced that a particular crypto accelerator driver
> is the right place for this change, at least I don't see in the change
> anything S5P-SSS specific, but it might be good to add the change.

The driver sets a .cra_alignmask but docs are saying that re-alignment
by crypto API might not happen. And in fact for selftets the data was
coming sometimes not-aligned.

On the other hand the CRYPTO_TEST was providing aligned data.

> 
> Briefly looking at other drivers similarly atmel-* and omap-* have
> slow path fallbacks, ./rockchip/rk3288_crypto_ablkcipher.c fails like
> s5p-sss etc.

Yes, I was kind of inspired by the omap solution.

> Anyway since it is a valid improvement, please feel free to add my
> 
> Acked-by: Vladimir Zapolskiy <vz@mleia.com>

Thanks!

Krzysztof
> 
>> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
>> ---
>>  drivers/crypto/s5p-sss.c | 149 +++++++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 137 insertions(+), 12 deletions(-)
>>

Re: [PATCH 1/2] crypto: s5p-sss - Minor coding cleanups

[Vladimir Zapolskiy]

Hi Krzysztof,

On 06.03.2016 12:17, Krzysztof Kozlowski wrote:
> Remove unneeded inclusion of delay.h and get rid of indentation from
> labels.

just in case are you aware of this discussion regarding goto labels
http://lkml.iu.edu/hypermail/linux/kernel/0706.0/0809.html ?

Practically there is no big need (neither it is a clean-up IMHO)
to change column 1 indentation of goto labels, however I do agree that
^[a-z0-9_]\+:$ style of goto labels is 10 times more commonly used,
and that means it's a good time to update my ~/.emacs config.

Someone may want to fix the style of another 5000+ goto labels one day,
though it is better to have an officially agreed policy beforehand.

Anyway I do not strictly object against the change, see one my comment
below and feel free to add my

Acked-by: Vladimir Zapolskiy <vz@mleia.com>

> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
>  drivers/crypto/s5p-sss.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
> index 5f161a9777e3..60f835455a41 100644
> --- a/drivers/crypto/s5p-sss.c
> +++ b/drivers/crypto/s5p-sss.c
> @@ -11,7 +11,6 @@
>   *
>   */
>  
> -#include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/module.h>
>  #include <linux/init.h>

Since you are here, can you sort headers out, please?

And I'm pretty sure some more inclusions can be safely removed
from the list.

> @@ -284,7 +283,7 @@ static int s5p_set_outdata(struct s5p_aes_dev *dev, struct scatterlist *sg)
>  	dev->sg_dst = sg;
>  	err = 0;
>  
> - exit:
> +exit:
>  	return err;
>  }
>  
> @@ -310,7 +309,7 @@ static int s5p_set_indata(struct s5p_aes_dev *dev, struct scatterlist *sg)
>  	dev->sg_src = sg;
>  	err = 0;
>  
> - exit:
> +exit:
>  	return err;
>  }
>  
> @@ -452,10 +451,10 @@ static void s5p_aes_crypt_start(struct s5p_aes_dev *dev, unsigned long mode)
>  
>  	return;
>  
> - outdata_error:
> +outdata_error:
>  	s5p_unset_indata(dev);
>  
> - indata_error:
> +indata_error:
>  	s5p_aes_complete(dev, err);
>  	spin_unlock_irqrestore(&dev->lock, flags);
>  }
> @@ -506,7 +505,7 @@ static int s5p_aes_handle_req(struct s5p_aes_dev *dev,
>  
>  	tasklet_schedule(&dev->tasklet);
>  
> - exit:
> +exit:
>  	return err;
>  }
>  
> @@ -705,7 +704,7 @@ static int s5p_aes_probe(struct platform_device *pdev)
>  
>  	return 0;
>  
> - err_algs:
> +err_algs:
>  	dev_err(dev, "can't register '%s': %d\n", algs[i].cra_name, err);
>  
>  	for (j = 0; j < i; j++)
> @@ -713,7 +712,7 @@ static int s5p_aes_probe(struct platform_device *pdev)
>  
>  	tasklet_kill(&pdata->tasklet);
>  
> - err_irq:
> +err_irq:
>  	clk_disable_unprepare(pdata->clk);
>  
>  	s5p_dev = NULL;
> 

--
With best wishes,
Vladimir

Re: [PATCH 1/2] crypto: s5p-sss - Minor coding cleanups

[Krzysztof Kozlowski]

On 07.03.2016 10:01, Vladimir Zapolskiy wrote:
> Hi Krzysztof,
> 
> On 06.03.2016 12:17, Krzysztof Kozlowski wrote:
>> Remove unneeded inclusion of delay.h and get rid of indentation from
>> labels.
> 
> just in case are you aware of this discussion regarding goto labels
> http://lkml.iu.edu/hypermail/linux/kernel/0706.0/0809.html ?
> 
> Practically there is no big need (neither it is a clean-up IMHO)
> to change column 1 indentation of goto labels, however I do agree that
> ^[a-z0-9_]\+:$ style of goto labels is 10 times more commonly used,
> and that means it's a good time to update my ~/.emacs config.
> 
> Someone may want to fix the style of another 5000+ goto labels one day,
> though it is better to have an officially agreed policy beforehand.
> 
> Anyway I do not strictly object against the change, see one my comment
> below and feel free to add my
> 
> Acked-by: Vladimir Zapolskiy <vz@mleia.com>

Actually I was not aware of that discussion, thanks for pointing this
out. Mostly I encountered the style without indentation but of course
that does not mean it is the only one.

> 
>> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
>> ---
>>  drivers/crypto/s5p-sss.c | 15 +++++++--------
>>  1 file changed, 7 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
>> index 5f161a9777e3..60f835455a41 100644
>> --- a/drivers/crypto/s5p-sss.c
>> +++ b/drivers/crypto/s5p-sss.c
>> @@ -11,7 +11,6 @@
>>   *
>>   */
>>  
>> -#include <linux/delay.h>
>>  #include <linux/err.h>
>>  #include <linux/module.h>
>>  #include <linux/init.h>
> 
> Since you are here, can you sort headers out, please?

You mean sort by name (or by christmas tree)? I can... I am not sure
about benefits of such patch (which should be the motivation for change).


> And I'm pretty sure some more inclusions can be safely removed
> from the list.

I checked them while removing delay - all of them seem to be used.

Best regards,
Krzysztof

Re: [PATCH 1/2] crypto: s5p-sss - Minor coding cleanups

[Vladimir Zapolskiy]

Hi Krzysztof,

On 07.03.2016 06:05, Krzysztof Kozlowski wrote:
> On 07.03.2016 10:01, Vladimir Zapolskiy wrote:
>> Hi Krzysztof,
>>
>> On 06.03.2016 12:17, Krzysztof Kozlowski wrote:
>>> Remove unneeded inclusion of delay.h and get rid of indentation from
>>> labels.
>>
>> just in case are you aware of this discussion regarding goto labels
>> http://lkml.iu.edu/hypermail/linux/kernel/0706.0/0809.html ?
>>
>> Practically there is no big need (neither it is a clean-up IMHO)
>> to change column 1 indentation of goto labels, however I do agree that
>> ^[a-z0-9_]\+:$ style of goto labels is 10 times more commonly used,
>> and that means it's a good time to update my ~/.emacs config.
>>
>> Someone may want to fix the style of another 5000+ goto labels one day,
>> though it is better to have an officially agreed policy beforehand.
>>
>> Anyway I do not strictly object against the change, see one my comment
>> below and feel free to add my
>>
>> Acked-by: Vladimir Zapolskiy <vz@mleia.com>
> 
> Actually I was not aware of that discussion, thanks for pointing this
> out. Mostly I encountered the style without indentation but of course
> that does not mean it is the only one.
> 
>>
>>> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
>>> ---
>>>  drivers/crypto/s5p-sss.c | 15 +++++++--------
>>>  1 file changed, 7 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/crypto/s5p-sss.c b/drivers/crypto/s5p-sss.c
>>> index 5f161a9777e3..60f835455a41 100644
>>> --- a/drivers/crypto/s5p-sss.c
>>> +++ b/drivers/crypto/s5p-sss.c
>>> @@ -11,7 +11,6 @@
>>>   *
>>>   */
>>>  
>>> -#include <linux/delay.h>
>>>  #include <linux/err.h>
>>>  #include <linux/module.h>
>>>  #include <linux/init.h>
>>
>> Since you are here, can you sort headers out, please?
> 
> You mean sort by name (or by christmas tree)? I can... 

Thank you in advance.

> I am not sure
> about benefits of such patch (which should be the motivation for change).

In general it simplifies navigation through the list of headers for
humans IMHO.

By example, let's look at drivers/media/usb/dvb-usb/dw2102.c :

#include "dw2102.h"
#include "si21xx.h"
#include "stv0299.h"
#include "z0194a.h"
#include "stv0288.h"
#include "stb6000.h"
#include "eds1547.h"
#include "cx24116.h"
#include "tda1002x.h"
#include "mt312.h"
#include "zl10039.h"
#include "ts2020.h"
#include "ds3000.h"
#include "stv0900.h"
#include "stv6110.h"
#include "stb6100.h"
#include "stb6100_proc.h"
#include "m88rs2000.h"
#include "tda18271.h"
#include "cxd2820r.h"
#include "m88ds3103.h"
#include "ts2020.h"

Was it obvious for an author/reviewers that commit 0fecb6c0944f adds a
duplicate?

What is the chance to add a duplicate, if the list of headers is sorted?

>> And I'm pretty sure some more inclusions can be safely removed
>> from the list.
> 
> I checked them while removing delay - all of them seem to be used.

Good, thank you for checking it :)

--
With best wishes,
Vladimir