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