Dear Fabio Estevam, > Hi Marek, > > On Thu, Sep 26, 2013 at 8:18 AM, Marek Vasut <marex@xxxxxxx> wrote: > > Add support for the MXS DCP block. The driver currently supports > > SHA-1/SHA-256 hashing and AES-128 CBC/ECB modes. The non-standard > > CRC32 is not yet supported. > > > > Signed-off-by: Marek Vasut <marex@xxxxxxx> > > Cc: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx> > > Cc: David S. Miller <davem@xxxxxxxxxxxxx> > > --- > > > > drivers/crypto/Kconfig | 17 + > > drivers/crypto/Makefile | 1 + > > drivers/crypto/mxs-dcp.c | 1082 > > ++++++++++++++++++++++++++++++++++++++++++++++ > > What about the existing DCP driver at drivers/crypto/dcp.c ? I was not aware of that one. > Why do we need to have two drivers for the same IP block? It looks > confusing to have both. Sure, I agree. I reviewed the one in mainline just now and I see some deficiencies of the dcp.c driver: 1) It only supports AES_CBC (mine does support AES_ECB, AES_CBC, SHA1 and SH256) 2) The driver was apparently never ran behind anyone working with MXS. ie.: -> Restarting the DCP block is not done via mxs_reset_block() -> The DT name is not "fsl,dcp" or "fsl,mxs-dcp" as other MXS drivers 3) What are those ugly new IOCTLs in the dcp.c driver? 4) The VMI IRQ is never used, yet it even calls the IRQ handler, this is bogus -> The DCP always triggers the dcp_irq upon DMA completion -> VMI IRQ is not handled in the handler at all as I see it 5) The IRQ handler can't use usual completion() in the driver because that'd trigger "scheduling while atomic" oops, yes? Finally, because the dcp.c driver only supports AES128 CBC, it depends on kernel _always_ passing the DCP scatterlist such that each of it's elements is 16-bytes long. If each of the elements is 16 bytes, there is no problem and the DCP will operate correctly. That is because the DCP has the following limitations: -> For AES128, each buffer in the DMA chain must be multiple of 16 bytes. -> For SHA1/SHA256, each buffer in the DMA chain must by multiple of 64 bytes BUT only the last buffer in the DMA chain can be shorter. So, in the AES128 case, if the hardware is passed two (4 bytes + 12 bytes for example) DMA descriptors instead of single 16 bytes descriptor, the DCP will simply stall or produce incorrect result. This can happen if the user of the async crypto API passes such a scatterlist. It is true this is not caught by the in-kernel crypto tests, because they mostly trigger the software fallback in this driver, since this driver only supports 16 byte key (klen == 16 , see crypto/testmgr.h). It can be triggered by modifying the crypto tests so that they pass two buffers, multiple of 16 bytes in total, but each of them not multiple of 16 bytes. I ran into many such problems when I was developing this driver I submitted here, I managed to trigger those by running the Cryptodev [1] tests [2] against this driver as well as testing the performance of this driver via Cryptodev/OpenSSL combination: $ openssl speed sha1 $ openssl speed sha256 $ openssl speed aes-128-cbc I also measured the performance via OpenSSL encryption/decryption. On larger files, the performance of encryption/decryption via DCP, even with fixups for unaligned buffers and the overhead of userspace-kernel-userspace switches due to cryptodev is still higher than software implementation: $ time openssl enc -aes-128-ecb -in $IFILE -out $OFILE -k "test" -nosalt $ time openssl enc -d -aes-128-ecb -in $OFILE -out $DFILE -k "test" -nosalt Also, since OpenSSH uses OpenSSL, this helped my testing too. [1] http://cryptodev-linux.org/ [2] https://github.com/nmav/cryptodev-linux/tree/master/tests Best regards, Marek Vasut -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
