by Corentin Labbe

[permalink] [raw]

Subject: Re: [PATCH 10/11] crypto: sun4i-ss: fix large block size support

On Wed, May 24, 2017 at 09:06:51PM +0200, Antoine Tenart wrote:
> The run-time self-tests fail quite early, as soon as the input block
> size is larger than 64 bytes:
>
> alg: hash: Test 4 failed for sha1-sun4i-ss
> 00000000: b9 c9 1e 52 c0 26 d8 39 81 ff f2 3c 99 b1 27 b2
> 00000010: 30 d6 c9 85
>
> One thing to notice is the value of the last word, which is the one
> expected (it can sometime be the last two words). The datasheet isn't
> very clear about when the digest is ready to retrieve and is seems the
> bit SS_DATA_END is cleared when the digest was computed *but* that
> doesn't mean the digest is ready to retrieve in the registers.
>
> A ndelay(1) is added before reading the computed digest to ensure it is
> available in the SS_MD[] registers.
>
> Signed-off-by: Antoine Tenart <[email protected]>
> ---
> drivers/crypto/sunxi-ss/sun4i-ss-hash.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/drivers/crypto/sunxi-ss/sun4i-ss-hash.c b/drivers/crypto/sunxi-ss/sun4i-ss-hash.c
> index 685de5b6ab17..6da8d2bbd4da 100644
> --- a/drivers/crypto/sunxi-ss/sun4i-ss-hash.c
> +++ b/drivers/crypto/sunxi-ss/sun4i-ss-hash.c
> @@ -358,6 +358,15 @@ static int sun4i_hash(struct ahash_request *areq)
> goto release_ss;
> }
>
> + /*
> + * The datasheet isn't very clear about when to retrieve the digest. The
> + * bit SS_DATA_END is cleared when the engine has processed the data and
> + * when the digest is computed *but* it doesn't mean the digest is
> + * available in the diesgt registers. Hence the delay to be sure we can

Hello

Small typo here (diesgt/digest)

> + * read it.
> + */
> + ndelay(1);
> +
> for (i = 0; i < crypto_ahash_digestsize(tfm) / 4; i++)
> op->hash[i] = readl(ss->base + SS_MD0 + i * 4);
>
> @@ -446,6 +455,15 @@ static int sun4i_hash(struct ahash_request *areq)
> goto release_ss;
> }
>
> + /*
> + * The datasheet isn't very clear about when to retrieve the digest. The
> + * bit SS_DATA_END is cleared when the engine has processed the data and
> + * when the digest is computed *but* it doesn't mean the digest is
> + * available in the diesgt registers. Hence the delay to be sure we can

and here.

This behaviour is very strange since I didnt get it on other platform.
Could you give me the board name where you get it ?
Does any wmb() after the writel(..SS_DATA_END..) do the trick ?
Which speed are the SS clocks ?

Anyway the whole serie seems good, I will test it just after this weekend.

Thanks
Regards
Corentin Labbe

2017-05-29 08:10:01

by Antoine Tenart

[permalink] [raw]

Subject: Re: [PATCH 10/11] crypto: sun4i-ss: fix large block size support

Hi Corentin,

On Fri, May 26, 2017 at 04:55:01PM +0200, Corentin Labbe wrote:
> On Wed, May 24, 2017 at 09:06:51PM +0200, Antoine Tenart wrote:
> >
> > + /*
> > + * The datasheet isn't very clear about when to retrieve the digest. The
> > + * bit SS_DATA_END is cleared when the engine has processed the data and
> > + * when the digest is computed *but* it doesn't mean the digest is
> > + * available in the diesgt registers. Hence the delay to be sure we can
>
> Small typo here (diesgt/digest)

Oops. I'll fix it.

> This behaviour is very strange since I didnt get it on other platform.
> Could you give me the board name where you get it ?

I used a CHIP (sun5i-r8-chip).

> Does any wmb() after the writel(..SS_DATA_END..) do the trick ?

Nope. I tried this and it didn't help.

> Which speed are the SS clocks ?

The AHB SS clk is running at 300 MHz and the SS clk at 150 MHz. SS clk
is at the expected rate but the AHB SS clk has a higher rate that what's
expected.

In the probing function only the SS clk rate is explicitly set. I tried
to set the AHB clk rate as well and removed the delays. This didn't fix
the framework selftests at boot time. Is there any reason the AHB SS clk
rate isn't explicitly set when probing the driver? (Should it?)

> Anyway the whole serie seems good, I will test it just after this weekend.

Let me know if your tests went well, I'll send a v2 fixing the typo
below then (and setting the AHB SS clk rate explicitly if needed).

Thanks,
Antoine

--
Antoine T?nart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Attachments:

(No filename) (1.56 kB)
signature.asc (819.00 B)
Download all attachments

2017-05-29 08:30:04

Hi Corentin,

On Mon, May 29, 2017 at 04:47:57PM +0200, Corentin Labbe wrote:
> On Wed, May 24, 2017 at 09:06:50PM +0200, Antoine Tenart wrote:
> > + .cra_flags = CRYPTO_ALG_TYPE_SKCIPHER |
> > + CRYPTO_ALG_KERN_DRIVER_ONLY,
>
> You add CRYPTO_ALG_KERN_DRIVER_ONLY on all flags which is unrelated to
> the patch description.

Right, I forgot to make an extra patch for this. I'll fix this (and the
other ones).

Thanks,
Antoine

--
Antoine T?nart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Attachments:

(No filename) (539.00 B)
signature.asc (819.00 B)
Download all attachments

2017-05-29 09:15:15

by Antoine Tenart

[permalink] [raw]

Subject: Re: [PATCH 10/11] crypto: sun4i-ss: fix large block size support

Hi Maxime,

On Mon, May 29, 2017 at 10:29:31AM +0200, Maxime Ripard wrote:
> On Mon, May 29, 2017 at 10:09:44AM +0200, Antoine Tenart wrote:
> > > Which speed are the SS clocks ?
> >
> > The AHB SS clk is running at 300 MHz and the SS clk at 150 MHz. SS clk
> > is at the expected rate but the AHB SS clk has a higher rate that what's
> > expected.
> >
> > In the probing function only the SS clk rate is explicitly set. I tried
> > to set the AHB clk rate as well and removed the delays. This didn't fix
> > the framework selftests at boot time. Is there any reason the AHB SS clk
> > rate isn't explicitly set when probing the driver? (Should it?)
>
> It probably shouldn't.
>
> The AHB clock is shared by most of the drivers, some of them actually
> using that clock to generate their signals.
>
> You would have to unbreak all those drivers first, which is probably
> not needed at all. I haven't seen a case where a block had a module
> clock and did care for its AHB clock rate.

OK, makes sense.

I'll wait for Corentin to test the series, and I'll send a v2 only
fixing the typos then.

Thanks!
Antoine

--
Antoine T?nart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Attachments:

(No filename) (1.19 kB)
signature.asc (819.00 B)
Download all attachments