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
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
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
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/17... X86_64 (debug) : https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/17...
aarch64 : https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/17... aarch64(debug): https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/17... aarch64-64kpagesize: https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/17... aarch64-64kpagesize (debug): https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/17...
ppc64le: https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/17... ppc64le(debug): https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/17...
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
Sorry links got mangled
X86_64 : https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/17...
X86_64 (debug) : https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/17...
aarch64 : https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/17...
aarch64(debug): https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/17...
aarch64-64kpagesize: https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/17...
aarch64-64kpagesize (debug): https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/17...
ppc64le: https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/17...
ppc64le(debug): https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/17...
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/17... X86_64 (debug) : https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/17...
aarch64 : https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/17... aarch64(debug): https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/17... aarch64-64kpagesize: https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/17... aarch64-64kpagesize (debug): https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/17...
ppc64le: https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/17... ppc64le(debug): https://s3.amazonaws.com/arr-cki-prod-trusted-artifacts/trusted-artifacts/17...
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
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@...
I got lucky on all the UM, X86 and ARM builds I tested.
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.
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 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.
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&...
The unterminated string bugfix is this: https://lore.kernel.org/all/20250211-cs_dsp-kunit-strings-v1-1-d9bc2035d154@...
I got lucky on all the UM, X86 and ARM builds I tested.
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.
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_DSPThis 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
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.
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_DSPThis 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 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?
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 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
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.
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.
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@opensour...
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@opensour...
linux-kselftest-mirror@lists.linaro.org
-
Mark Brown -
Nico Pache -
Richard Fitzgerald