| Message ID | 20250319230539.140869-1-npache@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | kunit: cs_dsp: Depend on FW_CS_DSP rather then enabling it | expand |
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/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 > >> > > >
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 > > >> > > > > >
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_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
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_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 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 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.
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 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 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 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.
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
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(-)