Re: [PATCH v5 4/4] crypto: Add Allwinner Security System crypto accelerator
On Thu, Nov 06, 2014 at 10:32:18PM +0800, Herbert Xu wrote: > On Thu, Nov 06, 2014 at 03:26:33PM +0100, Maxime Ripard wrote: > > > > But you still haven't explain why the driver, while it doesn't handle > > the user space buffer at any time, should be worried that the data the > > framework has given him are actually mapped. > > Encryption is used by IPsec and SKBs can be allocated in highmem. > algif also exposes all ciphers to user-space memory which can also > be in highmem. Ok. We keep going in circles here. I know that algif handles userspace memory that can be in highmem. What I don't get, is that just like a *driver* doesn't have to call copy_from_user, why would it need to call kmap... That's something that should be in the framework itself, not the driver. And the argument that most drivers use DMA seems like a broken assumption. But hey, you're the one that will maintain this mess, so I guess you have the final word. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com signature.asc Description: Digital signature
Re: [PATCH v5 4/4] crypto: Add Allwinner Security System crypto accelerator
On Thu, Nov 06, 2014 at 03:26:33PM +0100, Maxime Ripard wrote: > > But you still haven't explain why the driver, while it doesn't handle > the user space buffer at any time, should be worried that the data the > framework has given him are actually mapped. Encryption is used by IPsec and SKBs can be allocated in highmem. algif also exposes all ciphers to user-space memory which can also be in highmem. 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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v5 4/4] crypto: Add Allwinner Security System crypto accelerator
On Mon, Nov 03, 2014 at 06:35:28PM +0800, Herbert Xu wrote: > On Mon, Nov 03, 2014 at 10:34:46AM +0100, Maxime Ripard wrote: > > What I mean is that since you are saying that drivers should do the > > kmap themselves, then *all* of the drivers are broken if they are not > > using it. And all of them are missing this kmap. > > kmap is used by the software implementations to map the input/output > into virtual address space. Drivers typically use DMA and operate > on physical addresses so they don't need kmap. Yes, plus all memory allocated with GFP_KERNEL is in lowmem. But you still haven't explain why the driver, while it doesn't handle the user space buffer at any time, should be worried that the data the framework has given him are actually mapped. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com signature.asc Description: Digital signature
Re: [PATCH v5 4/4] crypto: Add Allwinner Security System crypto accelerator
On Sun, Oct 19, 2014 at 04:16:22PM +0200, LABBE Corentin wrote:
> Add support for the Security System included in Allwinner SoC A20.
> The Security System is a hardware cryptographic accelerator that support
> AES/MD5/SHA1/DES/3DES/PRNG algorithms.
>
> Signed-off-by: LABBE Corentin
OK this is much better. However it seems that export/import
is still missing?
> + src_addr = kmap_atomic(sg_page(in_sg)) + in_sg->offset;
> + if (src_addr == NULL) {
> + dev_err(ss->dev, "kmap_atomic error for src SG\n");
> + writel(0, ss->base + SS_CTL);
> + mutex_unlock(&ss->lock);
I overlooked this the last time around. You cannot use mutexes
here as you can be called from softirq context so you need spin
locks.
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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v5 4/4] crypto: Add Allwinner Security System crypto accelerator
On Mon, Nov 03, 2014 at 10:34:46AM +0100, Maxime Ripard wrote: > What I mean is that since you are saying that drivers should do the > kmap themselves, then *all* of the drivers are broken if they are not > using it. And all of them are missing this kmap. kmap is used by the software implementations to map the input/output into virtual address space. Drivers typically use DMA and operate on physical addresses so they don't need kmap. 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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v5 4/4] crypto: Add Allwinner Security System crypto accelerator
On Fri, Oct 31, 2014 at 06:05:22PM +0800, Herbert Xu wrote: > On Fri, Oct 31, 2014 at 10:57:06AM +0100, Maxime Ripard wrote: > > > > On a 3.18-rc2 kernel: > > > > $ git grep kmap -- crypto/ > > crypto/ahash.c: walk->data = kmap(walk->pg); > > crypto/ahash.c: walk->data = kmap_atomic(walk->pg); > > crypto/async_tx/async_memcpy.c: dest_buf = kmap_atomic(dest) + > > dest_offset; > > crypto/async_tx/async_memcpy.c: src_buf = kmap_atomic(src) + > > src_offset; > > crypto/scatterwalk.c: return > > kmap_atomic(scatterwalk_page(walk)) + > > crypto/shash.c: data = kmap_atomic(sg_page(sg)); > > crypto/shash.c: data = kmap_atomic(sg_page(sg)); > > > > None of the drivers are. > > What do you mean? It's precisely because the page can be in highmem > that we are mapping it. If it's not in highmem it'll be a noop. What I mean is that since you are saying that drivers should do the kmap themselves, then *all* of the drivers are broken if they are not using it. And all of them are missing this kmap. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com signature.asc Description: Digital signature
Re: [PATCH v5 4/4] crypto: Add Allwinner Security System crypto accelerator
On Fri, Oct 31, 2014 at 10:57:06AM +0100, Maxime Ripard wrote: > > On a 3.18-rc2 kernel: > > $ git grep kmap -- crypto/ > crypto/ahash.c: walk->data = kmap(walk->pg); > crypto/ahash.c: walk->data = kmap_atomic(walk->pg); > crypto/async_tx/async_memcpy.c: dest_buf = kmap_atomic(dest) + > dest_offset; > crypto/async_tx/async_memcpy.c: src_buf = kmap_atomic(src) + > src_offset; > crypto/scatterwalk.c: return > kmap_atomic(scatterwalk_page(walk)) + > crypto/shash.c: data = kmap_atomic(sg_page(sg)); > crypto/shash.c: data = kmap_atomic(sg_page(sg)); > > None of the drivers are. What do you mean? It's precisely because the page can be in highmem that we are mapping it. If it's not in highmem it'll be a noop. Admittedly I haven't tested highmem since moving over to x86-64 some years ago, but it definitely used to work on x86-32. 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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v5 4/4] crypto: Add Allwinner Security System crypto accelerator
On Fri, Oct 31, 2014 at 04:18:03PM +0800, Herbert Xu wrote: > On Fri, Oct 31, 2014 at 09:13:23AM +0100, Maxime Ripard wrote: > > > > I don't understand here. Why would other drivers *not* being affected? > > > > If the scatter list passed by AF_ALG can be in highmem, I guess it's > > the case for every driver out there. Almost every kernel code I've > > seen so far makes the assumption that the memory it has is mapped and > > accessible. > > > > Somehow, it's the driver's fault now, and not the part of kernel that > > actually does the allocation? > > If you are implementing a crypto driver that is meant to handle > requests from the crypto API then yes you need to handle highmem. Is that documented somewhere? > As I said if enough drivers are unable to address highmem and > require copying/software fallbacks then we could provide this > through the API and the driver would then only need to declare > its lack of highmem support or use a helper. On a 3.18-rc2 kernel: $ git grep kmap -- crypto/ crypto/ahash.c: walk->data = kmap(walk->pg); crypto/ahash.c: walk->data = kmap_atomic(walk->pg); crypto/async_tx/async_memcpy.c: dest_buf = kmap_atomic(dest) + dest_offset; crypto/async_tx/async_memcpy.c: src_buf = kmap_atomic(src) + src_offset; crypto/scatterwalk.c: return kmap_atomic(scatterwalk_page(walk)) + crypto/shash.c: data = kmap_atomic(sg_page(sg)); crypto/shash.c: data = kmap_atomic(sg_page(sg)); None of the drivers are. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com signature.asc Description: Digital signature
Re: [PATCH v5 4/4] crypto: Add Allwinner Security System crypto accelerator
On Fri, Oct 31, 2014 at 09:13:23AM +0100, Maxime Ripard wrote: > > I don't understand here. Why would other drivers *not* being affected? > > If the scatter list passed by AF_ALG can be in highmem, I guess it's > the case for every driver out there. Almost every kernel code I've > seen so far makes the assumption that the memory it has is mapped and > accessible. > > Somehow, it's the driver's fault now, and not the part of kernel that > actually does the allocation? If you are implementing a crypto driver that is meant to handle requests from the crypto API then yes you need to handle highmem. As I said if enough drivers are unable to address highmem and require copying/software fallbacks then we could provide this through the API and the driver would then only need to declare its lack of highmem support or use a helper. 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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v5 4/4] crypto: Add Allwinner Security System crypto accelerator
On Fri, Oct 31, 2014 at 03:20:30PM +0800, Herbert Xu wrote: > On Thu, Oct 30, 2014 at 06:19:33PM +0100, Maxime Ripard wrote: > > > > > With AF_ALG and cryptodev, the SG is in highmem. Verified with some > > > PageHighMem(). > > > > Then fix AF_ALG and cryptodev, because all of the other drivers might > > be affected. > > No it's the driver that needs to be fixed. Of course if there > are enough drivers it may be worthwhile adding either copying or > a software fallback for highmem requests. I don't understand here. Why would other drivers *not* being affected? If the scatter list passed by AF_ALG can be in highmem, I guess it's the case for every driver out there. Almost every kernel code I've seen so far makes the assumption that the memory it has is mapped and accessible. Somehow, it's the driver's fault now, and not the part of kernel that actually does the allocation? -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com signature.asc Description: Digital signature
Re: [PATCH v5 4/4] crypto: Add Allwinner Security System crypto accelerator
On Thu, Oct 30, 2014 at 06:19:33PM +0100, Maxime Ripard wrote: > > > With AF_ALG and cryptodev, the SG is in highmem. Verified with some > > PageHighMem(). > > Then fix AF_ALG and cryptodev, because all of the other drivers might > be affected. No it's the driver that needs to be fixed. Of course if there are enough drivers it may be worthwhile adding either copying or a software fallback for highmem requests. 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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v5 4/4] crypto: Add Allwinner Security System crypto accelerator
On Fri, Oct 24, 2014 at 08:52:26PM +0200, Corentin LABBE wrote:
> On 10/21/14 21:11, Maxime Ripard wrote:
> > Hi Corentin,
> >
> > Thanks for resending it.
> >
> > On Sun, Oct 19, 2014 at 04:16:22PM +0200, LABBE Corentin wrote:
> >> Add support for the Security System included in Allwinner SoC A20.
> >> The Security System is a hardware cryptographic accelerator that support
> >> AES/MD5/SHA1/DES/3DES/PRNG algorithms.
> >>
> >> Signed-off-by: LABBE Corentin
> >> ---
> >> drivers/crypto/Kconfig| 17 ++
> >> +static int sunxi_ss_aes_poll_atomic(struct ablkcipher_request *areq)
> >> +{
> >> + u32 spaces;
> >> + struct scatterlist *in_sg = areq->src;
> >> + struct scatterlist *out_sg = areq->dst;
> >> + void *src_addr;
> >> + void *dst_addr;
> >> + unsigned int ileft = areq->nbytes;
> >> + unsigned int oleft = areq->nbytes;
> >> + unsigned int todo;
> >> + u32 *src32;
> >> + u32 *dst32;
> >> + u32 rx_cnt = 32;
> >> + u32 tx_cnt = 0;
> >> + int i;
> >> +
> >> + src_addr = kmap_atomic(sg_page(in_sg)) + in_sg->offset;
> >
> > Where does this scatter_list is coming from? Can it even be allocated
> > in highmem?
> >
>
> With AF_ALG and cryptodev, the SG is in highmem. Verified with some
> PageHighMem().
Then fix AF_ALG and cryptodev, because all of the other drivers might
be affected.
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
signature.asc
Description: Digital signature
Re: [PATCH v5 4/4] crypto: Add Allwinner Security System crypto accelerator
On 10/21/14 21:11, Maxime Ripard wrote:
> Hi Corentin,
>
> Thanks for resending it.
>
> On Sun, Oct 19, 2014 at 04:16:22PM +0200, LABBE Corentin wrote:
>> Add support for the Security System included in Allwinner SoC A20.
>> The Security System is a hardware cryptographic accelerator that support
>> AES/MD5/SHA1/DES/3DES/PRNG algorithms.
>>
>> Signed-off-by: LABBE Corentin
>> ---
>> drivers/crypto/Kconfig| 17 ++
>> +static int sunxi_ss_aes_poll_atomic(struct ablkcipher_request *areq)
>> +{
>> +u32 spaces;
>> +struct scatterlist *in_sg = areq->src;
>> +struct scatterlist *out_sg = areq->dst;
>> +void *src_addr;
>> +void *dst_addr;
>> +unsigned int ileft = areq->nbytes;
>> +unsigned int oleft = areq->nbytes;
>> +unsigned int todo;
>> +u32 *src32;
>> +u32 *dst32;
>> +u32 rx_cnt = 32;
>> +u32 tx_cnt = 0;
>> +int i;
>> +
>> +src_addr = kmap_atomic(sg_page(in_sg)) + in_sg->offset;
>
> Where does this scatter_list is coming from? Can it even be allocated
> in highmem?
>
With AF_ALG and cryptodev, the SG is in highmem. Verified with some
PageHighMem().
>> +if (src_addr == NULL) {
>> +dev_err(ss->dev, "kmap_atomic error for src SG\n");
>> +writel(0, ss->base + SS_CTL);
>> +mutex_unlock(&ss->lock);
>> +return -EINVAL;
>> +}
>> +
>> +dst_addr = kmap_atomic(sg_page(out_sg)) + out_sg->offset;
>> +if (dst_addr == NULL) {
>> +dev_err(ss->dev, "kmap_atomic error for dst SG\n");
>> +writel(0, ss->base + SS_CTL);
>> +kunmap_atomic(src_addr);
>> +mutex_unlock(&ss->lock);
>> +return -EINVAL;
>
> Please use gotos in your error path.
>
Ok
>> +}
>> +
>> +src32 = (u32 *)src_addr;
>> +dst32 = (u32 *)dst_addr;
>> +ileft = areq->nbytes / 4;
>> +oleft = areq->nbytes / 4;
>> +i = 0;
>> +do {
>> +if (ileft > 0 && rx_cnt > 0) {
>> +todo = min(rx_cnt, ileft);
>> +ileft -= todo;
>> +do {
>> +writel_relaxed(*src32++,
>
> Please put some braces around that referencing/increment.
>
Ok
>> +ss->base +
>> +SS_RXFIFO);
>> +todo--;
>> +} while (todo > 0);
>> +}
>> +if (tx_cnt > 0) {
>> +todo = min(tx_cnt, oleft);
>> +oleft -= todo;
>> +do {
>> +*dst32++ = readl_relaxed(ss->base +
>> +SS_TXFIFO);
>> +todo--;
>> +} while (todo > 0);
>> +}
>> +spaces = readl_relaxed(ss->base + SS_FCSR);
>> +rx_cnt = SS_RXFIFO_SPACES(spaces);
>> +tx_cnt = SS_TXFIFO_SPACES(spaces);
>> +} while (oleft > 0);
>> +writel(0, ss->base + SS_CTL);
>> +kunmap_atomic(src_addr);
>> +kunmap_atomic(dst_addr);
>> +mutex_unlock(&ss->lock);
>
> You never took that mutex in that function...
>
Solved with comment from Vladimir Zapolskiy
>> +return 0;
[]
>> +int no_chunk = 1;
>> +struct scatterlist *in_sg = areq->src;
>> +struct scatterlist *out_sg = areq->dst;
>> +
>> +/*
>> + * if we have only SGs with size multiple of 4,
>> + * we can use the SS AES function
>> + */
>> +while (in_sg != NULL && no_chunk == 1) {
>> +if ((in_sg->length % 4) != 0)
>> +no_chunk = 0;
>> +in_sg = sg_next(in_sg);
>> +}
>> +while (out_sg != NULL && no_chunk == 1) {
>> +if ((out_sg->length % 4) != 0)
>> +no_chunk = 0;
>> +out_sg = sg_next(out_sg);
>> +}
>> +
>> +if (no_chunk == 1)
>> +return sunxi_ss_aes_poll(areq, mode);
>> +
>> +in_sg = areq->src;
>> +out_sg = areq->dst;
>> +
>> +nb_in_sg_rx = sg_nents(in_sg);
>> +nb_in_sg_tx = sg_nents(out_sg);
>> +
>> +/*
>> + * buf_in and buf_out are allocated only one time
>> + * then we keep the buffer until driver end
>> + * the allocation can only grow more
>> + * we do not reduce it for simplification
>> + */
>> +mutex_lock(&ss->bufin_lock);
>> +if (ss->buf_in == NULL) {
>> +ss->buf_in = kmalloc(areq->nbytes, GFP_KERNEL);
>> +ss->buf_in_size = areq->nbytes;
>> +} else {
>> +if (areq->nbytes > ss->buf_in_size) {
>> +kfree(ss->buf_in);
>> +ss->buf_in = kmalloc(areq->nbytes, GFP_KERNEL);
>> +ss->buf_in_size = areq->nbytes;
>> +}
>> +}
>> +if (ss->buf_in == NULL) {
>> +ss->buf_in_size = 0;
>> +mutex_unlock(&ss->bufin_lock);
>> +dev_err(ss->dev, "Unable to allocate pages.\n");
>
Re: [linux-sunxi] Re: [PATCH v5 4/4] crypto: Add Allwinner Security System crypto accelerator
Le 22/10/2014 11:00, Arnd Bergmann a écrit :
> On Sunday 19 October 2014 16:16:22 LABBE Corentin wrote:
>> Add support for the Security System included in Allwinner SoC A20.
>> The Security System is a hardware cryptographic accelerator that support
>> AES/MD5/SHA1/DES/3DES/PRNG algorithms.
>>
>> Signed-off-by: LABBE Corentin
>
> Please wrap lines in the changelog after about 70 characters.
>
Oups I just see the corresponding part in submittingpatches.txt
Sorry
>> --- /dev/null
>> +++ b/drivers/crypto/sunxi-ss/sunxi-ss-cipher.c
>> @@ -0,0 +1,489 @@
>
>> +#include "sunxi-ss.h"
>> +
>> +extern struct sunxi_ss_ctx *ss;
>
> 'extern' declarations belong into header files, not .c files. It would
> be even better to avoid this completely and carry the pointer to the
> context in an object that gets passed around. In general we want drivers
> to be written in a way that allows having multiple instances of the
> device, which the global pointer prevents.
>
As I already said I think the driver will never be used with multiple instance.
But since many people want this pointer dead, I will work on it.
>> +
>> +src32 = (u32 *)src_addr;
>> +dst32 = (u32 *)dst_addr;
>
>
> You appear to be missing '__iomem' annotations for the mmio pointers.
> Please always run your code through the 'sparse' checker using 'make C=1'
> to catch and fix this and other erros.
>
Ok, but with which version of sparse do you have such a warning. I use the
0.5.0 version and I got no warning at all.
>> +ileft = areq->nbytes / 4;
>> +oleft = areq->nbytes / 4;
>> +i = 0;
>> +do {
>> +if (ileft > 0 && rx_cnt > 0) {
>> +todo = min(rx_cnt, ileft);
>> +ileft -= todo;
>> +do {
>> +writel_relaxed(*src32++,
>> +ss->base +
>> +SS_RXFIFO);
>> +todo--;
>> +} while (todo > 0);
>> +}
>
> This looks like it should be using writesl() instead of the
> writel_relaxed() loop. That should not only be faster but it will
> also change the byte ordering if you are running a big-endian
> kernel.
>
> Since this is a FIFO register, the ordering that writesl uses
> is likely the correct one.
Great, the code is much cleaner with it. (with up to 10% speed gain)
Thanks
Corentin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v5 4/4] crypto: Add Allwinner Security System crypto accelerator
On Sunday 19 October 2014 16:16:22 LABBE Corentin wrote:
> Add support for the Security System included in Allwinner SoC A20.
> The Security System is a hardware cryptographic accelerator that support
> AES/MD5/SHA1/DES/3DES/PRNG algorithms.
>
> Signed-off-by: LABBE Corentin
Please wrap lines in the changelog after about 70 characters.
> --- /dev/null
> +++ b/drivers/crypto/sunxi-ss/sunxi-ss-cipher.c
> @@ -0,0 +1,489 @@
> +#include "sunxi-ss.h"
> +
> +extern struct sunxi_ss_ctx *ss;
'extern' declarations belong into header files, not .c files. It would
be even better to avoid this completely and carry the pointer to the
context in an object that gets passed around. In general we want drivers
to be written in a way that allows having multiple instances of the
device, which the global pointer prevents.
> +
> + src32 = (u32 *)src_addr;
> + dst32 = (u32 *)dst_addr;
You appear to be missing '__iomem' annotations for the mmio pointers.
Please always run your code through the 'sparse' checker using 'make C=1'
to catch and fix this and other erros.
> + ileft = areq->nbytes / 4;
> + oleft = areq->nbytes / 4;
> + i = 0;
> + do {
> + if (ileft > 0 && rx_cnt > 0) {
> + todo = min(rx_cnt, ileft);
> + ileft -= todo;
> + do {
> + writel_relaxed(*src32++,
> + ss->base +
> + SS_RXFIFO);
> + todo--;
> + } while (todo > 0);
> + }
This looks like it should be using writesl() instead of the
writel_relaxed() loop. That should not only be faster but it will
also change the byte ordering if you are running a big-endian
kernel.
Since this is a FIFO register, the ordering that writesl uses
is likely the correct one.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v5 4/4] crypto: Add Allwinner Security System crypto accelerator
Hi Corentin,
Thanks for resending it.
On Sun, Oct 19, 2014 at 04:16:22PM +0200, LABBE Corentin wrote:
> Add support for the Security System included in Allwinner SoC A20.
> The Security System is a hardware cryptographic accelerator that support
> AES/MD5/SHA1/DES/3DES/PRNG algorithms.
>
> Signed-off-by: LABBE Corentin
> ---
> drivers/crypto/Kconfig| 17 ++
> drivers/crypto/Makefile | 1 +
> drivers/crypto/sunxi-ss/Makefile | 2 +
> drivers/crypto/sunxi-ss/sunxi-ss-cipher.c | 489
> ++
> drivers/crypto/sunxi-ss/sunxi-ss-core.c | 318 +++
> drivers/crypto/sunxi-ss/sunxi-ss-hash.c | 445 +++
> drivers/crypto/sunxi-ss/sunxi-ss.h| 193
> 7 files changed, 1465 insertions(+)
> create mode 100644 drivers/crypto/sunxi-ss/Makefile
> create mode 100644 drivers/crypto/sunxi-ss/sunxi-ss-cipher.c
> create mode 100644 drivers/crypto/sunxi-ss/sunxi-ss-core.c
> create mode 100644 drivers/crypto/sunxi-ss/sunxi-ss-hash.c
> create mode 100644 drivers/crypto/sunxi-ss/sunxi-ss.h
>
> diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
> index 2fb0fdf..9ba9759 100644
> --- a/drivers/crypto/Kconfig
> +++ b/drivers/crypto/Kconfig
> @@ -436,4 +436,21 @@ config CRYPTO_DEV_QCE
> hardware. To compile this driver as a module, choose M here. The
> module will be called qcrypto.
>
> +config CRYPTO_DEV_SUNXI_SS
> + tristate "Support for Allwinner Security System cryptographic
> accelerator"
> + depends on ARCH_SUNXI
> + select CRYPTO_MD5
> + select CRYPTO_SHA1
> + select CRYPTO_AES
> + select CRYPTO_DES
> + select CRYPTO_BLKCIPHER
> + help
> + Some Allwinner SoC have a crypto accelerator named
> + Security System. Select this if you want to use it.
> + The Security System handle AES/DES/3DES ciphers in CBC mode
> + and SHA1 and MD5 hash algorithms.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called sunxi-ss.
> +
> endif # CRYPTO_HW
> diff --git a/drivers/crypto/Makefile b/drivers/crypto/Makefile
> index 3924f93..856545c 100644
> --- a/drivers/crypto/Makefile
> +++ b/drivers/crypto/Makefile
> @@ -25,3 +25,4 @@ obj-$(CONFIG_CRYPTO_DEV_TALITOS) += talitos.o
> obj-$(CONFIG_CRYPTO_DEV_UX500) += ux500/
> obj-$(CONFIG_CRYPTO_DEV_QAT) += qat/
> obj-$(CONFIG_CRYPTO_DEV_QCE) += qce/
> +obj-$(CONFIG_CRYPTO_DEV_SUNXI_SS) += sunxi-ss/
> diff --git a/drivers/crypto/sunxi-ss/Makefile
> b/drivers/crypto/sunxi-ss/Makefile
> new file mode 100644
> index 000..8bb287d
> --- /dev/null
> +++ b/drivers/crypto/sunxi-ss/Makefile
> @@ -0,0 +1,2 @@
> +obj-$(CONFIG_CRYPTO_DEV_SUNXI_SS) += sunxi-ss.o
> +sunxi-ss-y += sunxi-ss-core.o sunxi-ss-hash.o sunxi-ss-cipher.o
> diff --git a/drivers/crypto/sunxi-ss/sunxi-ss-cipher.c
> b/drivers/crypto/sunxi-ss/sunxi-ss-cipher.c
> new file mode 100644
> index 000..8d0416e
> --- /dev/null
> +++ b/drivers/crypto/sunxi-ss/sunxi-ss-cipher.c
> @@ -0,0 +1,489 @@
> +/*
> + * sunxi-ss-cipher.c - hardware cryptographic accelerator for Allwinner A20
> SoC
> + *
> + * Copyright (C) 2013-2014 Corentin LABBE
> + *
> + * This file add support for AES cipher with 128,192,256 bits
> + * keysize in CBC mode.
> + * Add support also for DES and 3DES in CBC mode.
> + *
> + * You could find the datasheet in Documentation/arm/sunxi/README
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#include "sunxi-ss.h"
> +
> +extern struct sunxi_ss_ctx *ss;
> +
> +static int sunxi_ss_cipher(struct ablkcipher_request *areq, u32 mode)
> +{
> + struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(areq);
> + struct sunxi_tfm_ctx *op = crypto_ablkcipher_ctx(tfm);
> + const char *cipher_type;
> +
> + if (areq->nbytes == 0)
> + return 0;
> +
> + if (areq->info == NULL) {
> + dev_err(ss->dev, "ERROR: Empty IV\n");
> + return -EINVAL;
> + }
> +
> + if (areq->src == NULL || areq->dst == NULL) {
> + dev_err(ss->dev, "ERROR: Some SGs are NULL\n");
> + return -EINVAL;
> + }
> +
> + cipher_type = crypto_tfm_alg_name(crypto_ablkcipher_tfm(tfm));
> +
> + if (strcmp("cbc(aes)", cipher_type) == 0) {
> + mode |= SS_OP_AES | SS_CBC | SS_ENABLED | op->keymode;
> + return sunxi_ss_aes_poll(areq, mode);
> + }
> +
> + if (strcmp("cbc(des)", cipher_type) == 0) {
> + mode |= SS_OP_DES | SS_CBC | SS_ENABLED | op->keymode;
> + return sunxi_ss_des_poll(areq, mode);
> + }
> +
> + if (strcmp("cbc(des3_ede)", cipher_type) == 0) {
> + mode |= SS_OP_3DES | SS_CBC | SS_ENABLED | op->keymode
Re: [PATCH v5 4/4] crypto: Add Allwinner Security System crypto accelerator
Hi Corentin,
On 21.10.2014 19:25, Corentin LABBE wrote:
> On 10/21/14 01:28, Vladimir Zapolskiy wrote:
>> Hello LABBE,
>>
>> On 19.10.2014 17:16, LABBE Corentin wrote:
>>> Add support for the Security System included in Allwinner SoC A20.
>>> The Security System is a hardware cryptographic accelerator that support
>>> AES/MD5/SHA1/DES/3DES/PRNG algorithms.
>>>
> []
>>> +
>>> + /* If we have only one SG, we can use kmap_atomic */
>>> + if (sg_next(in_sg) == NULL && sg_next(out_sg) == NULL)
>>> + return sunxi_ss_aes_poll_atomic(areq);
>>
>> for clarity it might be better to move all "mutex_unlock(&ss->lock)"
>> calls from sunxi_ss_aes_poll_atomic() body right to here.
>>
>
> Ok
> I have moved all mutex_unlock/writel(0, SS_CTL) at the end of function, it is
> cleaner now.
please check that sunxi_ss_aes_poll_atomic() has no more mutex_unlock()
calls inside it.
With best wishes,
Vladimir
>>> +
>>> +};
>>> +
>>> +static int sunxi_ss_probe(struct platform_device *pdev)
>>> +{
>>> + struct resource *res;
>>> + u32 v;
>>> + int err;
>>> + unsigned long cr;
>>> + const unsigned long cr_ahb = 24 * 1000 * 1000;
>>> + const unsigned long cr_mod = 150 * 1000 * 1000;
>>> +
>>> + if (!pdev->dev.of_node)
>>> + return -ENODEV;
>>> +
>>> + ss = devm_kzalloc(&pdev->dev, sizeof(*ss), GFP_KERNEL);
>>> + if (ss == NULL)
>>> + return -ENOMEM;
>>
>> Why do you dynamically allocate memory for "struct sunxi_ss_ctx *ss"?
>> Since you have a single global pointer, it makes sense to declare
>> "struct sunxi_ss_ctx ss" statically instead.
>>
>> And even a better solution is to remove a single global pointer.
>
> All other crypto driver I have read use a global structure and it made things
> easy.
> Thanks to M. Ripard that pointed to me the talitos driver that solve the
> global device pointer by using alg template and container_of().
>
> But since I think there will never 2 Security System at the same time on the
> same SoC, I do not know if it is worth the cost to add more complexity just
> to remove a pointer.
>
>>
>>> +
>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> + ss->base = devm_ioremap_resource(&pdev->dev, res);
>>> + if (IS_ERR(ss->base)) {
>>> + dev_err(&pdev->dev, "Cannot request MMIO\n");
>>> + return PTR_ERR(ss->base);
>>> + }
>>> +
>>> + ss->ssclk = devm_clk_get(&pdev->dev, "mod");
>>> + if (IS_ERR(ss->ssclk)) {
>>> + err = PTR_ERR(ss->ssclk);
>>> + dev_err(&pdev->dev, "Cannot get SS clock err=%d\n", err);
>>> + return err;
>>> + }
>>> + dev_dbg(&pdev->dev, "clock ss acquired\n");
>>> +
>>> + ss->busclk = devm_clk_get(&pdev->dev, "ahb");
>>> + if (IS_ERR(ss->busclk)) {
>>> + err = PTR_ERR(ss->busclk);
>>> + dev_err(&pdev->dev, "Cannot get AHB SS clock err=%d\n", err);
>>> + return err;
>>> + }
>>> + dev_dbg(&pdev->dev, "clock ahb_ss acquired\n");
>>> +
>>> + /* Enable both clocks */
>>> + err = clk_prepare_enable(ss->busclk);
>>> + if (err != 0) {
>>> + dev_err(&pdev->dev, "Cannot prepare_enable busclk\n");
>>> + return err;
>>> + }
>>> + err = clk_prepare_enable(ss->ssclk);
>>> + if (err != 0) {
>>> + dev_err(&pdev->dev, "Cannot prepare_enable ssclk\n");
>>> + clk_disable_unprepare(ss->busclk);
>>
>> goto somewhere to the end of the function?
>
> OK
>
>>
>>> + return err;
>>> + }
>>> +
>>> + /*
>>> +* Check that clock have the correct rates gived in the datasheet
>>> +* Try to set the clock to the maximum allowed
>>> +*/
>>> + err = clk_set_rate(ss->ssclk, cr_mod);
>>> + if (err != 0) {
>>> + dev_err(&pdev->dev, "Cannot set clock rate to ssclk\n");
>>> + clk_disable_unprepare(ss->ssclk);
>>> + clk_disable_unprepare(ss->busclk);
>>
>> goto "error_md5"?
>
> Ok
>
>>
>>> + return err;
>>> + }
>>> +
>>> + cr = clk_get_rate(ss->busclk);
>>> + if (cr >= cr_ahb)
>>> + dev_dbg(&pdev->dev, "Clock bus %lu (%lu MHz) (must be >=
>>> %lu)\n",
>>> + cr, cr / 100, cr_ahb);
>>> + else
>>> + dev_warn(&pdev->dev, "Clock bus %lu (%lu MHz) (must be >=
>>> %lu)\n",
>>> + cr, cr / 100, cr_ahb);
>>
>> See next comment.
>>
>>> + cr = clk_get_rate(ss->ssclk);
>>> + if (cr <= cr_mod)
>>> + if (cr < cr_mod)
>>> + dev_info(&pdev->dev, "Clock ss %lu (%lu MHz) (must be
>>> <= %lu)\n",
>>> + cr, cr / 100, cr_mod);
>>> + else
>>> + dev_dbg(&pdev->dev, "Clock ss %lu (%lu MHz) (must be <=
>>> %lu)\n",
>>> + cr, cr / 100, cr_mod);
>>> + else
>>> + dev_warn(&pdev->dev, "Clock ss is at %lu (%lu MHz) (must be <=
>>> %lu)\n",
>>> + cr, cr / 100, cr_mod);
>>
>> The management of kernel l
Re: [PATCH v5 4/4] crypto: Add Allwinner Security System crypto accelerator
Le 21/10/2014 01:52, Joe Perches a écrit :
> On Tue, 2014-10-21 at 02:28 +0300, Vladimir Zapolskiy wrote:
>> On 19.10.2014 17:16, LABBE Corentin wrote:
>>> Add support for the Security System included in Allwinner SoC A20.
>>> The Security System is a hardware cryptographic accelerator that support
>>> AES/MD5/SHA1/DES/3DES/PRNG algorithms.
> []
>>> diff --git a/drivers/crypto/sunxi-ss/sunxi-ss-core.c
>>> b/drivers/crypto/sunxi-ss/sunxi-ss-core.c
> []
>>> + cr = clk_get_rate(ss->busclk);
>>> + if (cr >= cr_ahb)
>>> + dev_dbg(&pdev->dev, "Clock bus %lu (%lu MHz) (must be >=
>>> %lu)\n",
>>> + cr, cr / 100, cr_ahb);
>>> + else
>>> + dev_warn(&pdev->dev, "Clock bus %lu (%lu MHz) (must be >=
>>> %lu)\n",
>>> + cr, cr / 100, cr_ahb);
>>
>> See next comment.
>>
>>> + cr = clk_get_rate(ss->ssclk);
>>> + if (cr <= cr_mod)
>>> + if (cr < cr_mod)
>>> + dev_info(&pdev->dev, "Clock ss %lu (%lu MHz) (must be
>>> <= %lu)\n",
>>> + cr, cr / 100, cr_mod);
>>> + else
>>> + dev_dbg(&pdev->dev, "Clock ss %lu (%lu MHz) (must be <=
>>> %lu)\n",
>>> + cr, cr / 100, cr_mod);
>>> + else
>>> + dev_warn(&pdev->dev, "Clock ss is at %lu (%lu MHz) (must be <=
>>> %lu)\n",
>>> + cr, cr / 100, cr_mod);
>>
>> The management of kernel log levels looks pretty strange. As far as I
>> understand there is no error on any clock rate, I'd recommend to keep
>> only one information message.
>
> And if not, please add some braces.
>
>> hash_init: initialize request context */
>>> +int sunxi_hash_init(struct ahash_request *areq)
>>> +{
>>> + const char *hash_type;
>>> + struct sunxi_req_ctx *op = ahash_request_ctx(areq);
>>> +
>>> + memset(op, 0, sizeof(struct sunxi_req_ctx));
>>> +
>>> + hash_type = crypto_tfm_alg_name(areq->base.tfm);
>>> +
>>> + if (strcmp(hash_type, "sha1") == 0)
>>> + op->mode = SS_OP_SHA1;
>>> + if (strcmp(hash_type, "md5") == 0)
>>> + op->mode = SS_OP_MD5;
>
> else if ?
>
>>> + if (op->mode == 0)
>>> + return -EINVAL;
>
> maybe this?
>
> if (!strcmp(hash_type, "sha1"))
> op->mode = SS_OP_SHA1;
> else if (!strcmp(hash_type, "md5"))
> op->mode = SH_OP_MD5;
> else
> return -EINVAL;
>
Ok it is better
>>> +
>>> + return 0;
>>> +}
> []
>>> +int sunxi_hash_update(struct ahash_request *areq)
>>> +{
> []
>>> + dev_dbg(ss->dev, "%s %s bc=%llu len=%u mode=%x bw=%u ww=%u",
>>> + __func__, crypto_tfm_alg_name(areq->base.tfm),
>>> + op->byte_count, areq->nbytes, op->mode,
>>> + op->nbw, op->nwait);
>
> dev_dbg statements generally don't need __func__ as
> dynamic_debug can add it.
>
> If you want to keep it, the most common output form for
> __func__ is '"%s: ...", __func__'
>
It is a big debug that I forgot to remove but I fixed that in other dev_dbg
thanks
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v5 4/4] crypto: Add Allwinner Security System crypto accelerator
On 10/21/14 01:28, Vladimir Zapolskiy wrote:
> Hello LABBE,
>
> On 19.10.2014 17:16, LABBE Corentin wrote:
>> Add support for the Security System included in Allwinner SoC A20.
>> The Security System is a hardware cryptographic accelerator that support
>> AES/MD5/SHA1/DES/3DES/PRNG algorithms.
>>
[]
>> +
>> +/* If we have only one SG, we can use kmap_atomic */
>> +if (sg_next(in_sg) == NULL && sg_next(out_sg) == NULL)
>> +return sunxi_ss_aes_poll_atomic(areq);
>
> for clarity it might be better to move all "mutex_unlock(&ss->lock)"
> calls from sunxi_ss_aes_poll_atomic() body right to here.
>
Ok
I have moved all mutex_unlock/writel(0, SS_CTL) at the end of function, it is
cleaner now.
>> +
>> +};
>> +
>> +static int sunxi_ss_probe(struct platform_device *pdev)
>> +{
>> +struct resource *res;
>> +u32 v;
>> +int err;
>> +unsigned long cr;
>> +const unsigned long cr_ahb = 24 * 1000 * 1000;
>> +const unsigned long cr_mod = 150 * 1000 * 1000;
>> +
>> +if (!pdev->dev.of_node)
>> +return -ENODEV;
>> +
>> +ss = devm_kzalloc(&pdev->dev, sizeof(*ss), GFP_KERNEL);
>> +if (ss == NULL)
>> +return -ENOMEM;
>
> Why do you dynamically allocate memory for "struct sunxi_ss_ctx *ss"?
> Since you have a single global pointer, it makes sense to declare
> "struct sunxi_ss_ctx ss" statically instead.
>
> And even a better solution is to remove a single global pointer.
All other crypto driver I have read use a global structure and it made things
easy.
Thanks to M. Ripard that pointed to me the talitos driver that solve the global
device pointer by using alg template and container_of().
But since I think there will never 2 Security System at the same time on the
same SoC, I do not know if it is worth the cost to add more complexity just to
remove a pointer.
>
>> +
>> +res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +ss->base = devm_ioremap_resource(&pdev->dev, res);
>> +if (IS_ERR(ss->base)) {
>> +dev_err(&pdev->dev, "Cannot request MMIO\n");
>> +return PTR_ERR(ss->base);
>> +}
>> +
>> +ss->ssclk = devm_clk_get(&pdev->dev, "mod");
>> +if (IS_ERR(ss->ssclk)) {
>> +err = PTR_ERR(ss->ssclk);
>> +dev_err(&pdev->dev, "Cannot get SS clock err=%d\n", err);
>> +return err;
>> +}
>> +dev_dbg(&pdev->dev, "clock ss acquired\n");
>> +
>> +ss->busclk = devm_clk_get(&pdev->dev, "ahb");
>> +if (IS_ERR(ss->busclk)) {
>> +err = PTR_ERR(ss->busclk);
>> +dev_err(&pdev->dev, "Cannot get AHB SS clock err=%d\n", err);
>> +return err;
>> +}
>> +dev_dbg(&pdev->dev, "clock ahb_ss acquired\n");
>> +
>> +/* Enable both clocks */
>> +err = clk_prepare_enable(ss->busclk);
>> +if (err != 0) {
>> +dev_err(&pdev->dev, "Cannot prepare_enable busclk\n");
>> +return err;
>> +}
>> +err = clk_prepare_enable(ss->ssclk);
>> +if (err != 0) {
>> +dev_err(&pdev->dev, "Cannot prepare_enable ssclk\n");
>> +clk_disable_unprepare(ss->busclk);
>
> goto somewhere to the end of the function?
OK
>
>> +return err;
>> +}
>> +
>> +/*
>> + * Check that clock have the correct rates gived in the datasheet
>> + * Try to set the clock to the maximum allowed
>> + */
>> +err = clk_set_rate(ss->ssclk, cr_mod);
>> +if (err != 0) {
>> +dev_err(&pdev->dev, "Cannot set clock rate to ssclk\n");
>> +clk_disable_unprepare(ss->ssclk);
>> +clk_disable_unprepare(ss->busclk);
>
> goto "error_md5"?
Ok
>
>> +return err;
>> +}
>> +
>> +cr = clk_get_rate(ss->busclk);
>> +if (cr >= cr_ahb)
>> +dev_dbg(&pdev->dev, "Clock bus %lu (%lu MHz) (must be >=
>> %lu)\n",
>> +cr, cr / 100, cr_ahb);
>> +else
>> +dev_warn(&pdev->dev, "Clock bus %lu (%lu MHz) (must be >=
>> %lu)\n",
>> +cr, cr / 100, cr_ahb);
>
> See next comment.
>
>> +cr = clk_get_rate(ss->ssclk);
>> +if (cr <= cr_mod)
>> +if (cr < cr_mod)
>> +dev_info(&pdev->dev, "Clock ss %lu (%lu MHz) (must be
>> <= %lu)\n",
>> +cr, cr / 100, cr_mod);
>> +else
>> +dev_dbg(&pdev->dev, "Clock ss %lu (%lu MHz) (must be <=
>> %lu)\n",
>> +cr, cr / 100, cr_mod);
>> +else
>> +dev_warn(&pdev->dev, "Clock ss is at %lu (%lu MHz) (must be <=
>> %lu)\n",
>> +cr, cr / 100, cr_mod);
>
> The management of kernel log levels looks pretty strange. As far as I
> understand there is no error on any clock rate, I'd recommend to keep
> only one information message.
>
If clock rate are below the recommended value, the only impact I found was bad
performance.
So it exp
Re: [PATCH v5 4/4] crypto: Add Allwinner Security System crypto accelerator
On Tue, 2014-10-21 at 02:28 +0300, Vladimir Zapolskiy wrote:
> On 19.10.2014 17:16, LABBE Corentin wrote:
> > Add support for the Security System included in Allwinner SoC A20.
> > The Security System is a hardware cryptographic accelerator that support
> > AES/MD5/SHA1/DES/3DES/PRNG algorithms.
[]
> > diff --git a/drivers/crypto/sunxi-ss/sunxi-ss-core.c
> > b/drivers/crypto/sunxi-ss/sunxi-ss-core.c
[]
> > + cr = clk_get_rate(ss->busclk);
> > + if (cr >= cr_ahb)
> > + dev_dbg(&pdev->dev, "Clock bus %lu (%lu MHz) (must be >=
> > %lu)\n",
> > + cr, cr / 100, cr_ahb);
> > + else
> > + dev_warn(&pdev->dev, "Clock bus %lu (%lu MHz) (must be >=
> > %lu)\n",
> > + cr, cr / 100, cr_ahb);
>
> See next comment.
>
> > + cr = clk_get_rate(ss->ssclk);
> > + if (cr <= cr_mod)
> > + if (cr < cr_mod)
> > + dev_info(&pdev->dev, "Clock ss %lu (%lu MHz) (must be
> > <= %lu)\n",
> > + cr, cr / 100, cr_mod);
> > + else
> > + dev_dbg(&pdev->dev, "Clock ss %lu (%lu MHz) (must be <=
> > %lu)\n",
> > + cr, cr / 100, cr_mod);
> > + else
> > + dev_warn(&pdev->dev, "Clock ss is at %lu (%lu MHz) (must be <=
> > %lu)\n",
> > + cr, cr / 100, cr_mod);
>
> The management of kernel log levels looks pretty strange. As far as I
> understand there is no error on any clock rate, I'd recommend to keep
> only one information message.
And if not, please add some braces.
> hash_init: initialize request context */
> > +int sunxi_hash_init(struct ahash_request *areq)
> > +{
> > + const char *hash_type;
> > + struct sunxi_req_ctx *op = ahash_request_ctx(areq);
> > +
> > + memset(op, 0, sizeof(struct sunxi_req_ctx));
> > +
> > + hash_type = crypto_tfm_alg_name(areq->base.tfm);
> > +
> > + if (strcmp(hash_type, "sha1") == 0)
> > + op->mode = SS_OP_SHA1;
> > + if (strcmp(hash_type, "md5") == 0)
> > + op->mode = SS_OP_MD5;
else if ?
> > + if (op->mode == 0)
> > + return -EINVAL;
maybe this?
if (!strcmp(hash_type, "sha1"))
op->mode = SS_OP_SHA1;
else if (!strcmp(hash_type, "md5"))
op->mode = SH_OP_MD5;
else
return -EINVAL;
> > +
> > + return 0;
> > +}
[]
> > +int sunxi_hash_update(struct ahash_request *areq)
> > +{
[]
> > + dev_dbg(ss->dev, "%s %s bc=%llu len=%u mode=%x bw=%u ww=%u",
> > + __func__, crypto_tfm_alg_name(areq->base.tfm),
> > + op->byte_count, areq->nbytes, op->mode,
> > + op->nbw, op->nwait);
dev_dbg statements generally don't need __func__ as
dynamic_debug can add it.
If you want to keep it, the most common output form for
__func__ is '"%s: ...", __func__'
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v5 4/4] crypto: Add Allwinner Security System crypto accelerator
Hello LABBE,
On 19.10.2014 17:16, LABBE Corentin wrote:
> Add support for the Security System included in Allwinner SoC A20.
> The Security System is a hardware cryptographic accelerator that support
> AES/MD5/SHA1/DES/3DES/PRNG algorithms.
>
> Signed-off-by: LABBE Corentin
> ---
> drivers/crypto/Kconfig| 17 ++
> drivers/crypto/Makefile | 1 +
> drivers/crypto/sunxi-ss/Makefile | 2 +
> drivers/crypto/sunxi-ss/sunxi-ss-cipher.c | 489
> ++
> drivers/crypto/sunxi-ss/sunxi-ss-core.c | 318 +++
> drivers/crypto/sunxi-ss/sunxi-ss-hash.c | 445 +++
> drivers/crypto/sunxi-ss/sunxi-ss.h| 193
> 7 files changed, 1465 insertions(+)
> create mode 100644 drivers/crypto/sunxi-ss/Makefile
> create mode 100644 drivers/crypto/sunxi-ss/sunxi-ss-cipher.c
> create mode 100644 drivers/crypto/sunxi-ss/sunxi-ss-core.c
> create mode 100644 drivers/crypto/sunxi-ss/sunxi-ss-hash.c
> create mode 100644 drivers/crypto/sunxi-ss/sunxi-ss.h
>
> diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
> index 2fb0fdf..9ba9759 100644
> --- a/drivers/crypto/Kconfig
> +++ b/drivers/crypto/Kconfig
> @@ -436,4 +436,21 @@ config CRYPTO_DEV_QCE
> hardware. To compile this driver as a module, choose M here. The
> module will be called qcrypto.
>
> +config CRYPTO_DEV_SUNXI_SS
> + tristate "Support for Allwinner Security System cryptographic
> accelerator"
> + depends on ARCH_SUNXI
> + select CRYPTO_MD5
> + select CRYPTO_SHA1
> + select CRYPTO_AES
> + select CRYPTO_DES
> + select CRYPTO_BLKCIPHER
> + help
> + Some Allwinner SoC have a crypto accelerator named
> + Security System. Select this if you want to use it.
> + The Security System handle AES/DES/3DES ciphers in CBC mode
> + and SHA1 and MD5 hash algorithms.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called sunxi-ss.
> +
> endif # CRYPTO_HW
> diff --git a/drivers/crypto/Makefile b/drivers/crypto/Makefile
> index 3924f93..856545c 100644
> --- a/drivers/crypto/Makefile
> +++ b/drivers/crypto/Makefile
> @@ -25,3 +25,4 @@ obj-$(CONFIG_CRYPTO_DEV_TALITOS) += talitos.o
> obj-$(CONFIG_CRYPTO_DEV_UX500) += ux500/
> obj-$(CONFIG_CRYPTO_DEV_QAT) += qat/
> obj-$(CONFIG_CRYPTO_DEV_QCE) += qce/
> +obj-$(CONFIG_CRYPTO_DEV_SUNXI_SS) += sunxi-ss/
> diff --git a/drivers/crypto/sunxi-ss/Makefile
> b/drivers/crypto/sunxi-ss/Makefile
> new file mode 100644
> index 000..8bb287d
> --- /dev/null
> +++ b/drivers/crypto/sunxi-ss/Makefile
> @@ -0,0 +1,2 @@
> +obj-$(CONFIG_CRYPTO_DEV_SUNXI_SS) += sunxi-ss.o
> +sunxi-ss-y += sunxi-ss-core.o sunxi-ss-hash.o sunxi-ss-cipher.o
> diff --git a/drivers/crypto/sunxi-ss/sunxi-ss-cipher.c
> b/drivers/crypto/sunxi-ss/sunxi-ss-cipher.c
> new file mode 100644
> index 000..8d0416e
> --- /dev/null
> +++ b/drivers/crypto/sunxi-ss/sunxi-ss-cipher.c
> @@ -0,0 +1,489 @@
> +/*
> + * sunxi-ss-cipher.c - hardware cryptographic accelerator for Allwinner A20
> SoC
> + *
> + * Copyright (C) 2013-2014 Corentin LABBE
> + *
> + * This file add support for AES cipher with 128,192,256 bits
> + * keysize in CBC mode.
> + * Add support also for DES and 3DES in CBC mode.
> + *
> + * You could find the datasheet in Documentation/arm/sunxi/README
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#include "sunxi-ss.h"
> +
> +extern struct sunxi_ss_ctx *ss;
> +
> +static int sunxi_ss_cipher(struct ablkcipher_request *areq, u32 mode)
> +{
> + struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(areq);
> + struct sunxi_tfm_ctx *op = crypto_ablkcipher_ctx(tfm);
> + const char *cipher_type;
> +
> + if (areq->nbytes == 0)
> + return 0;
> +
> + if (areq->info == NULL) {
> + dev_err(ss->dev, "ERROR: Empty IV\n");
> + return -EINVAL;
> + }
> +
> + if (areq->src == NULL || areq->dst == NULL) {
> + dev_err(ss->dev, "ERROR: Some SGs are NULL\n");
> + return -EINVAL;
> + }
> +
> + cipher_type = crypto_tfm_alg_name(crypto_ablkcipher_tfm(tfm));
> +
> + if (strcmp("cbc(aes)", cipher_type) == 0) {
> + mode |= SS_OP_AES | SS_CBC | SS_ENABLED | op->keymode;
> + return sunxi_ss_aes_poll(areq, mode);
> + }
> +
> + if (strcmp("cbc(des)", cipher_type) == 0) {
> + mode |= SS_OP_DES | SS_CBC | SS_ENABLED | op->keymode;
> + return sunxi_ss_des_poll(areq, mode);
> + }
> +
> + if (strcmp("cbc(des3_ede)", cipher_type) == 0) {
> + mode |= SS_OP_3DES | SS_CBC | SS_ENABLED | op->keymode;
> + return sunxi_ss_des_poll(areq
