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

From: Paul Louvel

Date: Thu Jun 04 2026 - 09:11:56 EST

On Thu Jun 4, 2026 at 11:37 AM CEST, Christophe Leroy (CS GROUP) wrote:
>
>
> 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
>

Thanks you for that suggestion.
This was a lack of knowledge about this mechanism.

>>
>>
>> Signed-off-by: Paul Louvel <paul.louvel@xxxxxxxxxxx>
>> ---
>> 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);
>>
>>

--
Paul Louvel, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com