ASoC: SOF: topology: skip adding unsupported widgets to the list

[ranj063]

Some topologies like the HDA generic topologies add virtual widgets in order to reuse the HDA generic machine driver without any changes. When parsing such widgets, there's no need to add an swidget pointer to the sdev's widget list.

Add such virtual widgets lead to a null pointer exception when tearing down the pipelines during suspend and only seems to manifest in some platforms. Fix this by skipping adding the swidget pointer for such widgets during topology parsing.

Link: #3988
Fixes: #3988

[plbossart]

Merging blindly since CI does not give us ANY results. Yay @keqiaozhang@marc-hb@greg-intel@mengdonglin. This is a post-Halloween nightmare.

[marc-hb]

@plbossart while the public mirroring is down (stuck in unexpected red tape), all the results should be available on the internal server. Please contact us privately if you need guidance.

[marc-hb]

@ranj063 , @aiChaoSONG was this regression bisected to any particular commit? Also, shouldn't this have been a fixup commit to avoid introducing the regression and boot failure #3988 upstream?

[ranj063]

@ranj063 , @aiChaoSONG was this regression bisected to any particular commit? Also, shouldn't this have been a fixup commit to avoid introducing the regression and boot failure #3988 upstream?

It was a fixup. Did you see the first patch?

[plbossart]

@plbossart while the public mirroring is down (stuck in unexpected red tape), all the results should be available on the internal server. Please contact us privately if you need guidance.

It's not just the test results, even the built status is not updated. It's getting worse every day.

[marc-hb]

It's not just the test results, even the built status is not updated.

Thanks I missed that. I see it now when I click on the View details button.

Indeed the build status follows a different, more direct path and was so far not affected. I don't know what happened in this PR.

I just looked at two of the most recent pull requests and the build status is back:

• fixup! ASoC: SOF: Introduce struct snd_sof_pipeline #3990
• audio: up_down_mixer: add stubs for generic ANSI C version sof#6532

cc: @greg-intel , @keqiaozhang , @fredoh9

[marc-hb]

It was a fixup. Did you see the first patch?

My bad, I didn't pay attention. I didn't expect a "quickfix" PR to have more than one commit.

So now partially answering my own questions for the record: this PR has a fixup of commit 6e2cdc2 in PR #3957.

Note there was another, earlier fixup between initial commit 6e2cdc2 and this second fixup here. So now I wonder which one introduced the boot failure: the initial commit or the first fixup?

Both the initial commit and the first fixup had red build status and they were merged anyway.

I'm asking all these questions because the boot failure knocked down most of our CI (it shouldn't have, grub-reboot should have saved us but it didn't) so we need at the very least a good record of it.

[ranj063]

Note there was another, earlier fixup between initial commit 6e2cdc2 and this second fixup here. So now I wonder which one introduced the boot failure: the initial commit or the first fixup?

the first commit. Eventually before upstreaming I will squash all these into one. So it wouldnt matter

[ujfalusi]

For the record:
I think the proper fixup would be:

diff --git a/sound/soc/sof/ipc3-topology.c b/sound/soc/sof/ipc3-topology.c
index 7870260c6f3e..88b9b9507231 100644
--- a/sound/soc/sof/ipc3-topology.c+++ b/sound/soc/sof/ipc3-topology.c@@ -2317,7 +2317,8 @@ static int sof_ipc3_tear_down_all_pipelines(struct snd_sof_dev *sdev, bool verif
if (!verify && !swidget->dynamic_pipeline_widget &&
SOF_FW_VER(v->major, v->minor, v->micro) < SOF_FW_VER(2, 2, 0)) {
swidget->use_count = 0;
- swidget->spipe->complete = 0;+ if (swidget->spipe)+ swidget->spipe->complete = 0;
continue;
}

Because the reason for the NULL pointer dereference is the fact that sof_ipc3_tear_down_all_pipelines() iterates through the global swidget list and the "ASoC: SOF: Introduce struct snd_sof_pipeline" move the complete flag under spipe which can be NULL if the swidget is not part of any path (virtual/not handled widgets).

This change would be better on it's own to be backportable when sending upstream.