[RFC]: Add i.MX tests for module insert/removal #927

LaurentiuM1234 · 2022-07-07T14:46:00Z

This depends on PR: #926

plbossart

Sounds good to me, just not clear on the new PLATFORM variable.

plbossart · 2022-07-07T15:02:00Z

tools/kmod/sof_remove.sh

-
+TOPDIR=$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)
+
+case $PLATFORM in


how is PLATFORM set?

"This depends on PR: #926"

Please make that an optional command line parameter. Environment variables suck, they hide from logs.

Something like this:

main() { local subscript="$1" # e.g.: 'intel' if [ -n "$subscript" ]; then "$TOPDIR"/tools/kmod/sof_remove_"$subscript".sh return $? fi for remove_sub_script in "$TOPDIR"/tools/kmod/sof_remove_*.sh; do "$remove_sub_script" done } main "$@"

A main function because #740

@marc-hb very good suggestion. One single comment about:

for remove_sub_script in "$TOPDIR"/tools/kmod/sof_remove_*.sh; do "$remove_sub_script" done

I'm not sure this is the correct way to run the sof_remove* scripts as for example it doesn't make any sens to try to remove IMX modules on Intel platform or viceversa.

marc-hb

~~I don't understand why this required~~, the current code is already able to skip missing or unloaded modules, it already does that routinely. Can you explain why you can't just append other modules to the existing lists? What problem does this solve?

Also, are all snd_soc_* really Intel-specific? If they are, then why are some modules named snd_soc_intel_*? Also, where would amd/snd-soc-acp-rt5645-mach.ko fit?

plbossart · 2022-07-07T21:44:13Z

I don't understand why this required, the current code is already able to skip missing or unloaded modules, it already does that routinely. Can you explain why you can't just append other modules to the existing lists? What problem does this solve?

Also, are all snd_soc_* really Intel-specific? If they are, then why are some modules named snd_soc_intel_*? Also, where would amd/snd-soc-acp-rt5645-mach.ko fit?

You're right @marc-hb but in practice I don't think we want to have a giant file with all modules managed by all SOF contributors though. Too many changes left and right and too much churn. It's wise to split the parts that are platform specific and let each contributor deal with their own additions.

In general, all modules with just 'sof' are intel only, except that we didn't do this for machine drivers. snd_soc_sof_sdw is intel specific but we forgot the prefix. So there will be some interesting cases to deal with.

tools/kmod/sof_insert_intel.sh

tools/kmod/sof_remove_intel.sh

marc-hb · 2022-07-07T22:24:17Z

tools/kmod/sof_remove.sh

-
+TOPDIR=$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)
+
+case $PLATFORM in


Please make that an optional command line parameter. Environment variables suck, they hide from logs.

Something like this:

main() { local subscript="$1" # e.g.: 'intel' if [ -n "$subscript" ]; then "$TOPDIR"/tools/kmod/sof_remove_"$subscript".sh return $? fi for remove_sub_script in "$TOPDIR"/tools/kmod/sof_remove_*.sh; do "$remove_sub_script" done } main "$@"

A main function because #740

LaurentiuM1234 · 2022-07-11T14:51:21Z

Thanks for the comments @plbossart @marc-hb ! Based on your suggestions, I have made the following changes:

Based on:

This line is very old and obsolete, feel free to drop it. It predates trap, before trap there was a custom exit function overriding the default one. That's gone.

I have decided to remove said line.

Based on:

No copy/paste/diverge please. Create new functions in lib.sh if needed. Is a function needed though? Why not keep it in the main sof_remove.sh script? Because you don't want it for IMX? Which parts don't you want more specifically and why?

It was a poor choice from my part to copy and paste those lines instead of adding them in one place. Now all of those pre-insert/pre-remove steps have been moved to a setup function which in turn is called by the main function in sof_{remove/insert}.sh.

sof_{remove/insert}.sh now contains a main function in which the appropriate module insertion/removal script is called based on the first parameter received from CL. Reason: clean-up: move all shell script code to a function and use a "main" #740
I have added a new parameter to check-kmod-load-unload{-after-playback}.sh which in turn is used when calling sof_{remove/insert}.sh. The purpose of this parameter is to allow sof_{remove/insert}.sh to call the appropriate module
insertion/removal scripts.

marc-hb · 2022-07-11T16:46:33Z

@LaurentiuM1234 please add a # shellcheck source=... line before each source command, see examples in other scripts in the same repo, This should fix most of the shellcheck warnings. Then please take a look at the remaining warnings. To use it that line, invoke shellcheck with the -x option at the repo's top-level.

(I didn't review the current version yet)

marc-hb · 2022-07-11T16:47:45Z

I don't know why yet but the consistent check-sof-logger failures in https://sof-ci.01.org/softestpr/PR927/build63/devicetest/ really seem caused by this.

/home/ubuntu/sof-test/tools/kmod/sof_remove.sh: line 21: kill_trace_users: command not found
/home/ubuntu/sof-test/tools/kmod/sof_remove.sh: line 1: exit_handler: command not found

marc-hb

First, thanks for the stellar breakdown into multiple commits, it makes the review so much easier, I wish we'd see that more often!

It will also help git blame work across renames.

Looks mostly good to me except for the new mandatory "$platform" argument. @plbossart convinced me this new argument is a good thing but I do not agree it should be mandatory. It should also not be called "platform", more on that below.

I'm not sure this is the correct way to run the sof_remove* scripts as for example it doesn't make any sens to try to remove IMX modules on Intel platform or viceversa.

I see a few reasons why it makes sense to have a stupid for loop that runs all the scripts for all platforms when no $platform argument is passed

Backward-compatibility. This script is invoked in a number of places and not just in sof-test.
"snd_soc_sof_sdw is intel specific but we forgot the prefix. So there will be some interesting cases to deal with." => there will be corner cases and bugs and then a stupid for loop will help
I know Yocto is all about optimizing and keeping only the absolute minimum necessary but "traditional" Linux distributions are not like that.
Someone could actually want to try to load and unload modules from all vendors even when some don't drive any hardware for increased test coverage
If you don't want to use the stupid default for IMX/yocto then simply ignore it and don't use it and it will make no difference to you. It will be only 3 lines of sof-test code.

As I wrote above, the new argument must not be called "platform" not because the name "platform" is super vague and sucks (even when it does) but because it's already being used all across sof and sof-test for making the difference between intel product generations. Just git grep -i platform in sof and sof-test and you will see. I suggest $vendor?

tools/kmod/sof_insert_intel.sh

marc-hb · 2022-07-12T01:03:56Z

Please make that an optional command line parameter. Environment variables suck, they hide from logs.

I have added a new [platform->vendor] parameter to check-kmod-load-unload{-after-playback}.sh which in turn is used when calling sof_{remove/insert}.sh

This parameter is also needed in check-sof-logger.sh (which could be the reason why https://sof-ci.01.org/softestpr/PR927/build63/devicetest/ failed) and also needed in check-suspend-resume.sh (recently added) and maybe others in the future. This does not scale, I didn't fully realize this when I asked you to switch to a CLI parameter, apologies. I still hate environment variables (they hide in CI and don't show in logs) but in this case I think your first shot was right.

Please add a new, optional $VENDOR (or some better name) to config.sh so you don't have to modify any test. Still default to empty = everything, I did not change mind about the default.

marc-hb · 2022-07-12T18:30:28Z

Current status just for the record: modified tests are failing in https://sof-ci.01.org/softestpr/PR927/build69/devicetest/ and there is a config.sh conflict with your other PR that I just merged.

dbaluta · 2022-07-13T12:48:51Z

I see a few reasons why it makes sense to have a stupid for loop that runs all the scripts for all platforms when no $platform argument is passed

@marc-hb i have no strong preference on this. Current implementation actually follows your advice. If no parameter is given then we iterate over all insert/remove *sh scripts and we ran them.

The problem is that for example trying to insmod Intel drivers on IMX will cause a fail. Similar happens when you try to insert i.MX ARM compiled drivers on Intel.

Maybe there is something I'm missing. Trying to call sof_insert_imx.sh or sof_remove_imx.sh on Intel most likely will fail with an error so the entire test will fail.

For example, we can see the following log when running current PR:

SKIP	snd_sof_imx8  	not loaded
SKIP	snd_sof_imx8m  	not loaded
SKIP	imx_common  	not loaded
RMMOD	snd_sof_xtensa_dsp
rmmod: ERROR: Module snd_sof_xtensa_dsp is in use by: snd_sof_intel_hda_common

We have this common module snd_sof_xtensa_dsp which sof_remove_imx.sh tries to remove. But this is nonsense. Because sof_remove_imx.sh doesn't correctly know the order of modules to be removed on Intel. And it doesn't need to know.

What we can do for backward compatibility is to assume if no argument is given that the vendor is Intel and we preserve the behavior before this patch.

marc-hb · 2022-07-13T16:14:58Z

rmmod: ERROR: Module snd_sof_xtensa_dsp is in use by: snd_sof_intel_hda_common

We have this common module snd_sof_xtensa_dsp which sof_remove_imx.sh tries to remove. But this is nonsense. Because sof_remove_imx.sh doesn't correctly know the order of modules to be removed on Intel. And it doesn't need to know.

I think this just shows a copy/paste/diverge issue. So the rationale for this split is:

but in practice I don't think we want to have a giant file with all modules managed by all SOF contributors though. Too many changes left and right and too much churn. It's wise to split the parts that are platform specific and let each contributor deal with their own additions.

If better maintenance is the goal then common modules should be in some common.sh file, their maintenance should note be copied/pasted/diverged across multiple vendor scripts.

For instance something like this:

    if [ -n "$VENDOR" ]; then
        "$TOPDIR"/tools/kmod/sof_remove_"$VENDOR".sh || return $?
    fi

    dlogi "VENDOR not specified. Running all removal subscripts."

    for remove_subscript in "$TOPDIR"/tools/kmod/sof_remove_*.sh; do
        dlogi "Currently running: $remove_subscript"
        "$remove_subscript" || return $?
    done

   "$TOPDIR"/tools/kmod/sof_common_remove.sh

Insertion should NOT have that problem thanks to dependencies, common modules should not be loaded explicitly.

BTW I'm still curious what stopped you from simply adding IMX modules to the current "giant files".

Speaking of return $?, while you're making all these changes and testing them please try to add set -e here and there. It's not perfect but it helps a lot (#312)

dbaluta · 2022-07-13T16:33:03Z

BTW I'm still curious what stopped you from simply adding IMX modules to the current "giant files".

Our first thought was to modularize things so that we developers should only care about their platform. Adding IMX modules to the giant files would make them even more "giant" :).

Anyway, can we draw a conclusion on this?

LaurentiuM1234 · 2022-07-13T16:56:10Z

BTW I'm still curious what stopped you from simply adding IMX modules to the current "giant files".

Because I believe it wouldn't scale very well:

You're right @marc-hb but in practice I don't think we want to have a giant file with all modules managed by all SOF contributors though. Too many changes left and right and too much churn. It's wise to split the parts that are platform specific and let each contributor deal with their own additions.

Let's assume we go for the huge script and let's assume sof_remove.sh contains the following statements:

...
remove_module A
remove_module B
...

Let's also assume that A uses B. What if there's another contributor n who has the following modules: A, B and C with A using C and C using B. Wouldn't it be easier for said contributor to just create his own sof_remove_n.sh instead of having to find modules A and B in the initial file and inserting module C in between them? What if the file contains hundreds of lines?

Also, what would be the point of trying to remove/insert let's say hundreds of modules that you know can't be used on your platform?

LaurentiuM1234 · 2022-07-13T17:08:01Z

    if [ -n "$VENDOR" ]; then
        "$TOPDIR"/tools/kmod/sof_remove_"$VENDOR".sh || return $?
    fi

    dlogi "VENDOR not specified. Running all removal subscripts."

    for remove_subscript in "$TOPDIR"/tools/kmod/sof_remove_*.sh; do
        dlogi "Currently running: $remove_subscript"
        "$remove_subscript" || return $?
    done

   "$TOPDIR"/tools/kmod/sof_common_remove.sh

What if we had the following case:

Contributor 1 has modules A and C. C is being used by A.
Contributor 2 has modules B and C. B is being used by C.

Since C is a common module then it would have to be placed in sof_common_remove.sh, right? That would mean that either A and B are removed before C or C is removed before A and B. First case wouldn't work on contributor 2's platform because B is used by C so C should be removed first. Second case wouldn't work on contributor 1's platform.

marc-hb · 2022-07-13T17:13:14Z

Contributor 1 has modules A and C. C is being used by A.
Contributor 2 has modules B and C. B is being used by C.

Since C is a common module...

vendor-A depends on common-C which depends on vendor-B... how can a common module depend on a vendor module?

marc-hb · 2022-07-13T17:20:32Z

Wouldn't it be easier for said contributor to just create his own sof_remove_n.sh instead of having to find modules A and B in the initial file and inserting module C in between them? What if the file contains hundreds of lines?

That's more or less what exists already and it has required little effort. The "giant" file already used groups to make that easier, moving these groups to separate files does not seem like a ground-breaking improvement to me and it does complicate the scripting.

Also, what would be the point of trying to remove/insert let's say hundreds of modules that you know can't be used on your platform?

Because it's simple and it works.

The only potential problem I see is this:

Too many changes left and right and too much churn

So yes, conflicts are not fun. But they haven't happened in practice yet and based on past history I'm not sure they would have been that frequent. It's not like modules get created or renamed every day.

LaurentiuM1234 · 2022-07-13T17:32:12Z

    if [ -n "$VENDOR" ]; then
        "$TOPDIR"/tools/kmod/sof_remove_"$VENDOR".sh || return $?
    fi

    dlogi "VENDOR not specified. Running all removal subscripts."

    for remove_subscript in "$TOPDIR"/tools/kmod/sof_remove_*.sh; do
        dlogi "Currently running: $remove_subscript"
        "$remove_subscript" || return $?
    done

   "$TOPDIR"/tools/kmod/sof_common_remove.sh
What if we had the following case:

Contributor 1 has modules A and C. C is being used by A.
Contributor 2 has modules B and C. B is being used by C.

Since C is a common module then it would have to be placed in sof_common_remove.sh, right? That would mean that either A and B are removed before C or C is removed before A and B. First case wouldn't work on contributor 2's platform because B is used by C so C should be removed first. Second case wouldn't work on contributor 1's platform.

@marc-hb You're right, this is not possible, sorry.

I'll just add the i.MX modules to the existing sof_{remove/insert}.sh scripts.

marc-hb · 2022-07-13T18:31:02Z

I'll just add the i.MX modules to the existing sof_{remove/insert}.sh scripts.

Hey I'm not totally opposed to this split, don't get me wrong. I'm just saying it may cause more problems than it solves, at least initially.

dbaluta · 2022-07-13T18:32:50Z

I agree we can add the imx modules to current scripts.

dbaluta · 2022-07-14T08:46:30Z

tools/kmod/sof_insert.sh

@@ -96,6 +98,20 @@ insert_module snd_sof_pci_intel_icl
 insert_module snd_sof_pci_intel_tgl
 insert_module snd_sof_pci_intel_mtl

+# core SOF driver
+insert_module snd_sof
+


I would move core SOF driver before `top level ACPI/PCI SOF drivers.

Thanks ! Fixed.

Added modules used by i.MX. This change will allow tests based on module insertion/removal to work on i.MX. Signed-off-by: Laurentiu Mihalcea <[email protected]>

test-case/check-kmod-load-unload.sh

test-case/check-kmod-load-unload-after-playback.sh

marc-hb · 2022-07-14T22:38:47Z

Sorry for the Outdated (and now "resolved") comments above, please ignore.

The 2 errors in https://sof-ci.01.org/softestpr/PR927/build79/devicetest/ are unrelated.

LaurentiuM1234 requested review from a team and plbossart as code owners July 7, 2022 14:46

plbossart requested a review from marc-hb July 7, 2022 15:01

plbossart reviewed Jul 7, 2022

View reviewed changes

marc-hb reviewed Jul 7, 2022

View reviewed changes

marc-hb requested changes Jul 7, 2022

View reviewed changes

LaurentiuM1234 force-pushed the add_insert branch from 3d340f3 to 071f0ae Compare July 11, 2022 14:34

marc-hb requested changes Jul 11, 2022

View reviewed changes

tools/kmod/sof_insert_intel.sh Outdated Show resolved Hide resolved

LaurentiuM1234 force-pushed the add_insert branch 2 times, most recently from 6e0cd3a to bf0411b Compare July 12, 2022 12:04

LaurentiuM1234 marked this pull request as draft July 12, 2022 13:32

LaurentiuM1234 force-pushed the add_insert branch from bf0411b to 64deeca Compare July 12, 2022 13:49

LaurentiuM1234 marked this pull request as ready for review July 12, 2022 13:50

LaurentiuM1234 force-pushed the add_insert branch 2 times, most recently from d112f2d to 4a03669 Compare July 13, 2022 10:29

LaurentiuM1234 force-pushed the add_insert branch from 4a03669 to 6fb7cba Compare July 14, 2022 08:35

dbaluta reviewed Jul 14, 2022

View reviewed changes

sof_{insert/remove}.sh: Add support for i.MX modules

6987b36

Added modules used by i.MX. This change will allow tests based on module insertion/removal to work on i.MX. Signed-off-by: Laurentiu Mihalcea <[email protected]>

LaurentiuM1234 force-pushed the add_insert branch from 6fb7cba to 6987b36 Compare July 14, 2022 10:20

marc-hb approved these changes Jul 14, 2022

View reviewed changes

dbaluta approved these changes Jul 15, 2022

View reviewed changes

dbaluta merged commit 165e416 into thesofproject:main Jul 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC]: Add i.MX tests for module insert/removal #927

[RFC]: Add i.MX tests for module insert/removal #927

LaurentiuM1234 commented Jul 7, 2022

plbossart left a comment

plbossart Jul 7, 2022

marc-hb Jul 7, 2022

marc-hb Jul 7, 2022

dbaluta Jul 11, 2022

marc-hb left a comment •

edited

Loading

plbossart commented Jul 7, 2022

marc-hb Jul 7, 2022

LaurentiuM1234 commented Jul 11, 2022

marc-hb commented Jul 11, 2022

marc-hb commented Jul 11, 2022 •

edited

Loading

marc-hb left a comment •

edited

Loading

marc-hb commented Jul 12, 2022 •

edited

Loading

marc-hb commented Jul 12, 2022

dbaluta commented Jul 13, 2022 •

edited

Loading

marc-hb commented Jul 13, 2022

dbaluta commented Jul 13, 2022

LaurentiuM1234 commented Jul 13, 2022 •

edited

Loading

LaurentiuM1234 commented Jul 13, 2022

marc-hb commented Jul 13, 2022

marc-hb commented Jul 13, 2022

LaurentiuM1234 commented Jul 13, 2022

marc-hb commented Jul 13, 2022

dbaluta commented Jul 13, 2022

dbaluta Jul 14, 2022

LaurentiuM1234 Jul 14, 2022

marc-hb commented Jul 14, 2022 •

edited

Loading


		TOPDIR=$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)

		case $PLATFORM in

[RFC]: Add i.MX tests for module insert/removal #927

[RFC]: Add i.MX tests for module insert/removal #927

Conversation

LaurentiuM1234 commented Jul 7, 2022

plbossart left a comment

Choose a reason for hiding this comment

plbossart Jul 7, 2022

Choose a reason for hiding this comment

marc-hb Jul 7, 2022

Choose a reason for hiding this comment

marc-hb Jul 7, 2022

Choose a reason for hiding this comment

dbaluta Jul 11, 2022

Choose a reason for hiding this comment

marc-hb left a comment • edited Loading

Choose a reason for hiding this comment

plbossart commented Jul 7, 2022

marc-hb Jul 7, 2022

Choose a reason for hiding this comment

LaurentiuM1234 commented Jul 11, 2022

marc-hb commented Jul 11, 2022

marc-hb commented Jul 11, 2022 • edited Loading

marc-hb left a comment • edited Loading

Choose a reason for hiding this comment

marc-hb commented Jul 12, 2022 • edited Loading

marc-hb commented Jul 12, 2022

dbaluta commented Jul 13, 2022 • edited Loading

marc-hb commented Jul 13, 2022

dbaluta commented Jul 13, 2022

LaurentiuM1234 commented Jul 13, 2022 • edited Loading

LaurentiuM1234 commented Jul 13, 2022

marc-hb commented Jul 13, 2022

marc-hb commented Jul 13, 2022

LaurentiuM1234 commented Jul 13, 2022

marc-hb commented Jul 13, 2022

dbaluta commented Jul 13, 2022

dbaluta Jul 14, 2022

Choose a reason for hiding this comment

LaurentiuM1234 Jul 14, 2022

Choose a reason for hiding this comment

marc-hb commented Jul 14, 2022 • edited Loading

marc-hb left a comment •

edited

Loading

marc-hb commented Jul 11, 2022 •

edited

Loading

marc-hb left a comment •

edited

Loading

marc-hb commented Jul 12, 2022 •

edited

Loading

dbaluta commented Jul 13, 2022 •

edited

Loading

LaurentiuM1234 commented Jul 13, 2022 •

edited

Loading

marc-hb commented Jul 14, 2022 •

edited

Loading