Module loading/Unloading fixes for SOF and legacy drivers by ranj063 · Pull Request #765 · thesofproject/linux

ranj063 · 2019-04-01T16:13:17Z

This pull request contains a series of fixes needed for fixing issues with module unloading/loading.

Fix null pointer dereferencing in remove_widget in soc-topology to
Move topology unloading to component driver remove()
Fix clean_up_card_resources() to remove dai links before freeing snd_card
Make component dev resource managed in SOF
Add remove() callback for component drv in the SKL legacy driver
Finally address the issue with the pci device being allowed to be removed while a pcm stream is open.

Tests performed:

This passed the sof_bootloop test on Up2 with SOF
With the legacy driver, the snd_soc_skl device can now be unloaded without any null pointer dereferences. Tested on GLK chromebook.

plbossart

@ranj063 nice patch set with a number of style issues to be fixed. The main concern I have is the dailink removal in soc-core.c, it looks very invasive and will impact all SOC implementations. Wondering if we can avoid and decouple this from topology removal.

plbossart · 2019-04-01T17:53:04Z

how do you deal with pending operations if they are handled in snd_card_free()?

@plbossart I'm not sure how to handle this at all. The problem is we need to unload topology before the snd_card is freed. Let me think about it a bit more.

@plbossart the only way I can think of avoiding this is by calling snd_soc_tplg_component_remove(component, SND_SOC_TPLG_INDEX_ALL);

in snd_sof_device_remove() for SOF and in skl_remove() for the legacy driver to make sure that the topoogy is unloaded before the card is freed (this would be instead of calling it in the component driver remove()). What do you think about that?

or the other option is before cleaning up the card resources in snd_soc_unbind_card(), I could call soc_remove_link_components() to remove the link components which would remove the topology.
snd_soc_bind_card() calls soc_probe_link_components(), so it would be symmetric

@plbossart i've change PR to call remove component in unbind_card instead. Let me know what you think

ghost · 2019-04-01T17:56:34Z

if (component)

maybe we should rename ignore_module_refcount as check_module_refcount_on_open or something more meaningful than 'ignore' and then use it.

@plbossart sorry about the style fixes. I'll fix them.
I agree it makes sense to rename this field. I havent been able to come up with a nice name that indicates what we're really trying to do here. Let me think about it and rename it today.

done, I've rename it to module_get_upon_open. hopefully thats a bit more indicative of what we want to achieve.

ghost

@ranj063 I am still struggling with the part on the _unbind, mostly since I am not 100% familiar with the code and your comments did not provide the pointer I was looking for. Can you try and made it more self-explanatory, it'll help even with more expert reviewers explain how this works.
Also a couple of commit messages, style, squash and bad refcounts to fix.
Thanks!

ghost · 2019-04-01T22:07:55Z

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.

@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.

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

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

ghost · 2019-04-01T22:15:27Z

"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.

ghost · 2019-04-01T22:16:36Z

commit message still riddled with spelling mistakes :-(

@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.

ghost · 2019-04-01T22:18:18Z

isn't this comment still relevant, even with the use of devm_ ? does it need to be removed completely or reworded?

ghost · 2019-04-01T22:20:16Z

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

ghost · 2019-04-01T22:21:06Z

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

ghost · 2019-04-01T22:21:57Z

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

ghost · 2019-04-01T22:25:10Z

this isn't balanced with the module_get(), which is done only when you have a .ops and a .open. You might decrease references that were not previously incremented.

@plbossart good catch. I did infact put it in the wrong place.

ghost · 2019-04-01T22:28:35Z

@xiulipan the Intel CI seems to be broken?

ghost · 2019-04-01T23:47:15Z

@plbossart sorry for giving you a hard time with this one. It wasnt a very straight forward PR with fixes everywhere. Hopefully the latest version has no style issue anymore.

ghost · 2019-04-02T10:41:50Z

@plbossart
Not sure. It seems a lot of attempt for this PR. I will check for detail and have a round bug fix for it.

…texts "dobj" is the widget dobj. dvalues and dtexts belong to the enum kcontrol's dobj (se->dobj). So, use this to free the control values and texts to prevent the NULL pointer dereference while removing widgets. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Topology is loaded when the component driver is probed. So, it should be removed in the component driver's remove(). This will prevent null pointer dereference while unregistering the component driver. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Topology is not unloaded in the core during unregister_component() anymore. So, add the remove() callback that will unload the topology. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

snd_soc_bind_card() calls snd_soc_instantiate_card() to first register the snd_card and then probe the link components by calling soc_probe_link_components(). The snd_card is used by the component driver during its probe() while loading topology widgets to add the controls to the snd_card. The snd_card also needs to be valid while unloading the topology widgets when the component driver is removed. Therefore, in snd_soc_unbind_card(), the link components should be removed with the call to soc_remove_link_components() before the card resources are cleaned up and the snd_card itself is freed. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

This is no longer done in the core during unregister_component. So, it should be done in the component driver's remove callback(). Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Previously, we couldn't make the component driver resource-managed because topology unloading was done by the core when the component driver was unregistered. But this has been fixed to allow topology removal during the component driver remove() callback and there aren't any NULL pointer dereferences anymore. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Recently, for Intel platforms the "ignore_module_refcount" field was introduced for the component driver which prevented the refcounts being incremented for the device driver module during component probe. However, this leads to the problem of allowing the device driver module to be unloaded while a pcm stream is open. So rename the field to "module_get_upon_open" which is more meaningful in terms of allowing the refcount for the module to be incremented when a pcm is opened. Also, align with the skylake component driver with the new name. Fixes: b450b87('ASoC: core: don't increase component module refcount unconditionally' Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

The "ignore_module_refcount" field in the component driver has been renamed to "module_get_upon_open". So align with the new name. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Setting the module_get_upon_open field for component driver prevents the module refcount from being incremented during component probe(). But this could lead to the module being allowed to be unloaded when a pcm stream is open. So, if this field is set, the module's refcount should be incremented/decremented during pcm open/close to prevent it. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ghost · 2019-04-04T00:07:26Z

@plbossart I'm going to break this PR into 3 separate ones for easier review. Closing this one for now

ranj063 requested review from bardliao, lgirdwood and plbossart and removed request for bardliao April 1, 2019 16:13

ranj063 force-pushed the fix/module_loading branch from c836329 to c2d341b Compare April 1, 2019 16:22

ranj063 requested a review from bardliao April 1, 2019 16:22

ranj063 mentioned this pull request Apr 1, 2019

ASoC:SOF:update refcount of sof_pci_dev during playing #707

Closed

plbossart requested changes Apr 1, 2019

View reviewed changes

ranj063 force-pushed the fix/module_loading branch 7 times, most recently from 0ff6da0 to 413dffe Compare April 1, 2019 21:31

plbossart requested changes Apr 1, 2019

View reviewed changes

ranj063 force-pushed the fix/module_loading branch 4 times, most recently from 386eb34 to 52df475 Compare April 1, 2019 23:46

Ranjani Sridharan added 7 commits April 2, 2019 09:46

ASoC: intel: skylake: add remove() callback for component driver

f6d9670

Topology is not unloaded in the core during unregister_component() anymore. So, add the remove() callback that will unload the topology. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ASoC: SOF: remove topology in component driver remove()

e5b1280

This is no longer done in the core during unregister_component. So, it should be done in the component driver's remove callback(). Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Ranjani Sridharan added 2 commits April 2, 2019 09:46

ASoC: SOF: change the ignore_module_refcount to align with the new name

fe57de4

The "ignore_module_refcount" field in the component driver has been renamed to "module_get_upon_open". So align with the new name. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

ranj063 force-pushed the fix/module_loading branch from 52df475 to 53c42c5 Compare April 2, 2019 16:53

ranj063 closed this Apr 4, 2019

Conversation

ranj063 commented Apr 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

plbossart Apr 1, 2019

Choose a reason for hiding this comment

Uh oh!

ranj063 Apr 1, 2019

Choose a reason for hiding this comment

Uh oh!

ranj063 Apr 1, 2019

Choose a reason for hiding this comment

Uh oh!

ranj063 Apr 1, 2019

Choose a reason for hiding this comment

Uh oh!

ranj063 Apr 1, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghost Apr 1, 2019

Choose a reason for hiding this comment

Uh oh!

ghost Apr 1, 2019

Choose a reason for hiding this comment

Uh oh!

ghost Apr 1, 2019

Choose a reason for hiding this comment

Uh oh!

ghost Apr 1, 2019

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ghost Apr 1, 2019

Choose a reason for hiding this comment

Uh oh!

ghost Apr 1, 2019 • edited by ranj063 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Apr 2, 2019

Choose a reason for hiding this comment

Uh oh!

ghost Apr 2, 2019

Choose a reason for hiding this comment

Uh oh!

ghost Apr 1, 2019

Choose a reason for hiding this comment

Uh oh!

ghost Apr 1, 2019

Choose a reason for hiding this comment

Uh oh!

ghost Apr 2, 2019

Choose a reason for hiding this comment

Uh oh!

ghost Apr 1, 2019

Choose a reason for hiding this comment

Uh oh!

ghost Apr 1, 2019

Choose a reason for hiding this comment

Uh oh!

ghost Apr 1, 2019

Choose a reason for hiding this comment

Uh oh!

ghost Apr 1, 2019

Choose a reason for hiding this comment

Uh oh!

ghost Apr 1, 2019

Choose a reason for hiding this comment

Uh oh!

ghost Apr 1, 2019

Choose a reason for hiding this comment

Uh oh!

ranj063 commented Apr 1, 2019 •

edited

Loading

ghost Apr 1, 2019 •

edited by ranj063

Loading