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

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

[Boris Brezillon]

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

-- 
1.9.1

[PATCH 2/2] crypto: marvell/CESA: update DT bindings documentation

[Boris Brezillon]

Document new compatible strings, document the new method to reference the
crypto SRAM and deprecate the old one and document the the 'clocks' and
'clock-names' properties.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 .../devicetree/bindings/crypto/mv_cesa.txt         | 50 ++++++++++++++++------
 1 file changed, 38 insertions(+), 12 deletions(-)

diff --git a/Documentation/devicetree/bindings/crypto/mv_cesa.txt b/Documentation/devicetree/bindings/crypto/mv_cesa.txt
index 47229b1..4ce9bc5 100644
--- a/Documentation/devicetree/bindings/crypto/mv_cesa.txt
+++ b/Documentation/devicetree/bindings/crypto/mv_cesa.txt
@@ -1,20 +1,46 @@
 Marvell Cryptographic Engines And Security Accelerator
 
 Required properties:
-- compatible : should be "marvell,orion-crypto"
-- reg : base physical address of the engine and length of memory mapped
-        region, followed by base physical address of sram and its memory
-        length
-- reg-names : "regs" , "sram";
-- interrupts : interrupt number
+- compatible: should be one of the following string
+	      "marvell,orion-crypto"
+	      "marvell,kirkwood-crypto"
+	      "marvell,armada-370-crypto"
+	      "marvell,armada-xp-crypto"
+	      "marvell,armada-375-crypto"
+	      "marvell,armada-38x-crypto"
+- reg: base physical address of the engine and length of memory mapped
+       region
+- reg-names: "regs"
+- interrupts: interrupt number
+- clocks: reference to the crypto engines clocks. This property is not
+	  required for orion and kirkwood platforms
+- clock-names: "cesaX" and "cesazX", X should be replaced by the crypto engine
+	       id.
+	       This property is not required for the orion and kirkwoord
+	       platforms.
+	       "cesazX" clocks are not required on armada-370 platforms
+- marvell,crypto-srams: phandle to crypto SRAM definitions
+
+Optional properties:
+- marvell,crypto-sram-size: SRAM size reserved for crypto operations, if not
+			    specified the whole SRAM is used (2KB)
+
+Deprecated properties:
+- reg: base physical address of the engine and length of memory mapped
+       region, followed by base physical address of sram and its memory
+       length
+- reg-names: "regs" , "sram"
 
 Examples:
 
-	crypto at 30000 {
-		compatible = "marvell,orion-crypto";
-		reg = <0x30000 0x10000>,
-		      <0x4000000 0x800>;
-		reg-names = "regs" , "sram";
-		interrupts = <22>;
+	crypto at 90000 {
+		compatible = "marvell,armada-xp-crypto";
+		reg = <0x90000 0x10000>;
+		reg-names = "regs";
+		interrupts = <48>, <49>;
+		clocks = <&gateclk 23>, <&gateclk 23>;
+		clock-names = "cesa0", "cesa1";
+		marvell,crypto-srams = <&crypto_sram0>, <&crypto_sram1>;
+		marvell,crypto-sram-size = <0x600>;
 		status = "okay";
 	};
-- 
1.9.1

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

[Andrew Lunn]

On Thu, Apr 09, 2015 at 04:58:41PM +0200, Boris Brezillon 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.

Hi Boris

What is the situation with backwards compatibility? I see you have
kept the old compatibility string, and added lots of new ones, and
deprecated some properties. Will an old DT blob still work?
 
 Thanks
	Andrew

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

[Andrew Lunn]

On Thu, Apr 09, 2015 at 05:28:26PM +0200, Boris Brezillon wrote:
> Hi Andrew,
> 
> On Thu, 9 Apr 2015 17:18:46 +0200
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > On Thu, Apr 09, 2015 at 04:58:41PM +0200, Boris Brezillon 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.
> > 
> > Hi Boris
> > 
> > What is the situation with backwards compatibility? I see you have
> > kept the old compatibility string, and added lots of new ones, and
> > deprecated some properties. Will an old DT blob still work?
> 
> Yep, I tried on an Orion platform, and Arnaud tried on a Kirkwood one
> without any changes to the existing DT and it works.

Great. It would be nice to state this in the ChangeLog or cover note.

> Anyway, IMHO even those DT should be updated to use the new bindings
> (sram nodes, new compatible if available, addition of clock-names
> properties, ...).

Agreed. Maybe you can extend the patchset to modify the relevant .dtsi
files?

Thanks
	Andrew

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

[Sebastian Hesselbarth]

On 09.04.2015 16:58, Boris Brezillon wrote:
> 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.

Boris,

if you include a bunch of performance measurements, I guess it will help
you to get an agreement of replacing the driver instead of reworking it.

> 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)

Unfortunately, the list above is missing Dove SoCs which also have a
CESA engine with TDMA support. I checked the registers _very_ quickly
but it seems that they are compatible with Kirkwood's CESA.

> - DMA mode to offload the CPU in case of intensive crypto usage
> - new algorithms: SHA256, DES and 3DES
>
[...]
> Boris Brezillon (2):
>    crypto: add new driver for Marvell CESA
>    crypto: marvell/CESA: update DT bindings documentation

IMHO, the patch set should be split up in:
- new core driver
- add support for TDMA on platforms that support it
- new cipher algorithms
- removal of old mv_cesa

I'd love to test on Dove, but time still is very limited. I guess the
patches will receive another round anyway, maybe I find some until the
final version.

Sebastian

>   .../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
>

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

[Boris Brezillon]

Hi Sebastian,

On Thu, 09 Apr 2015 17:34:29 +0200
Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote:

> 
> if you include a bunch of performance measurements, I guess it will help
> you to get an agreement of replacing the driver instead of reworking it.

Yep, I made some measurements using tcrypt a while ago, I'll provide
them in the next round.

> 
> > 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)
> 
> Unfortunately, the list above is missing Dove SoCs which also have a
> CESA engine with TDMA support. I checked the registers _very_ quickly
> but it seems that they are compatible with Kirkwood's CESA.

I checked it too: it should work, but I don't have any board to test
it :-/.

> 
> > - DMA mode to offload the CPU in case of intensive crypto usage
> > - new algorithms: SHA256, DES and 3DES
> >
> [...]
> > Boris Brezillon (2):
> >    crypto: add new driver for Marvell CESA
> >    crypto: marvell/CESA: update DT bindings documentation
> 
> IMHO, the patch set should be split up in:
> - new core driver
> - add support for TDMA on platforms that support it
> - new cipher algorithms
> - removal of old mv_cesa

I agree for the 2 steps operation:
- add new driver code without compiling it
- remove old code, update Kconfig (add new dependencies) and Makefile
  entries (compile the code in marvell/ instead of mv_cesa.c)

Is there a good reason for separating the core, TDMA and algorithms
support in different patches (keep Arnaud's authorship ?) ?
Anyway, this should be feasible, but I thought the policy was to
minimize the number of patches when submitting new drivers.

> 
> I'd love to test on Dove, but time still is very limited. I guess the
> patches will receive another round anyway, maybe I find some until the
> final version.

No problem (thanks for the offer).

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

[Arnaud Ebalard]

Hi Sebastien,

Boris Brezillon <boris.brezillon@free-electrons.com> writes:

>> if you include a bunch of performance measurements, I guess it will help
>> you to get an agreement of replacing the driver instead of reworking it.
>
> Yep, I made some measurements using tcrypt a while ago, I'll provide
> them in the next round.

Here are some tests on 2 Marvell SoC (I do not have dove platforms at
hand and did not collect the results for A370):

  - Kirkwood 88F6282 (Feroceon 88FR131 rev 1) at 1.6GHz
  - Armada XP (mv78230, i.e. 2 core @1.2GHz)

The targets are AES ECB and CBC encryption (decryption is similar
performance-wise), done w/ tcrypt (mode=500 passed to tcrypt module).

For each SoC, the various tests done by tcrypt are the following:

AES ECB/CBC encryption:

t  0 (128 bit key, 16 byte blocks)
t  1 (128 bit key, 64 byte blocks)
t  2 (128 bit key, 256 byte blocks)
t  3 (128 bit key, 1024 byte blocks)
t  4 (128 bit key, 8192 byte blocks)
t  5 (192 bit key, 16 byte blocks)
t  6 (192 bit key, 64 byte blocks)
t  7 (192 bit key, 256 byte blocks)
t  8 (192 bit key, 1024 byte blocks)
t  9 (192 bit key, 8192 byte blocks)
t 10 (256 bit key, 16 byte blocks)
t 11 (256 bit key, 64 byte blocks)
t 12 (256 bit key, 256 byte blocks)
t 13 (256 bit key, 1024 byte blocks)
t 14 (256 bit key, 8192 byte blocks)

The three columns provide the value for software implementation
(aes-asm), current driver (if available for that SoC), submitted
v0. The percentage is the improvement against software
implementation.

         soft         current driver          submitted v0     
                      (if available)


KW:

ECB
   t 0:  5.23 MB/s   1.01 MB/s (-80.58%)    1.11 MB/s (-78.75%) 
   t 1: 12.40 MB/s   3.70 MB/s (-70.16%)    4.14 MB/s (-66.59%) 
   t 2: 18.94 MB/s  10.81 MB/s (-42.94%)   13.86 MB/s (-26.78%) 
   t 3: 21.79 MB/s  20.69 MB/s  (-5.05%)   33.80 MB/s  (55.12%)  
   t 4: 22.54 MB/s  25.97 MB/s  (15.23%)   50.27 MB/s (123.05%) 

   t 5:  5.00 MB/s   1.01 MB/s (-79.75%)    1.10 MB/s (-78.02%) 
   t 6: 11.35 MB/s   3.70 MB/s (-67.41%)    3.84 MB/s (-66.17%) 
   t 7: 16.60 MB/s  10.66 MB/s (-35.81%)   13.59 MB/s (-18.14%) 
   t 8: 18.76 MB/s  20.13 MB/s   (7.29%)   32.30 MB/s  (72.15%)  
   t 9: 19.20 MB/s  25.10 MB/s  (30.74%)   47.11 MB/s (145.37%) 

   t10:  4.85 MB/s   1.02 MB/s (-79.02%)    1.10 MB/s (-77.25%) 
   t11: 10.50 MB/s   3.74 MB/s (-64.35%)    4.10 MB/s (-60.89%) 
   t12: 14.80 MB/s   4.65 MB/s (-68.55%)   13.40 MB/s (-9.43%)  
   t13: 16.47 MB/s  19.22 MB/s  (16.69%)   31.14 MB/s  (89.02%)  
   t14: 16.89 MB/s  24.36 MB/s  (44.18%)   44.33 MB/s (162.40%) 

CBC
   t 0:  4.78 MB/s   0.98 MB/s (-79.50%)    1.09 MB/s (-77.12%) 
   t 1: 11.44 MB/s   3.59 MB/s (-68.62%)    4.07 MB/s (-64.41%) 
   t 2: 17.66 MB/s  10.53 MB/s (-40.38%)   13.67 MB/s (-22.58%) 
   t 3: 20.41 MB/s  20.42 MB/s   (0.00%)   33.50 MB/s  (64.10%)  
   t 4: 21.14 MB/s  25.86 MB/s  (22.36%)   50.02 MB/s (136.63%) 

   t 5:  4.58 MB/s   0.98 MB/s (-78.64%)    1.08 MB/s (-76.31%) 
   t 6: 10.54 MB/s   3.58 MB/s (-66.00%)    4.04 MB/s (-61.68%) 
   t 7: 15.61 MB/s  10.39 MB/s (-33.49%)   13.40 MB/s (-14.16%) 
   t 8: 17.73 MB/s  19.88 MB/s  (12.10%)   32.04 MB/s  (80.69%)  
   t 9: 18.18 MB/s  25.02 MB/s  (37.60%)   46.90 MB/s (157.97%) 

   t10:  4.45 MB/s   0.98 MB/s (-77.96%)    1.09 MB/s (-75.62%) 
   t11:  9.80 MB/s   3.60 MB/s (-63.28%)    4.03 MB/s (-58.83%) 
   t12: 14.01 MB/s   4.34 MB/s (-69.01%)   13.24 MB/s  (-5.48%)  
   t13: 15.67 MB/s  19.44 MB/s  (24.01%)   30.90 MB/s  (97.17%)  
   t14: 16.09 MB/s  24.28 MB/s  (50.85%)   44.15 MB/s (174.34%) 

XP:

ECB
   t 0:  8.85 MB/s                          0.77 MB/s (-91.25%) 
   t 1: 21.73 MB/s                          3.09 MB/s (-85.79%) 
   t 2: 34.81 MB/s                         12.35 MB/s (-64.52%) 
   t 3: 40.81 MB/s                         38.68 MB/s  (-5.22%) 
   t 4: 42.69 MB/s                         84.52 MB/s  (98.00%) 

   t 5:  8.55 MB/s                          0.78 MB/s (-90.92%) 
   t 6: 20.63 MB/s                          3.11 MB/s (-84.92%) 
   t 7: 31.47 MB/s                         12.43 MB/s (-60.52%) 
   t 8: 36.07 MB/s                         38.08 MB/s   (5.58%) 
   t 9: 37.09 MB/s                         80.43 MB/s (116.85%) 

   t10:  8.25 MB/s                          0.78 MB/s (-90.56%) 
   t11: 19.19 MB/s                          3.11 MB/s (-83.80%) 
   t12: 28.61 MB/s                         12.42 MB/s (-56.59%) 
   t13: 32.49 MB/s                         37.28 MB/s  (14.74%) 
   t14: 33.56 MB/s                         77.11 MB/s (129.79%) 

CBC   
   t 0:  8.20 MB/s                          0.78 MB/s (-90.53%) 
   t 1: 19.85 MB/s                          3.10 MB/s (-84.36%) 
   t 2: 31.60 MB/s                         12.42 MB/s (-60.69%) 
   t 3: 37.03 MB/s                         38.70 MB/s   (4.51%) 
   t 4: 38.76 MB/s                         84.05 MB/s (116.87%) 

   t 5:  7.69 MB/s                          0.78 MB/s (-89.90%) 
   t 6: 18.62 MB/s                          3.10 MB/s (-83.32%) 
   t 7: 28.47 MB/s                         12.40 MB/s (-56.44%) 
   t 8: 32.73 MB/s                         37.97 MB/s  (16.02%) 
   t 9: 33.73 MB/s                         79.96 MB/s (137.07%) 

   t10:  7.58 MB/s                          0.77 MB/s (-89.88%) 
   t11: 17.59 MB/s                          3.07 MB/s (-82.56%) 
   t12: 26.26 MB/s                         12.28 MB/s (-53.23%) 
   t13: 29.89 MB/s                         37.02 MB/s  (23.87%) 
   t14: 30.87 MB/s                         76.70 MB/s (148.45%)


Cheers,

a+

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

[Stephan Mueller]

Am Donnerstag, 9. April 2015, 16:58:41 schrieb Boris Brezillon:

Hi Boris,

>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.

Your patch 1/2 did not make it to the crypto list. To big? It is on the lkml 
list though.
>
>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


Ciao
Stephan

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

[Paul Bolle]

(This patch needed a trivial context change in drivers/crypto/Makefile
to get it applied on top of next-20150409.)

On Thu, 2015-04-09 at 16:58 +0200, Boris Brezillon wrote:
> --- a/drivers/crypto/Kconfig
> +++ b/drivers/crypto/Kconfig
> @@ -164,8 +164,10 @@ config CRYPTO_DEV_MV_CESA
>  	depends on PLAT_ORION
>  	select CRYPTO_ALGAPI
>  	select CRYPTO_AES
> +	select CRYPTO_DES
>  	select CRYPTO_BLKCIPHER2
>  	select CRYPTO_HASH
> +	select SRAM
>  	help
>  	  This driver allows you to utilize the Cryptographic Engines and
>  	  Security Accelerator (CESA) which can be found on the Marvell Orion

> --- a/drivers/crypto/Makefile
> +++ b/drivers/crypto/Makefile

> -obj-$(CONFIG_CRYPTO_DEV_MV_CESA) += mv_cesa.o
> +obj-$(CONFIG_CRYPTO_DEV_MV_CESA) += marvell/

> --- /dev/null
> +++ b/drivers/crypto/marvell/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_CRYPTO_DEV_MV_CESA) += cesa.o cipher.o hash.o tdma.o

For a modular build (which is all I tried) this doesn't do what you
probably want, as this will generate four modules. Assuming you want to
keep the mv_cesa name for the module, you could try something like:
    obj-$(CONFIG_CRYPTO_DEV_MV_CESA) += mv_cesa.o
    mv_cesa-objs := cesa.o cipher.o hash.o tdma.o

Does that do what you want?

> --- /dev/null
> +++ b/drivers/crypto/marvell/cesa.c

> + * 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.

This states the license is GPL v2.

> +static struct platform_driver marvell_cesa = {
> +	.probe		= mv_cesa_probe,
> +	.remove		= mv_cesa_remove,
> +	.driver		= {
> +		.owner	= THIS_MODULE,
> +		.name	= "mv_crypto",
> +		.of_match_table = mv_cesa_of_match_table,
> +	},
> +};
> +MODULE_ALIAS("platform:mv_crypto");

(It's nicer to make that macro be a part of the block of the other
MODULE_ macros.)

> +module_platform_driver(marvell_cesa);

(And it's nicer to have this directly follow the definition of
marvell_cesa.)

> +MODULE_AUTHOR("Boris Brezillon <boris.brezillon@free-electrons.com>");
> +MODULE_AUTHOR("Arnaud Ebalard <arno@natisbad.org>");
> +MODULE_DESCRIPTION("Support for Marvell's cryptographic engine");
> +MODULE_LICENSE("GPL");

And this states the license is GPL v2 or later. So either the comment at
the top of this file or this macro need to be changed.


Paul Bolle

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

[Boris Brezillon]

Hi Paul,

On Fri, 10 Apr 2015 12:38:59 +0200
Paul Bolle <pebolle@tiscali.nl> wrote:

> (This patch needed a trivial context change in drivers/crypto/Makefile
> to get it applied on top of next-20150409.)

I'll rebase my work on linux-next.

> 
> On Thu, 2015-04-09 at 16:58 +0200, Boris Brezillon wrote:
> > --- a/drivers/crypto/Kconfig
> > +++ b/drivers/crypto/Kconfig
> > @@ -164,8 +164,10 @@ config CRYPTO_DEV_MV_CESA
> >  	depends on PLAT_ORION
> >  	select CRYPTO_ALGAPI
> >  	select CRYPTO_AES
> > +	select CRYPTO_DES
> >  	select CRYPTO_BLKCIPHER2
> >  	select CRYPTO_HASH
> > +	select SRAM
> >  	help
> >  	  This driver allows you to utilize the Cryptographic Engines and
> >  	  Security Accelerator (CESA) which can be found on the Marvell Orion
> 
> > --- a/drivers/crypto/Makefile
> > +++ b/drivers/crypto/Makefile
> 
> > -obj-$(CONFIG_CRYPTO_DEV_MV_CESA) += mv_cesa.o
> > +obj-$(CONFIG_CRYPTO_DEV_MV_CESA) += marvell/
> 
> > --- /dev/null
> > +++ b/drivers/crypto/marvell/Makefile
> > @@ -0,0 +1 @@
> > +obj-$(CONFIG_CRYPTO_DEV_MV_CESA) += cesa.o cipher.o hash.o tdma.o
> 
> For a modular build (which is all I tried) this doesn't do what you
> probably want, as this will generate four modules. Assuming you want to
> keep the mv_cesa name for the module, you could try something like:
>     obj-$(CONFIG_CRYPTO_DEV_MV_CESA) += mv_cesa.o
>     mv_cesa-objs := cesa.o cipher.o hash.o tdma.o
> 
> Does that do what you want?

Yes, I'll fix that in v2.

> 
> > --- /dev/null
> > +++ b/drivers/crypto/marvell/cesa.c
> 
> > + * 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.
> 
> This states the license is GPL v2.
> 
> > +static struct platform_driver marvell_cesa = {
> > +	.probe		= mv_cesa_probe,
> > +	.remove		= mv_cesa_remove,
> > +	.driver		= {
> > +		.owner	= THIS_MODULE,
> > +		.name	= "mv_crypto",
> > +		.of_match_table = mv_cesa_of_match_table,
> > +	},
> > +};
> > +MODULE_ALIAS("platform:mv_crypto");
> 
> (It's nicer to make that macro be a part of the block of the other
> MODULE_ macros.)
> 
> > +module_platform_driver(marvell_cesa);
> 
> (And it's nicer to have this directly follow the definition of
> marvell_cesa.)

Absolutely.

> 
> > +MODULE_AUTHOR("Boris Brezillon <boris.brezillon@free-electrons.com>");
> > +MODULE_AUTHOR("Arnaud Ebalard <arno@natisbad.org>");
> > +MODULE_DESCRIPTION("Support for Marvell's cryptographic engine");
> > +MODULE_LICENSE("GPL");
> 
> And this states the license is GPL v2 or later. So either the comment at
> the top of this file or this macro need to be changed.

I'll change the MODULE_LICENSE definition.

Thanks,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

[Jason Cooper]

Hey Boris,

On Thu, Apr 09, 2015 at 04:58:41PM +0200, Boris Brezillon wrote:
> 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.

I'm sorry, but this makes me *very* uncomfortable.  Any organization
worth it's salt will do a very careful audit of code touching
cryptographic material and sensitive (decrypted) data.  From that point
on, small audits of the changes to the code allow the organization to
build a comfort level with kernel updates.  iow, following the git
history of the driver.

By apply this series, we are basically forcing those organizations to
either a) stop updating, or b) expend significant resources to do
another full audit.

In short, this series breaks the audit chain for the mv_cesa driver.

Maybe I'm the only person with this level of paranoia.  If so, I'm sure
others will override me.

>From my POV, it looks like the *only* reason we've chosen this route is
developer convenience.  I don't think that's sufficient reason to break
the change history of a driver handling sensitive data.

For an example of how I use the git history and binary differences to
audit a series of changes to cryptographic code, please take a look at
objdiff [1]. You can even duplicate my audit of my submission for the
skein/threefish driver currently in the staging tree, starting at [2]
and going up to [3].

thx,

Jason.

[1] scripts/objdiff [4]
[2] 449bb8125e3f "staging: crypto: skein: import code from Skein3Fish.git"
[3] 0264b7b7fb44 "staging: crypto: skein: rename macros"
[4] There are better tools out there for auditing actual code changes,
    radare (http://radare.org/r/) is one of them.  objdiff is good only
    at validating object code *hasn't* been changed by style commits.

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

[Boris Brezillon]

Hi Jason,

On Fri, 10 Apr 2015 13:50:56 +0000
Jason Cooper <jason@lakedaemon.net> wrote:

> Hey Boris,
> 
> On Thu, Apr 09, 2015 at 04:58:41PM +0200, Boris Brezillon wrote:
> > 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.
> 
> I'm sorry, but this makes me *very* uncomfortable.  Any organization
> worth it's salt will do a very careful audit of code touching
> cryptographic material and sensitive (decrypted) data.  From that point
> on, small audits of the changes to the code allow the organization to
> build a comfort level with kernel updates.  iow, following the git
> history of the driver.
> 
> By apply this series, we are basically forcing those organizations to
> either a) stop updating, or b) expend significant resources to do
> another full audit.
> 
> In short, this series breaks the audit chain for the mv_cesa driver.
> 
> Maybe I'm the only person with this level of paranoia.  If so, I'm sure
> others will override me.
> 
> From my POV, it looks like the *only* reason we've chosen this route is
> developer convenience.  I don't think that's sufficient reason to break
> the change history of a driver handling sensitive data.

Well, I understand you concern, but if you read carefully the old and
new drivers, you'll notice that they are completely different (the only
thing I kept are the macro definitions).
I really tried to adapt the existing driver to add the missing
features (especially the support for TDMA), but all my attempts
ended up introducing hackish code (not even talking about the
performance penalty of this approach). Is that really what we want ?
How would you make such big changes on the existing driver (I mean, the
core infrastructure dealing with crypto requests is completely
different) ?

I have another solution though: keep the existing driver for old
marvell SoCs (orion, kirkwood and dove), and add a new one for modern
SoCs (armada 370, XP, 375 and 38x), so that users of the mv_cesa driver
won't have to audit the new code.

> 
> For an example of how I use the git history and binary differences to
> audit a series of changes to cryptographic code, please take a look at
> objdiff [1]. You can even duplicate my audit of my submission for the
> skein/threefish driver currently in the staging tree, starting at [2]
> and going up to [3].

Thanks for the pointers.

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

[Jason Cooper]

Hey Boris,

On Fri, Apr 10, 2015 at 05:11:48PM +0200, Boris Brezillon wrote:
> On Fri, 10 Apr 2015 13:50:56 +0000 Jason Cooper <jason@lakedaemon.net> wrote:
> > On Thu, Apr 09, 2015 at 04:58:41PM +0200, Boris Brezillon wrote:
> > > 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.
> > 
> > I'm sorry, but this makes me *very* uncomfortable.  Any organization
> > worth it's salt will do a very careful audit of code touching
> > cryptographic material and sensitive (decrypted) data.  From that point
> > on, small audits of the changes to the code allow the organization to
> > build a comfort level with kernel updates.  iow, following the git
> > history of the driver.
> > 
> > By apply this series, we are basically forcing those organizations to
> > either a) stop updating, or b) expend significant resources to do
> > another full audit.
> > 
> > In short, this series breaks the audit chain for the mv_cesa driver.
> > 
> > Maybe I'm the only person with this level of paranoia.  If so, I'm sure
> > others will override me.
> > 
> > From my POV, it looks like the *only* reason we've chosen this route is
> > developer convenience.  I don't think that's sufficient reason to break
> > the change history of a driver handling sensitive data.
> 
> Well, I understand you concern, but if you read carefully the old and
> new drivers, you'll notice that they are completely different (the only
> thing I kept are the macro definitions).

Yes, that's the worrying part for me. ;-)

> I really tried to adapt the existing driver to add the missing
> features (especially the support for TDMA), but all my attempts
> ended up introducing hackish code (not even talking about the
> performance penalty of this approach).

Ok, fair enough.  It would be helpful if this account of attempting to
reconcile the old driver made it into the commit message.  This puts us
in "perfect is the enemy of getting it done" territory.

> I have another solution though: keep the existing driver for old
> marvell SoCs (orion, kirkwood and dove), and add a new one for modern
> SoCs (armada 370, XP, 375 and 38x), so that users of the mv_cesa driver
> won't have to audit the new code.

A fair proposal, but I'll freely admit the number of people actually auditing
their code paths is orders of magnitude smaller than the number of users
of the driver.

There's such a large population of compatible legacy SoCs in the wild,
adding an artificial boundary doesn't make sense.  Especially since
we're talking about features everyone would want to use.

Perhaps we should keep both around, and deprecate the legacy driver over
3 to 4 cycles?

thx,

Jason.

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

[Gregory CLEMENT]

Hi Jason, Boris,

On 11/04/2015 00:30, Jason Cooper wrote:
> Hey Boris,
> 
> On Fri, Apr 10, 2015 at 05:11:48PM +0200, Boris Brezillon wrote:
>> On Fri, 10 Apr 2015 13:50:56 +0000 Jason Cooper <jason@lakedaemon.net> wrote:
>>> On Thu, Apr 09, 2015 at 04:58:41PM +0200, Boris Brezillon wrote:
>>>> 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.
>>>
>>> I'm sorry, but this makes me *very* uncomfortable.  Any organization
>>> worth it's salt will do a very careful audit of code touching
>>> cryptographic material and sensitive (decrypted) data.  From that point
>>> on, small audits of the changes to the code allow the organization to
>>> build a comfort level with kernel updates.  iow, following the git
>>> history of the driver.
>>>
>>> By apply this series, we are basically forcing those organizations to
>>> either a) stop updating, or b) expend significant resources to do
>>> another full audit.
>>>
>>> In short, this series breaks the audit chain for the mv_cesa driver.
>>>
>>> Maybe I'm the only person with this level of paranoia.  If so, I'm sure
>>> others will override me.
>>>
>>> From my POV, it looks like the *only* reason we've chosen this route is
>>> developer convenience.  I don't think that's sufficient reason to break
>>> the change history of a driver handling sensitive data.
>>
>> Well, I understand you concern, but if you read carefully the old and
>> new drivers, you'll notice that they are completely different (the only
>> thing I kept are the macro definitions).
> 
> Yes, that's the worrying part for me. ;-)

I understand the logic behind your concern, but I wonder if it is really
an issue. My knowledge ans my background around crypto is very limited,
so I really would like the opinion of other people on the subject.

> 
>> I really tried to adapt the existing driver to add the missing
>> features (especially the support for TDMA), but all my attempts
>> ended up introducing hackish code (not even talking about the
>> performance penalty of this approach).
> 
> Ok, fair enough.  It would be helpful if this account of attempting to
> reconcile the old driver made it into the commit message.  This puts us
> in "perfect is the enemy of getting it done" territory.
> 
>> I have another solution though: keep the existing driver for old
>> marvell SoCs (orion, kirkwood and dove), and add a new one for modern
>> SoCs (armada 370, XP, 375 and 38x), so that users of the mv_cesa driver
>> won't have to audit the new code.
> 
> A fair proposal, but I'll freely admit the number of people actually auditing
> their code paths is orders of magnitude smaller than the number of users
> of the driver.
> 
> There's such a large population of compatible legacy SoCs in the wild,
> adding an artificial boundary doesn't make sense.  Especially since
> we're talking about features everyone would want to use.
> 
> Perhaps we should keep both around, and deprecate the legacy driver over
> 3 to 4 cycles?

But I guess that some users will want to use the new driver on the "old" marvell
SoCs (especially kirkwood and dove). If we go to this path, then the best solution
would be to still update all the the dts, and modifying the old driver to be able to
use the new binding: for my point of view the only adaptation should be related
to the SRAM. It will be also needed to find a way to be able to load only one driver
at a time: either the old or the new, but not both.

However I still wonder if it worth the effort.


Thanks,

Gregory




> 
> thx,
> 
> Jason.
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

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

[Jason Cooper]

Hey Gregory,

On Mon, Apr 13, 2015 at 11:39:16AM +0200, Gregory CLEMENT wrote:
> Hi Jason, Boris,
> 
> On 11/04/2015 00:30, Jason Cooper wrote:
> > Hey Boris,
> > 
> > On Fri, Apr 10, 2015 at 05:11:48PM +0200, Boris Brezillon wrote:
> >> On Fri, 10 Apr 2015 13:50:56 +0000 Jason Cooper <jason@lakedaemon.net> wrote:
> >>> On Thu, Apr 09, 2015 at 04:58:41PM +0200, Boris Brezillon wrote:
> >>>> 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.
> >>>
> >>> I'm sorry, but this makes me *very* uncomfortable.  Any organization
> >>> worth it's salt will do a very careful audit of code touching
> >>> cryptographic material and sensitive (decrypted) data.  From that point
> >>> on, small audits of the changes to the code allow the organization to
> >>> build a comfort level with kernel updates.  iow, following the git
> >>> history of the driver.
> >>>
> >>> By apply this series, we are basically forcing those organizations to
> >>> either a) stop updating, or b) expend significant resources to do
> >>> another full audit.
> >>>
> >>> In short, this series breaks the audit chain for the mv_cesa driver.
> >>>
> >>> Maybe I'm the only person with this level of paranoia.  If so, I'm sure
> >>> others will override me.
> >>>
> >>> From my POV, it looks like the *only* reason we've chosen this route is
> >>> developer convenience.  I don't think that's sufficient reason to break
> >>> the change history of a driver handling sensitive data.
> >>
> >> Well, I understand you concern, but if you read carefully the old and
> >> new drivers, you'll notice that they are completely different (the only
> >> thing I kept are the macro definitions).
> > 
> > Yes, that's the worrying part for me. ;-)
> 
> I understand the logic behind your concern, but I wonder if it is really
> an issue. My knowledge ans my background around crypto is very limited,
> so I really would like the opinion of other people on the subject.

It's not about the crypto, it's about trust.  imho, one of the most
important security advances in the past 20 years is the default use of
git (or other SCMs) by open source projects.  Now, no one is forced to
trust the authors and maintainers tarball dumps.  Regular code audits
and security updates are *much* more feasible because you can audit
small changes.  It can even be automated to a large extent.

All this means the user has a choice: they can trust the authors and
maintainers, or they can trust their own audits.  Since updates are an
essential part of a security posture, small commits facilitate
maintaining the 'trust in audits'.

It's not about "Should you trust free-electrons?"  Or, "Should you trust
Jason / Herbert / Linus?"  It's about "Should you have to trust any of
them?"

> >> I really tried to adapt the existing driver to add the missing
> >> features (especially the support for TDMA), but all my attempts
> >> ended up introducing hackish code (not even talking about the
> >> performance penalty of this approach).
> > 
> > Ok, fair enough.  It would be helpful if this account of attempting to
> > reconcile the old driver made it into the commit message.  This puts us
> > in "perfect is the enemy of getting it done" territory.
> > 
> >> I have another solution though: keep the existing driver for old
> >> marvell SoCs (orion, kirkwood and dove), and add a new one for modern
> >> SoCs (armada 370, XP, 375 and 38x), so that users of the mv_cesa driver
> >> won't have to audit the new code.
> > 
> > A fair proposal, but I'll freely admit the number of people actually auditing
> > their code paths is orders of magnitude smaller than the number of users
> > of the driver.
> > 
> > There's such a large population of compatible legacy SoCs in the wild,
> > adding an artificial boundary doesn't make sense.  Especially since
> > we're talking about features everyone would want to use.
> > 
> > Perhaps we should keep both around, and deprecate the legacy driver over
> > 3 to 4 cycles?
> 
> But I guess that some users will want to use the new driver on the "old" marvell
> SoCs (especially kirkwood and dove).

Yes, despite my arguments, I'm one of those people.  :-P

> If we go to this path, then the best solution would be to still update
> all the the dts, and modifying the old driver to be able to use the
> new binding: for my point of view the only adaptation should be
> related to the SRAM. It will be also needed to find a way to be able
> to load only one driver at a time: either the old or the new, but not
> both.

We can look at how the wireless drivers handle this.  They often have to
choose between multiple drivers (foss, proprietary, ndis-something, etc)
for a given card.  Not much different here.

> However I still wonder if it worth the effort.

I'd appreciate if we'd look into it.  I understand from on-list and
off-list discussion that the rewrite was unavoidable.  So I'm willing to
concede that.  Giving people time to migrate from old to new while still
being able to update for other security fixes seems reasonable.

thx,

Jason.

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

[Arnaud Ebalard]

Hi Jason,

Jason Cooper <jason@lakedaemon.net> writes:

> It's not about the crypto, it's about trust.  imho, one of the most
> important security advances in the past 20 years is the default use of
> git (or other SCMs) by open source projects.  Now, no one is forced to
> trust the authors and maintainers tarball dumps.  Regular code audits
> and security updates are *much* more feasible because you can audit
> small changes.  It can even be automated to a large extent.
>
> All this means the user has a choice: they can trust the authors and
> maintainers, or they can trust their own audits.  Since updates are an
> essential part of a security posture, small commits facilitate
> maintaining the 'trust in audits'.
>
> It's not about "Should you trust free-electrons?"  Or, "Should you trust
> Jason / Herbert / Linus?"  It's about "Should you have to trust any of
> them?"

It's ok, you can call our driver fat. It is ;-) More seriously, I tend
to agree w/ what you write above.


>> >> I really tried to adapt the existing driver to add the missing
>> >> features (especially the support for TDMA), but all my attempts
>> >> ended up introducing hackish code (not even talking about the
>> >> performance penalty of this approach).
>> > 
>> > Ok, fair enough.  It would be helpful if this account of attempting to
>> > reconcile the old driver made it into the commit message.  This puts us
>> > in "perfect is the enemy of getting it done" territory.
>> > 
>> >> I have another solution though: keep the existing driver for old
>> >> marvell SoCs (orion, kirkwood and dove), and add a new one for modern
>> >> SoCs (armada 370, XP, 375 and 38x), so that users of the mv_cesa driver
>> >> won't have to audit the new code.
>> > 
>> > A fair proposal, but I'll freely admit the number of people actually auditing
>> > their code paths is orders of magnitude smaller than the number of users
>> > of the driver.
>> > 
>> > There's such a large population of compatible legacy SoCs in the wild,
>> > adding an artificial boundary doesn't make sense.  Especially since
>> > we're talking about features everyone would want to use.
>> > 
>> > Perhaps we should keep both around, and deprecate the legacy driver over
>> > 3 to 4 cycles?
>> 
>> But I guess that some users will want to use the new driver on the "old" marvell
>> SoCs (especially kirkwood and dove).
>
> Yes, despite my arguments, I'm one of those people.  :-P
>
>> If we go to this path, then the best solution would be to still update
>> all the the dts, and modifying the old driver to be able to use the
>> new binding: for my point of view the only adaptation should be
>> related to the SRAM. It will be also needed to find a way to be able
>> to load only one driver at a time: either the old or the new, but not
>> both.

The approach Boris proposed above seems to make everyone happy:

 1) Keep the old driver for old marvells SoCs (kirkwood, dove and orion)
 2) Introduce the new driver for those that are not supported by the old
    driver, i.e. armada (370, XP, 375, 38x)

AFAICT, this can easily be done (based on compatible strings) and it
will let everyone the time to audit the new driver. Current users will
not be taken by surprise. At some point, when everyone is confident w/
the new driver, we can then switch to that one for all SoCs so that
old platform get more performance.

Additionnally, for those who want to get the feature of the new driver
for their old SoC right now, we *could* add a simple kernel config option
for the new driver to use it for the old SoC too (that one disabling the
old one).


> I'd appreciate if we'd look into it.  I understand from on-list and
> off-list discussion that the rewrite was unavoidable.  So I'm willing to
> concede that.  Giving people time to migrate from old to new while still
> being able to update for other security fixes seems reasonable.

Jason, what do you think of the approach above? 

Cheers,

a+

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

[Jason Cooper]

Hey Arnaud,

On Mon, Apr 13, 2015 at 06:06:49PM +0200, Arnaud Ebalard wrote:
> Jason Cooper <jason@lakedaemon.net> writes:
...
> >> >> I really tried to adapt the existing driver to add the missing
> >> >> features (especially the support for TDMA), but all my attempts
> >> >> ended up introducing hackish code (not even talking about the
> >> >> performance penalty of this approach).
> >> > 
> >> > Ok, fair enough.  It would be helpful if this account of attempting to
> >> > reconcile the old driver made it into the commit message.  This puts us
> >> > in "perfect is the enemy of getting it done" territory.
> >> > 
> >> >> I have another solution though: keep the existing driver for old
> >> >> marvell SoCs (orion, kirkwood and dove), and add a new one for modern
> >> >> SoCs (armada 370, XP, 375 and 38x), so that users of the mv_cesa driver
> >> >> won't have to audit the new code.
> >> > 
> >> > A fair proposal, but I'll freely admit the number of people actually auditing
> >> > their code paths is orders of magnitude smaller than the number of users
> >> > of the driver.
> >> > 
> >> > There's such a large population of compatible legacy SoCs in the wild,
> >> > adding an artificial boundary doesn't make sense.  Especially since
> >> > we're talking about features everyone would want to use.
> >> > 
> >> > Perhaps we should keep both around, and deprecate the legacy driver over
> >> > 3 to 4 cycles?
> >> 
> >> But I guess that some users will want to use the new driver on the "old" marvell
> >> SoCs (especially kirkwood and dove).
> >
> > Yes, despite my arguments, I'm one of those people.  :-P
> >
> >> If we go to this path, then the best solution would be to still update
> >> all the the dts, and modifying the old driver to be able to use the
> >> new binding: for my point of view the only adaptation should be
> >> related to the SRAM. It will be also needed to find a way to be able
> >> to load only one driver at a time: either the old or the new, but not
> >> both.
> 
> The approach Boris proposed above seems to make everyone happy:
> 
>  1) Keep the old driver for old marvells SoCs (kirkwood, dove and orion)
>  2) Introduce the new driver for those that are not supported by the old
>     driver, i.e. armada (370, XP, 375, 38x)
> 
> AFAICT, this can easily be done (based on compatible strings) and it
> will let everyone the time to audit the new driver. Current users will
> not be taken by surprise. At some point, when everyone is confident w/
> the new driver, we can then switch to that one for all SoCs so that
> old platform get more performance.
> 
> Additionnally, for those who want to get the feature of the new driver
> for their old SoC right now, we *could* add a simple kernel config option
> for the new driver to use it for the old SoC too (that one disabling the
> old one).
> 
> 
> > I'd appreciate if we'd look into it.  I understand from on-list and
> > off-list discussion that the rewrite was unavoidable.  So I'm willing to
> > concede that.  Giving people time to migrate from old to new while still
> > being able to update for other security fixes seems reasonable.
> 
> Jason, what do you think of the approach above? 

I say keep it simple.  We shouldn't use the DT changes to trigger one
vice the other.  We need to be able to build both, but only load one at
a time.  If that's anything other than simple to do, then we make it a
Kconfig binary choice and move on.

thx,

Jason.

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

[Boris Brezillon]

Hi Jason,

On Mon, 13 Apr 2015 20:11:46 +0000
Jason Cooper <jason@lakedaemon.net> wrote:

> > 
> > > I'd appreciate if we'd look into it.  I understand from on-list and
> > > off-list discussion that the rewrite was unavoidable.  So I'm willing to
> > > concede that.  Giving people time to migrate from old to new while still
> > > being able to update for other security fixes seems reasonable.
> > 
> > Jason, what do you think of the approach above? 
> 
> I say keep it simple.  We shouldn't use the DT changes to trigger one
> vice the other.  We need to be able to build both, but only load one at
> a time.  If that's anything other than simple to do, then we make it a
> Kconfig binary choice and move on.

Actually I was planning to handle it with a Kconfig dependency rule
(NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends
on !NEW_DRIVER).
I don't know how to make it a runtime check without adding new
compatible strings for the kirkwood, dove and orion platforms, and I'm
sure sure this is a good idea.
Do you have any ideas ?

Regards,

Boris


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

[Boris Brezillon]

On Fri, 17 Apr 2015 10:33:56 +0200
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> Hi Jason,
> 
> On Mon, 13 Apr 2015 20:11:46 +0000
> Jason Cooper <jason@lakedaemon.net> wrote:
> 
> > > 
> > > > I'd appreciate if we'd look into it.  I understand from on-list and
> > > > off-list discussion that the rewrite was unavoidable.  So I'm willing to
> > > > concede that.  Giving people time to migrate from old to new while still
> > > > being able to update for other security fixes seems reasonable.
> > > 
> > > Jason, what do you think of the approach above? 
> > 
> > I say keep it simple.  We shouldn't use the DT changes to trigger one
> > vice the other.  We need to be able to build both, but only load one at
> > a time.  If that's anything other than simple to do, then we make it a
> > Kconfig binary choice and move on.
> 
> Actually I was planning to handle it with a Kconfig dependency rule
> (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends
> on !NEW_DRIVER).
> I don't know how to make it a runtime check without adding new
> compatible strings for the kirkwood, dove and orion platforms, and I'm
> sure sure this is a good idea.
  ^ not

> Do you have any ideas ?
> 
> Regards,
> 
> Boris
> 
> 



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

[Jason Cooper]

Hey Boris,

On Fri, Apr 17, 2015 at 10:39:46AM +0200, Boris Brezillon wrote:
> On Fri, 17 Apr 2015 10:33:56 +0200 Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> > On Mon, 13 Apr 2015 20:11:46 +0000 Jason Cooper <jason@lakedaemon.net> wrote:
> > > > 
> > > > > I'd appreciate if we'd look into it.  I understand from on-list and
> > > > > off-list discussion that the rewrite was unavoidable.  So I'm willing to
> > > > > concede that.  Giving people time to migrate from old to new while still
> > > > > being able to update for other security fixes seems reasonable.
> > > > 
> > > > Jason, what do you think of the approach above? 
> > > 
> > > I say keep it simple.  We shouldn't use the DT changes to trigger one
> > > vice the other.  We need to be able to build both, but only load one at
> > > a time.  If that's anything other than simple to do, then we make it a
> > > Kconfig binary choice and move on.
> > 
> > Actually I was planning to handle it with a Kconfig dependency rule
> > (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends
> > on !NEW_DRIVER).
> > I don't know how to make it a runtime check without adding new
> > compatible strings for the kirkwood, dove and orion platforms, and I'm
> > sure sure this is a good idea.
>   ^ not
> 
> > Do you have any ideas ?

I'm kinda wrapped up with dayjob stuff atm...  But I'd look at the wireless
drivers.  eg b43, b43legacy, brcm80211.  There are devices they overlap for.
So, they need to deconflict in some way.

thx,

Jason.

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

[Gregory CLEMENT]

Hi Boris,

On 17/04/2015 10:39, Boris Brezillon wrote:
> On Fri, 17 Apr 2015 10:33:56 +0200
> Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> 
>> Hi Jason,
>>
>> On Mon, 13 Apr 2015 20:11:46 +0000
>> Jason Cooper <jason@lakedaemon.net> wrote:
>>
>>>>
>>>>> I'd appreciate if we'd look into it.  I understand from on-list and
>>>>> off-list discussion that the rewrite was unavoidable.  So I'm willing to
>>>>> concede that.  Giving people time to migrate from old to new while still
>>>>> being able to update for other security fixes seems reasonable.
>>>>
>>>> Jason, what do you think of the approach above? 
>>>
>>> I say keep it simple.  We shouldn't use the DT changes to trigger one
>>> vice the other.  We need to be able to build both, but only load one at
>>> a time.  If that's anything other than simple to do, then we make it a
>>> Kconfig binary choice and move on.
>>
>> Actually I was planning to handle it with a Kconfig dependency rule
>> (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends
>> on !NEW_DRIVER).
>> I don't know how to make it a runtime check without adding new
>> compatible strings for the kirkwood, dove and orion platforms, and I'm
>> sure sure this is a good idea.
>   ^ not
> 
>> Do you have any ideas ?

You use devm_ioremap_resource() in the new driver, so if the old one
is already loaded the memory region will be already hold and the new
driver will simply fail during the probe. So for this part it is OK.

However, the old driver doesn't try to reserve the region, it directly
uses an ioremap(). So if the new driver is loaded first, then the old
one will manage to be loaded too. I think that just adding a
request_region()/release_region() (or converting the ioremap in a
devm_ioremap_resource() in the old driver would be enough.


Gregory



-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

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

[Boris Brezillon]

Hi Gregory,

On Fri, 17 Apr 2015 15:01:01 +0200
Gregory CLEMENT <gregory.clement@free-electrons.com> wrote:

> Hi Boris,
> 
> On 17/04/2015 10:39, Boris Brezillon wrote:
> > On Fri, 17 Apr 2015 10:33:56 +0200
> > Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> > 
> >> Hi Jason,
> >>
> >> On Mon, 13 Apr 2015 20:11:46 +0000
> >> Jason Cooper <jason@lakedaemon.net> wrote:
> >>
> >>>>
> >>>>> I'd appreciate if we'd look into it.  I understand from on-list and
> >>>>> off-list discussion that the rewrite was unavoidable.  So I'm willing to
> >>>>> concede that.  Giving people time to migrate from old to new while still
> >>>>> being able to update for other security fixes seems reasonable.
> >>>>
> >>>> Jason, what do you think of the approach above? 
> >>>
> >>> I say keep it simple.  We shouldn't use the DT changes to trigger one
> >>> vice the other.  We need to be able to build both, but only load one at
> >>> a time.  If that's anything other than simple to do, then we make it a
> >>> Kconfig binary choice and move on.
> >>
> >> Actually I was planning to handle it with a Kconfig dependency rule
> >> (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends
> >> on !NEW_DRIVER).
> >> I don't know how to make it a runtime check without adding new
> >> compatible strings for the kirkwood, dove and orion platforms, and I'm
> >> sure sure this is a good idea.
> >   ^ not
> > 
> >> Do you have any ideas ?
> 
> You use devm_ioremap_resource() in the new driver, so if the old one
> is already loaded the memory region will be already hold and the new
> driver will simply fail during the probe. So for this part it is OK.

I like the idea :-).

> 
> However, the old driver doesn't try to reserve the region, it directly
> uses an ioremap(). So if the new driver is loaded first, then the old
> one will manage to be loaded too. I think that just adding a
> request_region()/release_region() (or converting the ioremap in a
> devm_ioremap_resource() in the old driver would be enough.

Absolutely. Unless someone is opposed to this solution I think I'll
choose this solution.

Thanks,

Boris


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

[Maxime Ripard]

On Fri, Apr 17, 2015 at 04:19:22PM +0200, Boris Brezillon wrote:
> Hi Gregory,
> 
> On Fri, 17 Apr 2015 15:01:01 +0200
> Gregory CLEMENT <gregory.clement@free-electrons.com> wrote:
> 
> > Hi Boris,
> > 
> > On 17/04/2015 10:39, Boris Brezillon wrote:
> > > On Fri, 17 Apr 2015 10:33:56 +0200
> > > Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> > > 
> > >> Hi Jason,
> > >>
> > >> On Mon, 13 Apr 2015 20:11:46 +0000
> > >> Jason Cooper <jason@lakedaemon.net> wrote:
> > >>
> > >>>>
> > >>>>> I'd appreciate if we'd look into it.  I understand from on-list and
> > >>>>> off-list discussion that the rewrite was unavoidable.  So I'm willing to
> > >>>>> concede that.  Giving people time to migrate from old to new while still
> > >>>>> being able to update for other security fixes seems reasonable.
> > >>>>
> > >>>> Jason, what do you think of the approach above? 
> > >>>
> > >>> I say keep it simple.  We shouldn't use the DT changes to trigger one
> > >>> vice the other.  We need to be able to build both, but only load one at
> > >>> a time.  If that's anything other than simple to do, then we make it a
> > >>> Kconfig binary choice and move on.
> > >>
> > >> Actually I was planning to handle it with a Kconfig dependency rule
> > >> (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends
> > >> on !NEW_DRIVER).
> > >> I don't know how to make it a runtime check without adding new
> > >> compatible strings for the kirkwood, dove and orion platforms, and I'm
> > >> sure sure this is a good idea.
> > >   ^ not
> > > 
> > >> Do you have any ideas ?
> > 
> > You use devm_ioremap_resource() in the new driver, so if the old one
> > is already loaded the memory region will be already hold and the new
> > driver will simply fail during the probe. So for this part it is OK.
> 
> I like the idea :-).

Not really, how do you know which device is going to be probed? For
that matter, it's pretty much random, and you have no control over it.

Why not just have a choice option, and select which one you want to
enable?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150417/1428ad9e/attachment.sig>

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

[Gregory CLEMENT]

Hi Maxime,

On 17/04/2015 16:32, Maxime Ripard wrote:
> On Fri, Apr 17, 2015 at 04:19:22PM +0200, Boris Brezillon wrote:
>> Hi Gregory,
>>
>> On Fri, 17 Apr 2015 15:01:01 +0200
>> Gregory CLEMENT <gregory.clement@free-electrons.com> wrote:
>>
>>> Hi Boris,
>>>
>>> On 17/04/2015 10:39, Boris Brezillon wrote:
>>>> On Fri, 17 Apr 2015 10:33:56 +0200
>>>> Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
>>>>
>>>>> Hi Jason,
>>>>>
>>>>> On Mon, 13 Apr 2015 20:11:46 +0000
>>>>> Jason Cooper <jason@lakedaemon.net> wrote:
>>>>>
>>>>>>>
>>>>>>>> I'd appreciate if we'd look into it.  I understand from on-list and
>>>>>>>> off-list discussion that the rewrite was unavoidable.  So I'm willing to
>>>>>>>> concede that.  Giving people time to migrate from old to new while still
>>>>>>>> being able to update for other security fixes seems reasonable.
>>>>>>>
>>>>>>> Jason, what do you think of the approach above? 
>>>>>>
>>>>>> I say keep it simple.  We shouldn't use the DT changes to trigger one
>>>>>> vice the other.  We need to be able to build both, but only load one at
>>>>>> a time.  If that's anything other than simple to do, then we make it a
>>>>>> Kconfig binary choice and move on.
>>>>>
>>>>> Actually I was planning to handle it with a Kconfig dependency rule
>>>>> (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends
>>>>> on !NEW_DRIVER).
>>>>> I don't know how to make it a runtime check without adding new
>>>>> compatible strings for the kirkwood, dove and orion platforms, and I'm
>>>>> sure sure this is a good idea.
>>>>   ^ not
>>>>
>>>>> Do you have any ideas ?
>>>
>>> You use devm_ioremap_resource() in the new driver, so if the old one
>>> is already loaded the memory region will be already hold and the new
>>> driver will simply fail during the probe. So for this part it is OK.
>>
>> I like the idea :-).
> 
> Not really, how do you know which device is going to be probed? For
> that matter, it's pretty much random, and you have no control over it.
> 
> Why not just have a choice option, and select which one you want to
> enable?

Because you can't prevent an user to build a module, then modifying the
configuration and building the other module. So even if there is a choice at
build time, and I think that it is something expected for the v2, we still need
preventing having the both drivers trying accessing the same hardware in the
same time.


Thanks,

Gregory



> 
> Maxime
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

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

[Maxime Ripard]

On Fri, Apr 17, 2015 at 04:40:43PM +0200, Gregory CLEMENT wrote:
> Hi Maxime,
> 
> On 17/04/2015 16:32, Maxime Ripard wrote:
> > On Fri, Apr 17, 2015 at 04:19:22PM +0200, Boris Brezillon wrote:
> >> Hi Gregory,
> >>
> >> On Fri, 17 Apr 2015 15:01:01 +0200
> >> Gregory CLEMENT <gregory.clement@free-electrons.com> wrote:
> >>
> >>> Hi Boris,
> >>>
> >>> On 17/04/2015 10:39, Boris Brezillon wrote:
> >>>> On Fri, 17 Apr 2015 10:33:56 +0200
> >>>> Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> >>>>
> >>>>> Hi Jason,
> >>>>>
> >>>>> On Mon, 13 Apr 2015 20:11:46 +0000
> >>>>> Jason Cooper <jason@lakedaemon.net> wrote:
> >>>>>
> >>>>>>>
> >>>>>>>> I'd appreciate if we'd look into it.  I understand from on-list and
> >>>>>>>> off-list discussion that the rewrite was unavoidable.  So I'm willing to
> >>>>>>>> concede that.  Giving people time to migrate from old to new while still
> >>>>>>>> being able to update for other security fixes seems reasonable.
> >>>>>>>
> >>>>>>> Jason, what do you think of the approach above? 
> >>>>>>
> >>>>>> I say keep it simple.  We shouldn't use the DT changes to trigger one
> >>>>>> vice the other.  We need to be able to build both, but only load one at
> >>>>>> a time.  If that's anything other than simple to do, then we make it a
> >>>>>> Kconfig binary choice and move on.
> >>>>>
> >>>>> Actually I was planning to handle it with a Kconfig dependency rule
> >>>>> (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends
> >>>>> on !NEW_DRIVER).
> >>>>> I don't know how to make it a runtime check without adding new
> >>>>> compatible strings for the kirkwood, dove and orion platforms, and I'm
> >>>>> sure sure this is a good idea.
> >>>>   ^ not
> >>>>
> >>>>> Do you have any ideas ?
> >>>
> >>> You use devm_ioremap_resource() in the new driver, so if the old one
> >>> is already loaded the memory region will be already hold and the new
> >>> driver will simply fail during the probe. So for this part it is OK.
> >>
> >> I like the idea :-).
> > 
> > Not really, how do you know which device is going to be probed? For
> > that matter, it's pretty much random, and you have no control over it.
> > 
> > Why not just have a choice option, and select which one you want to
> > enable?
> 
> Because you can't prevent an user to build a module, then modifying the
> configuration and building the other module.

Well, actually, you don't even know if it's going to be a module. You
might very well have both drivers compiled statically in the kernel
image, and this is where the trouble begins.

> So even if there is a choice at build time, and I think that it is
> something expected for the v2, we still need preventing having the
> both drivers trying accessing the same hardware in the same time.

Of course, but this is already there, and doesn't really address the
same issue.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150417/274bd08b/attachment.sig>

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

[Gregory CLEMENT]

On 17/04/2015 16:50, Maxime Ripard wrote:
> On Fri, Apr 17, 2015 at 04:40:43PM +0200, Gregory CLEMENT wrote:
>> Hi Maxime,
>>
>> On 17/04/2015 16:32, Maxime Ripard wrote:
>>> On Fri, Apr 17, 2015 at 04:19:22PM +0200, Boris Brezillon wrote:
>>>> Hi Gregory,
>>>>
>>>> On Fri, 17 Apr 2015 15:01:01 +0200
>>>> Gregory CLEMENT <gregory.clement@free-electrons.com> wrote:
>>>>
>>>>> Hi Boris,
>>>>>
>>>>> On 17/04/2015 10:39, Boris Brezillon wrote:
>>>>>> On Fri, 17 Apr 2015 10:33:56 +0200
>>>>>> Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
>>>>>>
>>>>>>> Hi Jason,
>>>>>>>
>>>>>>> On Mon, 13 Apr 2015 20:11:46 +0000
>>>>>>> Jason Cooper <jason@lakedaemon.net> wrote:
>>>>>>>
>>>>>>>>>
>>>>>>>>>> I'd appreciate if we'd look into it.  I understand from on-list and
>>>>>>>>>> off-list discussion that the rewrite was unavoidable.  So I'm willing to
>>>>>>>>>> concede that.  Giving people time to migrate from old to new while still
>>>>>>>>>> being able to update for other security fixes seems reasonable.
>>>>>>>>>
>>>>>>>>> Jason, what do you think of the approach above? 
>>>>>>>>
>>>>>>>> I say keep it simple.  We shouldn't use the DT changes to trigger one
>>>>>>>> vice the other.  We need to be able to build both, but only load one at
>>>>>>>> a time.  If that's anything other than simple to do, then we make it a
>>>>>>>> Kconfig binary choice and move on.
>>>>>>>
>>>>>>> Actually I was planning to handle it with a Kconfig dependency rule
>>>>>>> (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends
>>>>>>> on !NEW_DRIVER).
>>>>>>> I don't know how to make it a runtime check without adding new
>>>>>>> compatible strings for the kirkwood, dove and orion platforms, and I'm
>>>>>>> sure sure this is a good idea.
>>>>>>   ^ not
>>>>>>
>>>>>>> Do you have any ideas ?
>>>>>
>>>>> You use devm_ioremap_resource() in the new driver, so if the old one
>>>>> is already loaded the memory region will be already hold and the new
>>>>> driver will simply fail during the probe. So for this part it is OK.
>>>>
>>>> I like the idea :-).
>>>
>>> Not really, how do you know which device is going to be probed? For
>>> that matter, it's pretty much random, and you have no control over it.
>>>
>>> Why not just have a choice option, and select which one you want to
>>> enable?
>>
>> Because you can't prevent an user to build a module, then modifying the
>> configuration and building the other module.
> 
> Well, actually, you don't even know if it's going to be a module. You
> might very well have both drivers compiled statically in the kernel
> image, and this is where the trouble begins.

No it won't be possible, Boris already speak about this issue (see below):
"Actually I was planning to handle it with a Kconfig dependency rule
(NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends
on !NEW_DRIVER)."

> 
>> So even if there is a choice at build time, and I think that it is
>> something expected for the v2, we still need preventing having the
>> both drivers trying accessing the same hardware in the same time.
> 
> Of course, but this is already there, and doesn't really address the
> same issue.

This was the only issue remaining, (see below again):
"I don't know how to make it a runtime check ". And my last emails
was bout it.

Gregory


> 
> Maxime
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

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

[Maxime Ripard]

On Fri, Apr 17, 2015 at 05:01:55PM +0200, Gregory CLEMENT wrote:
> On 17/04/2015 16:50, Maxime Ripard wrote:
> > On Fri, Apr 17, 2015 at 04:40:43PM +0200, Gregory CLEMENT wrote:
> >> Hi Maxime,
> >>
> >> On 17/04/2015 16:32, Maxime Ripard wrote:
> >>> On Fri, Apr 17, 2015 at 04:19:22PM +0200, Boris Brezillon wrote:
> >>>> Hi Gregory,
> >>>>
> >>>> On Fri, 17 Apr 2015 15:01:01 +0200
> >>>> Gregory CLEMENT <gregory.clement@free-electrons.com> wrote:
> >>>>
> >>>>> Hi Boris,
> >>>>>
> >>>>> On 17/04/2015 10:39, Boris Brezillon wrote:
> >>>>>> On Fri, 17 Apr 2015 10:33:56 +0200
> >>>>>> Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> >>>>>>
> >>>>>>> Hi Jason,
> >>>>>>>
> >>>>>>> On Mon, 13 Apr 2015 20:11:46 +0000
> >>>>>>> Jason Cooper <jason@lakedaemon.net> wrote:
> >>>>>>>
> >>>>>>>>>
> >>>>>>>>>> I'd appreciate if we'd look into it.  I understand from on-list and
> >>>>>>>>>> off-list discussion that the rewrite was unavoidable.  So I'm willing to
> >>>>>>>>>> concede that.  Giving people time to migrate from old to new while still
> >>>>>>>>>> being able to update for other security fixes seems reasonable.
> >>>>>>>>>
> >>>>>>>>> Jason, what do you think of the approach above? 
> >>>>>>>>
> >>>>>>>> I say keep it simple.  We shouldn't use the DT changes to trigger one
> >>>>>>>> vice the other.  We need to be able to build both, but only load one at
> >>>>>>>> a time.  If that's anything other than simple to do, then we make it a
> >>>>>>>> Kconfig binary choice and move on.
> >>>>>>>
> >>>>>>> Actually I was planning to handle it with a Kconfig dependency rule
> >>>>>>> (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends
> >>>>>>> on !NEW_DRIVER).
> >>>>>>> I don't know how to make it a runtime check without adding new
> >>>>>>> compatible strings for the kirkwood, dove and orion platforms, and I'm
> >>>>>>> sure sure this is a good idea.
> >>>>>>   ^ not
> >>>>>>
> >>>>>>> Do you have any ideas ?
> >>>>>
> >>>>> You use devm_ioremap_resource() in the new driver, so if the old one
> >>>>> is already loaded the memory region will be already hold and the new
> >>>>> driver will simply fail during the probe. So for this part it is OK.
> >>>>
> >>>> I like the idea :-).
> >>>
> >>> Not really, how do you know which device is going to be probed? For
> >>> that matter, it's pretty much random, and you have no control over it.
> >>>
> >>> Why not just have a choice option, and select which one you want to
> >>> enable?
> >>
> >> Because you can't prevent an user to build a module, then modifying the
> >> configuration and building the other module.
> > 
> > Well, actually, you don't even know if it's going to be a module. You
> > might very well have both drivers compiled statically in the kernel
> > image, and this is where the trouble begins.
> 
> No it won't be possible, Boris already speak about this issue (see below):
> "Actually I was planning to handle it with a Kconfig dependency rule
> (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends
> on !NEW_DRIVER)."

Which is a circular dependency and won't work.

> >> So even if there is a choice at build time, and I think that it is
> >> something expected for the v2, we still need preventing having the
> >> both drivers trying accessing the same hardware in the same time.
> > 
> > Of course, but this is already there, and doesn't really address the
> > same issue.
> 
> This was the only issue remaining, (see below again):
> "I don't know how to make it a runtime check ". And my last emails
> was bout it.

Ok, my bad then :)

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150417/23e64995/attachment.sig>

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

[Gregory CLEMENT]

On 17/04/2015 17:49, Maxime Ripard wrote:
> On Fri, Apr 17, 2015 at 05:01:55PM +0200, Gregory CLEMENT wrote:
>> On 17/04/2015 16:50, Maxime Ripard wrote:
>>> On Fri, Apr 17, 2015 at 04:40:43PM +0200, Gregory CLEMENT wrote:
>>>> Hi Maxime,
>>>>
>>>> On 17/04/2015 16:32, Maxime Ripard wrote:
>>>>> On Fri, Apr 17, 2015 at 04:19:22PM +0200, Boris Brezillon wrote:
>>>>>> Hi Gregory,
>>>>>>
>>>>>> On Fri, 17 Apr 2015 15:01:01 +0200
>>>>>> Gregory CLEMENT <gregory.clement@free-electrons.com> wrote:
>>>>>>
>>>>>>> Hi Boris,
>>>>>>>
>>>>>>> On 17/04/2015 10:39, Boris Brezillon wrote:
>>>>>>>> On Fri, 17 Apr 2015 10:33:56 +0200
>>>>>>>> Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
>>>>>>>>
>>>>>>>>> Hi Jason,
>>>>>>>>>
>>>>>>>>> On Mon, 13 Apr 2015 20:11:46 +0000
>>>>>>>>> Jason Cooper <jason@lakedaemon.net> wrote:
>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> I'd appreciate if we'd look into it.  I understand from on-list and
>>>>>>>>>>>> off-list discussion that the rewrite was unavoidable.  So I'm willing to
>>>>>>>>>>>> concede that.  Giving people time to migrate from old to new while still
>>>>>>>>>>>> being able to update for other security fixes seems reasonable.
>>>>>>>>>>>
>>>>>>>>>>> Jason, what do you think of the approach above? 
>>>>>>>>>>
>>>>>>>>>> I say keep it simple.  We shouldn't use the DT changes to trigger one
>>>>>>>>>> vice the other.  We need to be able to build both, but only load one at
>>>>>>>>>> a time.  If that's anything other than simple to do, then we make it a
>>>>>>>>>> Kconfig binary choice and move on.
>>>>>>>>>
>>>>>>>>> Actually I was planning to handle it with a Kconfig dependency rule
>>>>>>>>> (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends
>>>>>>>>> on !NEW_DRIVER).
>>>>>>>>> I don't know how to make it a runtime check without adding new
>>>>>>>>> compatible strings for the kirkwood, dove and orion platforms, and I'm
>>>>>>>>> sure sure this is a good idea.
>>>>>>>>   ^ not
>>>>>>>>
>>>>>>>>> Do you have any ideas ?
>>>>>>>
>>>>>>> You use devm_ioremap_resource() in the new driver, so if the old one
>>>>>>> is already loaded the memory region will be already hold and the new
>>>>>>> driver will simply fail during the probe. So for this part it is OK.
>>>>>>
>>>>>> I like the idea :-).
>>>>>
>>>>> Not really, how do you know which device is going to be probed? For
>>>>> that matter, it's pretty much random, and you have no control over it.
>>>>>
>>>>> Why not just have a choice option, and select which one you want to
>>>>> enable?
>>>>
>>>> Because you can't prevent an user to build a module, then modifying the
>>>> configuration and building the other module.
>>>
>>> Well, actually, you don't even know if it's going to be a module. You
>>> might very well have both drivers compiled statically in the kernel
>>> image, and this is where the trouble begins.
>>
>> No it won't be possible, Boris already speak about this issue (see below):
>> "Actually I was planning to handle it with a Kconfig dependency rule
>> (NEW_DRIVER depends on !OLD_DRIVER and OLD_DRIVER depends
>> on !NEW_DRIVER)."
> 
> Which is a circular dependency and won't work.

Indeed.

Boris what about using choice/endchoice ?

Thanks,

Gregory



> 
>>>> So even if there is a choice at build time, and I think that it is
>>>> something expected for the v2, we still need preventing having the
>>>> both drivers trying accessing the same hardware in the same time.
>>>
>>> Of course, but this is already there, and doesn't really address the
>>> same issue.
>>
>> This was the only issue remaining, (see below again):
>> "I don't know how to make it a runtime check ". And my last emails
>> was bout it.
> 
> Ok, my bad then :)
> 
> Maxime
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[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@free-electrons.com> 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

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

[Herbert Xu]

On Tue, Apr 28, 2015 at 09:52:32PM +0200, Boris Brezillon wrote:
> 
> 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).

Either approach is fine with me.

Cheers,
-- 
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