kmod: sof_remove: Drop Pulseaudio test and stop including case-lib/li…

[ujfalusi]

…b.sh

The PA test does not make much sense anymore as distros are moving or moved to use PW and the audio servers should have been disabled in the DUT anyways to not interfere with tests (PA/PW will probe the new card on module load for example).

Including case-lib/lib.sh have the side effect that it will 'start a test'. This involves checking if the firmware is booted, but if we have booted up in legacy or DSPless mode the script aborts and one has to figure out [1] how to force the script to do what it supposed to be doing: removing the modules.

This is a helper script, not a test case.

[1]
NO_POLL_FW_LOADING=false tools/kmod/sof_remove.sh
Signed-off-by: Peter Ujfalusi [email protected]

[plbossart]

Our CI devices are using older distros, so not sure if anyone has pipewire just yet? that would be for @fredoh9 and @marc-hb to comment.

[ujfalusi]

Our CI devices are using older distros, so not sure if anyone has pipewire just yet? that would be for @fredoh9 and @marc-hb to comment.

True, but this is a helper script and it invokes the whole sof-test infra to 'run a test', which fails under certain cases and it is not a friendly endeavor to figure out how to just remove the modules.

[marc-hb]

Thanks @ujfalusi , good catch.

Totally agree that this is not a "test-case" and that pulling the entire lib.sh dependency is way too much.

Also, that lib.sh has become "too heavy" (as you noted: "starting a test"...) and should probably be broken down but that's for another day.

This being said pulseaudio is still in Ubuntu 22 LTS. Ubuntu 24 LTS is around the corner but not quite there yet. Not sure about other Linux distros. We want to keep a reasonable amount of "backwards compatibility" for a reasonable amount of time.

I just noticed the function systemctl_show_pulseaudio is relatively short and used only in sof_remove.sh. So how about you just move the former to the latter? Maybe not the best "architecture" but very simple change and problem solved.

BTW: a2062a3

[marc-hb]

Also, that lib.sh has become "too heavy" (as you noted: "starting a test"...) and should probably be broken down but that's for another day.

Actually, merely moving that start_test call away from the bottom of lib.sh and into each test script instead would be repetitive but pretty easy and low risk if you feel like it. I guess it would also achieve (some of) what you want? It would for sure make lib.sh "free of (most) side-effects" and possible to source it interactively again (I just verified that).

A good place to invoke start_test would be just before logger_disabled || func_lib_start_log_collect. Pretty much every test has this line.

[marc-hb]

Actually, merely moving that start_test call away from the bottom of lib.sh and into each test script instead would be repetitive but pretty easy and low risk if you feel like it.

Of course there are a few unexpected complications but I think I have it. First part is here, please review:

• make start_test() optional: part 1 #1170

[ujfalusi]

@marc-hb, @plbossart, it is fine to make the case-lib leaner, but the point is still why the helper script needs to include that?
It would be great to be able to just copy the sof_insert.sh / sof_remove.sh to DUTs as standalone if needed.

[marc-hb]

but the point is still why the helper script needs to include that?

Work in progress in

• make start_test() optional: part 1 #1170

[marc-hb]

I think my previous comment missed your point, sorry I read too fast.

This previous suggestion of mine is hopefully more relevant:

I just noticed the function systemctl_show_pulseaudio is relatively short and used only in sof_remove.sh. So how about you just move the former to the latter? Maybe not the best "architecture" but very simple change and problem solved.

---

It would be great to be able to just copy the sof_insert.sh / sof_remove.sh to DUTs as standalone if needed.

I really don't see why anyone would NOT want to copy the entire sof-test tree sorry.

[ujfalusi]

I think my previous comment missed your point, sorry I read too fast.

This previous suggestion of mine is hopefully more relevant:

I just noticed the function systemctl_show_pulseaudio is relatively short and used only in sof_remove.sh. So how about you just move the former to the latter? Maybe not the best "architecture" but very simple change and problem solved.

It would be great to be able to just copy the sof_insert.sh / sof_remove.sh to DUTs as standalone if needed.

I really don't see why anyone would NOT want to copy the entire sof-test tree sorry.

I got the point, but I don't see why removing modules should depend on systemd.
Arguably PA/PW can cause more problems on sof_insert as they will probe the new card right away.

And if PA/PW is using the card, the sof_remove will just wait until the module can be removed. By this logic we should be also looking for dtrace/mtrace/aplay/arecord/crecord?

What I'm saying is that if you are running the tests then yes, PA (and PW) should be disabled, no question about that, but it is the test case lib which should be testing this.
I use sof_insert/remove when I have PA/PW enabled and I use it intentionally to test what they do when probing the new card. I'm sure other do the same, we are not always running test cases.

[marc-hb]

I got the point, but I don't see why removing modules should depend on systemd.

sof_remote.sh does not really "depend" on systemd. It only tries to invoke systemd to provide more information on failure because pulseaudio is/was the most common culprit by one order of magnitude. If you don't have systemd[*] then that will fail too but by that time it does not matter much any more, does it? Two failures or one failure is just a FAIL either way.

[*] ... then you probably Peter Ujfalusi? :-D

[marc-hb]

Actually, merely moving that start_test call away from the bottom of lib.sh and into each test script instead would be repetitive but pretty easy and low risk if you feel like it.

Submitted in #1187, all PR tests passing. With it sof_remove.sh does not "start a test" anymore.

[marc-hb]

I make lib.sh optional in:

• sof_remove.sh: make lib.sh optional #1220

This means you can copy sof_remove.sh alone to a different device and it still works. Please review.

And if PA/PW is using the card, the sof_remove will just wait until the module can be removed.

I don't know about pulseaudio but that's not what happens with aplay: sof_remove.sh fails immediately. I think that's a good thing; we don't want this script to hang for an indefinite time.

By this logic we should be also looking for dtrace/mtrace/aplay/arecord/crecord?

These are not daemons and don't have a life of their own.

What I'm saying is that if you are running the tests then yes, PA (and PW) should be disabled, no question about that, but it is the test case lib which should be testing this.
I use sof_insert/remove when I have PA/PW enabled and I use it intentionally to test what they do when probing the new card.

sof_remove.sh has never "tested" whether PA or PW is running. The most it has ever done is to try to display whether they're running or not and why and ONLY AFTER modules could not be unloaded.

[ujfalusi]

I make lib.sh optional in:

* [sof_remove.sh: make lib.sh optional #1220](https://github.com/thesofproject/sof-test/pull/1220)

This means you can copy sof_remove.sh alone to a different device and it still works. Please review.

Thank you, see my comment on #1220.

And if PA/PW is using the card, the sof_remove will just wait until the module can be removed.

I don't know about pulseaudio but that's not what happens with aplay: sof_remove.sh fails immediately. I think that's a good thing; we don't want this script to hang for an indefinite time.

OK, that might be the case, probably I saw the wait with still running mtrace... I agree, it is better to not wait, but fail for CI.

By this logic we should be also looking for dtrace/mtrace/aplay/arecord/crecord?

These are not daemons and don't have a life of their own.

What I'm saying is that if you are running the tests then yes, PA (and PW) should be disabled, no question about that, but it is the test case lib which should be testing this.
I use sof_insert/remove when I have PA/PW enabled and I use it intentionally to test what they do when probing the new card.

sof_remove.sh has never "tested" whether PA or PW is running. The most it has ever done is to try to display whether they're running or not and why and ONLY AFTER modules could not be unloaded.

Right, but it expects atm that at least once the firmware was booted, it should not care about that.

Imho the sof-remove.sh should be 'dumb' and if you need these extra features for CI then have a simple wrapper for it in lib.sh or somewhere else to do some checking to catch reasons why the remove should not work or if it failed then give hints on why it might?

[ujfalusi]

Closing for now as sof_remove.sh appears to be working now without hiccups. Thanks @marc-hb for the updates, one of them surely fixed this!

[marc-hb]

Right, but it expects atm that at least once the firmware was booted, it should not care about that.

That was never by design. It was by lack of design :-)

I fixed that particular issue a while ago in commit 7c2ede0. Maybe you were on vacation at the time? :-) EDIT: you were not; you commented!

Thanks @marc-hb for the updates, one of them surely fixed this!