[PATCH] kunit: cs_dsp: Depend on FW_CS_DSP rather then enabling it

[PATCH] kunit: cs_dsp: Depend on FW_CS_DSP rather then enabling it

[Nico Pache]

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(-)

diff --git a/drivers/firmware/cirrus/Kconfig b/drivers/firmware/cirrus/Kconfig
index 0a883091259a..989568ab5712 100644
--- a/drivers/firmware/cirrus/Kconfig
+++ b/drivers/firmware/cirrus/Kconfig
@@ -11,9 +11,8 @@ config FW_CS_DSP_KUNIT_TEST_UTILS
 
 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
 	select FW_CS_DSP_KUNIT_TEST_UTILS
 	help
 	  This builds KUnit tests for cs_dsp.
diff --git a/tools/testing/kunit/configs/all_tests.config b/tools/testing/kunit/configs/all_tests.config
index b0049be00c70..96c6b4aca87d 100644
--- a/tools/testing/kunit/configs/all_tests.config
+++ b/tools/testing/kunit/configs/all_tests.config
@@ -49,3 +49,5 @@ CONFIG_SOUND=y
 CONFIG_SND=y
 CONFIG_SND_SOC=y
 CONFIG_SND_SOC_TOPOLOGY_BUILD=y
+
+CONFIG_FW_CS_DSP=y
\ No newline at end of file
-- 
2.48.1

Re: [PATCH] kunit: cs_dsp: Depend on FW_CS_DSP rather then enabling it

[Nico Pache]

On Wed, Mar 19, 2025 at 5:05 PM Nico Pache <npache@redhat.com> wrote:
>
> 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.

A further note here:

This test is failing and panicing across multiple arches, and
triggering kasan slats on debug kernels. I think this test needs more
testing ;P

>
> 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(-)
>
> diff --git a/drivers/firmware/cirrus/Kconfig b/drivers/firmware/cirrus/Kconfig
> index 0a883091259a..989568ab5712 100644
> --- a/drivers/firmware/cirrus/Kconfig
> +++ b/drivers/firmware/cirrus/Kconfig
> @@ -11,9 +11,8 @@ config FW_CS_DSP_KUNIT_TEST_UTILS
>
>  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
>         select FW_CS_DSP_KUNIT_TEST_UTILS
>         help
>           This builds KUnit tests for cs_dsp.
> diff --git a/tools/testing/kunit/configs/all_tests.config b/tools/testing/kunit/configs/all_tests.config
> index b0049be00c70..96c6b4aca87d 100644
> --- a/tools/testing/kunit/configs/all_tests.config
> +++ b/tools/testing/kunit/configs/all_tests.config
> @@ -49,3 +49,5 @@ CONFIG_SOUND=y
>  CONFIG_SND=y
>  CONFIG_SND_SOC=y
>  CONFIG_SND_SOC_TOPOLOGY_BUILD=y
> +
> +CONFIG_FW_CS_DSP=y
> \ No newline at end of file
> --
> 2.48.1
>

Re: [PATCH] kunit: cs_dsp: Depend on FW_CS_DSP rather then enabling it

[Richard Fitzgerald]

On 19/3/25 23:11, Nico Pache wrote:
> On Wed, Mar 19, 2025 at 5:05 PM Nico Pache <npache@redhat.com> wrote:
>>
>> 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.
> 
> A further note here:
> 
> This test is failing and panicing across multiple arches, and
> triggering kasan slats on debug kernels. I think this test needs more
> testing ;P
>

Please supply details of failures or links to bug reports.
"is failing" and "panicing" doesn't tell me enough to fix anything.
Failing how? Panicking how? On what architectures?
I tested it on the architectures I have available, and the kunit um
architecture. Unfortunately not everyone has hardware for every
architecture supported by Linux so we have to trust somewhat that
other architectures don't do anything unexpectedly different from
what we _can_ test it on.

Also, are any of these failures the unterminated string bug that someone
fixed recently?

>>
>> 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(-)
>>
>> diff --git a/drivers/firmware/cirrus/Kconfig b/drivers/firmware/cirrus/Kconfig
>> index 0a883091259a..989568ab5712 100644
>> --- a/drivers/firmware/cirrus/Kconfig
>> +++ b/drivers/firmware/cirrus/Kconfig
>> @@ -11,9 +11,8 @@ config FW_CS_DSP_KUNIT_TEST_UTILS
>>
>>   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
>>          select FW_CS_DSP_KUNIT_TEST_UTILS
>>          help
>>            This builds KUnit tests for cs_dsp.
>> diff --git a/tools/testing/kunit/configs/all_tests.config b/tools/testing/kunit/configs/all_tests.config
>> index b0049be00c70..96c6b4aca87d 100644
>> --- a/tools/testing/kunit/configs/all_tests.config
>> +++ b/tools/testing/kunit/configs/all_tests.config
>> @@ -49,3 +49,5 @@ CONFIG_SOUND=y
>>   CONFIG_SND=y
>>   CONFIG_SND_SOC=y
>>   CONFIG_SND_SOC_TOPOLOGY_BUILD=y
>> +
>> +CONFIG_FW_CS_DSP=y
>> \ No newline at end of file
>> --
>> 2.48.1
>>
> 

Re: [PATCH] kunit: cs_dsp: Depend on FW_CS_DSP rather then enabling it

[Nico Pache]

On Thu, Mar 20, 2025 at 6:21 AM Richard Fitzgerald
<rf@opensource.cirrus.com> wrote:
>
> On 19/3/25 23:11, Nico Pache wrote:
> > On Wed, Mar 19, 2025 at 5:05 PM Nico Pache <npache@redhat.com> wrote:
> >>
> >> 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.
> >
> > A further note here:
> >
> > This test is failing and panicing across multiple arches, and
> > triggering kasan slats on debug kernels. I think this test needs more
> > testing ;P
> >
>
> Please supply details of failures or links to bug reports.
> "is failing" and "panicing" doesn't tell me enough to fix anything.
> Failing how? Panicking how? On what architectures?
> I tested it on the architectures I have available, and the kunit um
> architecture. Unfortunately not everyone has hardware for every
> architecture supported by Linux so we have to trust somewhat that
> other architectures don't do anything unexpectedly different from
> what we _can_ test it on.

Some of the runs return not ok on a bunch of tests, debug kernels
print splats, and some seem to brick the system, leading to a reboot.
Below are all the failures per arch/variant.

Failing on
---------------------
X86_64 : https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/1723153780/test_x86_64/9451298630/artifacts/run.done.01/job.01/recipes/18353773/tasks/7/results/1742341634/logs/resultoutputfile.log
X86_64 (debug) :
https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/1717840829/test_x86_64/9419724200/artifacts/run.done.01/results_0001/console.log

aarch64 : https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/1723153780/test_aarch64/9451298664/artifacts/run.done.01/job.01/recipes/18352965/tasks/7/results/1742330044/logs/resultoutputfile.log
aarch64(debug):
https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/1717840829/test_aarch64/9419724214/artifacts/run.done.01/results_0001/console.log
aarch64-64kpagesize:
https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/1723154540/test_aarch64/9451303359/artifacts/run.done.01/job.01/recipes/18352963/tasks/7/results/1742331192/logs/resultoutputfile.log
aarch64-64kpagesize (debug):
https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/1723154644/test_aarch64/9451304808/artifacts/run.done.01/job.01/recipes/18354911/tasks/6/results/1742356729/logs/resultoutputfile.log

ppc64le: https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/1723153780/test_ppc64le/9451298644/artifacts/run.done.01/results_0001/console.log
ppc64le(debug):
https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/1717840829/test_ppc64le/9419724210/artifacts/run.done.01/results_0001/console.log

>
> Also, are any of these failures the unterminated string bug that someone
> fixed recently?
Not sure. That fix doesn't seem to have been merged yet.
>
> >>
> >> 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(-)
> >>
> >> diff --git a/drivers/firmware/cirrus/Kconfig b/drivers/firmware/cirrus/Kconfig
> >> index 0a883091259a..989568ab5712 100644
> >> --- a/drivers/firmware/cirrus/Kconfig
> >> +++ b/drivers/firmware/cirrus/Kconfig
> >> @@ -11,9 +11,8 @@ config FW_CS_DSP_KUNIT_TEST_UTILS
> >>
> >>   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
> >>          select FW_CS_DSP_KUNIT_TEST_UTILS
> >>          help
> >>            This builds KUnit tests for cs_dsp.
> >> diff --git a/tools/testing/kunit/configs/all_tests.config b/tools/testing/kunit/configs/all_tests.config
> >> index b0049be00c70..96c6b4aca87d 100644
> >> --- a/tools/testing/kunit/configs/all_tests.config
> >> +++ b/tools/testing/kunit/configs/all_tests.config
> >> @@ -49,3 +49,5 @@ CONFIG_SOUND=y
> >>   CONFIG_SND=y
> >>   CONFIG_SND_SOC=y
> >>   CONFIG_SND_SOC_TOPOLOGY_BUILD=y
> >> +
> >> +CONFIG_FW_CS_DSP=y
> >> \ No newline at end of file
> >> --
> >> 2.48.1
> >>
> >
>

Re: [PATCH] kunit: cs_dsp: Depend on FW_CS_DSP rather then enabling it

[Nico Pache]

Sorry links got mangled

X86_64 : https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/1723153780/test_x86_64/9451298630/artifacts/run.done.01/job.01/recipes/18353773/tasks/7/results/1742341634/logs/resultoutputfile.log

X86_64 (debug) :
https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/1717840829/test_x86_64/9419724200/artifacts/run.done.01/results_0001/console.log

aarch64 : https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/1723153780/test_aarch64/9451298664/artifacts/run.done.01/job.01/recipes/18352965/tasks/7/results/1742330044/logs/resultoutputfile.log

aarch64(debug):
https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/1717840829/test_aarch64/9419724214/artifacts/run.done.01/results_0001/console.log

aarch64-64kpagesize:
https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/1723154540/test_aarch64/9451303359/artifacts/run.done.01/job.01/recipes/18352963/tasks/7/results/1742331192/logs/resultoutputfile.log

aarch64-64kpagesize (debug):
https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/1723154644/test_aarch64/9451304808/artifacts/run.done.01/job.01/recipes/18354911/tasks/6/results/1742356729/logs/resultoutputfile.log

ppc64le: https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/1723153780/test_ppc64le/9451298644/artifacts/run.done.01/results_0001/console.log

ppc64le(debug):
https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/1717840829/test_ppc64le/9419724210/artifacts/run.done.01/results_0001/console.log

On Thu, Mar 20, 2025 at 11:33 AM Nico Pache <npache@redhat.com> wrote:
>
> On Thu, Mar 20, 2025 at 6:21 AM Richard Fitzgerald
> <rf@opensource.cirrus.com> wrote:
> >
> > On 19/3/25 23:11, Nico Pache wrote:
> > > On Wed, Mar 19, 2025 at 5:05 PM Nico Pache <npache@redhat.com> wrote:
> > >>
> > >> 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.
> > >
> > > A further note here:
> > >
> > > This test is failing and panicing across multiple arches, and
> > > triggering kasan slats on debug kernels. I think this test needs more
> > > testing ;P
> > >
> >
> > Please supply details of failures or links to bug reports.
> > "is failing" and "panicing" doesn't tell me enough to fix anything.
> > Failing how? Panicking how? On what architectures?
> > I tested it on the architectures I have available, and the kunit um
> > architecture. Unfortunately not everyone has hardware for every
> > architecture supported by Linux so we have to trust somewhat that
> > other architectures don't do anything unexpectedly different from
> > what we _can_ test it on.
>
> Some of the runs return not ok on a bunch of tests, debug kernels
> print splats, and some seem to brick the system, leading to a reboot.
> Below are all the failures per arch/variant.
>
> Failing on
> ---------------------
> X86_64 : https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/1723153780/test_x86_64/9451298630/artifacts/run.done.01/job.01/recipes/18353773/tasks/7/results/1742341634/logs/resultoutputfile.log
> X86_64 (debug) :
> https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/1717840829/test_x86_64/9419724200/artifacts/run.done.01/results_0001/console.log
>
> aarch64 : https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/1723153780/test_aarch64/9451298664/artifacts/run.done.01/job.01/recipes/18352965/tasks/7/results/1742330044/logs/resultoutputfile.log
> aarch64(debug):
> https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/1717840829/test_aarch64/9419724214/artifacts/run.done.01/results_0001/console.log
> aarch64-64kpagesize:
> https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/1723154540/test_aarch64/9451303359/artifacts/run.done.01/job.01/recipes/18352963/tasks/7/results/1742331192/logs/resultoutputfile.log
> aarch64-64kpagesize (debug):
> https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/1723154644/test_aarch64/9451304808/artifacts/run.done.01/job.01/recipes/18354911/tasks/6/results/1742356729/logs/resultoutputfile.log
>
> ppc64le: https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/1723153780/test_ppc64le/9451298644/artifacts/run.done.01/results_0001/console.log
> ppc64le(debug):
> https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/1717840829/test_ppc64le/9419724210/artifacts/run.done.01/results_0001/console.log
>
> >
> > Also, are any of these failures the unterminated string bug that someone
> > fixed recently?
> Not sure. That fix doesn't seem to have been merged yet.
> >
> > >>
> > >> 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(-)
> > >>
> > >> diff --git a/drivers/firmware/cirrus/Kconfig b/drivers/firmware/cirrus/Kconfig
> > >> index 0a883091259a..989568ab5712 100644
> > >> --- a/drivers/firmware/cirrus/Kconfig
> > >> +++ b/drivers/firmware/cirrus/Kconfig
> > >> @@ -11,9 +11,8 @@ config FW_CS_DSP_KUNIT_TEST_UTILS
> > >>
> > >>   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
> > >>          select FW_CS_DSP_KUNIT_TEST_UTILS
> > >>          help
> > >>            This builds KUnit tests for cs_dsp.
> > >> diff --git a/tools/testing/kunit/configs/all_tests.config b/tools/testing/kunit/configs/all_tests.config
> > >> index b0049be00c70..96c6b4aca87d 100644
> > >> --- a/tools/testing/kunit/configs/all_tests.config
> > >> +++ b/tools/testing/kunit/configs/all_tests.config
> > >> @@ -49,3 +49,5 @@ CONFIG_SOUND=y
> > >>   CONFIG_SND=y
> > >>   CONFIG_SND_SOC=y
> > >>   CONFIG_SND_SOC_TOPOLOGY_BUILD=y
> > >> +
> > >> +CONFIG_FW_CS_DSP=y
> > >> \ No newline at end of file
> > >> --
> > >> 2.48.1
> > >>
> > >
> >

Re: [PATCH] kunit: cs_dsp: Depend on FW_CS_DSP rather then enabling it

[Richard Fitzgerald]

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.

Re: [PATCH] kunit: cs_dsp: Depend on FW_CS_DSP rather then enabling it

[Richard Fitzgerald]

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.

Re: [PATCH] kunit: cs_dsp: Depend on FW_CS_DSP rather then enabling it

[Richard Fitzgerald]

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.

Re: [PATCH] kunit: cs_dsp: Depend on FW_CS_DSP rather then enabling it

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 386 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] kunit: cs_dsp: Depend on FW_CS_DSP rather then enabling it

[Nico Pache]

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

Re: [PATCH] kunit: cs_dsp: Depend on FW_CS_DSP rather then enabling it

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 896 bytes --]

On Wed, Mar 19, 2025 at 05:05:39PM -0600, Nico Pache wrote:
> 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.

>  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
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
seems unlikely to change the outcome of the tests when run from KUnit
which is independent of any hardware.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] kunit: cs_dsp: Depend on FW_CS_DSP rather then enabling it

[Nico Pache]

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

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

Cheers,
-- Nico

Re: [PATCH] kunit: cs_dsp: Depend on FW_CS_DSP rather then enabling it

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2177 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] kunit: cs_dsp: Depend on FW_CS_DSP rather then enabling it

[Nico Pache]

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.

Re: [PATCH] kunit: cs_dsp: Depend on FW_CS_DSP rather then enabling it

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 957 bytes --]

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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] kunit: cs_dsp: Depend on FW_CS_DSP rather then enabling it

[Nico Pache]

On Fri, Mar 21, 2025 at 8:51 AM Mark Brown <broonie@kernel.org> wrote:
>
> 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.
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.

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

Re: [PATCH] kunit: cs_dsp: Depend on FW_CS_DSP rather then enabling it

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 916 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] kunit: cs_dsp: Depend on FW_CS_DSP rather then enabling it

[Nico Pache]

On Fri, Mar 21, 2025 at 9:51 AM Mark Brown <broonie@kernel.org> wrote:
>
> 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.

Ah ok I see! I'm not sure what the 'proper' KUNIT behavior is for this
config case. But I'll trust your judgement and expertise, we can
probably leave this as is for now.

Cheers,
-- Nico

Re: [PATCH] kunit: cs_dsp: Depend on FW_CS_DSP rather then enabling it

[Richard Fitzgerald]

On 21/3/25 15:01, Nico Pache wrote:
> 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.
> 
All this cs_dsp code was originally in sound/soc/codecs/wm_adsp.c.
The main DSP support was moved out into the generic cs_dsp library
so we can support using the DSP with non-ASoC drivers, and on devices
that are not sound chips. The drivers that use the cs_dsp library
select the Kconfig.
(ISTR that the config option may have been renamed to FW_CS_DSP at some
point, so could have a different name in older kernels.)

>>
>>> 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?
> 
Yes, that is explained in the commit message that added this config
symbol. As this has been split out of wm_adsp, we still need to test
the functionality that was left in wm_adsp. I wrote the test utils
functions as a library so that a wm_adsp kunit test can re-use it.
I am still working on the wm_adsp kunit test.

Re: [PATCH] kunit: cs_dsp: Depend on FW_CS_DSP rather then enabling it

[Richard Fitzgerald]

On 19/03/2025 11:05 pm, Nico Pache wrote:
> 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>

This patch doesn't actually work and breaks kunit.py.

> ---
>   drivers/firmware/cirrus/Kconfig              | 3 +--
>   tools/testing/kunit/configs/all_tests.config | 2 ++
>   2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firmware/cirrus/Kconfig b/drivers/firmware/cirrus/Kconfig
> index 0a883091259a..989568ab5712 100644
> --- a/drivers/firmware/cirrus/Kconfig
> +++ b/drivers/firmware/cirrus/Kconfig
> @@ -11,9 +11,8 @@ config FW_CS_DSP_KUNIT_TEST_UTILS
>   
>   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

These changes result in a circular reference:

error: recursive dependency detected!
	symbol FW_CS_DSP is selected by FW_CS_DSP_KUNIT_TEST_UTILS
	symbol FW_CS_DSP_KUNIT_TEST_UTILS is selected by
FW_CS_DSP_KUNIT_TEST
	symbol FW_CS_DSP_KUNIT_TEST depends on FW_CS_DSP

>   	select FW_CS_DSP_KUNIT_TEST_UTILS
>   	help
>   	  This builds KUnit tests for cs_dsp.
> diff --git a/tools/testing/kunit/configs/all_tests.config b/tools/testing/kunit/configs/all_tests.config
> index b0049be00c70..96c6b4aca87d 100644
> --- a/tools/testing/kunit/configs/all_tests.config
> +++ b/tools/testing/kunit/configs/all_tests.config
> @@ -49,3 +49,5 @@ CONFIG_SOUND=y
>   CONFIG_SND=y
>   CONFIG_SND_SOC=y
>   CONFIG_SND_SOC_TOPOLOGY_BUILD=y
> +
> +CONFIG_FW_CS_DSP=y

This won't work because CONFIG_FW_CS_DSP isn't a visible option so it
breaks kunit.py:

ERROR:root:Not all Kconfig options selected in kunitconfig were in the
generated .config.
This is probably due to unsatisfied dependencies.
Missing: CONFIG_FW_CS_DSP=y

I suggest dropping this change to all_tests.config and only fixing
the Kconfig so that CONFIG_KUNIT_ALL_TESTS doesn't force
CONFIG_FW_CS_DSP.

Re: [PATCH] kunit: cs_dsp: Depend on FW_CS_DSP rather then enabling it

[Richard Fitzgerald]

On 08/04/2025 10:25 am, Richard Fitzgerald wrote:
> On 19/03/2025 11:05 pm, Nico Pache wrote:
>> 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>
> 
> This patch doesn't actually work and breaks kunit.py.
> 

I was working on a series to make the same fixes to another Cirrus
KUnit test. That series makes the necessary changes to all_tests.config
so I took the liberty of fixing your patch and including it in
my series.

https://lore.kernel.org/linux-kselftest/20250411123608.1676462-1-rf@opensource.cirrus.com/T/#t

Re: [PATCH] kunit: cs_dsp: Depend on FW_CS_DSP rather then enabling it

[Nico Pache]

On Fri, Apr 11, 2025 at 6:39 AM Richard Fitzgerald
<rf@opensource.cirrus.com> wrote:
>
> On 08/04/2025 10:25 am, Richard Fitzgerald wrote:
> > On 19/03/2025 11:05 pm, Nico Pache wrote:
> >> 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>
> >
> > This patch doesn't actually work and breaks kunit.py.
> >
>
> I was working on a series to make the same fixes to another Cirrus
> KUnit test. That series makes the necessary changes to all_tests.config
> so I took the liberty of fixing your patch and including it in
> my series.

Thank you! much appreciated. I will try to apply and test your series
next week.

Sorry, meant to get back to you sooner, I've been very overloaded.

Cheers,
-- Nico
>
> https://lore.kernel.org/linux-kselftest/20250411123608.1676462-1-rf@opensource.cirrus.com/T/#t
>