Re: [PATCH 05/29] crypto: talitos - Prepare crypto implementation file splitting

[David Laight <david.laight.linux@gmail.com>]

On Fri, 29 May 2026 15:21:39 +0200
"Christophe Leroy (CS GROUP)" <chleroy@kernel.org> wrote:

> Hi Paul,
> 
> Le 28/05/2026 à 11:08, Paul Louvel a écrit :
> > Remove the static qualifier on multiple function that will be called
> > inside each crypto implementation file.
> > Add them to the main driver header file.  
> 
> I didn't have time to look at the generated text yet but I'm a bit 
> sceptic with this change, or more than the change itself, about its 
> purpose. And even more when I see patches 24 and 25.
> 
> Most functions here are small helpers. To be shared between several C 
> files they deserve becoming static inlines in talitos.h, not global 
> functions.
> 
> Indeed, most of the time is_sec1 is known at build time because in most 
> cases has_ftr_sec1() will constant fold into true or false during build. 
> This is because it is very unlikely that someone build a kernel to run 
> on both MPC 82xx and MPC 83xx at the same time. Therefore it is really 
> unlikely that this in built with both CRYPTO_DEV_TALITOS1 and 
> CRYPTO_DEV_TALITOS2 at the same time.
> 
> I can understand for a function like talitos_submit() but not for 
> functions like to_talitos_ptr() or to_talitos_ptr_ext_set() whose 
> purpose is really to get inlined into the caller.

I also spotted a couple of indirect calls being added.
They will be slower than you might think.
If there are only two options it is better to use an 'if'.

-- David

> 
> Christophe
> 
> 
> > 
> > Add the common structures too.
> > 
> > Signed-off-by: Paul Louvel <paul.louvel@bootlin.com>
> > ---
> >   drivers/crypto/talitos/talitos.c | 123 ++++++++++++++-------------------------
> >   drivers/crypto/talitos/talitos.h |  91 +++++++++++++++++++++++++++++
> >   2 files changed, 135 insertions(+), 79 deletions(-)
> > 
> > diff --git a/drivers/crypto/talitos/talitos.c b/drivers/crypto/talitos/talitos.c
> > index f5feff8f7d3d..3fc1069062da 100644
> > --- a/drivers/crypto/talitos/talitos.c
> > +++ b/drivers/crypto/talitos/talitos.c
> > @@ -40,8 +40,8 @@
> >   
> >   #include "talitos.h"
> >   
> > -static void to_talitos_ptr(struct talitos_ptr *ptr, dma_addr_t dma_addr,
> > -			   unsigned int len, bool is_sec1)
> > +void to_talitos_ptr(struct talitos_ptr *ptr, dma_addr_t dma_addr,
> > +		    unsigned int len, bool is_sec1)
> >   {
> >   	ptr->ptr = cpu_to_be32(lower_32_bits(dma_addr));
> >   	if (is_sec1) {
> > @@ -52,8 +52,8 @@ static void to_talitos_ptr(struct talitos_ptr *ptr, dma_addr_t dma_addr,
> >   	}
> >   }
> >   
> > -static void copy_talitos_ptr(struct talitos_ptr *dst_ptr,
> > -			     struct talitos_ptr *src_ptr, bool is_sec1)
> > +void copy_talitos_ptr(struct talitos_ptr *dst_ptr,
> > +		      struct talitos_ptr *src_ptr, bool is_sec1)
> >   {
> >   	dst_ptr->ptr = src_ptr->ptr;
> >   	if (is_sec1) {
> > @@ -64,8 +64,8 @@ static void copy_talitos_ptr(struct talitos_ptr *dst_ptr,
> >   	}
> >   }
> >   
> > -static unsigned short from_talitos_ptr_len(struct talitos_ptr *ptr,
> > -					   bool is_sec1)
> > +unsigned short from_talitos_ptr_len(struct talitos_ptr *ptr,
> > +				    bool is_sec1)
> >   {
> >   	if (is_sec1)
> >   		return be16_to_cpu(ptr->len1);
> > @@ -73,14 +73,14 @@ static unsigned short from_talitos_ptr_len(struct talitos_ptr *ptr,
> >   		return be16_to_cpu(ptr->len);
> >   }
> >   
> > -static void to_talitos_ptr_ext_set(struct talitos_ptr *ptr, u8 val,
> > -				   bool is_sec1)
> > +void to_talitos_ptr_ext_set(struct talitos_ptr *ptr, u8 val,
> > +			    bool is_sec1)
> >   {
> >   	if (!is_sec1)
> >   		ptr->j_extent = val;
> >   }
> >   
> > -static void to_talitos_ptr_ext_or(struct talitos_ptr *ptr, u8 val, bool is_sec1)
> > +void to_talitos_ptr_ext_or(struct talitos_ptr *ptr, u8 val, bool is_sec1)
> >   {
> >   	if (!is_sec1)
> >   		ptr->j_extent |= val;
> > @@ -102,15 +102,15 @@ static void __map_single_talitos_ptr(struct device *dev,
> >   	to_talitos_ptr(ptr, dma_addr, len, is_sec1);
> >   }
> >   
> > -static void map_single_talitos_ptr(struct device *dev,
> > -				   struct talitos_ptr *ptr,
> > -				   unsigned int len, void *data,
> > -				   enum dma_data_direction dir)
> > +void map_single_talitos_ptr(struct device *dev,
> > +			    struct talitos_ptr *ptr,
> > +			    unsigned int len, void *data,
> > +			    enum dma_data_direction dir)
> >   {
> >   	__map_single_talitos_ptr(dev, ptr, len, data, dir, 0);
> >   }
> >   
> > -static void map_single_talitos_ptr_nosync(struct device *dev,
> > +void map_single_talitos_ptr_nosync(struct device *dev,
> >   					  struct talitos_ptr *ptr,
> >   					  unsigned int len, void *data,
> >   					  enum dma_data_direction dir)
> > @@ -122,9 +122,9 @@ static void map_single_talitos_ptr_nosync(struct device *dev,
> >   /*
> >    * unmap bus single (contiguous) h/w descriptor pointer
> >    */
> > -static void unmap_single_talitos_ptr(struct device *dev,
> > -				     struct talitos_ptr *ptr,
> > -				     enum dma_data_direction dir)
> > +void unmap_single_talitos_ptr(struct device *dev,
> > +			      struct talitos_ptr *ptr,
> > +			      enum dma_data_direction dir)
> >   {
> >   	struct talitos_private *priv = dev_get_drvdata(dev);
> >   	bool is_sec1 = has_ftr_sec1(priv);
> > @@ -303,11 +303,11 @@ static void dma_map_request(struct device *dev, struct talitos_request *request,
> >    * callback must check err and feedback in descriptor header
> >    * for device processing status.
> >    */
> > -static int talitos_submit(struct device *dev, int ch, struct talitos_desc *desc,
> > -			  void (*callback)(struct device *dev,
> > -					   struct talitos_desc *desc,
> > -					   void *context, int error),
> > -			  void *context)
> > +int talitos_submit(struct device *dev, int ch, struct talitos_desc *desc,
> > +		   void (*callback)(struct device *dev,
> > +				    struct talitos_desc *desc,
> > +				    void *context, int error),
> > +		   void *context)
> >   {
> >   	struct talitos_private *priv = dev_get_drvdata(dev);
> >   	struct talitos_request *request;
> > @@ -830,24 +830,6 @@ DEF_TALITOS2_INTERRUPT(ch1_3, TALITOS2_ISR_CH_1_3_DONE, TALITOS2_ISR_CH_1_3_ERR,
> >    * HMAC_SNOOP_NO_AFEA (HSNA) instead of type IPSEC_ESP
> >    */
> >   #define TALITOS_CRA_PRIORITY_AEAD_HSNA	(TALITOS_CRA_PRIORITY - 1)
> > -#ifdef CONFIG_CRYPTO_DEV_TALITOS2
> > -#define TALITOS_MAX_KEY_SIZE		(AES_MAX_KEY_SIZE + SHA512_BLOCK_SIZE)
> > -#else
> > -#define TALITOS_MAX_KEY_SIZE		(AES_MAX_KEY_SIZE + SHA256_BLOCK_SIZE)
> > -#endif
> > -#define TALITOS_MAX_IV_LENGTH		16 /* max of AES_BLOCK_SIZE, DES3_EDE_BLOCK_SIZE */
> > -
> > -struct talitos_ctx {
> > -	struct device *dev;
> > -	int ch;
> > -	__be32 desc_hdr_template;
> > -	u8 key[TALITOS_MAX_KEY_SIZE];
> > -	u8 iv[TALITOS_MAX_IV_LENGTH];
> > -	dma_addr_t dma_key;
> > -	unsigned int keylen;
> > -	unsigned int enckeylen;
> > -	unsigned int authkeylen;
> > -};
> >   
> >   #define HASH_MAX_BLOCK_SIZE		SHA512_BLOCK_SIZE
> >   #define TALITOS_MDEU_MAX_CONTEXT_SIZE	TALITOS_MDEU_CONTEXT_SIZE_SHA384_SHA512
> > @@ -939,7 +921,7 @@ static int aead_des3_setkey(struct crypto_aead *authenc,
> >   	return err;
> >   }
> >   
> > -static void talitos_sg_unmap(struct device *dev,
> > +void talitos_sg_unmap(struct device *dev,
> >   			     struct talitos_edesc *edesc,
> >   			     struct scatterlist *src,
> >   			     struct scatterlist *dst,
> > @@ -1124,7 +1106,7 @@ static int sg_to_link_tbl_offset(struct scatterlist *sg, int sg_count,
> >   	return count;
> >   }
> >   
> > -static int talitos_sg_map_ext(struct device *dev, struct scatterlist *src,
> > +int talitos_sg_map_ext(struct device *dev, struct scatterlist *src,
> >   			      unsigned int len, struct talitos_edesc *edesc,
> >   			      struct talitos_ptr *ptr, int sg_count,
> >   			      unsigned int offset, int tbl_off, int elen,
> > @@ -1161,7 +1143,7 @@ static int talitos_sg_map_ext(struct device *dev, struct scatterlist *src,
> >   	return sg_count;
> >   }
> >   
> > -static int talitos_sg_map(struct device *dev, struct scatterlist *src,
> > +int talitos_sg_map(struct device *dev, struct scatterlist *src,
> >   			  unsigned int len, struct talitos_edesc *edesc,
> >   			  struct talitos_ptr *ptr, int sg_count,
> >   			  unsigned int offset, int tbl_off)
> > @@ -1299,17 +1281,17 @@ static int ipsec_esp(struct talitos_edesc *edesc, struct aead_request *areq,
> >   /*
> >    * allocate and map the extended descriptor
> >    */
> > -static struct talitos_edesc *talitos_edesc_alloc(struct device *dev,
> > -						 struct scatterlist *src,
> > -						 struct scatterlist *dst,
> > -						 u8 *iv,
> > -						 unsigned int assoclen,
> > -						 unsigned int cryptlen,
> > -						 unsigned int authsize,
> > -						 unsigned int ivsize,
> > -						 int icv_stashing,
> > -						 u32 cryptoflags,
> > -						 bool encrypt)
> > +struct talitos_edesc *talitos_edesc_alloc(struct device *dev,
> > +					  struct scatterlist *src,
> > +					  struct scatterlist *dst,
> > +					  u8 *iv,
> > +					  unsigned int assoclen,
> > +					  unsigned int cryptlen,
> > +					  unsigned int authsize,
> > +					  unsigned int ivsize,
> > +					  int icv_stashing,
> > +					  u32 cryptoflags,
> > +					  bool encrypt)
> >   {
> >   	struct talitos_edesc *edesc;
> >   	int src_nents, dst_nents, alloc_len, dma_len, src_len, dst_len;
> > @@ -2172,18 +2154,6 @@ static int ahash_setkey(struct crypto_ahash *tfm, const u8 *key,
> >   	return 0;
> >   }
> >   
> > -
> > -struct talitos_alg_template {
> > -	u32 type;
> > -	u32 priority;
> > -	union {
> > -		struct skcipher_alg skcipher;
> > -		struct ahash_alg hash;
> > -		struct aead_alg aead;
> > -	} alg;
> > -	__be32 desc_hdr_template;
> > -};
> > -
> >   static struct talitos_alg_template driver_algs[] = {
> >   	/* AEAD algorithms.  These use a single-pass ipsec_esp descriptor */
> >   	{	.type = CRYPTO_ALG_TYPE_AEAD,
> > @@ -2998,14 +2968,8 @@ static struct talitos_alg_template driver_algs[] = {
> >   	}
> >   };
> >   
> > -struct talitos_crypto_alg {
> > -	struct list_head entry;
> > -	struct device *dev;
> > -	struct talitos_alg_template algt;
> > -};
> > -
> > -static int talitos_init_common(struct talitos_ctx *ctx,
> > -			       struct talitos_crypto_alg *talitos_alg)
> > +int talitos_init_common(struct talitos_ctx *ctx,
> > +			struct talitos_crypto_alg *talitos_alg)
> >   {
> >   	struct talitos_private *priv;
> >   
> > @@ -3066,7 +3030,7 @@ static int talitos_cra_init_ahash(struct crypto_tfm *tfm)
> >   	return talitos_init_common(ctx, talitos_alg);
> >   }
> >   
> > -static void talitos_cra_exit(struct crypto_tfm *tfm)
> > +void talitos_cra_exit(struct crypto_tfm *tfm)
> >   {
> >   	struct talitos_ctx *ctx = crypto_tfm_ctx(tfm);
> >   	struct device *dev = ctx->dev;
> > @@ -3080,7 +3044,7 @@ static void talitos_cra_exit(struct crypto_tfm *tfm)
> >    * type and primary/secondary execution units required match the hw
> >    * capabilities description provided in the device tree node.
> >    */
> > -static int hw_supports(struct device *dev, __be32 desc_hdr_template)
> > +int talitos_hw_supports(struct device *dev, __be32 desc_hdr_template)
> >   {
> >   	struct talitos_private *priv = dev_get_drvdata(dev);
> >   	int ret;
> > @@ -3117,7 +3081,7 @@ static void talitos_remove(struct platform_device *ofdev)
> >   		list_del(&t_alg->entry);
> >   	}
> >   
> > -	if (hw_supports(dev, DESC_HDR_SEL0_RNG))
> > +	if (talitos_hw_supports(dev, DESC_HDR_SEL0_RNG))
> >   		talitos_unregister_rng(dev);
> >   
> >   	for (i = 0; i < 2; i++)
> > @@ -3426,7 +3390,7 @@ static int talitos_probe(struct platform_device *ofdev)
> >   	}
> >   
> >   	/* register the RNG, if available */
> > -	if (hw_supports(dev, DESC_HDR_SEL0_RNG)) {
> > +	if (talitos_hw_supports(dev, DESC_HDR_SEL0_RNG)) {
> >   		err = talitos_register_rng(dev);
> >   		if (err) {
> >   			dev_err(dev, "failed to register hwrng: %d\n", err);
> > @@ -3437,7 +3401,8 @@ static int talitos_probe(struct platform_device *ofdev)
> >   
> >   	/* register crypto algorithms the device supports */
> >   	for (i = 0; i < ARRAY_SIZE(driver_algs); i++) {
> > -		if (hw_supports(dev, driver_algs[i].desc_hdr_template)) {
> > +		if (talitos_hw_supports(dev,
> > +					driver_algs[i].desc_hdr_template)) {
> >   			struct talitos_crypto_alg *t_alg;
> >   			struct crypto_alg *alg = NULL;
> >   
> > diff --git a/drivers/crypto/talitos/talitos.h b/drivers/crypto/talitos/talitos.h
> > index fa8c71b1f90f..1f81d336dae8 100644
> > --- a/drivers/crypto/talitos/talitos.h
> > +++ b/drivers/crypto/talitos/talitos.h
> > @@ -5,7 +5,13 @@
> >    * Copyright (c) 2006-2011 Freescale Semiconductor, Inc.
> >    */
> >   
> > +#include <crypto/aes.h>
> > +#include <crypto/internal/aead.h>
> > +#include <crypto/internal/hash.h>
> > +#include <crypto/internal/skcipher.h>
> > +#include <crypto/sha2.h>
> >   #include <linux/device.h>
> > +#include <linux/dma-mapping.h>
> >   #include <linux/hw_random.h>
> >   #include <linux/interrupt.h>
> >   #include <linux/scatterlist.h>
> > @@ -19,6 +25,13 @@
> >   #define PRIMARY_EU(desc_hdr) ((be32_to_cpu(desc_hdr) >> 28) & 0xf)
> >   #define SECONDARY_EU(desc_hdr) ((be32_to_cpu(desc_hdr) >> 16) & 0xf)
> >   
> > +#ifdef CONFIG_CRYPTO_DEV_TALITOS2
> > +#define TALITOS_MAX_KEY_SIZE		(AES_MAX_KEY_SIZE + SHA512_BLOCK_SIZE)
> > +#else
> > +#define TALITOS_MAX_KEY_SIZE		(AES_MAX_KEY_SIZE + SHA256_BLOCK_SIZE)
> > +#endif
> > +#define TALITOS_MAX_IV_LENGTH		16 /* max of AES_BLOCK_SIZE, DES3_EDE_BLOCK_SIZE */
> > +
> >   /* descriptor pointer entry */
> >   struct talitos_ptr {
> >   	union {
> > @@ -174,6 +187,35 @@ struct talitos_private {
> >   
> >   };
> >   
> > +struct talitos_ctx {
> > +	struct device *dev;
> > +	int ch;
> > +	__be32 desc_hdr_template;
> > +	u8 key[TALITOS_MAX_KEY_SIZE];
> > +	u8 iv[TALITOS_MAX_IV_LENGTH];
> > +	dma_addr_t dma_key;
> > +	unsigned int keylen;
> > +	unsigned int enckeylen;
> > +	unsigned int authkeylen;
> > +};
> > +
> > +struct talitos_alg_template {
> > +	u32 type;
> > +	u32 priority;
> > +	union {
> > +		struct skcipher_alg skcipher;
> > +		struct ahash_alg hash;
> > +		struct aead_alg aead;
> > +	} alg;
> > +	__be32 desc_hdr_template;
> > +};
> > +
> > +struct talitos_crypto_alg {
> > +	struct list_head entry;
> > +	struct device *dev;
> > +	struct talitos_alg_template algt;
> > +};
> > +
> >   /* .features flag */
> >   #define TALITOS_FTR_SRC_LINK_TBL_LEN_INCLUDES_EXTENT 0x00000001
> >   #define TALITOS_FTR_HW_AUTH_CHECK 0x00000002
> > @@ -432,6 +474,55 @@ static inline bool has_ftr_sec1(struct talitos_private *priv)
> >   #define DESC_PTR_LNKTBL_RET			0x02
> >   #define DESC_PTR_LNKTBL_NEXT			0x01
> >   
> > +void to_talitos_ptr(struct talitos_ptr *ptr, dma_addr_t dma_addr,
> > +		    unsigned int len, bool is_sec1);
> > +void copy_talitos_ptr(struct talitos_ptr *dst_ptr, struct talitos_ptr *src_ptr,
> > +		      bool is_sec1);
> > +unsigned short from_talitos_ptr_len(struct talitos_ptr *ptr, bool is_sec1);
> > +void to_talitos_ptr_ext_set(struct talitos_ptr *ptr, u8 val, bool is_sec1);
> > +void to_talitos_ptr_ext_or(struct talitos_ptr *ptr, u8 val, bool is_sec1);
> > +
> > +void map_single_talitos_ptr(struct device *dev, struct talitos_ptr *ptr,
> > +			    unsigned int len, void *data,
> > +			    enum dma_data_direction dir);
> > +void map_single_talitos_ptr_nosync(struct device *dev, struct talitos_ptr *ptr,
> > +				   unsigned int len, void *data,
> > +				   enum dma_data_direction dir);
> > +void unmap_single_talitos_ptr(struct device *dev, struct talitos_ptr *ptr,
> > +			      enum dma_data_direction dir);
> > +
> > +int talitos_submit(struct device *dev, int ch, struct talitos_desc *desc,
> > +		   void (*callback)(struct device *dev,
> > +				    struct talitos_desc *desc, void *context,
> > +				    int error),
> > +		   void *context);
> > +
> > +void talitos_sg_unmap(struct device *dev, struct talitos_edesc *edesc,
> > +		      struct scatterlist *src, struct scatterlist *dst,
> > +		      unsigned int len, unsigned int offset);
> > +int talitos_sg_map_ext(struct device *dev, struct scatterlist *src,
> > +		       unsigned int len, struct talitos_edesc *edesc,
> > +		       struct talitos_ptr *ptr, int sg_count,
> > +		       unsigned int offset, int tbl_off, int elen, bool force,
> > +		       int align);
> > +int talitos_sg_map(struct device *dev, struct scatterlist *src,
> > +		   unsigned int len, struct talitos_edesc *edesc,
> > +		   struct talitos_ptr *ptr, int sg_count, unsigned int offset,
> > +		   int tbl_off);
> > +
> > +struct talitos_edesc *
> > +talitos_edesc_alloc(struct device *dev, struct scatterlist *src,
> > +		    struct scatterlist *dst, u8 *iv, unsigned int assoclen,
> > +		    unsigned int cryptlen, unsigned int authsize,
> > +		    unsigned int ivsize, int icv_stashing, u32 cryptoflags,
> > +		    bool encrypt);
> > +
> > +int talitos_hw_supports(struct device *dev, __be32 desc_hdr_template);
> > +
> > +int talitos_init_common(struct talitos_ctx *ctx,
> > +			struct talitos_crypto_alg *talitos_alg);
> > +void talitos_cra_exit(struct crypto_tfm *tfm);
> > +
> >   /* Hardware RNG */
> >   
> >   int talitos_register_rng(struct device *dev);
> >   
> 
>