Re: [PATCH 20/29] crypto: talitos - Replace SEC1/SEC2 conditionals with ops dispatch

["Christophe Leroy (CS GROUP)" <chleroy@kernel.org>]

Le 28/05/2026 à 11:08, Paul Louvel a écrit :
> Replace the if/else is_sec1 dispatches in callers with indirect calls
> through priv->ops. Add static const sec1_ops and sec2_ops structs
> populated with the SEC1 and SEC2 function variants, and set priv->ops
> at probe time based on the detected hardware.

Why is that needed ?

I understand your objective at the end is to get rid of that is_sec1 
boolean that is carried over the entire call chain but using ops for 
that seems overkill.

What about changing it to a helper using static branches, something like 
(untested) :

#if defined(CONFIG_CRYPTO_DEV_TALITOS1) && 
defined(CONFIG_CRYPTO_DEV_TALITOS2)
DECLARE_STATIC_KEY_FALSE(talitos_is_sec1);
static __always_inline bool is_sec1(void)
{
	return static_branch_unlikely(&talitos_is_sec1);
}

static inline void talitos_init_branch(bool is_sec1)
{
	if (is_sec1)
		static_branch_enable(&talitos_is_sec1);
}
#else
static __always_inline bool is_sec1(void)
{
	return IS_ENABLED(CONFIG_CRYPTO_DEV_TALITOS1);
}

static inline void talitos_init_branch(bool is_sec1)
{
	BUILD_BUG_ON(is_sec1 && !IS_ENABLED(CONFIG_CRYPTO_DEV_TALITOS1));
}
#endif

> 
> 
> Signed-off-by: Paul Louvel <paul.louvel@bootlin.com>
> ---
>   drivers/crypto/talitos/talitos.c | 88 +++++++++++++++++++---------------------
>   1 file changed, 41 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/crypto/talitos/talitos.c b/drivers/crypto/talitos/talitos.c
> index b6793d97735e..c4a311a8e7fd 100644
> --- a/drivers/crypto/talitos/talitos.c
> +++ b/drivers/crypto/talitos/talitos.c
> @@ -258,7 +258,6 @@ static int init_device(struct device *dev)
>   {
>   	struct talitos_private *priv = dev_get_drvdata(dev);
>   	int ch, err;
> -	bool is_sec1 = has_ftr_sec1(priv);
>   
>   	/*
>   	 * Master reset
> @@ -266,35 +265,23 @@ static int init_device(struct device *dev)
>   	 * are not fully cleared by writing the MCR:SWR bit,
>   	 * set bit twice to completely reset
>   	 */
> -	if (is_sec1)
> -		err = sec1_reset_device(dev);
> -	else
> -		err = sec2_reset_device(dev);
> +	err = priv->ops->reset_device(dev);
>   
>   	if (err)
>   		return err;
>   
> -	if (is_sec1)
> -		err = sec1_reset_device(dev);
> -	else
> -		err = sec2_reset_device(dev);
> +	err = priv->ops->reset_device(dev);
>   	if (err)
>   		return err;
>   
>   	/* reset channels */
>   	for (ch = 0; ch < priv->num_channels; ch++) {
> -		if (is_sec1)
> -			err = sec1_reset_channel(dev, ch);
> -		else
> -			err = sec2_reset_channel(dev, ch);
> +		err = priv->ops->reset_channel(dev, ch);
>   		if (err)
>   			return err;
>   	}
>   
> -	if (is_sec1)
> -		sec1_configure_device(dev);
> -	else
> -		sec2_configure_device(dev);
> +	priv->ops->configure_device(dev);
>   
>   	return 0;
>   }
> @@ -363,7 +350,6 @@ int talitos_submit(struct device *dev, int ch, struct talitos_desc *desc,
>   	struct talitos_request *request;
>   	unsigned long flags;
>   	int head;
> -	bool is_sec1 = has_ftr_sec1(priv);
>   
>   	spin_lock_irqsave(&priv->chan[ch].head_lock, flags);
>   
> @@ -377,10 +363,8 @@ int talitos_submit(struct device *dev, int ch, struct talitos_desc *desc,
>   	request = &priv->chan[ch].fifo[head];
>   
>   	/* map descriptor and save caller data */
> -	if (is_sec1)
> -		sec1_dma_map_request(dev, request, desc);
> -	else
> -		sec2_dma_map_request(dev, request, desc);
> +	priv->ops->dma_map_request(dev, request, desc);
> +
>   	request->callback = callback;
>   	request->context = context;
>   
> @@ -461,7 +445,6 @@ static void flush_channel(struct device *dev, int ch, int error, int reset_ch)
>   	struct talitos_request *request, saved_req;
>   	unsigned long flags;
>   	int tail, status;
> -	bool is_sec1 = has_ftr_sec1(priv);
>   
>   	spin_lock_irqsave(&priv->chan[ch].tail_lock, flags);
>   
> @@ -473,10 +456,7 @@ static void flush_channel(struct device *dev, int ch, int error, int reset_ch)
>   
>   		/* descriptors with their done bits set don't get the error */
>   		rmb();
> -		if (is_sec1)
> -			hdr = sec1_get_request_hdr(dev, request);
> -		else
> -			hdr = sec2_get_request_hdr(dev, request);
> +		hdr = priv->ops->get_request_hdr(dev, request);
>   
>   		if ((hdr & DESC_HDR_DONE) == DESC_HDR_DONE)
>   			status = 0;
> @@ -486,10 +466,7 @@ static void flush_channel(struct device *dev, int ch, int error, int reset_ch)
>   			else
>   				status = error;
>   
> -		if (is_sec1)
> -			sec1_dma_unmap_request(dev, request);
> -		else
> -			sec2_dma_unmap_request(dev, request);
> +		priv->ops->dma_unmap_request(dev, request);
>   
>   		/* copy entries so we can call callback outside lock */
>   		saved_req.desc = request->desc;
> @@ -611,7 +588,6 @@ static __be32 sec2_search_desc_hdr_in_request(struct talitos_request *request,
>   static __be32 current_desc_hdr(struct device *dev, int ch)
>   {
>   	struct talitos_private *priv = dev_get_drvdata(dev);
> -	bool is_sec1 = has_ftr_sec1(priv);
>   	struct talitos_request *request;
>   	int tail, iter;
>   	dma_addr_t cur_desc;
> @@ -630,10 +606,7 @@ static __be32 current_desc_hdr(struct device *dev, int ch)
>   	do {
>   		request = &priv->chan[ch].fifo[iter];
>   
> -		if (is_sec1)
> -			hdr = sec1_search_desc_hdr_in_request(request, cur_desc);
> -		else
> -			hdr = sec2_search_desc_hdr_in_request(request, cur_desc);
> +		hdr = priv->ops->search_desc_hdr_in_request(request, cur_desc);
>   		if (hdr)
>   			break;
>   
> @@ -833,13 +806,9 @@ static int sec2_talitos_handle_error(struct device *dev, u32 isr, u32 isr_lo)
>   static void talitos_error(struct device *dev, u32 isr, u32 isr_lo)
>   {
>   	struct talitos_private *priv = dev_get_drvdata(dev);
> -	bool is_sec1 = has_ftr_sec1(priv);
>   	int ch, reset_dev;
>   
> -	if (is_sec1)
> -		reset_dev = sec1_talitos_handle_error(dev, isr, isr_lo);
> -	else
> -		reset_dev = sec2_talitos_handle_error(dev, isr, isr_lo);
> +	reset_dev = priv->ops->handle_error(dev, isr, isr_lo);
>   
>   	if (reset_dev) {
>   		dev_err(dev,
> @@ -1391,6 +1360,32 @@ static void sec2_init_task(struct device *dev)
>   	}
>   }
>   
> +static const struct talitos_ops sec1_ops = {
> +	.probe_irq = sec1_talitos_probe_irq,
> +	.init_task = sec1_init_task,
> +	.reset_device = sec1_reset_device,
> +	.reset_channel = sec1_reset_channel,
> +	.configure_device = sec1_configure_device,
> +	.dma_map_request = sec1_dma_map_request,
> +	.dma_unmap_request = sec1_dma_unmap_request,
> +	.get_request_hdr = sec1_get_request_hdr,
> +	.search_desc_hdr_in_request = sec1_search_desc_hdr_in_request,
> +	.handle_error = sec1_talitos_handle_error,
> +};
> +
> +static const struct talitos_ops sec2_ops = {
> +	.probe_irq = sec2_talitos_probe_irq,
> +	.init_task = sec2_init_task,
> +	.reset_device = sec2_reset_device,
> +	.reset_channel = sec2_reset_channel,
> +	.configure_device = sec2_configure_device,
> +	.dma_map_request = sec2_dma_map_request,
> +	.dma_unmap_request = sec2_dma_unmap_request,
> +	.get_request_hdr = sec2_get_request_hdr,
> +	.search_desc_hdr_in_request = sec2_search_desc_hdr_in_request,
> +	.handle_error = sec2_talitos_handle_error,
> +};
> +
>   static int talitos_probe(struct platform_device *ofdev)
>   {
>   	struct device *dev = &ofdev->dev;
> @@ -1474,16 +1469,15 @@ static int talitos_probe(struct platform_device *ofdev)
>   	}
>   
>   	if (has_ftr_sec1(priv))
> -		err = sec1_talitos_probe_irq(ofdev);
> +		priv->ops = &sec1_ops;
>   	else
> -		err = sec2_talitos_probe_irq(ofdev);
> +		priv->ops = &sec2_ops;
> +
> +	err = priv->ops->probe_irq(ofdev);
>   	if (err)
>   		goto err_out;
>   
> -	if (has_ftr_sec1(priv))
> -		sec1_init_task(dev);
> -	else
> -		sec2_init_task(dev);
> +	priv->ops->init_task(dev);
>   
>   	priv->fifo_len = roundup_pow_of_two(priv->chfifo_len);
>   
>