Re: [PATCH v4 1/2] certs: Move RSA self-test data to separate file

["Jarkko Sakkinen" <jarkko@kernel.org>]

On Mon May 13, 2024 at 4:11 AM EEST, Joachim Vandersmissen wrote:
> On 5/12/24 6:11 PM, Jarkko Sakkinen wrote:
> > On Sat May 11, 2024 at 9:23 AM EEST, Joachim Vandersmissen wrote:
> >> v4: FIPS_SIGNATURE_SELFTEST_RSA is no longer user-configurable and will
> >> be set when the dependencies are fulfilled.
> >>
> >> ---8<---
> > This is in wrong place. If the patch is applied it will be included to
> > the kernel git log. Please put your log before diffstat.
> I will keep it in mind for the next round.
> >
> >> In preparation of adding new ECDSA self-tests, the existing data is
> >> moved to a separate file. A new configuration option is added to
> >> control the compilation of the separate file. This configuration option
> >> also enforces dependencies that were missing from the existing
> >> CONFIG_FIPS_SIGNATURE_SELFTEST option.
> > 1. Please just call the thing by its name instead of building tension
> >     with "the new configuration option".
> > 2. Lacks the motivation of adding a new configuration option.
> The configuration option is there to ensure that the RSA (or ECDSA) 
> self-tests only get compiled in when RSA (or ECDSA) is actually enabled. 
> Otherwise, the self-test will panic on boot. I can make this more 
> explicit in the commit message.
> >
> >> The old fips_signature_selftest is no longer an init function, but now
> >> a helper function called from fips_signature_selftest_rsa.
> > This is confusing, please remove.
> Fair enough, I'll remove it from the commit message.

Yeah, I mean it is good to enough to have a code change no need to
document it here :-)

> >
> > So why just send this and not this plus the selftest? Feels incomplete
> > to me.
>
> Do you mean the ECDSA self-test? I didn't include that one here because 
> I didn't want to make the commit too big.

So, I'd suggest to make a patch set with the second patch containing
the tests.

BR, Jarkko