[PATCH 1/4] mv_cesa: add an expiry timer in case anything goes wrong

[PATCH 1/4] mv_cesa: add an expiry timer in case anything goes wrong

[Phil Sutter]

The timer triggers when 500ms have gone by after triggering the engine
and no completion interrupt was received. The callback then tries to
sanitise things as well as possible.

Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
---
 drivers/crypto/mv_cesa.c |   41 +++++++++++++++++++++++++++++++----------
 1 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/drivers/crypto/mv_cesa.c b/drivers/crypto/mv_cesa.c
index e6ecc5f..8327bed 100644
--- a/drivers/crypto/mv_cesa.c
+++ b/drivers/crypto/mv_cesa.c
@@ -23,6 +23,7 @@
 
 #define MV_CESA	"MV-CESA:"
 #define MAX_HW_HASH_SIZE	0xFFFF
+#define MV_CESA_EXPIRE		500 /* msec */
 
 /*
  * STM:
@@ -85,6 +86,7 @@ struct crypto_priv {
 	spinlock_t lock;
 	struct crypto_queue queue;
 	enum engine_status eng_st;
+	struct timer_list completion_timer;
 	struct crypto_async_request *cur_req;
 	struct req_progress p;
 	int max_req_size;
@@ -136,6 +138,29 @@ struct mv_req_hash_ctx {
 	int count_add;
 };
 
+static void mv_completion_timer_callback(unsigned long unused)
+{
+	int active = readl(cpg->reg + SEC_ACCEL_CMD) & SEC_CMD_EN_SEC_ACCL0;
+
+	printk(KERN_ERR MV_CESA
+	       "completion timer expired (CESA %sactive), cleaning up.\n",
+	       active ? "" : "in");
+
+	del_timer(&cpg->completion_timer);
+	writel(SEC_CMD_DISABLE_SEC, cpg->reg + SEC_ACCEL_CMD);
+	while(readl(cpg->reg + SEC_ACCEL_CMD) & SEC_CMD_DISABLE_SEC)
+		printk(KERN_INFO MV_CESA "%s: waiting for engine finishing\n", __func__);
+	cpg->eng_st = ENGINE_W_DEQUEUE;
+	wake_up_process(cpg->queue_th);
+}
+
+static void mv_setup_timer(void)
+{
+	setup_timer(&cpg->completion_timer, &mv_completion_timer_callback, 0);
+	mod_timer(&cpg->completion_timer,
+			jiffies + msecs_to_jiffies(MV_CESA_EXPIRE));
+}
+
 static void compute_aes_dec_key(struct mv_ctx *ctx)
 {
 	struct crypto_aes_ctx gen_aes_key;
@@ -271,12 +296,8 @@ static void mv_process_current_q(int first_block)
 			sizeof(struct sec_accel_config));
 
 	/* GO */
+	mv_setup_timer();
 	writel(SEC_CMD_EN_SEC_ACCL0, cpg->reg + SEC_ACCEL_CMD);
-
-	/*
-	 * XXX: add timer if the interrupt does not occur for some mystery
-	 * reason
-	 */
 }
 
 static void mv_crypto_algo_completion(void)
@@ -355,12 +376,8 @@ static void mv_process_hash_current(int first_block)
 	memcpy(cpg->sram + SRAM_CONFIG, &op, sizeof(struct sec_accel_config));
 
 	/* GO */
+	mv_setup_timer();
 	writel(SEC_CMD_EN_SEC_ACCL0, cpg->reg + SEC_ACCEL_CMD);
-
-	/*
-	* XXX: add timer if the interrupt does not occur for some mystery
-	* reason
-	*/
 }
 
 static inline int mv_hash_import_sha1_ctx(const struct mv_req_hash_ctx *ctx,
@@ -886,6 +903,10 @@ irqreturn_t crypto_int(int irq, void *priv)
 	if (!(val & SEC_INT_ACCEL0_DONE))
 		return IRQ_NONE;
 
+	if (!del_timer(&cpg->completion_timer)) {
+		printk(KERN_WARNING MV_CESA
+		       "got an interrupt but no pending timer?\n");
+	}
 	val &= ~SEC_INT_ACCEL0_DONE;
 	writel(val, cpg->reg + FPGA_INT_STATUS);
 	writel(val, cpg->reg + SEC_ACCEL_INT_STATUS);
-- 
1.7.3.4

[PATCH 2/4] mv_cesa: no need to write to that FPGA_INT_STATUS field

[Phil Sutter]

Also drop the whole definition, since it's unused otherwise.

Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
---
 drivers/crypto/mv_cesa.c |    1 -
 drivers/crypto/mv_cesa.h |    7 -------
 2 files changed, 0 insertions(+), 8 deletions(-)

diff --git a/drivers/crypto/mv_cesa.c b/drivers/crypto/mv_cesa.c
index 8327bed..4a1f872 100644
--- a/drivers/crypto/mv_cesa.c
+++ b/drivers/crypto/mv_cesa.c
@@ -908,7 +908,6 @@ irqreturn_t crypto_int(int irq, void *priv)
 		       "got an interrupt but no pending timer?\n");
 	}
 	val &= ~SEC_INT_ACCEL0_DONE;
-	writel(val, cpg->reg + FPGA_INT_STATUS);
 	writel(val, cpg->reg + SEC_ACCEL_INT_STATUS);
 	BUG_ON(cpg->eng_st != ENGINE_BUSY);
 	cpg->eng_st = ENGINE_W_DEQUEUE;
diff --git a/drivers/crypto/mv_cesa.h b/drivers/crypto/mv_cesa.h
index 08fcb11..81ce109 100644
--- a/drivers/crypto/mv_cesa.h
+++ b/drivers/crypto/mv_cesa.h
@@ -29,13 +29,6 @@
 #define SEC_ST_ACT_0		(1 << 0)
 #define SEC_ST_ACT_1		(1 << 1)
 
-/*
- * FPGA_INT_STATUS looks like a FPGA leftover and is documented only in Errata
- * 4.12. It looks like that it was part of an IRQ-controller in FPGA and
- * someone forgot to remove  it while switching to the core and moving to
- * SEC_ACCEL_INT_STATUS.
- */
-#define FPGA_INT_STATUS		0xdd68
 #define SEC_ACCEL_INT_STATUS	0xde20
 #define SEC_INT_AUTH_DONE	(1 << 0)
 #define SEC_INT_DES_E_DONE	(1 << 1)
-- 
1.7.3.4

[PATCH 3/4] mv_cesa: initialise the interrupt status field to zero

[Phil Sutter]

Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
---
 drivers/crypto/mv_cesa.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/crypto/mv_cesa.c b/drivers/crypto/mv_cesa.c
index 4a1f872..d4763fb 100644
--- a/drivers/crypto/mv_cesa.c
+++ b/drivers/crypto/mv_cesa.c
@@ -1073,6 +1073,7 @@ static int mv_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_thread;
 
+	writel(0, cpg->reg + SEC_ACCEL_INT_STATUS);
 	writel(SEC_INT_ACCEL0_DONE, cpg->reg + SEC_ACCEL_INT_MASK);
 	writel(SEC_CFG_STOP_DIG_ERR, cpg->reg + SEC_ACCEL_CFG);
 	writel(SRAM_CONFIG, cpg->reg + SEC_ACCEL_DESC_P0);
-- 
1.7.3.4

[PATCH 4/4] mv_cesa: fix for hash finalisation with data

[Phil Sutter]

Since mv_hash_final_fallback() uses ctx->state, read out the digest
state register before calling it.

Signed-off-by: Phil Sutter <phil.sutter@viprinet.com>
---
 drivers/crypto/mv_cesa.c |   19 +++++++++++++------
 1 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/crypto/mv_cesa.c b/drivers/crypto/mv_cesa.c
index d4763fb..3cc9237 100644
--- a/drivers/crypto/mv_cesa.c
+++ b/drivers/crypto/mv_cesa.c
@@ -421,6 +421,15 @@ out:
 	return rc;
 }
 
+static void mv_save_digest_state(struct mv_req_hash_ctx *ctx)
+{
+	ctx->state[0] = readl(cpg->reg + DIGEST_INITIAL_VAL_A);
+	ctx->state[1] = readl(cpg->reg + DIGEST_INITIAL_VAL_B);
+	ctx->state[2] = readl(cpg->reg + DIGEST_INITIAL_VAL_C);
+	ctx->state[3] = readl(cpg->reg + DIGEST_INITIAL_VAL_D);
+	ctx->state[4] = readl(cpg->reg + DIGEST_INITIAL_VAL_E);
+}
+
 static void mv_hash_algo_completion(void)
 {
 	struct ahash_request *req = ahash_request_cast(cpg->cur_req);
@@ -435,14 +444,12 @@ static void mv_hash_algo_completion(void)
 			memcpy(req->result, cpg->sram + SRAM_DIGEST_BUF,
 			       crypto_ahash_digestsize(crypto_ahash_reqtfm
 						       (req)));
-		} else
+		} else {
+			mv_save_digest_state(ctx);
 			mv_hash_final_fallback(req);
+		}
 	} else {
-		ctx->state[0] = readl(cpg->reg + DIGEST_INITIAL_VAL_A);
-		ctx->state[1] = readl(cpg->reg + DIGEST_INITIAL_VAL_B);
-		ctx->state[2] = readl(cpg->reg + DIGEST_INITIAL_VAL_C);
-		ctx->state[3] = readl(cpg->reg + DIGEST_INITIAL_VAL_D);
-		ctx->state[4] = readl(cpg->reg + DIGEST_INITIAL_VAL_E);
+		mv_save_digest_state(ctx);
 	}
 }
 
-- 
1.7.3.4

Re: [PATCH 2/4] mv_cesa: no need to write to that FPGA_INT_STATUS field

[cloudy.linux]

On 2012-5-25 21:54, Phil Sutter wrote:
> Also drop the whole definition, since it's unused otherwise.
>
> Signed-off-by: Phil Sutter<phil.sutter@viprinet.com>
> ---
>   drivers/crypto/mv_cesa.c |    1 -
>   drivers/crypto/mv_cesa.h |    7 -------
>   2 files changed, 0 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/crypto/mv_cesa.c b/drivers/crypto/mv_cesa.c
> index 8327bed..4a1f872 100644
> --- a/drivers/crypto/mv_cesa.c
> +++ b/drivers/crypto/mv_cesa.c
> @@ -908,7 +908,6 @@ irqreturn_t crypto_int(int irq, void *priv)
>   		       "got an interrupt but no pending timer?\n");
>   	}
>   	val&= ~SEC_INT_ACCEL0_DONE;
> -	writel(val, cpg->reg + FPGA_INT_STATUS);
>   	writel(val, cpg->reg + SEC_ACCEL_INT_STATUS);
>   	BUG_ON(cpg->eng_st != ENGINE_BUSY);
>   	cpg->eng_st = ENGINE_W_DEQUEUE;
> diff --git a/drivers/crypto/mv_cesa.h b/drivers/crypto/mv_cesa.h
> index 08fcb11..81ce109 100644
> --- a/drivers/crypto/mv_cesa.h
> +++ b/drivers/crypto/mv_cesa.h
> @@ -29,13 +29,6 @@
>   #define SEC_ST_ACT_0		(1<<  0)
>   #define SEC_ST_ACT_1		(1<<  1)
>
> -/*
> - * FPGA_INT_STATUS looks like a FPGA leftover and is documented only in Errata
> - * 4.12. It looks like that it was part of an IRQ-controller in FPGA and
> - * someone forgot to remove  it while switching to the core and moving to
> - * SEC_ACCEL_INT_STATUS.
> - */
> -#define FPGA_INT_STATUS		0xdd68
>   #define SEC_ACCEL_INT_STATUS	0xde20
>   #define SEC_INT_AUTH_DONE	(1<<  0)
>   #define SEC_INT_DES_E_DONE	(1<<  1)

According to the functional errata of 88F5182, the FPGA_INT_STATUS is 
needed (at least for 88F5182-A1/A2). Here is the quote from that errata:

4.12	Clearing the Cryptographic Engines and Security Accelerator 
Interrupt Cause Register
	Type:		Guideline
	Ref#:		GL-CESA-100
	Relevant for:	88F5182-A1/A2

Description:
Writing 0 to bits[6:0] of the Crytographic Engines ...  Interrupt Cause 
register (offset 0x9DE20) has no effect.

Steps to be performed by the designer
Before writing 0 to any of the bits[6:0] of the Cryptographic Engines .. 
Interrupt Cause register, the software must write 0 to the corresponding 
bit of the internal register at offset 0x9DD68.
Writing to offset 0x9DD68 is not possible when any of the Security 
Accelerators' sessions are active. Therefore, the software must verify 
that no channel is active before clearing any of those interrupts.

Re: [PATCH 2/4] mv_cesa: no need to write to that FPGA_INT_STATUS field

[Phil Sutter]

Hi,

On Mon, May 28, 2012 at 09:58:32AM +0800, cloudy.linux wrote:
> On 2012-5-25 21:54, Phil Sutter wrote:
> > Also drop the whole definition, since it's unused otherwise.
> >
> > Signed-off-by: Phil Sutter<phil.sutter@viprinet.com>
> > ---
> >   drivers/crypto/mv_cesa.c |    1 -
> >   drivers/crypto/mv_cesa.h |    7 -------
> >   2 files changed, 0 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/crypto/mv_cesa.c b/drivers/crypto/mv_cesa.c
> > index 8327bed..4a1f872 100644
> > --- a/drivers/crypto/mv_cesa.c
> > +++ b/drivers/crypto/mv_cesa.c
> > @@ -908,7 +908,6 @@ irqreturn_t crypto_int(int irq, void *priv)
> >   		       "got an interrupt but no pending timer?\n");
> >   	}
> >   	val&= ~SEC_INT_ACCEL0_DONE;
> > -	writel(val, cpg->reg + FPGA_INT_STATUS);
> >   	writel(val, cpg->reg + SEC_ACCEL_INT_STATUS);
> >   	BUG_ON(cpg->eng_st != ENGINE_BUSY);
> >   	cpg->eng_st = ENGINE_W_DEQUEUE;
> > diff --git a/drivers/crypto/mv_cesa.h b/drivers/crypto/mv_cesa.h
> > index 08fcb11..81ce109 100644
> > --- a/drivers/crypto/mv_cesa.h
> > +++ b/drivers/crypto/mv_cesa.h
> > @@ -29,13 +29,6 @@
> >   #define SEC_ST_ACT_0		(1<<  0)
> >   #define SEC_ST_ACT_1		(1<<  1)
> >
> > -/*
> > - * FPGA_INT_STATUS looks like a FPGA leftover and is documented only in Errata
> > - * 4.12. It looks like that it was part of an IRQ-controller in FPGA and
> > - * someone forgot to remove  it while switching to the core and moving to
> > - * SEC_ACCEL_INT_STATUS.
> > - */
> > -#define FPGA_INT_STATUS		0xdd68
> >   #define SEC_ACCEL_INT_STATUS	0xde20
> >   #define SEC_INT_AUTH_DONE	(1<<  0)
> >   #define SEC_INT_DES_E_DONE	(1<<  1)
> 
> According to the functional errata of 88F5182, the FPGA_INT_STATUS is 
> needed (at least for 88F5182-A1/A2). Here is the quote from that errata:
> 
> 4.12	Clearing the Cryptographic Engines and Security Accelerator 
> Interrupt Cause Register
> 	Type:		Guideline
> 	Ref#:		GL-CESA-100
> 	Relevant for:	88F5182-A1/A2
> 
> Description:
> Writing 0 to bits[6:0] of the Crytographic Engines ...  Interrupt Cause 
> register (offset 0x9DE20) has no effect.
> 
> Steps to be performed by the designer
> Before writing 0 to any of the bits[6:0] of the Cryptographic Engines .. 
> Interrupt Cause register, the software must write 0 to the corresponding 
> bit of the internal register at offset 0x9DD68.
> Writing to offset 0x9DD68 is not possible when any of the Security 
> Accelerators' sessions are active. Therefore, the software must verify 
> that no channel is active before clearing any of those interrupts.

Oh, that explains why it's not needed on Kirkwood but still left there.
I could make it compile-time optional, depending on ARCH_ORION5X e.g. or
simply drop the patch and leave it alone since it really doesn't hurt
that much. 

Anyway, thanks a lot for your kind review!

Greetings, Phil



Phil Sutter
Software Engineer

-- 


Viprinet GmbH
Mainzer Str. 43
55411 Bingen am Rhein
Germany

Phone/Zentrale:         +49-6721-49030-0
Direct line/Durchwahl:  +49-6721-49030-134
Fax:                    +49-6721-49030-209

phil.sutter@viprinet.com
http://www.viprinet.com

Registered office/Sitz der Gesellschaft: Bingen am Rhein
Commercial register/Handelsregister: Amtsgericht Mainz HRB40380
CEO/Geschäftsführer: Simon Kissel

Re: [PATCH 2/4] mv_cesa: no need to write to that FPGA_INT_STATUS field

[Herbert Xu]

On Tue, May 29, 2012 at 01:45:29PM +0200, Phil Sutter wrote:
> 
> Oh, that explains why it's not needed on Kirkwood but still left there.
> I could make it compile-time optional, depending on ARCH_ORION5X e.g. or
> simply drop the patch and leave it alone since it really doesn't hurt
> that much. 

OK I've skipped number two and applied the other three.

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