Re: [PATCH 1/2] crypto: Add Imagination Technologies hw hash accelerator
On Sat, Nov 15, 2014 at 08:55:58AM +0100, Corentin LABBE wrote: > > and then get it via > struct crypto_priv *cp = req->base.tfm->__crt_alg->data; > (a function will be better than that) > > So what is the recommended way to get driver structure inside the cryptoAPI > function (init/udpate/final)? Have a look at talitos which deals with this by embedding crypto_alg within its own data structure. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] crypto: Add Imagination Technologies hw hash accelerator
Andrew,
Thanks for the review
> -Original Message-
> From: abres...@google.com [mailto:abres...@google.com] On Behalf Of
> Andrew Bresticker
> Sent: 14 November 2014 23:59
> To: James Hartley
> Cc: Herbert Xu; da...@davemloft.net; Grant Likely; Rob Herring;
> a...@linux-foundation.org; Greg Kroah-Hartman; j...@perches.com;
> mche...@osg.samsung.com; Antti Palosaari; jg1@samsung.com; linux-
> cry...@vger.kernel.org; devicet...@vger.kernel.org; Pawel Moll; Mark
> Rutland; Ian Campbell; Kumar Gala; Ezequiel Garcia
> Subject: Re: [PATCH 1/2] crypto: Add Imagination Technologies hw hash
> accelerator
>
> Hi James,
>
> On Mon, Nov 10, 2014 at 4:10 AM, James Hartley
> wrote:
> > This adds support for the Imagination Technologies hash accelerator
> > that provides hardware acceleration for
> > SHA1 SHA224 SHA256 and MD5 Hashes.
> >
> > Signed-off-by: James Hartley
>
> It's going to take me a little longer to get through the crypto API parts of
> the
> driver, but here are some initial comments.
>
> > diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig index
> > 2fb0fdf..4b931eb 100644
> > --- a/drivers/crypto/Kconfig
> > +++ b/drivers/crypto/Kconfig
> > @@ -436,4 +436,17 @@ config CRYPTO_DEV_QCE
> > hardware. To compile this driver as a module, choose M here. The
> > module will be called qcrypto.
> >
> > +config CRYPTO_DEV_IMGTEC_HASH
> > +tristate "Imagination Technologies hardware hash accelerator"
>
> Is there no meaningful depends-on here? There's no MACH_PISTACHIO yet,
> but perhaps MIPS for now? Also, {readl,writel}_relaxed aren't available on
> all
> architectures.
I have added MIPS || COMPILE_TEST for now.
On the basis of Arnd's comments I'll leave in the readl, writel.
>
> > +select CRYPTO_ALG_API
> > +select CRYPTO_MD5
> > +select CRYPTO_SHA1
> > +select CRYPTO_SHA224
> > +select CRYPTO_SHA256
> > +select CRYPTO_HASH
> > +help
> > + This driver interfaces with the Imagination Technologies
> > + hardware hash accelerator. Supporting MD5/SHA1/SHA224/SHA256
> > + hashing algorithms.
> > +
> > endif # CRYPTO_HW
>
> > diff --git a/drivers/crypto/img-hash.c b/drivers/crypto/img-hash.c new
> > file mode 100644 index 000..e58c81a
> > --- /dev/null
> > +++ b/drivers/crypto/img-hash.c
>
> > @@ -0,0 +1,1048 @@
> > +/*
> > +* Copyright (c) 2014 Imagination Technologies
> > +* Author: Will Thomas
> > +*
> > +* This program is free software; you can redistribute it and/or modify
> > +* it under the terms of the GNU General Public License version 2 as
> published
> > +* by the Free Software Foundation.
> > +*
> > +* Interface structure taken from omap-sham driver
> > +*/
>
> Comment style is incorrect here, the *s in multi-line comments should
> be aligned.
Fixed
>
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
>
> #includes should be sorted in alphabetical order, with a newline
> separating the linux/ and crypto/ includes.
Ok, done.
>
> > +#define MD5_BLOCK_SIZE 64
>
> There's already MD5_HMAC_BLOCK_SIZE #define'd in crypto/md5.h.
Ah yes, modified now.
>
> > +#define CR_RESET 0
> > +#define CR_RESET_SET 1
> > +#define CR_RESET_UNSET 0
> > +
> > +#define CR_MESSAGE_LENGTH_H0x4
> > +#define CR_MESSAGE_LENGTH_L0x8
> > +
> > +#defineCR_CONTROL 0xc
> > +#define CR_CONTROL_BYTE_ORDER_3210 0
> > +#define CR_CONTROL_BYTE_ORDER_0123 1
> > +#define CR_CONTROL_BYTE_ORDER_2310 2
> > +#define CR_CONTROL_BYTE_ORDER_1032 3
> > +#define CR_CONTROL_ALGO_MD50
> > +#define CR_CONTROL_ALGO_SHA1 1
> > +#define CR_CONTROL_ALGO_SHA224 2
> > +#define CR_CONTROL_ALGO_SHA256 3
> > +
> > +#define CR_INTSTAT 0x10
> > +#define CR_INTENAB 0x14
> > +#define CR_INTCLEAR0x18
> > +#define CR_INT_RESULTS_AVAILABLE (1<<0)
> > +#define CR_INT_NEW_RESULTS_SET (1<<1)
> > +#define CR_INT_RESULT_READ_ERR (1<<2)
> > +#define CR_INT_MESSAGE_WRITE_ERROR (1<<3)
> > +#define CR
Re: [PATCH 1/2] crypto: Add Imagination Technologies hw hash accelerator
On Fri, Nov 14, 2014 at 11:55 PM, Corentin LABBE
wrote:
> Le 15/11/2014 00:59, Andrew Bresticker a écrit :
>> Hi James,
>>
>>> +
>>> +struct img_hash_drv {
>>> + struct list_head dev_list;
>>> + spinlock_t lock;
>>> +};
>>> +
>>> +static struct img_hash_drv img_hash = {
>>> + .dev_list = LIST_HEAD_INIT(img_hash.dev_list),
>>> + .lock = __SPIN_LOCK_UNLOCKED(img_hash.lock),
>>> +};
>>
>> It looks like the only purpose of this list is to get the
>> corresponding struct img_hash_dev in img_hash_init(). If there's
>> never going to be multiple instances within an SoC, perhaps you could
>> just use a global? Otherwise, you could do something like the
>> qualcomm driver, see drivers/crypto/qce/sha.c. It looks like there is
>> some precedent for this device list though...
>>
>
> I don't understand, you propose to use a global, something that lots of
> people want to be removed in my driver.
> It is not better than this global list.
>
> I have the fealing that there no good way to get a pointer to a driver
> structure inside the cryptoAPI.
> What to you think about adding a void *data in struct crypto_alg
>
> Before registering an alg you could do:
> mv_aes_alg_ecb.data = myprivatedriverdata;
> ret = crypto_register_alg(&mv_aes_alg_ecb);
>
> and then get it via
> struct crypto_priv *cp = req->base.tfm->__crt_alg->data;
> (a function will be better than that)
That sounds good to me.
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] crypto: Add Imagination Technologies hw hash accelerator
On Friday 14 November 2014 15:59:02 Andrew Bresticker wrote:
> > diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
> > index 2fb0fdf..4b931eb 100644
> > --- a/drivers/crypto/Kconfig
> > +++ b/drivers/crypto/Kconfig
> > @@ -436,4 +436,17 @@ config CRYPTO_DEV_QCE
> > hardware. To compile this driver as a module, choose M here. The
> > module will be called qcrypto.
> >
> > +config CRYPTO_DEV_IMGTEC_HASH
> > +tristate "Imagination Technologies hardware hash accelerator"
>
> Is there no meaningful depends-on here? There's no MACH_PISTACHIO
> yet, but perhaps MIPS for now? Also, {readl,writel}_relaxed aren't
> available on all architectures.
>
I've queued up support for {readl,writel}_relaxed on all architectures
for 3.19, based on a series from Will Deacon.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] crypto: Add Imagination Technologies hw hash accelerator
Le 15/11/2014 00:59, Andrew Bresticker a écrit :
> Hi James,
>
>> +
>> +struct img_hash_drv {
>> + struct list_head dev_list;
>> + spinlock_t lock;
>> +};
>> +
>> +static struct img_hash_drv img_hash = {
>> + .dev_list = LIST_HEAD_INIT(img_hash.dev_list),
>> + .lock = __SPIN_LOCK_UNLOCKED(img_hash.lock),
>> +};
>
> It looks like the only purpose of this list is to get the
> corresponding struct img_hash_dev in img_hash_init(). If there's
> never going to be multiple instances within an SoC, perhaps you could
> just use a global? Otherwise, you could do something like the
> qualcomm driver, see drivers/crypto/qce/sha.c. It looks like there is
> some precedent for this device list though...
>
I don't understand, you propose to use a global, something that lots of people
want to be removed in my driver.
It is not better than this global list.
I have the fealing that there no good way to get a pointer to a driver
structure inside the cryptoAPI.
What to you think about adding a void *data in struct crypto_alg
Before registering an alg you could do:
mv_aes_alg_ecb.data = myprivatedriverdata;
ret = crypto_register_alg(&mv_aes_alg_ecb);
and then get it via
struct crypto_priv *cp = req->base.tfm->__crt_alg->data;
(a function will be better than that)
So what is the recommended way to get driver structure inside the cryptoAPI
function (init/udpate/final)?
Regards
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] crypto: Add Imagination Technologies hw hash accelerator
Hi James,
On Mon, Nov 10, 2014 at 4:10 AM, James Hartley wrote:
> This adds support for the Imagination Technologies hash
> accelerator that provides hardware acceleration for
> SHA1 SHA224 SHA256 and MD5 Hashes.
>
> Signed-off-by: James Hartley
It's going to take me a little longer to get through the crypto API
parts of the driver, but here are some initial comments.
> diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
> index 2fb0fdf..4b931eb 100644
> --- a/drivers/crypto/Kconfig
> +++ b/drivers/crypto/Kconfig
> @@ -436,4 +436,17 @@ config CRYPTO_DEV_QCE
> hardware. To compile this driver as a module, choose M here. The
> module will be called qcrypto.
>
> +config CRYPTO_DEV_IMGTEC_HASH
> +tristate "Imagination Technologies hardware hash accelerator"
Is there no meaningful depends-on here? There's no MACH_PISTACHIO
yet, but perhaps MIPS for now? Also, {readl,writel}_relaxed aren't
available on all architectures.
> +select CRYPTO_ALG_API
> +select CRYPTO_MD5
> +select CRYPTO_SHA1
> +select CRYPTO_SHA224
> +select CRYPTO_SHA256
> +select CRYPTO_HASH
> +help
> + This driver interfaces with the Imagination Technologies
> + hardware hash accelerator. Supporting MD5/SHA1/SHA224/SHA256
> + hashing algorithms.
> +
> endif # CRYPTO_HW
> diff --git a/drivers/crypto/img-hash.c b/drivers/crypto/img-hash.c
> new file mode 100644
> index 000..e58c81a
> --- /dev/null
> +++ b/drivers/crypto/img-hash.c
> @@ -0,0 +1,1048 @@
> +/*
> +* Copyright (c) 2014 Imagination Technologies
> +* Author: Will Thomas
> +*
> +* This program is free software; you can redistribute it and/or modify
> +* it under the terms of the GNU General Public License version 2 as published
> +* by the Free Software Foundation.
> +*
> +* Interface structure taken from omap-sham driver
> +*/
Comment style is incorrect here, the *s in multi-line comments should
be aligned.
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
#includes should be sorted in alphabetical order, with a newline
separating the linux/ and crypto/ includes.
> +#define MD5_BLOCK_SIZE 64
There's already MD5_HMAC_BLOCK_SIZE #define'd in crypto/md5.h.
> +#define CR_RESET 0
> +#define CR_RESET_SET 1
> +#define CR_RESET_UNSET 0
> +
> +#define CR_MESSAGE_LENGTH_H0x4
> +#define CR_MESSAGE_LENGTH_L0x8
> +
> +#defineCR_CONTROL 0xc
> +#define CR_CONTROL_BYTE_ORDER_3210 0
> +#define CR_CONTROL_BYTE_ORDER_0123 1
> +#define CR_CONTROL_BYTE_ORDER_2310 2
> +#define CR_CONTROL_BYTE_ORDER_1032 3
> +#define CR_CONTROL_ALGO_MD50
> +#define CR_CONTROL_ALGO_SHA1 1
> +#define CR_CONTROL_ALGO_SHA224 2
> +#define CR_CONTROL_ALGO_SHA256 3
> +
> +#define CR_INTSTAT 0x10
> +#define CR_INTENAB 0x14
> +#define CR_INTCLEAR0x18
> +#define CR_INT_RESULTS_AVAILABLE (1<<0)
> +#define CR_INT_NEW_RESULTS_SET (1<<1)
> +#define CR_INT_RESULT_READ_ERR (1<<2)
> +#define CR_INT_MESSAGE_WRITE_ERROR (1<<3)
> +#define CR_INT_STATUS (1<<8)
Use the BIT() macro for single-bit fields like this.
> +#define CR_RESULT_QUEUE0x1c
> +#define CR_RSD00x40
> +#define CR_CORE_REV0x50
> +#define CR_CORE_DES1 0x60
> +#define CR_CORE_DES2 0x70
> +
> +#define DRIVER_FLAGS_BUSY BIT(0)
> +#define DRIVER_FLAGS_FINAL BIT(1)
> +#define DRIVER_FLAGS_DMA_ACTIVEBIT(2)
> +#define DRIVER_FLAGS_OUTPUT_READY BIT(3)
> +#define DRIVER_FLAGS_INIT BIT(4)
> +#define DRIVER_FLAGS_CPU BIT(5)
> +#define DRIVER_FLAGS_DMA_READY BIT(6)
> +#define DRIVER_FLAGS_ERROR BIT(7)
> +#define DRIVER_FLAGS_SGBIT(8)
> +#define DRIVER_FLAGS_FINUP BIT(9)
> +#define DRIVER_FLAGS_SHA1 BIT(18)
> +#define DRIVER_FLAGS_SHA224BIT(19)
> +#define DRIVER_FLAGS_SHA256BIT(20)
> +#define DRIVER_FLAGS_MD5 BIT(21)
> +
> +#define OP_UPDATE 1
> +#define OP_FINAL 2
> +
> +#define IMG_HASH_QUEUE_LENGTH 20
> +#define IMG_HASH_DMA_THRESHOLD 64
> +#ifdef __LITTLE_ENDIAN
> +#define IMG_HASH_BYTE_ORDER(CR_CONTROL_BYTE_ORDER_3210)
> +#else
> +#define IMG_HASH_BYTE_ORDER(CR_CONTROL_BYTE_ORDER_0123)
> +#endif
> +
> +struct img_hash_dev;
> +
> +struct img_hash_request_ctx {
> + struct img_hash_dev *hdev;
> + u8 digest[SHA256_DIGEST_SIZE] __aligned(sizeof(u32));
> + unsigned long flags;
> + size_t digsize;
> +
> + dma_a
RE: [PATCH 1/2] crypto: Add Imagination Technologies hw hash accelerator
Hi Vladimir
> -Original Message-
> From: linux-crypto-ow...@vger.kernel.org [mailto:linux-crypto-
> ow...@vger.kernel.org] On Behalf Of Vladimir Zapolskiy
> Sent: 11 November 2014 15:12
> To: James Hartley; grant.lik...@linaro.org; robh...@kernel.org;
> a...@linux-foundation.org
> Cc: herb...@gondor.apana.org.au; da...@davemloft.net;
> gre...@linuxfoundation.org; j...@perches.com;
> mche...@osg.samsung.com; cr...@iki.fi; jg1@samsung.com; linux-
> cry...@vger.kernel.org; devicet...@vger.kernel.org;
> pawel.m...@arm.com; mark.rutl...@arm.com;
> ijc+devicet...@hellion.org.uk; ga...@codeaurora.org;
> abres...@chromium.org; Ezequiel Garcia
> Subject: Re: [PATCH 1/2] crypto: Add Imagination Technologies hw hash
> accelerator
>
> Hi James,
>
> On 11.11.2014 16:59, James Hartley wrote:
> > Hi Vladimir, thanks for the review!
> >
> >> -Original Message-
> >> From: Vladimir Zapolskiy [mailto:vladimir_zapols...@mentor.com]
> >> Sent: 10 November 2014 15:10
> >> To: James Hartley; herb...@gondor.apana.org.au;
> da...@davemloft.net;
> >> grant.lik...@linaro.org; robh...@kernel.org;
> >> a...@linux-foundation.org; gre...@linuxfoundation.org;
> >> j...@perches.com; mche...@osg.samsung.com; cr...@iki.fi;
> >> jg1@samsung.com; linux- cry...@vger.kernel.org
> >> Cc: devicet...@vger.kernel.org; pawel.m...@arm.com;
> >> mark.rutl...@arm.com; ijc+devicet...@hellion.org.uk;
> >> ga...@codeaurora.org; abres...@chromium.org; Ezequiel Garcia
> >> Subject: Re: [PATCH 1/2] crypto: Add Imagination Technologies hw hash
> >> accelerator
> >>
> >> Hello James,
> >>
> >> On 10.11.2014 14:10, James Hartley wrote:
> >>> This adds support for the Imagination Technologies hash accelerator
> >>> that provides hardware acceleration for
> >>> SHA1 SHA224 SHA256 and MD5 Hashes.
> >>>
> >>> Signed-off-by: James Hartley
> >>> ---
> >>
>
> [snip]
>
> >>> +
> >>> + return 0;
> >>> +
> >>> +err_algs:
> >>> + spin_lock(&img_hash.lock);
> >>> + list_del(&hdev->list);
> >>> + spin_unlock(&img_hash.lock);
> >>> + dma_release_channel(hdev->dma_lch);
> >>> +err_dma:
> >>> + iounmap(hdev->io_base);
> >>
> >> Mixing of devm_* resource initialization and commodity resource
> >> release leads to double decrement of clock usage count reference.
> >
> > Ok, changed to devm_iounmap
> >
>
> just one small comment, please double check, but most probably you don't
> need to call devm_iounmap() explicitly on error path.
Yes you are right, I will remove them
>
> [snip]
>
> >
> >>
> >>> +
> >>> +static int img_hash_remove(struct platform_device *pdev) {
> >>> + static struct img_hash_dev *hdev;
> >>> +
> >>> + hdev = platform_get_drvdata(pdev);
> >>> + if (!hdev)
> >>> + return -ENODEV;
> >>> + spin_lock(&img_hash.lock);
> >>> + list_del(&hdev->list);
> >>> + spin_unlock(&img_hash.lock);
> >>> +
> >>> + img_unregister_algs(hdev);
> >>> +
> >>> + tasklet_kill(&hdev->done_task);
> >>> + tasklet_kill(&hdev->dma_task);
> >>> + img_hash_dma_cleanup(hdev);
> >>> +
> >>> + iounmap(hdev->io_base);
> >>
> >> Same as above, devres iounmap() is good enough.
> >
> > Done
> >
>
> Same as above, I suppose you can simply remove iounmap() call without
> adding explicit devm_iounmap().
As above.
>
> --
> With best wishes,
> Vladimir
> --
> To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the
> body of a message to majord...@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html
Thanks again for the quick review. I'll post a V2 patchset when I've figured
out
what I can do about the error handling you mentioned previously.
Thanks
James
Re: [PATCH 1/2] crypto: Add Imagination Technologies hw hash accelerator
Hi James,
On 11.11.2014 16:59, James Hartley wrote:
> Hi Vladimir, thanks for the review!
>
>> -Original Message-
>> From: Vladimir Zapolskiy [mailto:vladimir_zapols...@mentor.com]
>> Sent: 10 November 2014 15:10
>> To: James Hartley; herb...@gondor.apana.org.au; da...@davemloft.net;
>> grant.lik...@linaro.org; robh...@kernel.org; a...@linux-foundation.org;
>> gre...@linuxfoundation.org; j...@perches.com;
>> mche...@osg.samsung.com; cr...@iki.fi; jg1@samsung.com; linux-
>> cry...@vger.kernel.org
>> Cc: devicet...@vger.kernel.org; pawel.m...@arm.com;
>> mark.rutl...@arm.com; ijc+devicet...@hellion.org.uk;
>> ga...@codeaurora.org; abres...@chromium.org; Ezequiel Garcia
>> Subject: Re: [PATCH 1/2] crypto: Add Imagination Technologies hw hash
>> accelerator
>>
>> Hello James,
>>
>> On 10.11.2014 14:10, James Hartley wrote:
>>> This adds support for the Imagination Technologies hash accelerator
>>> that provides hardware acceleration for
>>> SHA1 SHA224 SHA256 and MD5 Hashes.
>>>
>>> Signed-off-by: James Hartley
>>> ---
>>
[snip]
>>> +
>>> + return 0;
>>> +
>>> +err_algs:
>>> + spin_lock(&img_hash.lock);
>>> + list_del(&hdev->list);
>>> + spin_unlock(&img_hash.lock);
>>> + dma_release_channel(hdev->dma_lch);
>>> +err_dma:
>>> + iounmap(hdev->io_base);
>>
>> Mixing of devm_* resource initialization and commodity resource release
>> leads to double decrement of clock usage count reference.
>
> Ok, changed to devm_iounmap
>
just one small comment, please double check, but most probably you don't
need to call devm_iounmap() explicitly on error path.
[snip]
>
>>
>>> +
>>> +static int img_hash_remove(struct platform_device *pdev) {
>>> + static struct img_hash_dev *hdev;
>>> +
>>> + hdev = platform_get_drvdata(pdev);
>>> + if (!hdev)
>>> + return -ENODEV;
>>> + spin_lock(&img_hash.lock);
>>> + list_del(&hdev->list);
>>> + spin_unlock(&img_hash.lock);
>>> +
>>> + img_unregister_algs(hdev);
>>> +
>>> + tasklet_kill(&hdev->done_task);
>>> + tasklet_kill(&hdev->dma_task);
>>> + img_hash_dma_cleanup(hdev);
>>> +
>>> + iounmap(hdev->io_base);
>>
>> Same as above, devres iounmap() is good enough.
>
> Done
>
Same as above, I suppose you can simply remove iounmap() call without
adding explicit devm_iounmap().
--
With best wishes,
Vladimir
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] crypto: Add Imagination Technologies hw hash accelerator
Hi Vladimir, thanks for the review!
> -Original Message-
> From: Vladimir Zapolskiy [mailto:vladimir_zapols...@mentor.com]
> Sent: 10 November 2014 15:10
> To: James Hartley; herb...@gondor.apana.org.au; da...@davemloft.net;
> grant.lik...@linaro.org; robh...@kernel.org; a...@linux-foundation.org;
> gre...@linuxfoundation.org; j...@perches.com;
> mche...@osg.samsung.com; cr...@iki.fi; jg1@samsung.com; linux-
> cry...@vger.kernel.org
> Cc: devicet...@vger.kernel.org; pawel.m...@arm.com;
> mark.rutl...@arm.com; ijc+devicet...@hellion.org.uk;
> ga...@codeaurora.org; abres...@chromium.org; Ezequiel Garcia
> Subject: Re: [PATCH 1/2] crypto: Add Imagination Technologies hw hash
> accelerator
>
> Hello James,
>
> On 10.11.2014 14:10, James Hartley wrote:
> > This adds support for the Imagination Technologies hash accelerator
> > that provides hardware acceleration for
> > SHA1 SHA224 SHA256 and MD5 Hashes.
> >
> > Signed-off-by: James Hartley
> > ---
>
> [snip]
>
> > diff --git a/drivers/crypto/img-hash.c b/drivers/crypto/img-hash.c new
> > file mode 100644 index 000..e58c81a
> > --- /dev/null
> > +++ b/drivers/crypto/img-hash.c
> > @@ -0,0 +1,1048 @@
> > +/*
> > +* Copyright (c) 2014 Imagination Technologies
> > +* Author: Will Thomas
> > +*
> > +* This program is free software; you can redistribute it and/or
> > +modify
> > +* it under the terms of the GNU General Public License version 2 as
> > +published
> > +* by the Free Software Foundation.
> > +*
> > +* Interface structure taken from omap-sham driver
> > +*/
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#define MD5_BLOCK_SIZE 64
> > +
> > +#define CR_RESET 0
> > +#define CR_RESET_SET 1
> > +#define CR_RESET_UNSET 0
> > +
> > +#define CR_MESSAGE_LENGTH_H0x4
> > +#define CR_MESSAGE_LENGTH_L0x8
> > +
> > +#defineCR_CONTROL 0xc
>
> Tab symbol instead of space after #define.
Ah ok - fixed.
>
> > +#define CR_CONTROL_BYTE_ORDER_3210 0
> > +#define CR_CONTROL_BYTE_ORDER_0123 1
> > +#define CR_CONTROL_BYTE_ORDER_2310 2
> > +#define CR_CONTROL_BYTE_ORDER_1032 3
> > +#define CR_CONTROL_ALGO_MD50
> > +#define CR_CONTROL_ALGO_SHA1 1
> > +#define CR_CONTROL_ALGO_SHA224 2
> > +#define CR_CONTROL_ALGO_SHA256 3
> > +
>
> [snip]
>
> > +static int img_hash_hw_init(struct img_hash_dev *hdev) {
> > + unsigned long long nbits;
> > + u32 u, l;
> > +
> > + clk_prepare_enable(hdev->iclk);
>
> This call may fail, please add a check.
Ok - check added
>
> > +
> > + img_hash_write(hdev, CR_RESET, CR_RESET_SET);
> > + img_hash_write(hdev, CR_RESET, CR_RESET_UNSET);
> > + img_hash_write(hdev, CR_INTENAB, CR_INT_NEW_RESULTS_SET);
> > +
> > + nbits = (hdev->req->nbytes << 3);
> > + u = nbits >> 32;
> > + l = nbits;
> > + img_hash_write(hdev, CR_MESSAGE_LENGTH_H, u);
> > + img_hash_write(hdev, CR_MESSAGE_LENGTH_L, l);
> > +
> > + if (!(DRIVER_FLAGS_INIT & hdev->flags)) {
> > + hdev->flags |= DRIVER_FLAGS_INIT;
> > + hdev->err = 0;
> > + }
> > + pr_debug("hw initialized, nbits: %llx\n", nbits);
> > + return 0;
> > +}
> > +
>
> [snip]
>
> > +static void img_hash_xmit_dma(struct img_hash_dev *hdev, struct
> > +scatterlist *sg) {
> > + struct dma_async_tx_descriptor *desc;
> > + struct img_hash_request_ctx *ctx = ahash_request_ctx(hdev->req);
> > +
> > + ctx->dma_ct = dma_map_sg(hdev->dev, sg, 1,
> DMA_MEM_TO_DEV);
> > + if (ctx->dma_ct == 0) {
> > + dev_err(hdev->dev, "Invalid DMA sg\n");
> > + hdev->err = -EINVAL;
> > + return;
> > + }
> > +
> > + desc = dmaengine_prep_slave_sg(hdev->dma_lch,
> > + sg,
> > + ctx->dma_ct,
> > + DMA_MEM_TO_DEV,
> > + DMA_PREP_INTERRUPT |
> DMA_CTRL_ACK);
> > + if (!desc) {
> > + dev_err(hdev->dev, "Null DMA descriptor\n");
> &g
Re: [PATCH 1/2] crypto: Add Imagination Technologies hw hash accelerator
Hello James,
On 10.11.2014 14:10, James Hartley wrote:
> This adds support for the Imagination Technologies hash
> accelerator that provides hardware acceleration for
> SHA1 SHA224 SHA256 and MD5 Hashes.
>
> Signed-off-by: James Hartley
> ---
[snip]
> diff --git a/drivers/crypto/img-hash.c b/drivers/crypto/img-hash.c
> new file mode 100644
> index 000..e58c81a
> --- /dev/null
> +++ b/drivers/crypto/img-hash.c
> @@ -0,0 +1,1048 @@
> +/*
> +* Copyright (c) 2014 Imagination Technologies
> +* Author: Will Thomas
> +*
> +* This program is free software; you can redistribute it and/or modify
> +* it under the terms of the GNU General Public License version 2 as published
> +* by the Free Software Foundation.
> +*
> +*Interface structure taken from omap-sham driver
> +*/
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define MD5_BLOCK_SIZE 64
> +
> +#define CR_RESET 0
> +#define CR_RESET_SET 1
> +#define CR_RESET_UNSET 0
> +
> +#define CR_MESSAGE_LENGTH_H 0x4
> +#define CR_MESSAGE_LENGTH_L 0x8
> +
> +#define CR_CONTROL 0xc
Tab symbol instead of space after #define.
> +#define CR_CONTROL_BYTE_ORDER_3210 0
> +#define CR_CONTROL_BYTE_ORDER_0123 1
> +#define CR_CONTROL_BYTE_ORDER_2310 2
> +#define CR_CONTROL_BYTE_ORDER_1032 3
> +#define CR_CONTROL_ALGO_MD5 0
> +#define CR_CONTROL_ALGO_SHA1 1
> +#define CR_CONTROL_ALGO_SHA224 2
> +#define CR_CONTROL_ALGO_SHA256 3
> +
[snip]
> +static int img_hash_hw_init(struct img_hash_dev *hdev)
> +{
> + unsigned long long nbits;
> + u32 u, l;
> +
> + clk_prepare_enable(hdev->iclk);
This call may fail, please add a check.
> +
> + img_hash_write(hdev, CR_RESET, CR_RESET_SET);
> + img_hash_write(hdev, CR_RESET, CR_RESET_UNSET);
> + img_hash_write(hdev, CR_INTENAB, CR_INT_NEW_RESULTS_SET);
> +
> + nbits = (hdev->req->nbytes << 3);
> + u = nbits >> 32;
> + l = nbits;
> + img_hash_write(hdev, CR_MESSAGE_LENGTH_H, u);
> + img_hash_write(hdev, CR_MESSAGE_LENGTH_L, l);
> +
> + if (!(DRIVER_FLAGS_INIT & hdev->flags)) {
> + hdev->flags |= DRIVER_FLAGS_INIT;
> + hdev->err = 0;
> + }
> + pr_debug("hw initialized, nbits: %llx\n", nbits);
> + return 0;
> +}
> +
[snip]
> +static void img_hash_xmit_dma(struct img_hash_dev *hdev, struct scatterlist
> *sg)
> +{
> + struct dma_async_tx_descriptor *desc;
> + struct img_hash_request_ctx *ctx = ahash_request_ctx(hdev->req);
> +
> + ctx->dma_ct = dma_map_sg(hdev->dev, sg, 1, DMA_MEM_TO_DEV);
> + if (ctx->dma_ct == 0) {
> + dev_err(hdev->dev, "Invalid DMA sg\n");
> + hdev->err = -EINVAL;
> + return;
> + }
> +
> + desc = dmaengine_prep_slave_sg(hdev->dma_lch,
> + sg,
> + ctx->dma_ct,
> + DMA_MEM_TO_DEV,
> + DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
> + if (!desc) {
> + dev_err(hdev->dev, "Null DMA descriptor\n");
> + hdev->err = -EINVAL;
Missing dma_unmap_sg()
> + return;
> + }
> + desc->callback = img_hash_dma_callback;
> + desc->callback_param = hdev;
> + dmaengine_submit(desc);
> + dma_async_issue_pending(hdev->dma_lch);
> +}
> +
> +static void img_hash_dma_task(unsigned long d)
> +{
> + struct img_hash_dev *hdev = (struct img_hash_dev *) d;
> + struct img_hash_request_ctx *ctx = ahash_request_ctx(hdev->req);
> + char *addr;
> + size_t nbytes, bleft, bsend, len, tbc;
> + struct scatterlist tsg;
> +
> + if (!ctx->sg)
> + return;
> + if (!hdev)
> + pr_err("invalid ptr for hash device");
> +
> + addr = sg_virt(ctx->sg);
> + nbytes = ctx->sg->length - ctx->offset;
> + bleft = nbytes % 4;
> + bsend = (nbytes / 4);
> +
> +
> + if (bsend) {
> + sg_init_one(&tsg, addr + ctx->offset, bsend * 4);
> + img_hash_xmit_dma(hdev, &tsg);
What happens, if img_hash_xmit_dma() fails?
> + ctx->sent += bsend * 4;
> + }
> +
> + if (bleft) {
> + ctx->bufcnt = sg_pcopy_to_buffer(ctx->sgfirst, ctx->nents,
> + ctx->buffer, bleft, ctx->sent);
> + tbc = 0;
> + ctx->sg = sg_next(ctx->sg);
> + while (ctx->sg && (ctx->bufcnt < 4)) {
> + len = ctx->sg->length;
> + if (likely(len > (4 - ctx->bufcnt)))
> + len = 4 - ctx->bufcnt;
> + tbc = sg_pcopy_to_buffer(ctx->sgfirst, ctx->nents,
> + ctx->buffer + ctx->bufcnt, len,
> + ctx-
