[PATCH v2 0/4] Additional fixes on Talitos driver

[PATCH v2 0/4] Additional fixes on Talitos driver

[Christophe Leroy]

This series is the last set of fixes for the Talitos driver.

We now get a fully clean boot on both SEC1 (SEC1.2 on mpc885) and
SEC2 (SEC2.2 on mpc8321E) with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS:

[    3.385197] bus: 'platform': really_probe: probing driver talitos with device ff020000.crypto
[    3.450982] random: fast init done
[   12.252548] alg: No test for authenc(hmac(md5),cbc(aes)) (authenc-hmac-md5-cbc-aes-talitos-hsna)
[   12.262226] alg: No test for authenc(hmac(md5),cbc(des3_ede)) (authenc-hmac-md5-cbc-3des-talitos-hsna)
[   43.310737] Bug in SEC1, padding ourself
[   45.603318] random: crng init done
[   54.612333] talitos ff020000.crypto: fsl,sec1.2 algorithms registered in /proc/crypto
[   54.620232] driver: 'talitos': driver_bound: bound to device 'ff020000.crypto'

[    1.193721] bus: 'platform': really_probe: probing driver talitos with device b0030000.crypto
[    1.229197] random: fast init done
[    2.714920] alg: No test for authenc(hmac(sha224),cbc(aes)) (authenc-hmac-sha224-cbc-aes-talitos)
[    2.724312] alg: No test for authenc(hmac(sha224),cbc(aes)) (authenc-hmac-sha224-cbc-aes-talitos-hsna)
[    4.482045] alg: No test for authenc(hmac(md5),cbc(aes)) (authenc-hmac-md5-cbc-aes-talitos)
[    4.490940] alg: No test for authenc(hmac(md5),cbc(aes)) (authenc-hmac-md5-cbc-aes-talitos-hsna)
[    4.500280] alg: No test for authenc(hmac(md5),cbc(des3_ede)) (authenc-hmac-md5-cbc-3des-talitos)
[    4.509727] alg: No test for authenc(hmac(md5),cbc(des3_ede)) (authenc-hmac-md5-cbc-3des-talitos-hsna)
[    6.631781] random: crng init done
[   11.521795] talitos b0030000.crypto: fsl,sec2.2 algorithms registered in /proc/crypto
[   11.529803] driver: 'talitos': driver_bound: bound to device 'b0030000.crypto'

v2: dropped patch 1 which was irrelevant due to a rebase weirdness. Added Cc to stable on the 2 first patches.

Christophe Leroy (4):
  crypto: talitos - move struct talitos_edesc into talitos.h
  crypto: talitos - fix hash on SEC1.
  crypto: talitos - eliminate unneeded 'done' functions at build time
  crypto: talitos - drop icv_ool

 drivers/crypto/talitos.c | 98 ++++++++++++++++++++----------------------------
 drivers/crypto/talitos.h | 28 ++++++++++++++
 2 files changed, 69 insertions(+), 57 deletions(-)

-- 
2.13.3

[PATCH v2 0/4] Additional fixes on Talitos driver

[Christophe Leroy]

This series is the last set of fixes for the Talitos driver.

We now get a fully clean boot on both SEC1 (SEC1.2 on mpc885) and
SEC2 (SEC2.2 on mpc8321E) with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS:

[    3.385197] bus: 'platform': really_probe: probing driver talitos with device ff020000.crypto
[    3.450982] random: fast init done
[   12.252548] alg: No test for authenc(hmac(md5),cbc(aes)) (authenc-hmac-md5-cbc-aes-talitos-hsna)
[   12.262226] alg: No test for authenc(hmac(md5),cbc(des3_ede)) (authenc-hmac-md5-cbc-3des-talitos-hsna)
[   43.310737] Bug in SEC1, padding ourself
[   45.603318] random: crng init done
[   54.612333] talitos ff020000.crypto: fsl,sec1.2 algorithms registered in /proc/crypto
[   54.620232] driver: 'talitos': driver_bound: bound to device 'ff020000.crypto'

[    1.193721] bus: 'platform': really_probe: probing driver talitos with device b0030000.crypto
[    1.229197] random: fast init done
[    2.714920] alg: No test for authenc(hmac(sha224),cbc(aes)) (authenc-hmac-sha224-cbc-aes-talitos)
[    2.724312] alg: No test for authenc(hmac(sha224),cbc(aes)) (authenc-hmac-sha224-cbc-aes-talitos-hsna)
[    4.482045] alg: No test for authenc(hmac(md5),cbc(aes)) (authenc-hmac-md5-cbc-aes-talitos)
[    4.490940] alg: No test for authenc(hmac(md5),cbc(aes)) (authenc-hmac-md5-cbc-aes-talitos-hsna)
[    4.500280] alg: No test for authenc(hmac(md5),cbc(des3_ede)) (authenc-hmac-md5-cbc-3des-talitos)
[    4.509727] alg: No test for authenc(hmac(md5),cbc(des3_ede)) (authenc-hmac-md5-cbc-3des-talitos-hsna)
[    6.631781] random: crng init done
[   11.521795] talitos b0030000.crypto: fsl,sec2.2 algorithms registered in /proc/crypto
[   11.529803] driver: 'talitos': driver_bound: bound to device 'b0030000.crypto'

v2: dropped patch 1 which was irrelevant due to a rebase weirdness. Added Cc to stable on the 2 first patches.

Christophe Leroy (4):
  crypto: talitos - move struct talitos_edesc into talitos.h
  crypto: talitos - fix hash on SEC1.
  crypto: talitos - eliminate unneeded 'done' functions at build time
  crypto: talitos - drop icv_ool

 drivers/crypto/talitos.c | 98 ++++++++++++++++++++----------------------------
 drivers/crypto/talitos.h | 28 ++++++++++++++
 2 files changed, 69 insertions(+), 57 deletions(-)

-- 
2.13.3

[PATCH v2 1/4] crypto: talitos - move struct talitos_edesc into talitos.h

[Christophe Leroy]

Next patch will require struct talitos_edesc to be defined
earlier in talitos.c

This patch moves it into talitos.h so that it can be used
from any place in talitos.c

Fixes: 37b5e8897eb5 ("crypto: talitos - chain in buffered data for ahash on SEC1")
Cc: stable@vger.kernel.org
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 drivers/crypto/talitos.c | 30 ------------------------------
 drivers/crypto/talitos.h | 30 ++++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 30 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 3b3e99f1cddb..5b401aec6c84 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -951,36 +951,6 @@ static int aead_des3_setkey(struct crypto_aead *authenc,
 	goto out;
 }
 
-/*
- * talitos_edesc - s/w-extended descriptor
- * @src_nents: number of segments in input scatterlist
- * @dst_nents: number of segments in output scatterlist
- * @icv_ool: whether ICV is out-of-line
- * @iv_dma: dma address of iv for checking continuity and link table
- * @dma_len: length of dma mapped link_tbl space
- * @dma_link_tbl: bus physical address of link_tbl/buf
- * @desc: h/w descriptor
- * @link_tbl: input and output h/w link tables (if {src,dst}_nents > 1) (SEC2)
- * @buf: input and output buffeur (if {src,dst}_nents > 1) (SEC1)
- *
- * if decrypting (with authcheck), or either one of src_nents or dst_nents
- * is greater than 1, an integrity check value is concatenated to the end
- * of link_tbl data
- */
-struct talitos_edesc {
-	int src_nents;
-	int dst_nents;
-	bool icv_ool;
-	dma_addr_t iv_dma;
-	int dma_len;
-	dma_addr_t dma_link_tbl;
-	struct talitos_desc desc;
-	union {
-		struct talitos_ptr link_tbl[0];
-		u8 buf[0];
-	};
-};
-
 static void talitos_sg_unmap(struct device *dev,
 			     struct talitos_edesc *edesc,
 			     struct scatterlist *src,
diff --git a/drivers/crypto/talitos.h b/drivers/crypto/talitos.h
index 32ad4fc679ed..95f78c6d9206 100644
--- a/drivers/crypto/talitos.h
+++ b/drivers/crypto/talitos.h
@@ -42,6 +42,36 @@ struct talitos_desc {
 
 #define TALITOS_DESC_SIZE	(sizeof(struct talitos_desc) - sizeof(__be32))
 
+/*
+ * talitos_edesc - s/w-extended descriptor
+ * @src_nents: number of segments in input scatterlist
+ * @dst_nents: number of segments in output scatterlist
+ * @icv_ool: whether ICV is out-of-line
+ * @iv_dma: dma address of iv for checking continuity and link table
+ * @dma_len: length of dma mapped link_tbl space
+ * @dma_link_tbl: bus physical address of link_tbl/buf
+ * @desc: h/w descriptor
+ * @link_tbl: input and output h/w link tables (if {src,dst}_nents > 1) (SEC2)
+ * @buf: input and output buffeur (if {src,dst}_nents > 1) (SEC1)
+ *
+ * if decrypting (with authcheck), or either one of src_nents or dst_nents
+ * is greater than 1, an integrity check value is concatenated to the end
+ * of link_tbl data
+ */
+struct talitos_edesc {
+	int src_nents;
+	int dst_nents;
+	bool icv_ool;
+	dma_addr_t iv_dma;
+	int dma_len;
+	dma_addr_t dma_link_tbl;
+	struct talitos_desc desc;
+	union {
+		struct talitos_ptr link_tbl[0];
+		u8 buf[0];
+	};
+};
+
 /**
  * talitos_request - descriptor submission request
  * @desc: descriptor pointer (kernel virtual)
-- 
2.13.3

[PATCH v2 1/4] crypto: talitos - move struct talitos_edesc into talitos.h

[Christophe Leroy]

Next patch will require struct talitos_edesc to be defined
earlier in talitos.c

This patch moves it into talitos.h so that it can be used
from any place in talitos.c

Fixes: 37b5e8897eb5 ("crypto: talitos - chain in buffered data for ahash on SEC1")
Cc: stable@vger.kernel.org
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 drivers/crypto/talitos.c | 30 ------------------------------
 drivers/crypto/talitos.h | 30 ++++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 30 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 3b3e99f1cddb..5b401aec6c84 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -951,36 +951,6 @@ static int aead_des3_setkey(struct crypto_aead *authenc,
 	goto out;
 }
 
-/*
- * talitos_edesc - s/w-extended descriptor
- * @src_nents: number of segments in input scatterlist
- * @dst_nents: number of segments in output scatterlist
- * @icv_ool: whether ICV is out-of-line
- * @iv_dma: dma address of iv for checking continuity and link table
- * @dma_len: length of dma mapped link_tbl space
- * @dma_link_tbl: bus physical address of link_tbl/buf
- * @desc: h/w descriptor
- * @link_tbl: input and output h/w link tables (if {src,dst}_nents > 1) (SEC2)
- * @buf: input and output buffeur (if {src,dst}_nents > 1) (SEC1)
- *
- * if decrypting (with authcheck), or either one of src_nents or dst_nents
- * is greater than 1, an integrity check value is concatenated to the end
- * of link_tbl data
- */
-struct talitos_edesc {
-	int src_nents;
-	int dst_nents;
-	bool icv_ool;
-	dma_addr_t iv_dma;
-	int dma_len;
-	dma_addr_t dma_link_tbl;
-	struct talitos_desc desc;
-	union {
-		struct talitos_ptr link_tbl[0];
-		u8 buf[0];
-	};
-};
-
 static void talitos_sg_unmap(struct device *dev,
 			     struct talitos_edesc *edesc,
 			     struct scatterlist *src,
diff --git a/drivers/crypto/talitos.h b/drivers/crypto/talitos.h
index 32ad4fc679ed..95f78c6d9206 100644
--- a/drivers/crypto/talitos.h
+++ b/drivers/crypto/talitos.h
@@ -42,6 +42,36 @@ struct talitos_desc {
 
 #define TALITOS_DESC_SIZE	(sizeof(struct talitos_desc) - sizeof(__be32))
 
+/*
+ * talitos_edesc - s/w-extended descriptor
+ * @src_nents: number of segments in input scatterlist
+ * @dst_nents: number of segments in output scatterlist
+ * @icv_ool: whether ICV is out-of-line
+ * @iv_dma: dma address of iv for checking continuity and link table
+ * @dma_len: length of dma mapped link_tbl space
+ * @dma_link_tbl: bus physical address of link_tbl/buf
+ * @desc: h/w descriptor
+ * @link_tbl: input and output h/w link tables (if {src,dst}_nents > 1) (SEC2)
+ * @buf: input and output buffeur (if {src,dst}_nents > 1) (SEC1)
+ *
+ * if decrypting (with authcheck), or either one of src_nents or dst_nents
+ * is greater than 1, an integrity check value is concatenated to the end
+ * of link_tbl data
+ */
+struct talitos_edesc {
+	int src_nents;
+	int dst_nents;
+	bool icv_ool;
+	dma_addr_t iv_dma;
+	int dma_len;
+	dma_addr_t dma_link_tbl;
+	struct talitos_desc desc;
+	union {
+		struct talitos_ptr link_tbl[0];
+		u8 buf[0];
+	};
+};
+
 /**
  * talitos_request - descriptor submission request
  * @desc: descriptor pointer (kernel virtual)
-- 
2.13.3

Re: [PATCH v2 1/4] crypto: talitos - move struct talitos_edesc into talitos.h

[Horia Geanta]

On 6/11/2019 5:39 PM, Christophe Leroy wrote:
> Next patch will require struct talitos_edesc to be defined
> earlier in talitos.c
> 
> This patch moves it into talitos.h so that it can be used
> from any place in talitos.c
> 
> Fixes: 37b5e8897eb5 ("crypto: talitos - chain in buffered data for ahash on SEC1")
> Cc: stable@vger.kernel.org
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
Again, this patch does not qualify as a fix.

Horia

Re: [PATCH v2 1/4] crypto: talitos - move struct talitos_edesc into talitos.h

[Horia Geanta]

On 6/11/2019 5:39 PM, Christophe Leroy wrote:
> Next patch will require struct talitos_edesc to be defined
> earlier in talitos.c
> 
> This patch moves it into talitos.h so that it can be used
> from any place in talitos.c
> 
> Fixes: 37b5e8897eb5 ("crypto: talitos - chain in buffered data for ahash on SEC1")
> Cc: stable@vger.kernel.org
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
Again, this patch does not qualify as a fix.

Horia

Re: [PATCH v2 1/4] crypto: talitos - move struct talitos_edesc into talitos.h

[Christophe Leroy]

Le 13/06/2019 à 14:13, Horia Geanta a écrit :
> On 6/11/2019 5:39 PM, Christophe Leroy wrote:
>> Next patch will require struct talitos_edesc to be defined
>> earlier in talitos.c
>>
>> This patch moves it into talitos.h so that it can be used
>> from any place in talitos.c
>>
>> Fixes: 37b5e8897eb5 ("crypto: talitos - chain in buffered data for ahash on SEC1")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> Again, this patch does not qualify as a fix.
> 

But as I said, the following one is a fix and require that one, you told 
me to add stable in Cc: to make it explicit it was to go into stable.
If someone tries to merge following one into stable with taking that one 
first, build will fail.

Christophe

Re: [PATCH v2 1/4] crypto: talitos - move struct talitos_edesc into talitos.h

[Christophe Leroy]

Le 13/06/2019 à 14:13, Horia Geanta a écrit :
> On 6/11/2019 5:39 PM, Christophe Leroy wrote:
>> Next patch will require struct talitos_edesc to be defined
>> earlier in talitos.c
>>
>> This patch moves it into talitos.h so that it can be used
>> from any place in talitos.c
>>
>> Fixes: 37b5e8897eb5 ("crypto: talitos - chain in buffered data for ahash on SEC1")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> Again, this patch does not qualify as a fix.
> 

But as I said, the following one is a fix and require that one, you told 
me to add stable in Cc: to make it explicit it was to go into stable.
If someone tries to merge following one into stable with taking that one 
first, build will fail.

Christophe

Re: [PATCH v2 1/4] crypto: talitos - move struct talitos_edesc into talitos.h

[Horia Geanta]

On 6/13/2019 3:16 PM, Christophe Leroy wrote:
> 
> 
> Le 13/06/2019 à 14:13, Horia Geanta a écrit :
>> On 6/11/2019 5:39 PM, Christophe Leroy wrote:
>>> Next patch will require struct talitos_edesc to be defined
>>> earlier in talitos.c
>>>
>>> This patch moves it into talitos.h so that it can be used
>>> from any place in talitos.c
>>>
>>> Fixes: 37b5e8897eb5 ("crypto: talitos - chain in buffered data for ahash on SEC1")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> Again, this patch does not qualify as a fix.
>>
> 
> But as I said, the following one is a fix and require that one, you told 
> me to add stable in Cc: to make it explicit it was to go into stable.
Yes, but you should remove the Fixes tag.
And probably replace "Next patch" with the commit headline.

> If someone tries to merge following one into stable with taking that one 
> first, build will fail.
This shouldn't happen, order from main tree should be preserved.

Horia

Re: [PATCH v2 1/4] crypto: talitos - move struct talitos_edesc into talitos.h

[Horia Geanta]

On 6/13/2019 3:16 PM, Christophe Leroy wrote:
> 
> 
> Le 13/06/2019 à 14:13, Horia Geanta a écrit :
>> On 6/11/2019 5:39 PM, Christophe Leroy wrote:
>>> Next patch will require struct talitos_edesc to be defined
>>> earlier in talitos.c
>>>
>>> This patch moves it into talitos.h so that it can be used
>>> from any place in talitos.c
>>>
>>> Fixes: 37b5e8897eb5 ("crypto: talitos - chain in buffered data for ahash on SEC1")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> Again, this patch does not qualify as a fix.
>>
> 
> But as I said, the following one is a fix and require that one, you told 
> me to add stable in Cc: to make it explicit it was to go into stable.
Yes, but you should remove the Fixes tag.
And probably replace "Next patch" with the commit headline.

> If someone tries to merge following one into stable with taking that one 
> first, build will fail.
This shouldn't happen, order from main tree should be preserved.

Horia

Re: [PATCH v2 1/4] crypto: talitos - move struct talitos_edesc into talitos.h

[Christophe Leroy]

Le 13/06/2019 à 14:24, Horia Geanta a écrit :
> On 6/13/2019 3:16 PM, Christophe Leroy wrote:
>>
>>
>> Le 13/06/2019 à 14:13, Horia Geanta a écrit :
>>> On 6/11/2019 5:39 PM, Christophe Leroy wrote:
>>>> Next patch will require struct talitos_edesc to be defined
>>>> earlier in talitos.c
>>>>
>>>> This patch moves it into talitos.h so that it can be used
>>>> from any place in talitos.c
>>>>
>>>> Fixes: 37b5e8897eb5 ("crypto: talitos - chain in buffered data for ahash on SEC1")
>>>> Cc: stable@vger.kernel.org
>>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>>> Again, this patch does not qualify as a fix.
>>>
>>
>> But as I said, the following one is a fix and require that one, you told
>> me to add stable in Cc: to make it explicit it was to go into stable.
> Yes, but you should remove the Fixes tag.
> And probably replace "Next patch" with the commit headline.
> 
>> If someone tries to merge following one into stable with taking that one
>> first, build will fail.
> This shouldn't happen, order from main tree should be preserved.
> 

When they pick up fixes, AFAIK they don't take all the preceeding commits.

But ok, I'll resend.

Christophe

Re: [PATCH v2 1/4] crypto: talitos - move struct talitos_edesc into talitos.h

[Christophe Leroy]

Le 13/06/2019 à 14:24, Horia Geanta a écrit :
> On 6/13/2019 3:16 PM, Christophe Leroy wrote:
>>
>>
>> Le 13/06/2019 à 14:13, Horia Geanta a écrit :
>>> On 6/11/2019 5:39 PM, Christophe Leroy wrote:
>>>> Next patch will require struct talitos_edesc to be defined
>>>> earlier in talitos.c
>>>>
>>>> This patch moves it into talitos.h so that it can be used
>>>> from any place in talitos.c
>>>>
>>>> Fixes: 37b5e8897eb5 ("crypto: talitos - chain in buffered data for ahash on SEC1")
>>>> Cc: stable@vger.kernel.org
>>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>>> Again, this patch does not qualify as a fix.
>>>
>>
>> But as I said, the following one is a fix and require that one, you told
>> me to add stable in Cc: to make it explicit it was to go into stable.
> Yes, but you should remove the Fixes tag.
> And probably replace "Next patch" with the commit headline.
> 
>> If someone tries to merge following one into stable with taking that one
>> first, build will fail.
> This shouldn't happen, order from main tree should be preserved.
> 

When they pick up fixes, AFAIK they don't take all the preceeding commits.

But ok, I'll resend.

Christophe

Re: [PATCH v2 1/4] crypto: talitos - move struct talitos_edesc into talitos.h

[Horia Geanta]

On 6/13/2019 3:32 PM, Christophe Leroy wrote:
> 
> 
> Le 13/06/2019 à 14:24, Horia Geanta a écrit :
>> On 6/13/2019 3:16 PM, Christophe Leroy wrote:
>>>
>>>
>>> Le 13/06/2019 à 14:13, Horia Geanta a écrit :
>>>> On 6/11/2019 5:39 PM, Christophe Leroy wrote:
>>>>> Next patch will require struct talitos_edesc to be defined
>>>>> earlier in talitos.c
>>>>>
>>>>> This patch moves it into talitos.h so that it can be used
>>>>> from any place in talitos.c
>>>>>
>>>>> Fixes: 37b5e8897eb5 ("crypto: talitos - chain in buffered data for ahash on SEC1")
>>>>> Cc: stable@vger.kernel.org
>>>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>>>> Again, this patch does not qualify as a fix.
>>>>
>>>
>>> But as I said, the following one is a fix and require that one, you told
>>> me to add stable in Cc: to make it explicit it was to go into stable.
>> Yes, but you should remove the Fixes tag.
>> And probably replace "Next patch" with the commit headline.
>>
>>> If someone tries to merge following one into stable with taking that one
>>> first, build will fail.
>> This shouldn't happen, order from main tree should be preserved.
>>
> 
> When they pick up fixes, AFAIK they don't take all the preceeding commits.
> 
This is not about Fixes tag, but Cc tag:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html#option-1

Horia

Re: [PATCH v2 1/4] crypto: talitos - move struct talitos_edesc into talitos.h

[Horia Geanta]

On 6/13/2019 3:32 PM, Christophe Leroy wrote:
> 
> 
> Le 13/06/2019 à 14:24, Horia Geanta a écrit :
>> On 6/13/2019 3:16 PM, Christophe Leroy wrote:
>>>
>>>
>>> Le 13/06/2019 à 14:13, Horia Geanta a écrit :
>>>> On 6/11/2019 5:39 PM, Christophe Leroy wrote:
>>>>> Next patch will require struct talitos_edesc to be defined
>>>>> earlier in talitos.c
>>>>>
>>>>> This patch moves it into talitos.h so that it can be used
>>>>> from any place in talitos.c
>>>>>
>>>>> Fixes: 37b5e8897eb5 ("crypto: talitos - chain in buffered data for ahash on SEC1")
>>>>> Cc: stable@vger.kernel.org
>>>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>>>> Again, this patch does not qualify as a fix.
>>>>
>>>
>>> But as I said, the following one is a fix and require that one, you told
>>> me to add stable in Cc: to make it explicit it was to go into stable.
>> Yes, but you should remove the Fixes tag.
>> And probably replace "Next patch" with the commit headline.
>>
>>> If someone tries to merge following one into stable with taking that one
>>> first, build will fail.
>> This shouldn't happen, order from main tree should be preserved.
>>
> 
> When they pick up fixes, AFAIK they don't take all the preceeding commits.
> 
This is not about Fixes tag, but Cc tag:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html#option-1

Horia

Re: [PATCH v2 1/4] crypto: talitos - move struct talitos_edesc into talitos.h

[Christophe Leroy]

Le 13/06/2019 à 14:39, Horia Geanta a écrit :
> On 6/13/2019 3:32 PM, Christophe Leroy wrote:
>>
>>
>> Le 13/06/2019 à 14:24, Horia Geanta a écrit :
>>> On 6/13/2019 3:16 PM, Christophe Leroy wrote:
>>>>
>>>>
>>>> Le 13/06/2019 à 14:13, Horia Geanta a écrit :
>>>>> On 6/11/2019 5:39 PM, Christophe Leroy wrote:
>>>>>> Next patch will require struct talitos_edesc to be defined
>>>>>> earlier in talitos.c
>>>>>>
>>>>>> This patch moves it into talitos.h so that it can be used
>>>>>> from any place in talitos.c
>>>>>>
>>>>>> Fixes: 37b5e8897eb5 ("crypto: talitos - chain in buffered data for ahash on SEC1")
>>>>>> Cc: stable@vger.kernel.org
>>>>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>>>>> Again, this patch does not qualify as a fix.
>>>>>
>>>>
>>>> But as I said, the following one is a fix and require that one, you told
>>>> me to add stable in Cc: to make it explicit it was to go into stable.
>>> Yes, but you should remove the Fixes tag.
>>> And probably replace "Next patch" with the commit headline.
>>>
>>>> If someone tries to merge following one into stable with taking that one
>>>> first, build will fail.
>>> This shouldn't happen, order from main tree should be preserved.
>>>
>>
>> When they pick up fixes, AFAIK they don't take all the preceeding commits.
>>
> This is not about Fixes tag, but Cc tag:
> https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html#option-1
> 

Ah, ok. So I need to keep the Cc tag. I misunderstood sorry.

Re: [PATCH v2 1/4] crypto: talitos - move struct talitos_edesc into talitos.h

[Christophe Leroy]

Le 13/06/2019 à 14:39, Horia Geanta a écrit :
> On 6/13/2019 3:32 PM, Christophe Leroy wrote:
>>
>>
>> Le 13/06/2019 à 14:24, Horia Geanta a écrit :
>>> On 6/13/2019 3:16 PM, Christophe Leroy wrote:
>>>>
>>>>
>>>> Le 13/06/2019 à 14:13, Horia Geanta a écrit :
>>>>> On 6/11/2019 5:39 PM, Christophe Leroy wrote:
>>>>>> Next patch will require struct talitos_edesc to be defined
>>>>>> earlier in talitos.c
>>>>>>
>>>>>> This patch moves it into talitos.h so that it can be used
>>>>>> from any place in talitos.c
>>>>>>
>>>>>> Fixes: 37b5e8897eb5 ("crypto: talitos - chain in buffered data for ahash on SEC1")
>>>>>> Cc: stable@vger.kernel.org
>>>>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>>>>> Again, this patch does not qualify as a fix.
>>>>>
>>>>
>>>> But as I said, the following one is a fix and require that one, you told
>>>> me to add stable in Cc: to make it explicit it was to go into stable.
>>> Yes, but you should remove the Fixes tag.
>>> And probably replace "Next patch" with the commit headline.
>>>
>>>> If someone tries to merge following one into stable with taking that one
>>>> first, build will fail.
>>> This shouldn't happen, order from main tree should be preserved.
>>>
>>
>> When they pick up fixes, AFAIK they don't take all the preceeding commits.
>>
> This is not about Fixes tag, but Cc tag:
> https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html#option-1
> 

Ah, ok. So I need to keep the Cc tag. I misunderstood sorry.

[PATCH v2 2/4] crypto: talitos - fix hash on SEC1.

[Christophe Leroy]

On SEC1, hash provides wrong result when performing hashing in several
steps with input data SG list has more than one element. This was
detected with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS:

[   44.185947] alg: hash: md5-talitos test failed (wrong result) on test vector 6, cfg="random: may_sleep use_finup src_divs=[<reimport>25.88%@+8063, <flush>24.19%@+9588, 28.63%@+16333, <reimport>4.60%@+6756, 16.70%@+16281] dst_divs=[71.61%@alignmask+16361, 14.36%@+7756, 14.3%@+"
[   44.325122] alg: hash: sha1-talitos test failed (wrong result) on test vector 3, cfg="random: inplace use_final src_divs=[<flush,nosimd>16.56%@+16378, <reimport>52.0%@+16329, 21.42%@alignmask+16380, 10.2%@alignmask+16380] iv_offset=39"
[   44.493500] alg: hash: sha224-talitos test failed (wrong result) on test vector 4, cfg="random: use_final nosimd src_divs=[<reimport>52.27%@+7401, <reimport>17.34%@+16285, <flush>17.71%@+26, 12.68%@+10644] iv_offset=43"
[   44.673262] alg: hash: sha256-talitos test failed (wrong result) on test vector 4, cfg="random: may_sleep use_finup src_divs=[<reimport>60.6%@+12790, 17.86%@+1329, <reimport>12.64%@alignmask+16300, 8.29%@+15, 0.40%@+13506, <reimport>0.51%@+16322, <reimport>0.24%@+16339] dst_divs"

This is due to two issues:
- We have an overlap between the buffer used for copying the input
data (SEC1 doesn't do scatter/gather) and the chained descriptor.
- Data copy is wrong when the previous hash left less than one
blocksize of data to hash, implying a complement of the previous
block with a few bytes from the new request.

This patch fixes it by:
- Moving the second descriptor after the buffer, as moving the buffer
after the descriptor would make it more complex for other cipher
operations (AEAD, ABLKCIPHER)
- Rebuiding a new data SG list without the bytes taken from the new
request to complete the previous one.

Fixes: 37b5e8897eb5 ("crypto: talitos - chain in buffered data for ahash on SEC1")
Cc: stable@vger.kernel.org
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 drivers/crypto/talitos.c | 63 ++++++++++++++++++++++++++++++------------------
 1 file changed, 40 insertions(+), 23 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 5b401aec6c84..4f03baef952b 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -336,15 +336,18 @@ static void flush_channel(struct device *dev, int ch, int error, int reset_ch)
 	tail = priv->chan[ch].tail;
 	while (priv->chan[ch].fifo[tail].desc) {
 		__be32 hdr;
+		struct talitos_edesc *edesc;
 
 		request = &priv->chan[ch].fifo[tail];
+		edesc = container_of(request->desc, struct talitos_edesc, desc);
 
 		/* descriptors with their done bits set don't get the error */
 		rmb();
 		if (!is_sec1)
 			hdr = request->desc->hdr;
 		else if (request->desc->next_desc)
-			hdr = (request->desc + 1)->hdr1;
+			hdr = ((struct talitos_desc *)
+			       (edesc->buf + edesc->dma_len))->hdr1;
 		else
 			hdr = request->desc->hdr1;
 
@@ -476,8 +479,14 @@ static u32 current_desc_hdr(struct device *dev, int ch)
 		}
 	}
 
-	if (priv->chan[ch].fifo[iter].desc->next_desc == cur_desc)
-		return (priv->chan[ch].fifo[iter].desc + 1)->hdr;
+	if (priv->chan[ch].fifo[iter].desc->next_desc == cur_desc) {
+		struct talitos_edesc *edesc;
+
+		edesc = container_of(priv->chan[ch].fifo[iter].desc,
+				     struct talitos_edesc, desc);
+		return ((struct talitos_desc *)
+			(edesc->buf + edesc->dma_len))->hdr;
+	}
 
 	return priv->chan[ch].fifo[iter].desc->hdr;
 }
@@ -1402,15 +1411,11 @@ static struct talitos_edesc *talitos_edesc_alloc(struct device *dev,
 	edesc->dst_nents = dst_nents;
 	edesc->iv_dma = iv_dma;
 	edesc->dma_len = dma_len;
-	if (dma_len) {
-		void *addr = &edesc->link_tbl[0];
-
-		if (is_sec1 && !dst)
-			addr += sizeof(struct talitos_desc);
-		edesc->dma_link_tbl = dma_map_single(dev, addr,
+	if (dma_len)
+		edesc->dma_link_tbl = dma_map_single(dev, &edesc->link_tbl[0],
 						     edesc->dma_len,
 						     DMA_BIDIRECTIONAL);
-	}
+
 	return edesc;
 }
 
@@ -1722,14 +1727,16 @@ static void common_nonsnoop_hash_unmap(struct device *dev,
 	struct talitos_private *priv = dev_get_drvdata(dev);
 	bool is_sec1 = has_ftr_sec1(priv);
 	struct talitos_desc *desc = &edesc->desc;
-	struct talitos_desc *desc2 = desc + 1;
+	struct talitos_desc *desc2 = (struct talitos_desc *)
+				     (edesc->buf + edesc->dma_len);
 
 	unmap_single_talitos_ptr(dev, &edesc->desc.ptr[5], DMA_FROM_DEVICE);
 	if (desc->next_desc &&
 	    desc->ptr[5].ptr != desc2->ptr[5].ptr)
 		unmap_single_talitos_ptr(dev, &desc2->ptr[5], DMA_FROM_DEVICE);
 
-	talitos_sg_unmap(dev, edesc, req_ctx->psrc, NULL, 0, 0);
+	if (req_ctx->psrc)
+		talitos_sg_unmap(dev, edesc, req_ctx->psrc, NULL, 0, 0);
 
 	/* When using hashctx-in, must unmap it. */
 	if (from_talitos_ptr_len(&edesc->desc.ptr[1], is_sec1))
@@ -1796,7 +1803,6 @@ static void talitos_handle_buggy_hash(struct talitos_ctx *ctx,
 
 static int common_nonsnoop_hash(struct talitos_edesc *edesc,
 				struct ahash_request *areq, unsigned int length,
-				unsigned int offset,
 				void (*callback) (struct device *dev,
 						  struct talitos_desc *desc,
 						  void *context, int error))
@@ -1835,9 +1841,7 @@ static int common_nonsnoop_hash(struct talitos_edesc *edesc,
 
 	sg_count = edesc->src_nents ?: 1;
 	if (is_sec1 && sg_count > 1)
-		sg_pcopy_to_buffer(req_ctx->psrc, sg_count,
-				   edesc->buf + sizeof(struct talitos_desc),
-				   length, req_ctx->nbuf);
+		sg_copy_to_buffer(req_ctx->psrc, sg_count, edesc->buf, length);
 	else if (length)
 		sg_count = dma_map_sg(dev, req_ctx->psrc, sg_count,
 				      DMA_TO_DEVICE);
@@ -1850,7 +1854,7 @@ static int common_nonsnoop_hash(struct talitos_edesc *edesc,
 				       DMA_TO_DEVICE);
 	} else {
 		sg_count = talitos_sg_map(dev, req_ctx->psrc, length, edesc,
-					  &desc->ptr[3], sg_count, offset, 0);
+					  &desc->ptr[3], sg_count, 0, 0);
 		if (sg_count > 1)
 			sync_needed = true;
 	}
@@ -1874,7 +1878,8 @@ static int common_nonsnoop_hash(struct talitos_edesc *edesc,
 		talitos_handle_buggy_hash(ctx, edesc, &desc->ptr[3]);
 
 	if (is_sec1 && req_ctx->nbuf && length) {
-		struct talitos_desc *desc2 = desc + 1;
+		struct talitos_desc *desc2 = (struct talitos_desc *)
+					     (edesc->buf + edesc->dma_len);
 		dma_addr_t next_desc;
 
 		memset(desc2, 0, sizeof(*desc2));
@@ -1895,7 +1900,7 @@ static int common_nonsnoop_hash(struct talitos_edesc *edesc,
 						      DMA_TO_DEVICE);
 		copy_talitos_ptr(&desc2->ptr[2], &desc->ptr[2], is_sec1);
 		sg_count = talitos_sg_map(dev, req_ctx->psrc, length, edesc,
-					  &desc2->ptr[3], sg_count, offset, 0);
+					  &desc2->ptr[3], sg_count, 0, 0);
 		if (sg_count > 1)
 			sync_needed = true;
 		copy_talitos_ptr(&desc2->ptr[5], &desc->ptr[5], is_sec1);
@@ -2006,7 +2011,6 @@ static int ahash_process_req(struct ahash_request *areq, unsigned int nbytes)
 	struct device *dev = ctx->dev;
 	struct talitos_private *priv = dev_get_drvdata(dev);
 	bool is_sec1 = has_ftr_sec1(priv);
-	int offset = 0;
 	u8 *ctx_buf = req_ctx->buf[req_ctx->buf_idx];
 
 	if (!req_ctx->last && (nbytes + req_ctx->nbuf <= blocksize)) {
@@ -2046,6 +2050,9 @@ static int ahash_process_req(struct ahash_request *areq, unsigned int nbytes)
 			sg_chain(req_ctx->bufsl, 2, areq->src);
 		req_ctx->psrc = req_ctx->bufsl;
 	} else if (is_sec1 && req_ctx->nbuf && req_ctx->nbuf < blocksize) {
+		int offset;
+		struct scatterlist *sg;
+
 		if (nbytes_to_hash > blocksize)
 			offset = blocksize - req_ctx->nbuf;
 		else
@@ -2058,7 +2065,18 @@ static int ahash_process_req(struct ahash_request *areq, unsigned int nbytes)
 		sg_copy_to_buffer(areq->src, nents,
 				  ctx_buf + req_ctx->nbuf, offset);
 		req_ctx->nbuf += offset;
-		req_ctx->psrc = areq->src;
+		for (sg = areq->src; sg && offset >= sg->length;
+		     offset -= sg->length, sg = sg_next(sg))
+			;
+		if (offset) {
+			sg_init_table(req_ctx->bufsl, 2);
+			sg_set_buf(req_ctx->bufsl, sg_virt(sg) + offset,
+				   sg->length - offset);
+			sg_chain(req_ctx->bufsl, 2, sg_next(sg));
+			req_ctx->psrc = req_ctx->bufsl;
+		} else {
+			req_ctx->psrc = sg;
+		}
 	} else
 		req_ctx->psrc = areq->src;
 
@@ -2098,8 +2116,7 @@ static int ahash_process_req(struct ahash_request *areq, unsigned int nbytes)
 	if (ctx->keylen && (req_ctx->first || req_ctx->last))
 		edesc->desc.hdr |= DESC_HDR_MODE0_MDEU_HMAC;
 
-	return common_nonsnoop_hash(edesc, areq, nbytes_to_hash, offset,
-				    ahash_done);
+	return common_nonsnoop_hash(edesc, areq, nbytes_to_hash, ahash_done);
 }
 
 static int ahash_update(struct ahash_request *areq)
-- 
2.13.3

[PATCH v2 2/4] crypto: talitos - fix hash on SEC1.

[Christophe Leroy]

On SEC1, hash provides wrong result when performing hashing in several
steps with input data SG list has more than one element. This was
detected with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS:

[   44.185947] alg: hash: md5-talitos test failed (wrong result) on test vector 6, cfg="random: may_sleep use_finup src_divs=[<reimport>25.88%@+8063, <flush>24.19%@+9588, 28.63%@+16333, <reimport>4.60%@+6756, 16.70%@+16281] dst_divs=[71.61%@alignmask+16361, 14.36%@+7756, 14.3%@+"
[   44.325122] alg: hash: sha1-talitos test failed (wrong result) on test vector 3, cfg="random: inplace use_final src_divs=[<flush,nosimd>16.56%@+16378, <reimport>52.0%@+16329, 21.42%@alignmask+16380, 10.2%@alignmask+16380] iv_offset=39"
[   44.493500] alg: hash: sha224-talitos test failed (wrong result) on test vector 4, cfg="random: use_final nosimd src_divs=[<reimport>52.27%@+7401, <reimport>17.34%@+16285, <flush>17.71%@+26, 12.68%@+10644] iv_offset=43"
[   44.673262] alg: hash: sha256-talitos test failed (wrong result) on test vector 4, cfg="random: may_sleep use_finup src_divs=[<reimport>60.6%@+12790, 17.86%@+1329, <reimport>12.64%@alignmask+16300, 8.29%@+15, 0.40%@+13506, <reimport>0.51%@+16322, <reimport>0.24%@+16339] dst_divs"

This is due to two issues:
- We have an overlap between the buffer used for copying the input
data (SEC1 doesn't do scatter/gather) and the chained descriptor.
- Data copy is wrong when the previous hash left less than one
blocksize of data to hash, implying a complement of the previous
block with a few bytes from the new request.

This patch fixes it by:
- Moving the second descriptor after the buffer, as moving the buffer
after the descriptor would make it more complex for other cipher
operations (AEAD, ABLKCIPHER)
- Rebuiding a new data SG list without the bytes taken from the new
request to complete the previous one.

Fixes: 37b5e8897eb5 ("crypto: talitos - chain in buffered data for ahash on SEC1")
Cc: stable@vger.kernel.org
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 drivers/crypto/talitos.c | 63 ++++++++++++++++++++++++++++++------------------
 1 file changed, 40 insertions(+), 23 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 5b401aec6c84..4f03baef952b 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -336,15 +336,18 @@ static void flush_channel(struct device *dev, int ch, int error, int reset_ch)
 	tail = priv->chan[ch].tail;
 	while (priv->chan[ch].fifo[tail].desc) {
 		__be32 hdr;
+		struct talitos_edesc *edesc;
 
 		request = &priv->chan[ch].fifo[tail];
+		edesc = container_of(request->desc, struct talitos_edesc, desc);
 
 		/* descriptors with their done bits set don't get the error */
 		rmb();
 		if (!is_sec1)
 			hdr = request->desc->hdr;
 		else if (request->desc->next_desc)
-			hdr = (request->desc + 1)->hdr1;
+			hdr = ((struct talitos_desc *)
+			       (edesc->buf + edesc->dma_len))->hdr1;
 		else
 			hdr = request->desc->hdr1;
 
@@ -476,8 +479,14 @@ static u32 current_desc_hdr(struct device *dev, int ch)
 		}
 	}
 
-	if (priv->chan[ch].fifo[iter].desc->next_desc == cur_desc)
-		return (priv->chan[ch].fifo[iter].desc + 1)->hdr;
+	if (priv->chan[ch].fifo[iter].desc->next_desc == cur_desc) {
+		struct talitos_edesc *edesc;
+
+		edesc = container_of(priv->chan[ch].fifo[iter].desc,
+				     struct talitos_edesc, desc);
+		return ((struct talitos_desc *)
+			(edesc->buf + edesc->dma_len))->hdr;
+	}
 
 	return priv->chan[ch].fifo[iter].desc->hdr;
 }
@@ -1402,15 +1411,11 @@ static struct talitos_edesc *talitos_edesc_alloc(struct device *dev,
 	edesc->dst_nents = dst_nents;
 	edesc->iv_dma = iv_dma;
 	edesc->dma_len = dma_len;
-	if (dma_len) {
-		void *addr = &edesc->link_tbl[0];
-
-		if (is_sec1 && !dst)
-			addr += sizeof(struct talitos_desc);
-		edesc->dma_link_tbl = dma_map_single(dev, addr,
+	if (dma_len)
+		edesc->dma_link_tbl = dma_map_single(dev, &edesc->link_tbl[0],
 						     edesc->dma_len,
 						     DMA_BIDIRECTIONAL);
-	}
+
 	return edesc;
 }
 
@@ -1722,14 +1727,16 @@ static void common_nonsnoop_hash_unmap(struct device *dev,
 	struct talitos_private *priv = dev_get_drvdata(dev);
 	bool is_sec1 = has_ftr_sec1(priv);
 	struct talitos_desc *desc = &edesc->desc;
-	struct talitos_desc *desc2 = desc + 1;
+	struct talitos_desc *desc2 = (struct talitos_desc *)
+				     (edesc->buf + edesc->dma_len);
 
 	unmap_single_talitos_ptr(dev, &edesc->desc.ptr[5], DMA_FROM_DEVICE);
 	if (desc->next_desc &&
 	    desc->ptr[5].ptr != desc2->ptr[5].ptr)
 		unmap_single_talitos_ptr(dev, &desc2->ptr[5], DMA_FROM_DEVICE);
 
-	talitos_sg_unmap(dev, edesc, req_ctx->psrc, NULL, 0, 0);
+	if (req_ctx->psrc)
+		talitos_sg_unmap(dev, edesc, req_ctx->psrc, NULL, 0, 0);
 
 	/* When using hashctx-in, must unmap it. */
 	if (from_talitos_ptr_len(&edesc->desc.ptr[1], is_sec1))
@@ -1796,7 +1803,6 @@ static void talitos_handle_buggy_hash(struct talitos_ctx *ctx,
 
 static int common_nonsnoop_hash(struct talitos_edesc *edesc,
 				struct ahash_request *areq, unsigned int length,
-				unsigned int offset,
 				void (*callback) (struct device *dev,
 						  struct talitos_desc *desc,
 						  void *context, int error))
@@ -1835,9 +1841,7 @@ static int common_nonsnoop_hash(struct talitos_edesc *edesc,
 
 	sg_count = edesc->src_nents ?: 1;
 	if (is_sec1 && sg_count > 1)
-		sg_pcopy_to_buffer(req_ctx->psrc, sg_count,
-				   edesc->buf + sizeof(struct talitos_desc),
-				   length, req_ctx->nbuf);
+		sg_copy_to_buffer(req_ctx->psrc, sg_count, edesc->buf, length);
 	else if (length)
 		sg_count = dma_map_sg(dev, req_ctx->psrc, sg_count,
 				      DMA_TO_DEVICE);
@@ -1850,7 +1854,7 @@ static int common_nonsnoop_hash(struct talitos_edesc *edesc,
 				       DMA_TO_DEVICE);
 	} else {
 		sg_count = talitos_sg_map(dev, req_ctx->psrc, length, edesc,
-					  &desc->ptr[3], sg_count, offset, 0);
+					  &desc->ptr[3], sg_count, 0, 0);
 		if (sg_count > 1)
 			sync_needed = true;
 	}
@@ -1874,7 +1878,8 @@ static int common_nonsnoop_hash(struct talitos_edesc *edesc,
 		talitos_handle_buggy_hash(ctx, edesc, &desc->ptr[3]);
 
 	if (is_sec1 && req_ctx->nbuf && length) {
-		struct talitos_desc *desc2 = desc + 1;
+		struct talitos_desc *desc2 = (struct talitos_desc *)
+					     (edesc->buf + edesc->dma_len);
 		dma_addr_t next_desc;
 
 		memset(desc2, 0, sizeof(*desc2));
@@ -1895,7 +1900,7 @@ static int common_nonsnoop_hash(struct talitos_edesc *edesc,
 						      DMA_TO_DEVICE);
 		copy_talitos_ptr(&desc2->ptr[2], &desc->ptr[2], is_sec1);
 		sg_count = talitos_sg_map(dev, req_ctx->psrc, length, edesc,
-					  &desc2->ptr[3], sg_count, offset, 0);
+					  &desc2->ptr[3], sg_count, 0, 0);
 		if (sg_count > 1)
 			sync_needed = true;
 		copy_talitos_ptr(&desc2->ptr[5], &desc->ptr[5], is_sec1);
@@ -2006,7 +2011,6 @@ static int ahash_process_req(struct ahash_request *areq, unsigned int nbytes)
 	struct device *dev = ctx->dev;
 	struct talitos_private *priv = dev_get_drvdata(dev);
 	bool is_sec1 = has_ftr_sec1(priv);
-	int offset = 0;
 	u8 *ctx_buf = req_ctx->buf[req_ctx->buf_idx];
 
 	if (!req_ctx->last && (nbytes + req_ctx->nbuf <= blocksize)) {
@@ -2046,6 +2050,9 @@ static int ahash_process_req(struct ahash_request *areq, unsigned int nbytes)
 			sg_chain(req_ctx->bufsl, 2, areq->src);
 		req_ctx->psrc = req_ctx->bufsl;
 	} else if (is_sec1 && req_ctx->nbuf && req_ctx->nbuf < blocksize) {
+		int offset;
+		struct scatterlist *sg;
+
 		if (nbytes_to_hash > blocksize)
 			offset = blocksize - req_ctx->nbuf;
 		else
@@ -2058,7 +2065,18 @@ static int ahash_process_req(struct ahash_request *areq, unsigned int nbytes)
 		sg_copy_to_buffer(areq->src, nents,
 				  ctx_buf + req_ctx->nbuf, offset);
 		req_ctx->nbuf += offset;
-		req_ctx->psrc = areq->src;
+		for (sg = areq->src; sg && offset >= sg->length;
+		     offset -= sg->length, sg = sg_next(sg))
+			;
+		if (offset) {
+			sg_init_table(req_ctx->bufsl, 2);
+			sg_set_buf(req_ctx->bufsl, sg_virt(sg) + offset,
+				   sg->length - offset);
+			sg_chain(req_ctx->bufsl, 2, sg_next(sg));
+			req_ctx->psrc = req_ctx->bufsl;
+		} else {
+			req_ctx->psrc = sg;
+		}
 	} else
 		req_ctx->psrc = areq->src;
 
@@ -2098,8 +2116,7 @@ static int ahash_process_req(struct ahash_request *areq, unsigned int nbytes)
 	if (ctx->keylen && (req_ctx->first || req_ctx->last))
 		edesc->desc.hdr |= DESC_HDR_MODE0_MDEU_HMAC;
 
-	return common_nonsnoop_hash(edesc, areq, nbytes_to_hash, offset,
-				    ahash_done);
+	return common_nonsnoop_hash(edesc, areq, nbytes_to_hash, ahash_done);
 }
 
 static int ahash_update(struct ahash_request *areq)
-- 
2.13.3

[PATCH v2 3/4] crypto: talitos - eliminate unneeded 'done' functions at build time

[Christophe Leroy]

When building for SEC1 only, talitos2_done functions are unneeded
and should go away.

For this, use has_ftr_sec1() which will always return true when only
SEC1 support is being built, allowing GCC to drop TALITOS2 functions.

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 drivers/crypto/talitos.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 4f03baef952b..b2de931de623 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -3401,7 +3401,7 @@ static int talitos_probe(struct platform_device *ofdev)
 	if (err)
 		goto err_out;
 
-	if (of_device_is_compatible(np, "fsl,sec1.0")) {
+	if (has_ftr_sec1(priv)) {
 		if (priv->num_channels == 1)
 			tasklet_init(&priv->done_task[0], talitos1_done_ch0,
 				     (unsigned long)dev);
-- 
2.13.3

[PATCH v2 3/4] crypto: talitos - eliminate unneeded 'done' functions at build time

[Christophe Leroy]

When building for SEC1 only, talitos2_done functions are unneeded
and should go away.

For this, use has_ftr_sec1() which will always return true when only
SEC1 support is being built, allowing GCC to drop TALITOS2 functions.

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 drivers/crypto/talitos.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 4f03baef952b..b2de931de623 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -3401,7 +3401,7 @@ static int talitos_probe(struct platform_device *ofdev)
 	if (err)
 		goto err_out;
 
-	if (of_device_is_compatible(np, "fsl,sec1.0")) {
+	if (has_ftr_sec1(priv)) {
 		if (priv->num_channels == 1)
 			tasklet_init(&priv->done_task[0], talitos1_done_ch0,
 				     (unsigned long)dev);
-- 
2.13.3

Re: [PATCH v2 3/4] crypto: talitos - eliminate unneeded 'done' functions at build time

[Horia Geanta]

On 6/11/2019 5:39 PM, Christophe Leroy wrote:
> When building for SEC1 only, talitos2_done functions are unneeded
> and should go away.
> 
> For this, use has_ftr_sec1() which will always return true when only
> SEC1 support is being built, allowing GCC to drop TALITOS2 functions.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
Reviewed-by: Horia Geantă <horia.geanta@nxp.com>

Thanks,
Horia

Re: [PATCH v2 3/4] crypto: talitos - eliminate unneeded 'done' functions at build time

[Horia Geanta]

On 6/11/2019 5:39 PM, Christophe Leroy wrote:
> When building for SEC1 only, talitos2_done functions are unneeded
> and should go away.
> 
> For this, use has_ftr_sec1() which will always return true when only
> SEC1 support is being built, allowing GCC to drop TALITOS2 functions.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
Reviewed-by: Horia Geantă <horia.geanta@nxp.com>

Thanks,
Horia

[PATCH v2 4/4] crypto: talitos - drop icv_ool

[Christophe Leroy]

icv_ool is not used anymore, drop it.

Fixes: 9cc87bc3613b ("crypto: talitos - fix AEAD processing")
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 drivers/crypto/talitos.c | 3 ---
 drivers/crypto/talitos.h | 2 --
 2 files changed, 5 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index b2de931de623..03b7a5d28fb0 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -1278,9 +1278,6 @@ static int ipsec_esp(struct talitos_edesc *edesc, struct aead_request *areq,
 				 is_ipsec_esp && !encrypt);
 	tbl_off += ret;
 
-	/* ICV data */
-	edesc->icv_ool = !encrypt;
-
 	if (!encrypt && is_ipsec_esp) {
 		struct talitos_ptr *tbl_ptr = &edesc->link_tbl[tbl_off];
 
diff --git a/drivers/crypto/talitos.h b/drivers/crypto/talitos.h
index 95f78c6d9206..1469b956948a 100644
--- a/drivers/crypto/talitos.h
+++ b/drivers/crypto/talitos.h
@@ -46,7 +46,6 @@ struct talitos_desc {
  * talitos_edesc - s/w-extended descriptor
  * @src_nents: number of segments in input scatterlist
  * @dst_nents: number of segments in output scatterlist
- * @icv_ool: whether ICV is out-of-line
  * @iv_dma: dma address of iv for checking continuity and link table
  * @dma_len: length of dma mapped link_tbl space
  * @dma_link_tbl: bus physical address of link_tbl/buf
@@ -61,7 +60,6 @@ struct talitos_desc {
 struct talitos_edesc {
 	int src_nents;
 	int dst_nents;
-	bool icv_ool;
 	dma_addr_t iv_dma;
 	int dma_len;
 	dma_addr_t dma_link_tbl;
-- 
2.13.3

[PATCH v2 4/4] crypto: talitos - drop icv_ool

[Christophe Leroy]

icv_ool is not used anymore, drop it.

Fixes: 9cc87bc3613b ("crypto: talitos - fix AEAD processing")
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
 drivers/crypto/talitos.c | 3 ---
 drivers/crypto/talitos.h | 2 --
 2 files changed, 5 deletions(-)

diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index b2de931de623..03b7a5d28fb0 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -1278,9 +1278,6 @@ static int ipsec_esp(struct talitos_edesc *edesc, struct aead_request *areq,
 				 is_ipsec_esp && !encrypt);
 	tbl_off += ret;
 
-	/* ICV data */
-	edesc->icv_ool = !encrypt;
-
 	if (!encrypt && is_ipsec_esp) {
 		struct talitos_ptr *tbl_ptr = &edesc->link_tbl[tbl_off];
 
diff --git a/drivers/crypto/talitos.h b/drivers/crypto/talitos.h
index 95f78c6d9206..1469b956948a 100644
--- a/drivers/crypto/talitos.h
+++ b/drivers/crypto/talitos.h
@@ -46,7 +46,6 @@ struct talitos_desc {
  * talitos_edesc - s/w-extended descriptor
  * @src_nents: number of segments in input scatterlist
  * @dst_nents: number of segments in output scatterlist
- * @icv_ool: whether ICV is out-of-line
  * @iv_dma: dma address of iv for checking continuity and link table
  * @dma_len: length of dma mapped link_tbl space
  * @dma_link_tbl: bus physical address of link_tbl/buf
@@ -61,7 +60,6 @@ struct talitos_desc {
 struct talitos_edesc {
 	int src_nents;
 	int dst_nents;
-	bool icv_ool;
 	dma_addr_t iv_dma;
 	int dma_len;
 	dma_addr_t dma_link_tbl;
-- 
2.13.3

Re: [PATCH v2 4/4] crypto: talitos - drop icv_ool

[Horia Geanta]

On 6/11/2019 5:39 PM, Christophe Leroy wrote:
> icv_ool is not used anymore, drop it.
> 
> Fixes: 9cc87bc3613b ("crypto: talitos - fix AEAD processing")
I can't find this SHA1.

Are you referring to commit e345177ded17 ("crypto: talitos - fix AEAD processing.")?

Horia

Re: [PATCH v2 4/4] crypto: talitos - drop icv_ool

[Horia Geanta]

On 6/11/2019 5:39 PM, Christophe Leroy wrote:
> icv_ool is not used anymore, drop it.
> 
> Fixes: 9cc87bc3613b ("crypto: talitos - fix AEAD processing")
I can't find this SHA1.

Are you referring to commit e345177ded17 ("crypto: talitos - fix AEAD processing.")?

Horia

Re: [PATCH v2 4/4] crypto: talitos - drop icv_ool

[Christophe Leroy]

Le 13/06/2019 à 14:21, Horia Geanta a écrit :
> On 6/11/2019 5:39 PM, Christophe Leroy wrote:
>> icv_ool is not used anymore, drop it.
>>
>> Fixes: 9cc87bc3613b ("crypto: talitos - fix AEAD processing")
> I can't find this SHA1.
> 
> Are you referring to commit e345177ded17 ("crypto: talitos - fix AEAD processing.")?
> 
> Horia
> 

Oops yes, that's the sha1 it had in my tree before it got merged.

Do I have to resend it or can Herbert just drop the wrong reference and 
take the following one:

Fixes: e345177ded17 ("crypto: talitos - fix AEAD processing.")


Christophe

Re: [PATCH v2 4/4] crypto: talitos - drop icv_ool

[Christophe Leroy]

Le 13/06/2019 à 14:21, Horia Geanta a écrit :
> On 6/11/2019 5:39 PM, Christophe Leroy wrote:
>> icv_ool is not used anymore, drop it.
>>
>> Fixes: 9cc87bc3613b ("crypto: talitos - fix AEAD processing")
> I can't find this SHA1.
> 
> Are you referring to commit e345177ded17 ("crypto: talitos - fix AEAD processing.")?
> 
> Horia
> 

Oops yes, that's the sha1 it had in my tree before it got merged.

Do I have to resend it or can Herbert just drop the wrong reference and 
take the following one:

Fixes: e345177ded17 ("crypto: talitos - fix AEAD processing.")


Christophe

Re: [PATCH v2 4/4] crypto: talitos - drop icv_ool

[Herbert Xu]

On Thu, Jun 13, 2019 at 02:28:51PM +0200, Christophe Leroy wrote:
> 
> 
> Le 13/06/2019 à 14:21, Horia Geanta a écrit :
> > On 6/11/2019 5:39 PM, Christophe Leroy wrote:
> > > icv_ool is not used anymore, drop it.
> > > 
> > > Fixes: 9cc87bc3613b ("crypto: talitos - fix AEAD processing")
> > I can't find this SHA1.
> > 
> > Are you referring to commit e345177ded17 ("crypto: talitos - fix AEAD processing.")?
> > 
> > Horia
> > 
> 
> Oops yes, that's the sha1 it had in my tree before it got merged.
> 
> Do I have to resend it or can Herbert just drop the wrong reference and take
> the following one:

Please resend since you're going to change the other patches too.

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 4/4] crypto: talitos - drop icv_ool

[Herbert Xu]

On Thu, Jun 13, 2019 at 02:28:51PM +0200, Christophe Leroy wrote:
> 
> 
> Le 13/06/2019 à 14:21, Horia Geanta a écrit :
> > On 6/11/2019 5:39 PM, Christophe Leroy wrote:
> > > icv_ool is not used anymore, drop it.
> > > 
> > > Fixes: 9cc87bc3613b ("crypto: talitos - fix AEAD processing")
> > I can't find this SHA1.
> > 
> > Are you referring to commit e345177ded17 ("crypto: talitos - fix AEAD processing.")?
> > 
> > Horia
> > 
> 
> Oops yes, that's the sha1 it had in my tree before it got merged.
> 
> Do I have to resend it or can Herbert just drop the wrong reference and take
> the following one:

Please resend since you're going to change the other patches too.

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 0/4] Additional fixes on Talitos driver

[Horia Geanta]

On 6/11/2019 5:39 PM, Christophe Leroy wrote:
> This series is the last set of fixes for the Talitos driver.
> 
> We now get a fully clean boot on both SEC1 (SEC1.2 on mpc885) and
> SEC2 (SEC2.2 on mpc8321E) with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS:
> 
I am getting below failures on a sec 3.3.2 (p1020rdb) for hmac(sha384) and
hmac(sha512):

alg: ahash: hmac-sha384-talitos test failed (wrong result) on test vector "random: psize=2497 ksize=124", cfg="random: inplace use_finup nosimd src_divs=[<reimport>76.49%@+4002, <reimport>23.51%@alignmask+26] iv_offset=4"
alg: ahash: hmac-sha512-talitos test failed (wrong result) on test vector "random: psize=27 ksize=121", cfg="random: inplace may_sleep use_digest src_divs=[100.0%@+10] iv_offset=9"

Reproducibility rate is 100% so far, here are a few more runs - they might help finding a pattern:

1.
alg: ahash: hmac-sha384-talitos test failed (wrong result) on test vector "random: psize=184 ksize=121", cfg="random: use_finup src_divs=[<reimport,nosimd>100.0%@+3988] dst_divs=[100.0%@+547] iv_offset=44"
alg: ahash: hmac-sha512-talitos test failed (wrong result) on test vector "random: psize=7 ksize=122", cfg="random: may_sleep use_digest src_divs=[100.0%@+3968] dst_divs=[100.0%@+20]"

2.
alg: ahash: hmac-sha384-talitos test failed (wrong result) on test vector "random: psize=6481 ksize=120", cfg="random: use_final src_divs=[<reimport>100.0%@+6] dst_divs=[43.84%@alignmask+6, 56.16%@+22]"
alg: ahash: hmac-sha512-talitos test failed (wrong result) on test vector "random: psize=635 ksize=128", cfg="random: may_sleep use_finup src_divs=[100.0%@+4062] dst_divs=[20.47%@+2509, 72.36%@alignmask+2, 7.17%@alignmask+3990]"

3.
alg: ahash: hmac-sha384-talitos test failed (wrong result) on test vector "random: psize=2428 ksize=127", cfg="random: may_sleep use_finup src_divs=[<reimport>35.19%@+18, 64.81%@+1755] dst_divs=[100.0%@+111] iv_offset=5"
alg: ahash: hmac-sha512-talitos test failed (wrong result) on test vector "random: psize=4345 ksize=128", cfg="random: may_sleep use_digest src_divs=[100.0%@+2820] iv_offset=59"

If you run several times with fuzz testing enabled on your sec2.2,
are you able to see similar failures?

Thanks,
Horia

Re: [PATCH v2 0/4] Additional fixes on Talitos driver

[Horia Geanta]

On 6/11/2019 5:39 PM, Christophe Leroy wrote:
> This series is the last set of fixes for the Talitos driver.
> 
> We now get a fully clean boot on both SEC1 (SEC1.2 on mpc885) and
> SEC2 (SEC2.2 on mpc8321E) with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS:
> 
I am getting below failures on a sec 3.3.2 (p1020rdb) for hmac(sha384) and
hmac(sha512):

alg: ahash: hmac-sha384-talitos test failed (wrong result) on test vector "random: psize=2497 ksize=124", cfg="random: inplace use_finup nosimd src_divs=[<reimport>76.49%@+4002, <reimport>23.51%@alignmask+26] iv_offset=4"
alg: ahash: hmac-sha512-talitos test failed (wrong result) on test vector "random: psize=27 ksize=121", cfg="random: inplace may_sleep use_digest src_divs=[100.0%@+10] iv_offset=9"

Reproducibility rate is 100% so far, here are a few more runs - they might help finding a pattern:

1.
alg: ahash: hmac-sha384-talitos test failed (wrong result) on test vector "random: psize=184 ksize=121", cfg="random: use_finup src_divs=[<reimport,nosimd>100.0%@+3988] dst_divs=[100.0%@+547] iv_offset=44"
alg: ahash: hmac-sha512-talitos test failed (wrong result) on test vector "random: psize=7 ksize=122", cfg="random: may_sleep use_digest src_divs=[100.0%@+3968] dst_divs=[100.0%@+20]"

2.
alg: ahash: hmac-sha384-talitos test failed (wrong result) on test vector "random: psize=6481 ksize=120", cfg="random: use_final src_divs=[<reimport>100.0%@+6] dst_divs=[43.84%@alignmask+6, 56.16%@+22]"
alg: ahash: hmac-sha512-talitos test failed (wrong result) on test vector "random: psize=635 ksize=128", cfg="random: may_sleep use_finup src_divs=[100.0%@+4062] dst_divs=[20.47%@+2509, 72.36%@alignmask+2, 7.17%@alignmask+3990]"

3.
alg: ahash: hmac-sha384-talitos test failed (wrong result) on test vector "random: psize=2428 ksize=127", cfg="random: may_sleep use_finup src_divs=[<reimport>35.19%@+18, 64.81%@+1755] dst_divs=[100.0%@+111] iv_offset=5"
alg: ahash: hmac-sha512-talitos test failed (wrong result) on test vector "random: psize=4345 ksize=128", cfg="random: may_sleep use_digest src_divs=[100.0%@+2820] iv_offset=59"

If you run several times with fuzz testing enabled on your sec2.2,
are you able to see similar failures?

Thanks,
Horia

Re: [PATCH v2 0/4] Additional fixes on Talitos driver

[Christophe Leroy]

Le 11/06/2019 à 17:37, Horia Geanta a écrit :
> On 6/11/2019 5:39 PM, Christophe Leroy wrote:
>> This series is the last set of fixes for the Talitos driver.
>>
>> We now get a fully clean boot on both SEC1 (SEC1.2 on mpc885) and
>> SEC2 (SEC2.2 on mpc8321E) with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS:
>>
> I am getting below failures on a sec 3.3.2 (p1020rdb) for hmac(sha384) and
> hmac(sha512):

Is that new with this series or did you already have it before ?

What do you mean by "fuzz testing" enabled ? Is that 
CONFIG_CRYPTO_MANAGER_EXTRA_TESTS or something else ?

Christophe

> 
> alg: ahash: hmac-sha384-talitos test failed (wrong result) on test vector "random: psize=2497 ksize=124", cfg="random: inplace use_finup nosimd src_divs=[<reimport>76.49%@+4002, <reimport>23.51%@alignmask+26] iv_offset=4"
> alg: ahash: hmac-sha512-talitos test failed (wrong result) on test vector "random: psize=27 ksize=121", cfg="random: inplace may_sleep use_digest src_divs=[100.0%@+10] iv_offset=9"
> 
> Reproducibility rate is 100% so far, here are a few more runs - they might help finding a pattern:
> 
> 1.
> alg: ahash: hmac-sha384-talitos test failed (wrong result) on test vector "random: psize=184 ksize=121", cfg="random: use_finup src_divs=[<reimport,nosimd>100.0%@+3988] dst_divs=[100.0%@+547] iv_offset=44"
> alg: ahash: hmac-sha512-talitos test failed (wrong result) on test vector "random: psize=7 ksize=122", cfg="random: may_sleep use_digest src_divs=[100.0%@+3968] dst_divs=[100.0%@+20]"
> 
> 2.
> alg: ahash: hmac-sha384-talitos test failed (wrong result) on test vector "random: psize=6481 ksize=120", cfg="random: use_final src_divs=[<reimport>100.0%@+6] dst_divs=[43.84%@alignmask+6, 56.16%@+22]"
> alg: ahash: hmac-sha512-talitos test failed (wrong result) on test vector "random: psize=635 ksize=128", cfg="random: may_sleep use_finup src_divs=[100.0%@+4062] dst_divs=[20.47%@+2509, 72.36%@alignmask+2, 7.17%@alignmask+3990]"
> 
> 3.
> alg: ahash: hmac-sha384-talitos test failed (wrong result) on test vector "random: psize=2428 ksize=127", cfg="random: may_sleep use_finup src_divs=[<reimport>35.19%@+18, 64.81%@+1755] dst_divs=[100.0%@+111] iv_offset=5"
> alg: ahash: hmac-sha512-talitos test failed (wrong result) on test vector "random: psize=4345 ksize=128", cfg="random: may_sleep use_digest src_divs=[100.0%@+2820] iv_offset=59"
> 
> If you run several times with fuzz testing enabled on your sec2.2,
> are you able to see similar failures?
> 
> Thanks,
> Horia
> 

Re: [PATCH v2 0/4] Additional fixes on Talitos driver

[Christophe Leroy]

Le 11/06/2019 à 17:37, Horia Geanta a écrit :
> On 6/11/2019 5:39 PM, Christophe Leroy wrote:
>> This series is the last set of fixes for the Talitos driver.
>>
>> We now get a fully clean boot on both SEC1 (SEC1.2 on mpc885) and
>> SEC2 (SEC2.2 on mpc8321E) with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS:
>>
> I am getting below failures on a sec 3.3.2 (p1020rdb) for hmac(sha384) and
> hmac(sha512):

Is that new with this series or did you already have it before ?

What do you mean by "fuzz testing" enabled ? Is that 
CONFIG_CRYPTO_MANAGER_EXTRA_TESTS or something else ?

Christophe

> 
> alg: ahash: hmac-sha384-talitos test failed (wrong result) on test vector "random: psize=2497 ksize=124", cfg="random: inplace use_finup nosimd src_divs=[<reimport>76.49%@+4002, <reimport>23.51%@alignmask+26] iv_offset=4"
> alg: ahash: hmac-sha512-talitos test failed (wrong result) on test vector "random: psize=27 ksize=121", cfg="random: inplace may_sleep use_digest src_divs=[100.0%@+10] iv_offset=9"
> 
> Reproducibility rate is 100% so far, here are a few more runs - they might help finding a pattern:
> 
> 1.
> alg: ahash: hmac-sha384-talitos test failed (wrong result) on test vector "random: psize=184 ksize=121", cfg="random: use_finup src_divs=[<reimport,nosimd>100.0%@+3988] dst_divs=[100.0%@+547] iv_offset=44"
> alg: ahash: hmac-sha512-talitos test failed (wrong result) on test vector "random: psize=7 ksize=122", cfg="random: may_sleep use_digest src_divs=[100.0%@+3968] dst_divs=[100.0%@+20]"
> 
> 2.
> alg: ahash: hmac-sha384-talitos test failed (wrong result) on test vector "random: psize=6481 ksize=120", cfg="random: use_final src_divs=[<reimport>100.0%@+6] dst_divs=[43.84%@alignmask+6, 56.16%@+22]"
> alg: ahash: hmac-sha512-talitos test failed (wrong result) on test vector "random: psize=635 ksize=128", cfg="random: may_sleep use_finup src_divs=[100.0%@+4062] dst_divs=[20.47%@+2509, 72.36%@alignmask+2, 7.17%@alignmask+3990]"
> 
> 3.
> alg: ahash: hmac-sha384-talitos test failed (wrong result) on test vector "random: psize=2428 ksize=127", cfg="random: may_sleep use_finup src_divs=[<reimport>35.19%@+18, 64.81%@+1755] dst_divs=[100.0%@+111] iv_offset=5"
> alg: ahash: hmac-sha512-talitos test failed (wrong result) on test vector "random: psize=4345 ksize=128", cfg="random: may_sleep use_digest src_divs=[100.0%@+2820] iv_offset=59"
> 
> If you run several times with fuzz testing enabled on your sec2.2,
> are you able to see similar failures?
> 
> Thanks,
> Horia
> 

Re: [PATCH v2 0/4] Additional fixes on Talitos driver

[Horia Geanta]

On 6/11/2019 6:40 PM, Christophe Leroy wrote:
> 
> 
> Le 11/06/2019 à 17:37, Horia Geanta a écrit :
>> On 6/11/2019 5:39 PM, Christophe Leroy wrote:
>>> This series is the last set of fixes for the Talitos driver.
>>>
>>> We now get a fully clean boot on both SEC1 (SEC1.2 on mpc885) and
>>> SEC2 (SEC2.2 on mpc8321E) with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS:
>>>
>> I am getting below failures on a sec 3.3.2 (p1020rdb) for hmac(sha384) and
>> hmac(sha512):
> 
> Is that new with this series or did you already have it before ?
> 
Looks like this happens with or without this series.

I haven't checked the state of this driver for quite some time.
Since I've noticed increased activity, I thought it would be worth
actually testing the changes.

Are changes in patch 2/4 ("crypto: talitos - fix hash on SEC1.")
strictly for sec 1.x or they affect all revisions?

> What do you mean by "fuzz testing" enabled ? Is that 
> CONFIG_CRYPTO_MANAGER_EXTRA_TESTS or something else ?
> 
Yes, it's this config symbol.

Horia

Re: [PATCH v2 0/4] Additional fixes on Talitos driver

[Horia Geanta]

On 6/11/2019 6:40 PM, Christophe Leroy wrote:
> 
> 
> Le 11/06/2019 à 17:37, Horia Geanta a écrit :
>> On 6/11/2019 5:39 PM, Christophe Leroy wrote:
>>> This series is the last set of fixes for the Talitos driver.
>>>
>>> We now get a fully clean boot on both SEC1 (SEC1.2 on mpc885) and
>>> SEC2 (SEC2.2 on mpc8321E) with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS:
>>>
>> I am getting below failures on a sec 3.3.2 (p1020rdb) for hmac(sha384) and
>> hmac(sha512):
> 
> Is that new with this series or did you already have it before ?
> 
Looks like this happens with or without this series.

I haven't checked the state of this driver for quite some time.
Since I've noticed increased activity, I thought it would be worth
actually testing the changes.

Are changes in patch 2/4 ("crypto: talitos - fix hash on SEC1.")
strictly for sec 1.x or they affect all revisions?

> What do you mean by "fuzz testing" enabled ? Is that 
> CONFIG_CRYPTO_MANAGER_EXTRA_TESTS or something else ?
> 
Yes, it's this config symbol.

Horia

Re: [PATCH v2 0/4] Additional fixes on Talitos driver

[Christophe Leroy]

Le 11/06/2019 à 18:30, Horia Geanta a écrit :
> On 6/11/2019 6:40 PM, Christophe Leroy wrote:
>>
>>
>> Le 11/06/2019 à 17:37, Horia Geanta a écrit :
>>> On 6/11/2019 5:39 PM, Christophe Leroy wrote:
>>>> This series is the last set of fixes for the Talitos driver.
>>>>
>>>> We now get a fully clean boot on both SEC1 (SEC1.2 on mpc885) and
>>>> SEC2 (SEC2.2 on mpc8321E) with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS:
>>>>
>>> I am getting below failures on a sec 3.3.2 (p1020rdb) for hmac(sha384) and
>>> hmac(sha512):
>>
>> Is that new with this series or did you already have it before ?
>>
> Looks like this happens with or without this series.
> 
> I haven't checked the state of this driver for quite some time.
> Since I've noticed increased activity, I thought it would be worth
> actually testing the changes.
> 
> Are changes in patch 2/4 ("crypto: talitos - fix hash on SEC1.")
> strictly for sec 1.x or they affect all revisions?

They are strictly for sec 1.x

> 
>> What do you mean by "fuzz testing" enabled ? Is that
>> CONFIG_CRYPTO_MANAGER_EXTRA_TESTS or something else ?
>>
> Yes, it's this config symbol.

Indeed SEC 2.2 only supports up to SHA-256.

Christophe

> 
> Horia
> 

Re: [PATCH v2 0/4] Additional fixes on Talitos driver

[Christophe Leroy]

Le 11/06/2019 à 18:30, Horia Geanta a écrit :
> On 6/11/2019 6:40 PM, Christophe Leroy wrote:
>>
>>
>> Le 11/06/2019 à 17:37, Horia Geanta a écrit :
>>> On 6/11/2019 5:39 PM, Christophe Leroy wrote:
>>>> This series is the last set of fixes for the Talitos driver.
>>>>
>>>> We now get a fully clean boot on both SEC1 (SEC1.2 on mpc885) and
>>>> SEC2 (SEC2.2 on mpc8321E) with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS:
>>>>
>>> I am getting below failures on a sec 3.3.2 (p1020rdb) for hmac(sha384) and
>>> hmac(sha512):
>>
>> Is that new with this series or did you already have it before ?
>>
> Looks like this happens with or without this series.
> 
> I haven't checked the state of this driver for quite some time.
> Since I've noticed increased activity, I thought it would be worth
> actually testing the changes.
> 
> Are changes in patch 2/4 ("crypto: talitos - fix hash on SEC1.")
> strictly for sec 1.x or they affect all revisions?

They are strictly for sec 1.x

> 
>> What do you mean by "fuzz testing" enabled ? Is that
>> CONFIG_CRYPTO_MANAGER_EXTRA_TESTS or something else ?
>>
> Yes, it's this config symbol.

Indeed SEC 2.2 only supports up to SHA-256.

Christophe

> 
> Horia
> 

Re: [PATCH v2 0/4] Additional fixes on Talitos driver

[Christophe Leroy]

Le 11/06/2019 à 18:30, Horia Geanta a écrit :
> On 6/11/2019 6:40 PM, Christophe Leroy wrote:
>>
>>
>> Le 11/06/2019 à 17:37, Horia Geanta a écrit :
>>> On 6/11/2019 5:39 PM, Christophe Leroy wrote:
>>>> This series is the last set of fixes for the Talitos driver.
>>>>
>>>> We now get a fully clean boot on both SEC1 (SEC1.2 on mpc885) and
>>>> SEC2 (SEC2.2 on mpc8321E) with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS:
>>>>
>>> I am getting below failures on a sec 3.3.2 (p1020rdb) for hmac(sha384) and
>>> hmac(sha512):
>>
>> Is that new with this series or did you already have it before ?
>>
> Looks like this happens with or without this series.

Found the issue, that's in 
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=b8fbdc2bc4e71b62646031d5df5f08aafe15d5ad

CONFIG_CRYPTO_DEV_TALITOS_SEC2 should be CONFIG_CRYPTO_DEV_TALITOS2 instead.

Just sent a patch to fix it.

Thanks
Christophe

> 
> I haven't checked the state of this driver for quite some time.
> Since I've noticed increased activity, I thought it would be worth
> actually testing the changes.
> 
> Are changes in patch 2/4 ("crypto: talitos - fix hash on SEC1.")
> strictly for sec 1.x or they affect all revisions?
> 
>> What do you mean by "fuzz testing" enabled ? Is that
>> CONFIG_CRYPTO_MANAGER_EXTRA_TESTS or something else ?
>>
> Yes, it's this config symbol.
> 
> Horia
> 

Re: [PATCH v2 0/4] Additional fixes on Talitos driver

[Christophe Leroy]

Le 11/06/2019 à 18:30, Horia Geanta a écrit :
> On 6/11/2019 6:40 PM, Christophe Leroy wrote:
>>
>>
>> Le 11/06/2019 à 17:37, Horia Geanta a écrit :
>>> On 6/11/2019 5:39 PM, Christophe Leroy wrote:
>>>> This series is the last set of fixes for the Talitos driver.
>>>>
>>>> We now get a fully clean boot on both SEC1 (SEC1.2 on mpc885) and
>>>> SEC2 (SEC2.2 on mpc8321E) with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS:
>>>>
>>> I am getting below failures on a sec 3.3.2 (p1020rdb) for hmac(sha384) and
>>> hmac(sha512):
>>
>> Is that new with this series or did you already have it before ?
>>
> Looks like this happens with or without this series.

Found the issue, that's in 
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=b8fbdc2bc4e71b62646031d5df5f08aafe15d5ad

CONFIG_CRYPTO_DEV_TALITOS_SEC2 should be CONFIG_CRYPTO_DEV_TALITOS2 instead.

Just sent a patch to fix it.

Thanks
Christophe

> 
> I haven't checked the state of this driver for quite some time.
> Since I've noticed increased activity, I thought it would be worth
> actually testing the changes.
> 
> Are changes in patch 2/4 ("crypto: talitos - fix hash on SEC1.")
> strictly for sec 1.x or they affect all revisions?
> 
>> What do you mean by "fuzz testing" enabled ? Is that
>> CONFIG_CRYPTO_MANAGER_EXTRA_TESTS or something else ?
>>
> Yes, it's this config symbol.
> 
> Horia
> 

Re: [PATCH v2 0/4] Additional fixes on Talitos driver

[Horia Geanta]

On 6/12/2019 8:52 AM, Christophe Leroy wrote:
> 
> 
> Le 11/06/2019 à 18:30, Horia Geanta a écrit :
>> On 6/11/2019 6:40 PM, Christophe Leroy wrote:
>>>
>>>
>>> Le 11/06/2019 à 17:37, Horia Geanta a écrit :
>>>> On 6/11/2019 5:39 PM, Christophe Leroy wrote:
>>>>> This series is the last set of fixes for the Talitos driver.
>>>>>
>>>>> We now get a fully clean boot on both SEC1 (SEC1.2 on mpc885) and
>>>>> SEC2 (SEC2.2 on mpc8321E) with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS:
>>>>>
>>>> I am getting below failures on a sec 3.3.2 (p1020rdb) for hmac(sha384) and
>>>> hmac(sha512):
>>>
>>> Is that new with this series or did you already have it before ?
>>>
>> Looks like this happens with or without this series.
> 
> Found the issue, that's in 
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=b8fbdc2bc4e71b62646031d5df5f08aafe15d5ad
> 
> CONFIG_CRYPTO_DEV_TALITOS_SEC2 should be CONFIG_CRYPTO_DEV_TALITOS2 instead.
> 
> Just sent a patch to fix it.
> 
Thanks, I've tested it and the hmac failures go away.

However, testing gets stuck.
Seems there is another issue lurking in the driver.

Used cryptodev-2.6/master with the following on top:
crypto: testmgr - add some more preemption points
	https://patchwork.kernel.org/patch/10972337/
crypto: talitos - fix max key size for sha384 and sha512
	https://patchwork.kernel.org/patch/10988473/

[...]
alg: skcipher: skipping comparison tests for ecb-3des-talitos because ecb(des3_ede-generic) is unavailable
INFO: task cryptomgr_test:314 blocked for more than 120 seconds.
      Not tainted 5.2.0-rc1-g905bfd415e8a #1
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
cryptomgr_test  D    0   314      2 0x00000800
Call Trace:
[e78337e0] [00000004] 0x4 (unreliable)
[e78338a8] [c08a6e5c] __schedule+0x20c/0x4d4
[e78338f8] [c08a7158] schedule+0x34/0xc8
[e7833908] [c08aa5ec] schedule_timeout+0x1d4/0x350
[e7833958] [c08a7be4] wait_for_common+0xa0/0x164
[e7833998] [c03a7b14] do_ahash_op+0xa4/0xc4
[e78339b8] [c03aba00] test_ahash_vec_cfg+0x188/0x5e4
[e7833aa8] [c03ac1c8] test_hash_vs_generic_impl+0x1b0/0x2b4
[e7833de8] [c03ac498] __alg_test_hash+0x1cc/0x2d0
[e7833e28] [c03a9fb4] alg_test.part.37+0x8c/0x3ac
[e7833ef8] [c03a54d0] cryptomgr_test+0x4c/0x54
[e7833f08] [c006c410] kthread+0xf8/0x124
[e7833f38] [c001227c] ret_from_kernel_thread+0x14/0x1c

addr2line on c03aba00 points to crypto/testmgr.c:1335

   1327)  if (cfg->finalization_type == FINALIZATION_TYPE_DIGEST ||
   1328)      vec->digest_error) {
   1329)          /* Just using digest() */
   1330)          ahash_request_set_callback(req, req_flags, crypto_req_done,
   1331)                                     &wait);
   1332)          ahash_request_set_crypt(req, tsgl->sgl, result, vec->psize);
   1333)          err = do_ahash_op(crypto_ahash_digest, req, &wait, cfg->nosimd);
   1334)          if (err) {
-> 1335)                  if (err == vec->digest_error)
   1336)                          return 0;
   1337)                  pr_err("alg: ahash: %s digest() failed on test vector %s; expected_error=%d, actual_error=%d, cfg=\"%s\"\n",
   1338)                         driver, vec_name, vec->digest_error, err,
   1339)                         cfg->name);
   1340)                  return err;
   1341)          }
   1342)          if (vec->digest_error) {
   1343)                  pr_err("alg: ahash: %s digest() unexpectedly succeeded on test vector %s; expected_error=%d, cfg=\"%s\"\n",
   1344)                         driver, vec_name, vec->digest_error, cfg->name);
   1345)                  return -EINVAL;
   1346)          }
   1347)          goto result_ready;
   1348)  }

Seems that for some reason driver does not receive the interrupt from HW,
thus completion callback does not run.

Tried with or without current patch series, no change in behaviour.

If you cannot reproduce and don't have any idea, I'll try the hard way
(git bisect).

Thanks,
Horia

Re: [PATCH v2 0/4] Additional fixes on Talitos driver

[Horia Geanta]

On 6/12/2019 8:52 AM, Christophe Leroy wrote:
> 
> 
> Le 11/06/2019 à 18:30, Horia Geanta a écrit :
>> On 6/11/2019 6:40 PM, Christophe Leroy wrote:
>>>
>>>
>>> Le 11/06/2019 à 17:37, Horia Geanta a écrit :
>>>> On 6/11/2019 5:39 PM, Christophe Leroy wrote:
>>>>> This series is the last set of fixes for the Talitos driver.
>>>>>
>>>>> We now get a fully clean boot on both SEC1 (SEC1.2 on mpc885) and
>>>>> SEC2 (SEC2.2 on mpc8321E) with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS:
>>>>>
>>>> I am getting below failures on a sec 3.3.2 (p1020rdb) for hmac(sha384) and
>>>> hmac(sha512):
>>>
>>> Is that new with this series or did you already have it before ?
>>>
>> Looks like this happens with or without this series.
> 
> Found the issue, that's in 
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=b8fbdc2bc4e71b62646031d5df5f08aafe15d5ad
> 
> CONFIG_CRYPTO_DEV_TALITOS_SEC2 should be CONFIG_CRYPTO_DEV_TALITOS2 instead.
> 
> Just sent a patch to fix it.
> 
Thanks, I've tested it and the hmac failures go away.

However, testing gets stuck.
Seems there is another issue lurking in the driver.

Used cryptodev-2.6/master with the following on top:
crypto: testmgr - add some more preemption points
	https://patchwork.kernel.org/patch/10972337/
crypto: talitos - fix max key size for sha384 and sha512
	https://patchwork.kernel.org/patch/10988473/

[...]
alg: skcipher: skipping comparison tests for ecb-3des-talitos because ecb(des3_ede-generic) is unavailable
INFO: task cryptomgr_test:314 blocked for more than 120 seconds.
      Not tainted 5.2.0-rc1-g905bfd415e8a #1
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
cryptomgr_test  D    0   314      2 0x00000800
Call Trace:
[e78337e0] [00000004] 0x4 (unreliable)
[e78338a8] [c08a6e5c] __schedule+0x20c/0x4d4
[e78338f8] [c08a7158] schedule+0x34/0xc8
[e7833908] [c08aa5ec] schedule_timeout+0x1d4/0x350
[e7833958] [c08a7be4] wait_for_common+0xa0/0x164
[e7833998] [c03a7b14] do_ahash_op+0xa4/0xc4
[e78339b8] [c03aba00] test_ahash_vec_cfg+0x188/0x5e4
[e7833aa8] [c03ac1c8] test_hash_vs_generic_impl+0x1b0/0x2b4
[e7833de8] [c03ac498] __alg_test_hash+0x1cc/0x2d0
[e7833e28] [c03a9fb4] alg_test.part.37+0x8c/0x3ac
[e7833ef8] [c03a54d0] cryptomgr_test+0x4c/0x54
[e7833f08] [c006c410] kthread+0xf8/0x124
[e7833f38] [c001227c] ret_from_kernel_thread+0x14/0x1c

addr2line on c03aba00 points to crypto/testmgr.c:1335

   1327)  if (cfg->finalization_type == FINALIZATION_TYPE_DIGEST ||
   1328)      vec->digest_error) {
   1329)          /* Just using digest() */
   1330)          ahash_request_set_callback(req, req_flags, crypto_req_done,
   1331)                                     &wait);
   1332)          ahash_request_set_crypt(req, tsgl->sgl, result, vec->psize);
   1333)          err = do_ahash_op(crypto_ahash_digest, req, &wait, cfg->nosimd);
   1334)          if (err) {
-> 1335)                  if (err == vec->digest_error)
   1336)                          return 0;
   1337)                  pr_err("alg: ahash: %s digest() failed on test vector %s; expected_error=%d, actual_error=%d, cfg=\"%s\"\n",
   1338)                         driver, vec_name, vec->digest_error, err,
   1339)                         cfg->name);
   1340)                  return err;
   1341)          }
   1342)          if (vec->digest_error) {
   1343)                  pr_err("alg: ahash: %s digest() unexpectedly succeeded on test vector %s; expected_error=%d, cfg=\"%s\"\n",
   1344)                         driver, vec_name, vec->digest_error, cfg->name);
   1345)                  return -EINVAL;
   1346)          }
   1347)          goto result_ready;
   1348)  }

Seems that for some reason driver does not receive the interrupt from HW,
thus completion callback does not run.

Tried with or without current patch series, no change in behaviour.

If you cannot reproduce and don't have any idea, I'll try the hard way
(git bisect).

Thanks,
Horia

Re: [PATCH v2 0/4] Additional fixes on Talitos driver

[Christophe Leroy]

Le 12/06/2019 à 15:59, Horia Geanta a écrit :
> On 6/12/2019 8:52 AM, Christophe Leroy wrote:
>>
>>
>> Le 11/06/2019 à 18:30, Horia Geanta a écrit :
>>> On 6/11/2019 6:40 PM, Christophe Leroy wrote:
>>>>
>>>>
>>>> Le 11/06/2019 à 17:37, Horia Geanta a écrit :
>>>>> On 6/11/2019 5:39 PM, Christophe Leroy wrote:
>>>>>> This series is the last set of fixes for the Talitos driver.
>>>>>>
>>>>>> We now get a fully clean boot on both SEC1 (SEC1.2 on mpc885) and
>>>>>> SEC2 (SEC2.2 on mpc8321E) with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS:
>>>>>>
>>>>> I am getting below failures on a sec 3.3.2 (p1020rdb) for hmac(sha384) and
>>>>> hmac(sha512):
>>>>
>>>> Is that new with this series or did you already have it before ?
>>>>
>>> Looks like this happens with or without this series.
>>
>> Found the issue, that's in
>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=b8fbdc2bc4e71b62646031d5df5f08aafe15d5ad
>>
>> CONFIG_CRYPTO_DEV_TALITOS_SEC2 should be CONFIG_CRYPTO_DEV_TALITOS2 instead.
>>
>> Just sent a patch to fix it.
>>
> Thanks, I've tested it and the hmac failures go away.
> 
> However, testing gets stuck.
> Seems there is another issue lurking in the driver.
> 
> Used cryptodev-2.6/master with the following on top:
> crypto: testmgr - add some more preemption points
> 	https://patchwork.kernel.org/patch/10972337/
> crypto: talitos - fix max key size for sha384 and sha512
> 	https://patchwork.kernel.org/patch/10988473/
> 
> [...]
> alg: skcipher: skipping comparison tests for ecb-3des-talitos because ecb(des3_ede-generic) is unavailable
> INFO: task cryptomgr_test:314 blocked for more than 120 seconds.
>        Not tainted 5.2.0-rc1-g905bfd415e8a #1
> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> cryptomgr_test  D    0   314      2 0x00000800
> Call Trace:
> [e78337e0] [00000004] 0x4 (unreliable)
> [e78338a8] [c08a6e5c] __schedule+0x20c/0x4d4
> [e78338f8] [c08a7158] schedule+0x34/0xc8
> [e7833908] [c08aa5ec] schedule_timeout+0x1d4/0x350
> [e7833958] [c08a7be4] wait_for_common+0xa0/0x164
> [e7833998] [c03a7b14] do_ahash_op+0xa4/0xc4
> [e78339b8] [c03aba00] test_ahash_vec_cfg+0x188/0x5e4
> [e7833aa8] [c03ac1c8] test_hash_vs_generic_impl+0x1b0/0x2b4
> [e7833de8] [c03ac498] __alg_test_hash+0x1cc/0x2d0
> [e7833e28] [c03a9fb4] alg_test.part.37+0x8c/0x3ac
> [e7833ef8] [c03a54d0] cryptomgr_test+0x4c/0x54
> [e7833f08] [c006c410] kthread+0xf8/0x124
> [e7833f38] [c001227c] ret_from_kernel_thread+0x14/0x1c
> 
> addr2line on c03aba00 points to crypto/testmgr.c:1335
> 
>     1327)  if (cfg->finalization_type == FINALIZATION_TYPE_DIGEST ||
>     1328)      vec->digest_error) {
>     1329)          /* Just using digest() */
>     1330)          ahash_request_set_callback(req, req_flags, crypto_req_done,
>     1331)                                     &wait);
>     1332)          ahash_request_set_crypt(req, tsgl->sgl, result, vec->psize);
>     1333)          err = do_ahash_op(crypto_ahash_digest, req, &wait, cfg->nosimd);
>     1334)          if (err) {
> -> 1335)                  if (err == vec->digest_error)
>     1336)                          return 0;
>     1337)                  pr_err("alg: ahash: %s digest() failed on test vector %s; expected_error=%d, actual_error=%d, cfg=\"%s\"\n",
>     1338)                         driver, vec_name, vec->digest_error, err,
>     1339)                         cfg->name);
>     1340)                  return err;
>     1341)          }
>     1342)          if (vec->digest_error) {
>     1343)                  pr_err("alg: ahash: %s digest() unexpectedly succeeded on test vector %s; expected_error=%d, cfg=\"%s\"\n",
>     1344)                         driver, vec_name, vec->digest_error, cfg->name);
>     1345)                  return -EINVAL;
>     1346)          }
>     1347)          goto result_ready;
>     1348)  }
> 
> Seems that for some reason driver does not receive the interrupt from HW,
> thus completion callback does not run.
> 
> Tried with or without current patch series, no change in behaviour.
> 
> If you cannot reproduce and don't have any idea, I'll try the hard way
> (git bisect).

I cannot reproduce, both mpc885 and mpc8321e boot fine, and don't have 
any idea at first.

I know the SEC1 behaves that way when you submit zero-length data.

Christophe

> 
> Thanks,
> Horia
> 

Re: [PATCH v2 0/4] Additional fixes on Talitos driver

[Christophe Leroy]

Le 12/06/2019 à 15:59, Horia Geanta a écrit :
> On 6/12/2019 8:52 AM, Christophe Leroy wrote:
>>
>>
>> Le 11/06/2019 à 18:30, Horia Geanta a écrit :
>>> On 6/11/2019 6:40 PM, Christophe Leroy wrote:
>>>>
>>>>
>>>> Le 11/06/2019 à 17:37, Horia Geanta a écrit :
>>>>> On 6/11/2019 5:39 PM, Christophe Leroy wrote:
>>>>>> This series is the last set of fixes for the Talitos driver.
>>>>>>
>>>>>> We now get a fully clean boot on both SEC1 (SEC1.2 on mpc885) and
>>>>>> SEC2 (SEC2.2 on mpc8321E) with CONFIG_CRYPTO_MANAGER_EXTRA_TESTS:
>>>>>>
>>>>> I am getting below failures on a sec 3.3.2 (p1020rdb) for hmac(sha384) and
>>>>> hmac(sha512):
>>>>
>>>> Is that new with this series or did you already have it before ?
>>>>
>>> Looks like this happens with or without this series.
>>
>> Found the issue, that's in
>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=b8fbdc2bc4e71b62646031d5df5f08aafe15d5ad
>>
>> CONFIG_CRYPTO_DEV_TALITOS_SEC2 should be CONFIG_CRYPTO_DEV_TALITOS2 instead.
>>
>> Just sent a patch to fix it.
>>
> Thanks, I've tested it and the hmac failures go away.
> 
> However, testing gets stuck.
> Seems there is another issue lurking in the driver.
> 
> Used cryptodev-2.6/master with the following on top:
> crypto: testmgr - add some more preemption points
> 	https://patchwork.kernel.org/patch/10972337/
> crypto: talitos - fix max key size for sha384 and sha512
> 	https://patchwork.kernel.org/patch/10988473/
> 
> [...]
> alg: skcipher: skipping comparison tests for ecb-3des-talitos because ecb(des3_ede-generic) is unavailable
> INFO: task cryptomgr_test:314 blocked for more than 120 seconds.
>        Not tainted 5.2.0-rc1-g905bfd415e8a #1
> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> cryptomgr_test  D    0   314      2 0x00000800
> Call Trace:
> [e78337e0] [00000004] 0x4 (unreliable)
> [e78338a8] [c08a6e5c] __schedule+0x20c/0x4d4
> [e78338f8] [c08a7158] schedule+0x34/0xc8
> [e7833908] [c08aa5ec] schedule_timeout+0x1d4/0x350
> [e7833958] [c08a7be4] wait_for_common+0xa0/0x164
> [e7833998] [c03a7b14] do_ahash_op+0xa4/0xc4
> [e78339b8] [c03aba00] test_ahash_vec_cfg+0x188/0x5e4
> [e7833aa8] [c03ac1c8] test_hash_vs_generic_impl+0x1b0/0x2b4
> [e7833de8] [c03ac498] __alg_test_hash+0x1cc/0x2d0
> [e7833e28] [c03a9fb4] alg_test.part.37+0x8c/0x3ac
> [e7833ef8] [c03a54d0] cryptomgr_test+0x4c/0x54
> [e7833f08] [c006c410] kthread+0xf8/0x124
> [e7833f38] [c001227c] ret_from_kernel_thread+0x14/0x1c
> 
> addr2line on c03aba00 points to crypto/testmgr.c:1335
> 
>     1327)  if (cfg->finalization_type == FINALIZATION_TYPE_DIGEST ||
>     1328)      vec->digest_error) {
>     1329)          /* Just using digest() */
>     1330)          ahash_request_set_callback(req, req_flags, crypto_req_done,
>     1331)                                     &wait);
>     1332)          ahash_request_set_crypt(req, tsgl->sgl, result, vec->psize);
>     1333)          err = do_ahash_op(crypto_ahash_digest, req, &wait, cfg->nosimd);
>     1334)          if (err) {
> -> 1335)                  if (err == vec->digest_error)
>     1336)                          return 0;
>     1337)                  pr_err("alg: ahash: %s digest() failed on test vector %s; expected_error=%d, actual_error=%d, cfg=\"%s\"\n",
>     1338)                         driver, vec_name, vec->digest_error, err,
>     1339)                         cfg->name);
>     1340)                  return err;
>     1341)          }
>     1342)          if (vec->digest_error) {
>     1343)                  pr_err("alg: ahash: %s digest() unexpectedly succeeded on test vector %s; expected_error=%d, cfg=\"%s\"\n",
>     1344)                         driver, vec_name, vec->digest_error, cfg->name);
>     1345)                  return -EINVAL;
>     1346)          }
>     1347)          goto result_ready;
>     1348)  }
> 
> Seems that for some reason driver does not receive the interrupt from HW,
> thus completion callback does not run.
> 
> Tried with or without current patch series, no change in behaviour.
> 
> If you cannot reproduce and don't have any idea, I'll try the hard way
> (git bisect).

I cannot reproduce, both mpc885 and mpc8321e boot fine, and don't have 
any idea at first.

I know the SEC1 behaves that way when you submit zero-length data.

Christophe

> 
> Thanks,
> Horia
>