Re: [PATCH 0/2] crypto: add new driver for Marvell CESA

[Boris Brezillon]

Herbert, David,

Any comment on the crypto driver implementation ?

I've had several reviews focused on:
1/ splitting the patch series into smaller subsets
2/ allowing for smoother transition from the old driver to the new one

I'll address (or have addressed) all of these comments, but I'd like to
have your opinion on the crypto driver itself.

In particular, I'd like to discuss the threaded-irq approach taken in
this driver (other drivers are using tasklets).
The main reason behind this choice is the fact that crypto engines
are quite fast, and I'm worried about the CPU contention that might
happen in case of fully loaded crypto engines (the CPU might spend most
of its time in interrupt context until the crypto queue is emptied).
Using threaded-irq allows preemption of the crypto completion
operation, thus authorizing another thread (with higher priority) to be
scheduled. ITOH, the tasklet approach provide slightly performances (I
don't recall the exact numbers, but Arnaud did some tests).

On Thu,  9 Apr 2015 16:58:41 +0200
Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx> wrote:

> Hello,
> 
> This is an attempt to replace the mv_cesa driver by a new one to address
> some limitations of the existing driver.
> From a performance and CPU load point of view the most important
> limitation is the lack of DMA support, thus preventing us from chaining
> crypto operations.
> 
> I know we usually try to adapt existing drivers instead of replacing them
> by new ones, but after trying to refactor the mv_cesa driver I realized it
> would take longer than writing an new one from scratch.
> 
> Here are the main features brought by this new driver:
> - support for armada SoCs (up to 38x) while keeping support for older ones
>   (Orion and Kirkwood)
> - DMA mode to offload the CPU in case of intensive crypto usage
> - new algorithms: SHA256, DES and 3DES
> 
> I'd like to thank Arnaud, who has carefully reviewed several iterations of
> this driver, helped me improved my implementation, provided support for
> several crypto algorithms, provided support for armada-370 and tested
> the driver on different platforms, hence the SoB and dual MODULE_AUTHOR
> in the driver code.



> 
> Best Regards,
> 
> Boris
> 
> Boris Brezillon (2):
>   crypto: add new driver for Marvell CESA
>   crypto: marvell/CESA: update DT bindings documentation
> 
>  .../devicetree/bindings/crypto/mv_cesa.txt         |   50 +-
>  drivers/crypto/Kconfig                             |    2 +
>  drivers/crypto/Makefile                            |    2 +-
>  drivers/crypto/marvell/Makefile                    |    1 +
>  drivers/crypto/marvell/cesa.c                      |  539 ++++++++
>  drivers/crypto/marvell/cesa.h                      |  802 ++++++++++++
>  drivers/crypto/marvell/cipher.c                    |  761 +++++++++++
>  drivers/crypto/marvell/hash.c                      | 1349 ++++++++++++++++++++
>  drivers/crypto/marvell/tdma.c                      |  223 ++++
>  drivers/crypto/mv_cesa.c                           | 1193 -----------------
>  drivers/crypto/mv_cesa.h                           |  150 ---
>  11 files changed, 3716 insertions(+), 1356 deletions(-)
>  create mode 100644 drivers/crypto/marvell/Makefile
>  create mode 100644 drivers/crypto/marvell/cesa.c
>  create mode 100644 drivers/crypto/marvell/cesa.h
>  create mode 100644 drivers/crypto/marvell/cipher.c
>  create mode 100644 drivers/crypto/marvell/hash.c
>  create mode 100644 drivers/crypto/marvell/tdma.c
>  delete mode 100644 drivers/crypto/mv_cesa.c
>  delete mode 100644 drivers/crypto/mv_cesa.h
> 



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/