[PATCH v3 00/11] crypto: talitos - fix several issues in the Freescale talitos crypto driver

Paul Louvel posted 11 patches 1 month ago
drivers/crypto/talitos.c | 549 ++++++++++++++++++++++++-----------------------
drivers/crypto/talitos.h |  12 ++
2 files changed, 287 insertions(+), 274 deletions(-)
[PATCH v3 00/11] crypto: talitos - fix several issues in the Freescale talitos crypto driver
Posted by Paul Louvel 1 month ago
This series fixes several issues in the Freescale talitos crypto driver.

Patch 1 fixes a missing dma_sync_single_for_cpu() before reading a
descriptor header.

Patches 2-5 add support for chaining an arbitrary number of descriptors
in the driver for the SEC1 hardware.

Patches 6-8 rework the SEC1 hash implementation to build descriptor
chains instead of submitting one descriptor at a time via a workqueue.

Patches 9-10 are cleanups: rename first_desc/last_desc to
first_request/last_request, and remove a useless wrapper function.

Patch 11 fixes the same ahash request size limitation on SEC2 (64k - 1
bytes), by splitting ahash_done() into SEC1 and SEC2 paths so that SEC2
iterates through descriptors sequentially.

Tested on an MPC885 SoC (SEC1 Lite), and on an MPC8321EMP SoC (SEC2)
with CRYPTO_SELFTESTS_FULL=y.
For the SEC1 Lite, some tests are failing due to a timeout waiting for
request completion. These failed tests existed prior to this series.
On SEC2, there is no failed tests.

Signed-off-by: Paul Louvel <paul.louvel@bootlin.com>
---
Changes in v3:
- Patch 1 was reading the next chained descriptor header
  unconditionally. Fixed it by checking if a request actually contained
  a chained descriptor before deferencing it.
- For descriptor chaining introduced in patch 2, use a next pointer
  embedded inside struct talitos_edesc instead of the kernel's struct
  list_node. This removes the necessity for a desc_chain member in
  struct talitos_request. A dirty hack was previously used to assign a
  request->desc_chain to the current request by taking edesc->prev,
  assuming that the descriptor was added to a list before calling
  talitos_submit(). Not only was this non-idiomatic, but it also broke
  the skcipher and aead implementations because they do not use the
  descriptor chaining feature at all. The descriptor chaining mechanism
  does not need a doubly circular linked list; this change makes the
  code more readable than sticking with the kernel linked list
  implementation.
- Updated the performance measurement in patch 6.
- Drop patch 12, which was a revert of commit 4b24ea971a93 ("crypto:
  talitos - Preempt overflow interrupts off-by-one fix"). This patch was
  primarily motivated because the SEC1 has a Fetch Register rather than
  a Fetch FIFO per channel. As a result, having a value of 24 in the
  device tree node for the channel-fifo-len property does not make
  sense, as the hardware does not have a Fetch FIFO. Setting this value
  to 1 (which should be the correct value for SoCs featuring the SEC1
  engine family) breaks the driver because no descriptor can be
  submitted due to commit 4b24ea971a93, and the patch was primarily
  intended to fix this issue. As this issue is too deep to be addressed
  in this patch series, it has been dropped.
- Link to v2: https://patch.msgid.link/20260505-bootlin_test-7-1-rc1_sec_bugfix-v2-0-5818064bd190@bootlin.com

Changes in v2:
- Split the first patch into smaller, logically separated patches for
  easier review.
- Added more context on testing on the cover letter.
- Introduce a fix to correctly read hardware descriptor header. This fix
  was motivated by a remark of Sashiko on the v1:
  https://sashiko.dev/#/patchset/20260504-bootlin_test-7-1-rc1_sec_bugfix-v1-0-c97c641976f5%40bootlin.com
- Separate SEC2 64k-1 ahash limitation fix into its own patch.
- Link to v1: https://patch.msgid.link/20260504-bootlin_test-7-1-rc1_sec_bugfix-v1-0-c97c641976f5@bootlin.com

To: Herbert Xu <herbert@gondor.apana.org.au>
To: "David S. Miller" <davem@davemloft.net>
To: Christophe Leroy <chleroy@kernel.org>
To: Paolo Abeni <pabeni@redhat.com>
To: David Howells <dhowells@redhat.com>
Cc: linux-crypto@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

---
Paul Louvel (11):
      crypto: talitos - use dma_sync_single_for_cpu() before reading descriptor header
      crypto: talitos - add chaining of arbitrary number of descriptor for the SEC1
      crypto: talitos - move dma unmapping code in flush_channel() into a standalone dma_unmap_request() function
      crypto: talitos - move dma mapping code in talitos_submit() into a standalone dma_map_request() function
      crypto: talitos - move code in current_desc_hdr() into a standalone function
      crypto: talitos/hash - prepare SEC1 descriptor chaining, remove additional descriptor
      crypto: talitos/hash - use descriptor chaining for SEC1 instead of workqueue
      crypto: talitos/hash - drop workqueue mechanism for SEC1
      crypto: talitos/hash - rename first_desc/last_desc to first_request/last_request
      crypto: talitos/hash - remove useless wrapper
      crypto: talitos/hash - fix SEC2 64k - 1 ahash request limitation

 drivers/crypto/talitos.c | 549 ++++++++++++++++++++++++-----------------------
 drivers/crypto/talitos.h |  12 ++
 2 files changed, 287 insertions(+), 274 deletions(-)
---
base-commit: db8b9f227833e729faf44a512aa1e88a625b5ad8
change-id: 20260504-bootlin_test-7-1-rc1_sec_bugfix-13169ed07ddc

Best regards,
--  
Paul Louvel <paul.louvel@bootlin.com>
Re: [PATCH v3 00/11] crypto: talitos - fix several issues in the Freescale talitos crypto driver
Posted by Herbert Xu 4 weeks, 1 day ago
On Thu, May 07, 2026 at 04:41:46PM +0200, Paul Louvel wrote:
> This series fixes several issues in the Freescale talitos crypto driver.
> 
> Patch 1 fixes a missing dma_sync_single_for_cpu() before reading a
> descriptor header.
> 
> Patches 2-5 add support for chaining an arbitrary number of descriptors
> in the driver for the SEC1 hardware.
> 
> Patches 6-8 rework the SEC1 hash implementation to build descriptor
> chains instead of submitting one descriptor at a time via a workqueue.
> 
> Patches 9-10 are cleanups: rename first_desc/last_desc to
> first_request/last_request, and remove a useless wrapper function.
> 
> Patch 11 fixes the same ahash request size limitation on SEC2 (64k - 1
> bytes), by splitting ahash_done() into SEC1 and SEC2 paths so that SEC2
> iterates through descriptors sequentially.
> 
> Tested on an MPC885 SoC (SEC1 Lite), and on an MPC8321EMP SoC (SEC2)
> with CRYPTO_SELFTESTS_FULL=y.
> For the SEC1 Lite, some tests are failing due to a timeout waiting for
> request completion. These failed tests existed prior to this series.
> On SEC2, there is no failed tests.
> 
> Signed-off-by: Paul Louvel <paul.louvel@bootlin.com>
> ---
> Changes in v3:
> - Patch 1 was reading the next chained descriptor header
>   unconditionally. Fixed it by checking if a request actually contained
>   a chained descriptor before deferencing it.
> - For descriptor chaining introduced in patch 2, use a next pointer
>   embedded inside struct talitos_edesc instead of the kernel's struct
>   list_node. This removes the necessity for a desc_chain member in
>   struct talitos_request. A dirty hack was previously used to assign a
>   request->desc_chain to the current request by taking edesc->prev,
>   assuming that the descriptor was added to a list before calling
>   talitos_submit(). Not only was this non-idiomatic, but it also broke
>   the skcipher and aead implementations because they do not use the
>   descriptor chaining feature at all. The descriptor chaining mechanism
>   does not need a doubly circular linked list; this change makes the
>   code more readable than sticking with the kernel linked list
>   implementation.
> - Updated the performance measurement in patch 6.
> - Drop patch 12, which was a revert of commit 4b24ea971a93 ("crypto:
>   talitos - Preempt overflow interrupts off-by-one fix"). This patch was
>   primarily motivated because the SEC1 has a Fetch Register rather than
>   a Fetch FIFO per channel. As a result, having a value of 24 in the
>   device tree node for the channel-fifo-len property does not make
>   sense, as the hardware does not have a Fetch FIFO. Setting this value
>   to 1 (which should be the correct value for SoCs featuring the SEC1
>   engine family) breaks the driver because no descriptor can be
>   submitted due to commit 4b24ea971a93, and the patch was primarily
>   intended to fix this issue. As this issue is too deep to be addressed
>   in this patch series, it has been dropped.
> - Link to v2: https://patch.msgid.link/20260505-bootlin_test-7-1-rc1_sec_bugfix-v2-0-5818064bd190@bootlin.com
> 
> Changes in v2:
> - Split the first patch into smaller, logically separated patches for
>   easier review.
> - Added more context on testing on the cover letter.
> - Introduce a fix to correctly read hardware descriptor header. This fix
>   was motivated by a remark of Sashiko on the v1:
>   https://sashiko.dev/#/patchset/20260504-bootlin_test-7-1-rc1_sec_bugfix-v1-0-c97c641976f5%40bootlin.com
> - Separate SEC2 64k-1 ahash limitation fix into its own patch.
> - Link to v1: https://patch.msgid.link/20260504-bootlin_test-7-1-rc1_sec_bugfix-v1-0-c97c641976f5@bootlin.com
> 
> To: Herbert Xu <herbert@gondor.apana.org.au>
> To: "David S. Miller" <davem@davemloft.net>
> To: Christophe Leroy <chleroy@kernel.org>
> To: Paolo Abeni <pabeni@redhat.com>
> To: David Howells <dhowells@redhat.com>
> Cc: linux-crypto@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> 
> ---
> Paul Louvel (11):
>       crypto: talitos - use dma_sync_single_for_cpu() before reading descriptor header
>       crypto: talitos - add chaining of arbitrary number of descriptor for the SEC1
>       crypto: talitos - move dma unmapping code in flush_channel() into a standalone dma_unmap_request() function
>       crypto: talitos - move dma mapping code in talitos_submit() into a standalone dma_map_request() function
>       crypto: talitos - move code in current_desc_hdr() into a standalone function
>       crypto: talitos/hash - prepare SEC1 descriptor chaining, remove additional descriptor
>       crypto: talitos/hash - use descriptor chaining for SEC1 instead of workqueue
>       crypto: talitos/hash - drop workqueue mechanism for SEC1
>       crypto: talitos/hash - rename first_desc/last_desc to first_request/last_request
>       crypto: talitos/hash - remove useless wrapper
>       crypto: talitos/hash - fix SEC2 64k - 1 ahash request limitation
> 
>  drivers/crypto/talitos.c | 549 ++++++++++++++++++++++++-----------------------
>  drivers/crypto/talitos.h |  12 ++
>  2 files changed, 287 insertions(+), 274 deletions(-)
> ---
> base-commit: db8b9f227833e729faf44a512aa1e88a625b5ad8
> change-id: 20260504-bootlin_test-7-1-rc1_sec_bugfix-13169ed07ddc
> 
> Best regards,
> --  
> Paul Louvel <paul.louvel@bootlin.com>

All applied.  Thanks.
-- 
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