kunit: cs_dsp: Depend on FW_CS_DSP rather then enabling it

FW_CS_DSP gets enabled if KUNIT is enabled. The test should rather
depend on if the feature is enabled. Fix this by moving FW_CS_DSP to the
depends on clause, and set CONFIG_FW_CS_DSP=y in the kunit tooling.

Fixes: dd0b6b1f29b9 ("firmware: cs_dsp: Add KUnit testing of bin file download")
Signed-off-by: Nico Pache <npache@redhat.com>
---
 drivers/firmware/cirrus/Kconfig              | 3 +--
 tools/testing/kunit/configs/all_tests.config | 2 ++
 2 files changed, 3 insertions(+), 2 deletions(-)

On Thu, Mar 20, 2025 at 4:49 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Mar 20, 2025 at 04:21:16PM -0600, Nico Pache wrote:
> > On Thu, Mar 20, 2025 at 1:14 PM Mark Brown <broonie@kernel.org> wrote:
> > > On Wed, Mar 19, 2025 at 05:05:39PM -0600, Nico Pache wrote:
>
> > > >  config FW_CS_DSP_KUNIT_TEST
> > > >       tristate "KUnit tests for Cirrus Logic cs_dsp" if !KUNIT_ALL_TESTS
> > > > -     depends on KUNIT && REGMAP
> > > > +     depends on KUNIT && REGMAP && FW_CS_DSP
> > > >       default KUNIT_ALL_TESTS
> > > > -     select FW_CS_DSP
>
> > > This makes no sense to me, the select statement is forcing on the code
> > > it's testing which is a library and so is selected by it's users, this
>
> > Similarly to eb5c79828cfa ("firmware: cs_dsp: FW_CS_DSP_KUNIT_TEST
> > should not select REGMAP"), We shouldnt force a feature on when using
> > KUNIT_ALL_TESTS.
>
> This feature is not user selectable, at an absolute minimum you would
> need to make the library available in KUnit test builds.
>
> > > change will just stop the tests being run unless someone does the dance
> > > to enable a driver which relies on the library.  That is something that
>
> > My config also sets the UML wrapper to enable this FW_CS_DSP config so
> > it will continue to work in that environment.
>
> Simply adding it to the all_tests.config will just result in it getting
> turned off by Kconfig during the build since it's not a visible option
> so that's not accomplishing anything.  all_tests.config is not UML
> specific, it's for enabling all the KUnit tests that could run in UML no
> matter how you're running them.
>
> > > seems unlikely to change the outcome of the tests when run from KUnit
> > > which is independent of any hardware.
>
> > KUNIT is supported outside the UML environment, and some distros (like
> > fedora, and downstream flavors), use KUNIT as modules, with
> > KUNIT_ALL_TESTS=m. We only want the tests that are supported by our
> > config to be available, we dont want KUNIT going and enabling other
> > features so the test works.
>
> The point is not that KUnit is frequently run in UML (personally I
> mostly run it with emulated hardware instead) but rather that this is a
> library which can be tested independently of having a relevant DSP.

Ok, thank you for the carifying message!

So would the correct approach be allowing users to select FW_CS_DSP,
then apply these changes?

It also looks like FW_CS_DSP_KUNIT_TEST_UTILS and FW_CS_DSP_KUNIT_TEST
are redundant.

On Fri, Mar 21, 2025 at 05:37:50AM -0600, Nico Pache wrote:
> On Thu, Mar 20, 2025 at 4:49 PM Mark Brown <broonie@kernel.org> wrote:

> > Simply adding it to the all_tests.config will just result in it getting
> > turned off by Kconfig during the build since it's not a visible option
> > so that's not accomplishing anything.  all_tests.config is not UML
> > specific, it's for enabling all the KUnit tests that could run in UML no
> > matter how you're running them.

> So would the correct approach be allowing users to select FW_CS_DSP,
> then apply these changes?

That user select should only be visible if KUnit is enabled though,
it really is library code so shouldn't actually be user selectable.  I'm
not sure if there's some other strategy other KUnit tests for libraries
use.

> It also looks like FW_CS_DSP_KUNIT_TEST_UTILS and FW_CS_DSP_KUNIT_TEST
> are redundant.

Possibly there's more tests to come that'll use them?

On Fri, Mar 21, 2025 at 09:01:35AM -0600, Nico Pache wrote:
> On Fri, Mar 21, 2025 at 8:51 AM Mark Brown <broonie@kernel.org> wrote:

> > That user select should only be visible if KUnit is enabled though,
> > it really is library code so shouldn't actually be user selectable.  I'm
> > not sure if there's some other strategy other KUnit tests for libraries
> > use.

> I'm a little confused how the FW_CS_DSP config which was added in
> v5.16 is reliant (library code that is only used by KUNIT) on a config
> that was added in v6.14. Presumably the library is not just for the
> KUNIT test. What was the purpose of this config before the
> introduction of the KUNIT test. Im guessing it was not user selectable
> back then too.

This is support code for DSPs that Cirrus have in some of their devices,
the drivers for devices that have these DSPs select it - a git grep will
show the selects.

On 22/3/25 19:02, Richard Fitzgerald wrote:
> On 22/3/25 10:11, Richard Fitzgerald wrote:
>> On 20/3/25 17:35, Nico Pache wrote:
>>> Sorry links got mangled
>>>
>> Thanks. I'm on vacation right now but I'll take a look through
>> all those when I have time.
>>
>> The unterminated string bugfix is this:
>> https://lore.kernel.org/all/20250211-cs_dsp-kunit-strings-v1-1- 
>> d9bc2035d154@linutronix.de/
>>
>> I got lucky on all the UM, X86 and ARM builds I tested.
>>
> It looks like that bugfix has got lost.
> Mark sent an email on 20 Feb to say it has been merged into his
> sound tree. But that patch isn't in torvalds/master.
> 
> It's possible that the unterminated strings are causing the problems
> you have seen.

I've reproduced this, and it's not a bug in the test. The test is
correctly finding a change in behavior that was introduced into cs_dsp.
This doesn't affect normal firmware files that have real content. But
the tests create an "empty" firmware file that has only the header. This
is useful, so we can have a dummy firmware file that doesn't create
side-effects when it is loaded.

A normal file contains data blocks, which will set ret = 0 when they are
processed. An "empty" file does not have any data block so ret is never
set to 0. The end of the function used to do this:

ret = regmap_async_complete(regmap);

which would make ret == 0 on exit from the function. But a recent commit
removed the async regmap writes and so removed this line. A real
firmware file will already have set ret == 0 when it processed a data
block. The dummy files don't.

So... we have a kunit test, but KUNIT_ALL_TESTS doesn't mean "run all
tests". It means "run test for modules that are already selected."
Modules must be manually selected to get a KUNIT_ALL_TESTS run to
actually run the test code. So it didn't get a kunit test run.

I'll send a patch to fix this.

On Sat, Mar 22, 2025 at 4:11 AM Richard Fitzgerald
<rf@opensource.cirrus.com> wrote:
>
> On 20/3/25 17:35, Nico Pache wrote:
> > Sorry links got mangled
> >
> Thanks. I'm on vacation right now but I'll take a look through
> all those when I have time.
Ok thanks! no rush, enjoy your vacation!

If the links go stale (i'm not sure how long they are valid) just lmk,
I can get you new ones.

There is also this link if you want to track your upstream kunit test
on multiple arches.
https://datawarehouse.cki-project.org/kcidb/tests?tree_filter=fedora-eln&test_filter=KUNIT&testresult_filter=cs-dsp

>
> The unterminated string bugfix is this:
> https://lore.kernel.org/all/20250211-cs_dsp-kunit-strings-v1-1-d9bc2035d154@linutronix.de/
>
> I got lucky on all the UM, X86 and ARM builds I tested.
>

On Sat, Mar 22, 2025 at 07:02:26PM +0000, Richard Fitzgerald wrote:
> On 22/3/25 10:11, Richard Fitzgerald wrote:

> > I got lucky on all the UM, X86 and ARM builds I tested.

> It looks like that bugfix has got lost.
> Mark sent an email on 20 Feb to say it has been merged into his
> sound tree. But that patch isn't in torvalds/master.

It was queued for -next rather than as a fix.