Prevent sof module from being unloaded when a pcm is open by ranj063 · Pull Request #785 · thesofproject/linux

ranj063 · 2019-04-04T20:44:47Z

I said prevent SOF module in the title but it actually applies to the legacy driver as well.

plbossart

Suggested edits for the commit message:

ASoC: core: conditionally increase module refcount on component open

Recently, for Intel platforms the "ignore_module_refcount" field
was introduced for the component driver. In order to avoid a
deadlock preventing the PCI modules from being removed
even when the card was idle, the refcounts were not incremented
for the device driver module during component probe.

However, this change introduced a nasty side effect:
the device driver module can be unloaded while a pcm stream is open.

This patch allows the refcount to be incremented on open and decremented on close,
which enables the modules to be removed when no PCM playback happens but does not allow for removal when then component is actually in use.
The field is renamed as "module_get_upon_open" to reflect the functionality change.

Also, align with the skylake component driver with the new field name.

Fixes: b450b87('ASoC: core: don't increase component module refcount
unconditionally'

Signed-off-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com

ranj063 · 2019-04-05T00:11:07Z

@plbossart commit message fixed as suggested.

plbossart

minor change needed to make the code consistent for upstream

plbossart · 2019-04-05T00:24:28Z

maybe use the same pattern as before with an && instead of an additional if?

@plbossart fixed

Recently, for Intel platforms the "ignore_module_refcount" field was introduced for the component driver. In order to avoid a deadlock preventing the PCI modules from being removed even when the card was idle, the refcounts were not incremented for the device driver module during component probe. However, this change introduced a nasty side effect: the device driver module can be unloaded while a pcm stream is open. This patch proposes to change the field to be renamed as "module_get_upon_open". When this field is set, the module refcount should be incremented on pcm open amd decremented upon pcm close. This will enable modules to be removed when no PCM playback/capture happens and prevent removal when the component is actually in use. 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>

kv2019i

Without knowing the full context, it looks a bit weird why the behaviour is not like this for all devices (why to refcount in probe). But better to have the discussion in upstream, code and description looks clean and this is much better solution than the previous ones that were specific to SOF.

plbossart · 2019-04-05T16:32:57Z

Without knowing the full context, it looks a bit weird why the behaviour is not like this for all devices (why to refcount in probe). But better to have the discussion in upstream, code and description looks clean and this is much better solution than the previous ones that were specific to SOF.

see the ascii art I created at https://mailman.alsa-project.org/pipermail/alsa-devel/2019-February/145048.html, it explains the dependencies between PCI and machine driver.

plbossart

the 3rd patch contains changes that should be in the second. we should't mix core and SOF changes.

Setting the module_get_upon_open field for component driver prevents the module refcount from being incremented during component probe(). 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 during pcm open to prevent module removal when the component is in use. And, the refcount should be decremented upon pcm close. 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>

ranj063 · 2019-04-05T16:49:20Z

the 3rd patch contains changes that should be in the second. we should't mix core and SOF changes.

@plbossart I just realized I posted the wrong patch for soc-pcm.c changes to alsa-devel. I will update it with a V2. Could you please ack it there?

ranj063 · 2019-04-05T16:51:17Z

updated the PR now.

plbossart

thanks @ranj063

I got this crash more times during debugging of PCIe controller and crash happens somehow at the time when PCIe kernel code started link retraining (as part of ASPM code) when at the same time PCIe link went down and ath9k probably executed hw reset procedure. Currently I'm not able to reproduce this issue as it looks like to be some race condition between link training, ASPM, link down and reset path. And as always, race conditions which depends on more input parameters are hard to reproduce as it depends on precise timings. But it is clear that pointers are zero in this case and should be properly filled as same code pattern is used in ath9k_stop() function. Anyway I was able to reproduce this crash by manually triggering ath reset worker prior putting card up. I created simple patch to export reset functionality via debugfs and use it to "simulate" of triggering reset. s proved that NULL-pointer dereference issue is there. Function ath9k_hw_reset() is dereferencing chan structure pointer, so it needs to be non-NULL pointer. Function ath9k_stop() already contains code which sets ah->curchan to valid non-NULL pointer prior calling ath9k_hw_reset() function. Add same code pattern also into ath_reset_internal() function to prevent kernel NULL pointer dereference in ath9k_hw_reset() function. This change fixes kernel NULL pointer dereference in ath9k_hw_reset() which is caused by calling ath9k_hw_reset() from ath_reset_internal() with NULL chan structure. [ 45.334305] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000008 [ 45.344417] Mem abort info: [ 45.347301] ESR = 0x96000005 [ 45.350448] EC = 0x25: DABT (current EL), IL = 32 bits [ 45.356166] SET = 0, FnV = 0 [ 45.359350] EA = 0, S1PTW = 0 [ 45.362596] Data abort info: [ 45.365756] ISV = 0, ISS = 0x00000005 [ 45.369735] CM = 0, WnR = 0 [ 45.372814] user pgtable: 4k pages, 39-bit VAs, pgdp=000000000685d000 [ 45.379663] [0000000000000008] pgd=0000000000000000, p4d=0000000000000000, pud=0000000000000000 [ 45.388856] Internal error: Oops: 96000005 [thesofproject#1] SMP [ 45.393897] Modules linked in: ath9k ath9k_common ath9k_hw [ 45.399574] CPU: 1 PID: 309 Comm: kworker/u4:2 Not tainted 5.12.0-rc2-dirty thesofproject#785 [ 45.414746] Workqueue: phy0 ath_reset_work [ath9k] [ 45.419713] pstate: 40000005 (nZcv daif -PAN -UAO -TCO BTYPE=--) [ 45.425910] pc : ath9k_hw_reset+0xc4/0x1c48 [ath9k_hw] [ 45.431234] lr : ath9k_hw_reset+0xc0/0x1c48 [ath9k_hw] [ 45.436548] sp : ffffffc0118dbca0 [ 45.439961] x29: ffffffc0118dbca0 x28: 0000000000000000 [ 45.445442] x27: ffffff800dee4080 x26: 0000000000000000 [ 45.450923] x25: ffffff800df9b9d8 x24: 0000000000000000 [ 45.456404] x23: ffffffc0115f6000 x22: ffffffc008d0d408 [ 45.461885] x21: ffffff800dee5080 x20: ffffff800df9b9d8 [ 45.467366] x19: 0000000000000000 x18: 0000000000000000 [ 45.472846] x17: 0000000000000000 x16: 0000000000000000 [ 45.478326] x15: 0000000000000010 x14: ffffffffffffffff [ 45.483807] x13: ffffffc0918db94f x12: ffffffc011498720 [ 45.489289] x11: 0000000000000003 x10: ffffffc0114806e0 [ 45.494770] x9 : ffffffc01014b2ec x8 : 0000000000017fe8 [ 45.500251] x7 : c0000000ffffefff x6 : 0000000000000001 [ 45.505733] x5 : 0000000000000000 x4 : 0000000000000000 [ 45.511213] x3 : 0000000000000000 x2 : ffffff801fece870 [ 45.516693] x1 : ffffffc00eded000 x0 : 000000000000003f [ 45.522174] Call trace: [ 45.524695] ath9k_hw_reset+0xc4/0x1c48 [ath9k_hw] [ 45.529653] ath_reset_internal+0x1a8/0x2b8 [ath9k] [ 45.534696] ath_reset_work+0x2c/0x40 [ath9k] [ 45.539198] process_one_work+0x210/0x480 [ 45.543339] worker_thread+0x5c/0x510 [ 45.547115] kthread+0x12c/0x130 [ 45.550445] ret_from_fork+0x10/0x1c [ 45.554138] Code: 910922c2 9117e021 95ff0398 b4000294 (b9400a61) [ 45.560430] ---[ end trace 566410ba90b50e8b ]--- [ 45.565193] Kernel panic - not syncing: Oops: Fatal exception in interrupt [ 45.572282] SMP: stopping secondary CPUs [ 45.576331] Kernel Offset: disabled [ 45.579924] CPU features: 0x00040002,0000200c [ 45.584416] Memory Limit: none [ 45.587564] Rebooting in 3 seconds.. Signed-off-by: Pali Rohár <pali@kernel.org> Cc: stable@vger.kernel.org Signed-off-by: Kalle Valo <kvalo@codeaurora.org> Link: https://lore.kernel.org/r/20210402122653.24014-1-pali@kernel.org

ranj063 requested review from kv2019i, lyakh and plbossart April 4, 2019 20:44

ranj063 force-pushed the module_unload1 branch 3 times, most recently from 0fdb5ec to 21d677b Compare April 4, 2019 20:57

plbossart requested changes Apr 4, 2019

View reviewed changes

ranj063 force-pushed the module_unload1 branch from 21d677b to c873e2f Compare April 5, 2019 00:10

plbossart requested changes Apr 5, 2019

View reviewed changes

ranj063 force-pushed the module_unload1 branch from c873e2f to 3fbb503 Compare April 5, 2019 00:40

ranj063 force-pushed the module_unload1 branch from 3fbb503 to 68e49c0 Compare April 5, 2019 03:34

kv2019i approved these changes Apr 5, 2019

View reviewed changes

plbossart requested changes Apr 5, 2019

View reviewed changes

ranj063 added 2 commits April 5, 2019 09:47

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

91e89c6

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 module_unload1 branch from 68e49c0 to 91e89c6 Compare April 5, 2019 16:51

plbossart approved these changes Apr 5, 2019

View reviewed changes

plbossart merged commit bc524f9 into thesofproject:topic/sof-dev Apr 5, 2019

wenqingfu mentioned this pull request Apr 5, 2019

SMP: power down cores only if all pipelines running on it have been stopped #597

Closed

ranj063 deleted the module_unload1 branch April 5, 2019 22:48

Navigation Menu

Repository navigation

Prevent sof module from being unloaded when a pcm is open - #785

Prevent sof module from being unloaded when a pcm is open#785
plbossart merged 3 commits into
thesofproject:topic/sof-devfrom
ranj063:module_unload1

ranj063 commented Apr 4, 2019 •

edited

Loading

Uh oh!

plbossart left a comment

Uh oh!

ranj063 commented Apr 5, 2019

Uh oh!

plbossart left a comment

Uh oh!

plbossart Apr 5, 2019

Uh oh!

ranj063 Apr 5, 2019

Uh oh!

kv2019i left a comment

Uh oh!

plbossart commented Apr 5, 2019

Uh oh!

plbossart left a comment

Uh oh!

ranj063 commented Apr 5, 2019

Uh oh!

ranj063 commented Apr 5, 2019

Uh oh!

plbossart left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

ranj063 commented Apr 4, 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!

ranj063 commented Apr 5, 2019

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

plbossart Apr 5, 2019

Choose a reason for hiding this comment

Uh oh!

ranj063 Apr 5, 2019

Choose a reason for hiding this comment

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

plbossart commented Apr 5, 2019

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

ranj063 commented Apr 5, 2019

Uh oh!

ranj063 commented Apr 5, 2019

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ranj063 commented Apr 4, 2019 •

edited

Loading