Skip to content

Module loading/Unloading fixes for SOF and legacy drivers #765

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
ranj063 wants to merge 9 commits into from
Closed

Module loading/Unloading fixes for SOF and legacy drivers #765

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
22f0cad
ASoC: topology: Use the correct dobj to free enum control values and …
ranj063 Apr 1, 2019
05fb82d
ASoC: core: do not unload topology in unregister_component()
ranj063 Apr 1, 2019
f6d9670
ASoC: intel: skylake: add remove() callback for component driver
ranj063 Apr 1, 2019
745a007
ASoC: core: remove link components before cleaning up card resources
ranj063 Apr 1, 2019
e5b1280
ASoC: SOF: remove topology in component driver remove()
ranj063 Apr 1, 2019
8e8ab0c
ASoC: SOF: core: make component driver resource-managed
ranj063 Apr 1, 2019
feae2d4
ASoC: core: rename ignore_module_refcount in component driver
ranj063 Apr 1, 2019
fe57de4
ASoC: SOF: change the ignore_module_refcount to align with the new name
ranj063 Apr 1, 2019
53c42c5
ASoC: pcm: update module refcount if module_get_upon_open is set
ranj063 Apr 1, 2019
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
9 changes: 7 additions & 2 deletions include/sound/soc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -802,8 +802,13 @@ struct snd_soc_component_driver {
int probe_order;
int remove_order;

/* signal if the module handling the component cannot be removed */

@plbossart plbossart Apr 1, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You should add a
Fixes: SHA1 ('title') tag

unsigned int ignore_module_refcount:1;
/*
* signal if the module handling the component should not be removed
* if a pcm is open. Setting this would prevent the module
* refcount being incremented in probe() but allow it be incremented
* when a pcm is opened and decremented when it is closed.
*/
unsigned int module_get_upon_open:1;

/* bits */
unsigned int idle_bias_on:1;
Expand Down
10 changes: 9 additions & 1 deletion sound/soc/intel/skylake/skl-pcm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1462,13 +1462,21 @@ static int skl_platform_soc_probe(struct snd_soc_component *component)
return 0;
}

static void skl_pcm_remove(struct snd_soc_component *component)
{
/* remove topology */
snd_soc_tplg_component_remove(component, SND_SOC_TPLG_INDEX_ALL);
}


static const struct snd_soc_component_driver skl_component = {
.name = "pcm",
.probe = skl_platform_soc_probe,
.remove = skl_pcm_remove,
.ops = &skl_platform_ops,
.pcm_new = skl_pcm_new,
.pcm_free = skl_pcm_free,
.ignore_module_refcount = 1, /* do not increase the refcount in core */
.module_get_upon_open = 1, /* increase the refcount when a pcm is opened */

@plbossart plbossart Apr 1, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I wasn't crazy, this needs to be squashed with the previous patch to avoid bisect issues.

};

int skl_platform_register(struct device *dev)
Expand Down
18 changes: 14 additions & 4 deletions sound/soc/soc-core.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -947,7 +947,7 @@ static void soc_cleanup_component(struct snd_soc_component *component)
snd_soc_dapm_free(snd_soc_component_get_dapm(component));
soc_cleanup_component_debugfs(component);
component->card = NULL;
if (!component->driver->ignore_module_refcount)
if (!component->driver->module_get_upon_open)
module_put(component->dev->driver->owner);
}

Expand Down Expand Up @@ -1381,7 +1381,7 @@ static int soc_probe_component(struct snd_soc_card *card,
return 0;
}

if (!component->driver->ignore_module_refcount &&
if (!component->driver->module_get_upon_open &&

@plbossart plbossart Apr 1, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

wasn't there a patch for SKL as well that needs to be updated?

!try_module_get(component->dev->driver->owner))
return -ENODEV;

Expand Down Expand Up @@ -2831,10 +2831,22 @@ EXPORT_SYMBOL_GPL(snd_soc_register_card);

static void snd_soc_unbind_card(struct snd_soc_card *card, bool unregister)
{
struct snd_soc_pcm_runtime *rtd;
int order;

if (card->instantiated) {
card->instantiated = false;
snd_soc_dapm_shutdown(card);
snd_soc_flush_all_delayed_work(card);

/* remove all components used by DAI links on this card */
for_each_comp_order(order) {
for_each_card_rtds(card, rtd) {
soc_remove_link_components(card, rtd, order);
}
}

@plbossart plbossart Apr 1, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"snd_soc_bind_card() probes the link components after the snd_card
is registered". Looking at the code, this is far from obvious, and the code to remove link components does not seem to have a matching equivalent in snd_soc_bind_card(). Either the commit message is wrong or it omits a precision needed for your esteemed (?) reviewer to see the beauty of this PR.


/* clean up card resources */
soc_cleanup_card_resources(card);
if (!unregister)
list_add(&card->list, &unbind_card_list);
Expand Down Expand Up @@ -3327,8 +3339,6 @@ static int __snd_soc_unregister_component(struct device *dev)
if (dev != component->dev)
continue;

snd_soc_tplg_component_remove(component,
SND_SOC_TPLG_INDEX_ALL);
snd_soc_component_del_unlocked(component);
found = 1;
break;
Expand Down
8 changes: 8 additions & 0 deletions sound/soc/soc-pcm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include <linux/delay.h>
Comment thread
ranj063 marked this conversation as resolved.
Outdated
#include <linux/pinctrl/consumer.h>
#include <linux/pm_runtime.h>
#include <linux/module.h>
#include <linux/slab.h>
#include <linux/workqueue.h>
#include <linux/export.h>
Expand Down Expand Up @@ -463,6 +464,9 @@ static int soc_pcm_components_close(struct snd_pcm_substream *substream,
continue;

component->driver->ops->close(substream);

if (component->driver->module_get_upon_open)
module_put(component->dev->driver->owner);
}

return 0;
Expand Down Expand Up @@ -513,6 +517,10 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
!component->driver->ops->open)
continue;

if (component->driver->module_get_upon_open)
if (!try_module_get(component->dev->driver->owner))
return -ENODEV;

ret = component->driver->ops->open(substream);
if (ret < 0) {
dev_err(component->dev,
Expand Down
7 changes: 4 additions & 3 deletions sound/soc/soc-topology.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,10 +482,11 @@ static void remove_widget(struct snd_soc_component *comp,

snd_ctl_remove(card, kcontrol);

kfree(dobj->control.dvalues);
/* free enum kcontrol's dvalues and dtexts */
kfree(se->dobj.control.dvalues);
for (j = 0; j < se->items; j++)
kfree(dobj->control.dtexts[j]);
kfree(dobj->control.dtexts);
kfree(se->dobj.control.dtexts[j]);
kfree(se->dobj.control.dtexts);

@plbossart plbossart Apr 1, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

help me understand something here: the existing code pattern is repeated a number of times, e.g. it's also used for enums:
kfree(dobj->control.dvalues);
for (i = 0; i < se->items; i++)
kfree(dobj->control.dtexts[i]);
kfree(dobj->control.dtexts);

so either we fix this everywhere or we don't. I feel uncomfortable with a specific change that breaks a mostly regular pattern so far.

@ranj063 ranj063 Apr 1, 2019 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@plbossart it cant be changed everywhere. In the remove_enum() method, the dobj is actually the control's dobj. whereas in remove_widget, the input argument dobj is the widget's dobj and what we're trying to delete is the control's dobj's texts and values.
It was a genuine null pointer dereference I saw with the legacy driver that led to me discovering this error while trying to unload the snd_soc_skl module. We dont have enum controls in SOF. So we never encountered this problem before.

@plbossart plbossart Apr 2, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ranj063 then you'd really want a comment telling reviewers/users of that code that this is very specific and intentional with explanations

@ranj063 ranj063 Apr 2, 2019

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@plbossart updated this now. I know the change isnt quite obvious but it is a serious error and hopefully the comment clarifies it.


kfree(se);
kfree(w->kcontrol_news[i].name);
Expand Down
10 changes: 5 additions & 5 deletions sound/soc/sof/core.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ static int sof_probe_continue(struct snd_sof_dev *sdev)
sdev->first_boot = false;
Comment thread
ranj063 marked this conversation as resolved.
Outdated

/* now register audio DSP platform driver and dai */
ret = snd_soc_register_component(sdev->dev, &sdev->plat_drv,

@plbossart plbossart Apr 1, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

commit message still riddled with spelling mistakes :-(

@ranj063 ranj063 Apr 2, 2019

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@plbossart hmm I knew it wasnt a good idea to just stop wearing glasses. time to make that trip to the optometrist soon. fixed the typos now.

ret = devm_snd_soc_register_component(sdev->dev, &sdev->plat_drv,
sof_ops(sdev)->drv,
sof_ops(sdev)->num_drv);
if (ret < 0) {
Expand Down Expand Up @@ -461,16 +461,16 @@ int snd_sof_device_remove(struct device *dev)
if (IS_ENABLED(CONFIG_SND_SOC_SOF_PROBE_WORK_QUEUE))
cancel_work_sync(&sdev->probe_work);

snd_soc_unregister_component(dev);
snd_sof_fw_unload(sdev);
snd_sof_ipc_free(sdev);
snd_sof_free_debug(sdev);
snd_sof_free_trace(sdev);
snd_sof_remove(sdev);

/*
* platform_device_unregister() frees the card and its resources.
* So it should be called after unregistering the comp driver
* so that the card is valid while unregistering comp driver.
* Unregister machine driver. This will unbind the snd_card which
* will remove the component driver and unload the topology
* before freeing the snd_card.
*/
if (!IS_ERR_OR_NULL(pdata->pdev_mach))
platform_device_unregister(pdata->pdev_mach);
Expand Down
6 changes: 4 additions & 2 deletions sound/soc/sof/pcm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -703,6 +703,8 @@ static int sof_pcm_probe(struct snd_soc_component *component)

static void sof_pcm_remove(struct snd_soc_component *component)
{
/* remove topology */
snd_soc_tplg_component_remove(component, SND_SOC_TPLG_INDEX_ALL);
}

void snd_sof_new_platform_drv(struct snd_sof_dev *sdev)
Expand All @@ -727,6 +729,6 @@ void snd_sof_new_platform_drv(struct snd_sof_dev *sdev)
pd->use_dai_pcm_id = true;
pd->topology_name_prefix = "sof";

/* do not increase the refcount in core */
pd->ignore_module_refcount = 1;
/* increase the refcount when a pcm is opened */
pd->module_get_upon_open = 1;
}