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

[Christophe Leroy (CS GROUP)]

Le 01/06/2026 à 10:49, Paul Louvel a écrit :
> Hi Christophe,
>
> > 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.
>
> About patch 24 and 25: one of the main purpose of this series is to get rid of
> the is_sec1 scattered around the core code of the driver.

I think we can find more efficient ways to do it, for instance using 
static branches (jump label).

> > Most functions here are small helpers. To be shared between several C
> > files they deserve becoming static inlines in talitos.h, not global
> > functions.
>
> I did not look at the generated text either for this change but I bet the
> compiler is inlining these calls. Adding them as static inline is more explicit.

Today the compiler is inlining these calls for sure, and it also gets 
rid of the is_sec1 at all unless you unlikely build with both 
CRYPTO_DEV_TALITOS1 and CRYPTO_DEV_TALITOS2.
But once you call them from a different C file, then it can't be inlined 
anymore.

> I understand that there is a performance penalty, since there will be no
> inlining, and a memory dereferencing for a call through function pointer.

Not only a memory dereferencing, but all the preparation for calling a 
subfunction: creating a stack frame, saving the link register, saving 
volatile registers, loading the address to call into a register, saving 
it into CTR register, then doing a branch to CTR. And of course the 
impact on the call flow, instruction cache loading, pipeline, etc ...
So this is definitely a huge cost compared to the cost of the one or two 
loads done by the helper itself.

> > 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.
>
> As for patch 24, 25 and onwards, the same space optimization apply here. If the
> kernel is built with CRYPTO_DEV_TALITOS1, there will be no SEC2-related function
> in the generated text, and vice versa.

I'm not thinking about space optimisation but processing optimisation. 
In the current implementation you have a flat branchless execution flow. 
Here you are introducing branching, and worse: indirect branching. As 
mentioned by David, it has a serious cost, more than what (young) people 
may think. When only two choices are needed an if/else remains the most 
efficient.

Christophe

> > 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.
> >
> > Christophe
>
> These changes are needed to split the driver into multiple files.
>
> Best regards,
> Paul.