From: Tobias Rauter <tobiasrauter@gmail.com> To: Marek Vasut <marex@denx.de>, Fabio Estevam <festevam@gmail.com> Cc: linux-crypto@vger.kernel.org, Herbert Xu <herbert@gondor.apana.org.au>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, "David S. Miller" <davem@davemloft.net>, Shawn Guo <shawn.guo@linaro.org> Subject: Re: [PATCH 2/3] ARM: mxs: crypto: Add Freescale MXS DCP driver Date: Fri, 27 Sep 2013 18:03:39 +0200 [thread overview] Message-ID: <5245AC5B.80800@gmail.com> (raw) In-Reply-To: <201309261407.33923.marex@denx.de> Hi, Here are some thoughts of the design decisions I made when I wrote the dcp.c driver. Maybe it helps. On 2013-09-26 14:07, Marek Vasut wrote: > Dear Fabio Estevam, > >> Hi Marek, >> >> 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) Right, but for ECP only the interface is missing (and it is no real mode of operation) and hashes should be generally faster in SW. > 2) The driver was apparently never ran behind anyone working with MXS. That is probably right. > 3) What are those ugly new IOCTLs in the dcp.c driver? When I firstly posted the driver in the mailinglist, there where one person who actually used this interface (it was introduced in Freescale's SDK) to use the OTP keys for crypto. As far as I have seen, the crypto API does not support such keys (i.e. there seems to be no way to tell a driver to use some kind of special keys - which are not delivered by the user - via the API). Therefore I added this miscdevice and adopted Freescale's interface. > 4) The VMI IRQ is never used, yet it even calls the IRQ handler, this is bogus That's absolutely right. > -> The DCP always triggers the dcp_irq upon DMA completion The IRQ is triggered after every packet, to enable simultaneous work for CPU/DCP: While the DCP is computing, the CPU is able to fill more packets. I don't know how far this is useful, because the 20 Packets which are enabled by default can address up to 80kB of plain-/ciphertext. However, I think it is better to do the work simultaneously to safe time (actual real world time, not CPU time). > 5) The IRQ handler can't use usual completion() in the driver because that'd > trigger "scheduling while atomic" oops, yes? I decided to use the tasklets because of performance reasons. I don't remember numbers but a workqueue was significantly slower. The use of a kernel thread may reduce the overhead compared to the wq. I was not sure if it is appropriate to create an extra thread for a crypto-driver, without real reason (IMHO). > 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. [...] > 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. The scatterlist alignment and bounce-buffering to get full 16 Byte blocks is done by the ablkcipher_walk API (with the error parameter) when needed. As far as I see, you are copying the whole buffer to your coherent block and back. Wouldn't it be better to do that just for unaligned blocks? kind regards, tr
WARNING: multiple messages have this Message-ID (diff)
From: tobiasrauter@gmail.com (Tobias Rauter) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 2/3] ARM: mxs: crypto: Add Freescale MXS DCP driver Date: Fri, 27 Sep 2013 18:03:39 +0200 [thread overview] Message-ID: <5245AC5B.80800@gmail.com> (raw) In-Reply-To: <201309261407.33923.marex@denx.de> Hi, Here are some thoughts of the design decisions I made when I wrote the dcp.c driver. Maybe it helps. On 2013-09-26 14:07, Marek Vasut wrote: > Dear Fabio Estevam, > >> Hi Marek, >> >> 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) Right, but for ECP only the interface is missing (and it is no real mode of operation) and hashes should be generally faster in SW. > 2) The driver was apparently never ran behind anyone working with MXS. That is probably right. > 3) What are those ugly new IOCTLs in the dcp.c driver? When I firstly posted the driver in the mailinglist, there where one person who actually used this interface (it was introduced in Freescale's SDK) to use the OTP keys for crypto. As far as I have seen, the crypto API does not support such keys (i.e. there seems to be no way to tell a driver to use some kind of special keys - which are not delivered by the user - via the API). Therefore I added this miscdevice and adopted Freescale's interface. > 4) The VMI IRQ is never used, yet it even calls the IRQ handler, this is bogus That's absolutely right. > -> The DCP always triggers the dcp_irq upon DMA completion The IRQ is triggered after every packet, to enable simultaneous work for CPU/DCP: While the DCP is computing, the CPU is able to fill more packets. I don't know how far this is useful, because the 20 Packets which are enabled by default can address up to 80kB of plain-/ciphertext. However, I think it is better to do the work simultaneously to safe time (actual real world time, not CPU time). > 5) The IRQ handler can't use usual completion() in the driver because that'd > trigger "scheduling while atomic" oops, yes? I decided to use the tasklets because of performance reasons. I don't remember numbers but a workqueue was significantly slower. The use of a kernel thread may reduce the overhead compared to the wq. I was not sure if it is appropriate to create an extra thread for a crypto-driver, without real reason (IMHO). > 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. [...] > 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. The scatterlist alignment and bounce-buffering to get full 16 Byte blocks is done by the ablkcipher_walk API (with the error parameter) when needed. As far as I see, you are copying the whole buffer to your coherent block and back. Wouldn't it be better to do that just for unaligned blocks? kind regards, tr
next prev parent reply other threads:[~2013-09-27 16:03 UTC|newest] Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top 2013-09-26 11:18 [PATCH 1/3] crypto: Fully restore ahash request before completing Marek Vasut 2013-09-26 11:18 ` Marek Vasut 2013-09-26 11:18 ` [PATCH 2/3] ARM: mxs: crypto: Add Freescale MXS DCP driver Marek Vasut 2013-09-26 11:18 ` Marek Vasut 2013-09-26 11:27 ` Fabio Estevam 2013-09-26 11:27 ` Fabio Estevam 2013-09-26 12:07 ` Marek Vasut 2013-09-26 12:07 ` Marek Vasut 2013-09-27 16:03 ` Tobias Rauter [this message] 2013-09-27 16:03 ` Tobias Rauter 2013-09-28 3:35 ` Marek Vasut 2013-09-28 3:35 ` Marek Vasut 2013-10-07 9:50 ` Christoph G. Baumann 2013-10-07 9:50 ` Christoph G. Baumann 2013-10-07 15:48 ` Marek Vasut 2013-10-07 15:48 ` Marek Vasut 2013-10-08 4:36 ` Herbert Xu 2013-10-08 4:36 ` Herbert Xu 2013-10-08 10:33 ` Marek Vasut 2013-10-08 10:33 ` Marek Vasut 2013-11-10 17:48 ` Marek Vasut 2013-11-10 17:48 ` Marek Vasut 2013-11-20 11:20 ` Herbert Xu 2013-11-20 11:20 ` Herbert Xu 2013-12-01 22:20 ` Marek Vasut 2013-12-01 22:20 ` Marek Vasut 2013-09-26 12:13 ` Lothar Waßmann 2013-09-26 12:13 ` Lothar Waßmann 2013-09-26 14:04 ` Marek Vasut 2013-09-26 14:04 ` Marek Vasut 2013-09-26 14:09 ` Lothar Waßmann 2013-09-26 14:09 ` Lothar Waßmann 2013-09-29 22:05 ` Marek Vasut 2013-09-29 22:05 ` Marek Vasut 2013-09-26 12:37 ` Veli-Pekka Peltola 2013-09-26 12:37 ` Veli-Pekka Peltola 2013-09-26 14:02 ` Marek Vasut 2013-09-26 14:02 ` Marek Vasut 2013-09-26 12:48 ` Fabio Estevam 2013-09-26 12:48 ` Fabio Estevam 2013-09-29 22:09 ` Marek Vasut 2013-09-29 22:09 ` Marek Vasut 2013-09-26 11:18 ` [PATCH 3/3] ARM: mxs: dts: Enable DCP for MXS Marek Vasut 2013-09-26 11:18 ` Marek Vasut 2013-09-26 11:36 ` Lothar Waßmann 2013-09-26 11:36 ` Lothar Waßmann 2013-09-26 12:08 ` Marek Vasut 2013-09-26 12:08 ` Marek Vasut 2013-09-29 22:11 ` Marek Vasut 2013-09-29 22:11 ` Marek Vasut
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=5245AC5B.80800@gmail.com \ --to=tobiasrauter@gmail.com \ --cc=davem@davemloft.net \ --cc=festevam@gmail.com \ --cc=herbert@gondor.apana.org.au \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-crypto@vger.kernel.org \ --cc=marex@denx.de \ --cc=shawn.guo@linaro.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.