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

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

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.
>