Skip to content

ASoC/SoundWire: fix two issues for rmmod/modprobe machine driver tests #3534

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
plbossart wants to merge 2 commits into from
Closed

ASoC/SoundWire: fix two issues for rmmod/modprobe machine driver tests #3534

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
82d64fb
soundwire: bus_type: reset pointers on remove and shutdown
plbossart Mar 22, 2022
cbd8778
ASoC: SOF: topology: cleanup dailinks on widget unload
plbossart Mar 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions drivers/soundwire/bus_type.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,8 @@ static int sdw_drv_probe(struct device *dev)

ret = drv->probe(slave, id);
if (ret) {
slave->ops = NULL;

ghost Mar 23, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we set slave->ops = NULL; when dev_pm_domain_attach return error , too?

ghost Mar 23, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes indeed, thanks for spotting this @bardliao


name = drv->name;
if (!name)
name = drv->driver.name;
Expand Down Expand Up @@ -152,6 +154,9 @@ static int sdw_drv_remove(struct device *dev)
struct sdw_driver *drv = drv_to_sdw_driver(dev->driver);
int ret = 0;

slave->ops = NULL;
slave->probed = false;

if (drv->remove)
ret = drv->remove(slave);
Comment thread
RanderWang marked this conversation as resolved.

Expand All @@ -165,6 +170,9 @@ static void sdw_drv_shutdown(struct device *dev)
struct sdw_slave *slave = dev_to_sdw_dev(dev);
struct sdw_driver *drv = drv_to_sdw_driver(dev->driver);

slave->ops = NULL;
slave->probed = false;

if (drv->shutdown)
drv->shutdown(slave);
}
Expand Down
43 changes: 43 additions & 0 deletions sound/soc/sof/topology.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1065,6 +1065,46 @@ static int sof_connect_dai_widget(struct snd_soc_component *scomp,
return 0;
}

static void sof_disconnect_dai_widget(struct snd_soc_component *scomp,
struct snd_soc_dapm_widget *w)
{
struct snd_soc_card *card = scomp->card;
struct snd_soc_pcm_runtime *rtd;
struct snd_soc_dai *cpu_dai;
int i;

list_for_each_entry(rtd, &card->rtd_list, list) {

if (!w->sname || !rtd->dai_link->stream_name)

ghost Mar 23, 2022 •
edited by ujfalusi
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the if (!w->sname) can be moved before the big loop, it is not going to change within.

ghost Mar 23, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's the same loop as in the 'register' case, so I just copy-pasted. we can change the loop but we have to do it consistently in the register and unregister cases

continue;

/* does stream match DAI link ? */
if (strcmp(w->sname, rtd->dai_link->stream_name))

ghost Mar 23, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (!rtd->dai_link->stream_name || strcmp(w->sname, rtd->dai_link->stream_name)) ?

ghost Mar 23, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same, it's the same loop as in the 'register' case, so I just copy-pasted. we can change the loop but we have to do it consistently in the register and unregister cases

ghost Mar 24, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. The sof_connect_dai_widget() has similar problem. If w->sname == NULL then it is going to fail as it will just skip through the list_for_each_entry(rtd, &card->rtd_list, list) and will not set the dai->name.
The sof_disconnect_dai_widget() would not be called as we have already failed in the connect phase.

continue;

switch (w->id) {
case snd_soc_dapm_dai_out:
for_each_rtd_cpu_dais(rtd, i, cpu_dai) {
if (cpu_dai->capture_widget == w) {
cpu_dai->capture_widget = NULL;
break;
}
}
break;
case snd_soc_dapm_dai_in:
for_each_rtd_cpu_dais(rtd, i, cpu_dai) {
if (cpu_dai->playback_widget == w) {
cpu_dai->playback_widget = NULL;
break;
}
}
break;
default:
break;
}
}
}

/* bind PCM ID to host component ID */
static int spcm_bind(struct snd_soc_component *scomp, struct snd_sof_pcm *spcm,
int dir)
Expand Down Expand Up @@ -1383,6 +1423,9 @@ static int sof_widget_unload(struct snd_soc_component *scomp,

if (dai)
list_del(&dai->list);

sof_disconnect_dai_widget(scomp, widget);

break;
default:
break;
Expand Down