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

[Herbert Xu <herbert@gondor.apana.org.au>]

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