ASoC:SOF:update refcount of sof_pci_dev during playing

[zhuyingjiang]

when a PCM is playing, increase the refcount of the
sof_pci_dev,and decrease the refcount when the PCM
playing is stopped. This will give an error message
when remove the module during PCM playing.

Signed-off-by: Zhu Yingjiang yingjiang.zhu@linux.intel.com

[zhuyingjiang]

lsmod_no_play.txt
lsmod_playing.txt

tested on Up2, the diff of lsmod result between no_play/playing is:
lsmod.diff.txt

[plbossart]

Good idea, but the implementation needs to be redone to take into account errors.

[plbossart]

so right there the code is wrong. If you return any error during the open, the close will not be called and you will not release the module.
You need to fix the error flow.

[lyakh]

I might be wrong, but wouldn't it be better to use pci_dev_get() / get_device()? In fact such explicit refcount increments are rarely needed, I think, usually with device hierarchies?

[plbossart]

I must admit I don't fully understand the links between module refcount and device refcount. @ranj063 made a mention that the two are related but I don't have more information or knowledge on this.
The main issue is that we don't have a real hierarchy here. The PCM open happens at the card level, which is a platform device created by the PCI driver. It's a bit ugly.

[ranj063]

@plbossart @zhuyingjiang I agree with @lyakh I think it is generally a bad idea to increment the refcount for the pci device from itself. The reason we have this problem is because the pcm component driver's owner module is the machine driver and only its refcount is increased during pcm open.

I think a better way to tackle this is to adjust our sequence in snd_sof_device_remove(). Maybe we should not unregister the component if the pcm is open? Just thinking aloud

[zhuyingjiang]

so right there the code is wrong. If you return any error during the open, the close will not be called and you will not release the module.
You need to fix the error flow.

@plbossart Yes, thanks. Will update.

[ranj063]

@zhuyingjiang what you are referring to is the core incrementing the card module's ref count.
But what we're trying to do here is incrementing the sof_pci_dev's refcount within the SOF driver.
I'm not sure about how this will be received upstream.

[zhuyingjiang]

@ranj063 the module refcount and device refcount seems are not the same:
dev->kobj->kref->refcount
device_driver->module->refcnt
@lgirdwood how will this case handle in ASoC drivers, any suggestions with this?

[kv2019i]

Interesting case, I agree with @zhuyingjiang that this needs to be handled like in OSS in a systematic way. We are tracking this dependency partly via runtime-pm now, and I was wondering could we utilize runtime-pm to do this more cleanly (as sof-pci-dev owns the driver and initializes runtime pm and we have the appropriate runtime-pm calls in sof pcm.c already), but couldn't figure out a nice way.
So indeed, maybe @lgirdwood you have a recommendation? Otherwise I'd clean up the patch (clean the error handling) and send as RFC patch to alsa-devel to get comments.

[zhuyingjiang]

@kv2019i As SOF is still not upstreamed, maybe it is not proper to format a patch to discuss in ALSA mail list.
I will make a description about this and collect some suggestions in ALSA mail list. Could you give some advice there?

[lgirdwood]

@zhuyingjiang module refcount is the number of other modules that have a symbol dependency with this module. driver ref_count is the number of devices bound to this driver.
ASoC modules should not be removable after machine driver binding unless the machine driver is removed first. So I guess this is about stopping removal of the machine driver whilst and PCM or kcontrol IO is active ? Can you confirm this is the bug you are fixing. If so this needs fix at ASoC core level and not in SOF.

[zhuyingjiang]

it seems the files can't be attached to the reply window, so attach here.
HDA lsmod difference:
lsmod_hda_no_play.txt
lsmod_hda_playing.txt
and the diff of HDA lsmod playing and no_play
hda.lsmod.diff.txt

[zhuyingjiang]

@plbossart Updated.

[lgirdwood]

@zhuyingjiang module refcount is the number of other modules that have a symbol dependency with this module. driver ref_count is the number of devices bound to this driver.
ASoC modules should not be removable after machine driver binding unless the machine driver is removed first. So I guess this is about stopping removal of the machine driver whilst and PCM or kcontrol IO is active ? Can you confirm this is the bug you are fixing. If so this needs fix at ASoC core level and not in SOF.

[plbossart]

@lgirdwood we recently changed the ASoC core to allow the sof-pci-dev/acpi-dev to be removed, but that created the nasty side effect that you can now remove those devices while playing. It's really the whole model of having the machine driver created as a child of the pci/acpi driver that wrecks everything.

[lgirdwood]

@lgirdwood we recently changed the ASoC core to allow the sof-pci-dev/acpi-dev to be removed, but that created the nasty side effect that you can now remove those devices while playing. It's really the whole model of having the machine driver created as a child of the pci/acpi driver that wrecks everything.

Ok, so I'm guessing here we need the machine driver to be the PCI/ACPI device directly ? IIRC, the DAI link binding increased the module reference count in the past, I think we need a solution that also needs to be able to deal with kcontrol IO too. It seems we are already reference counting for runtime PM today so maybe hooking this into the machine driver remove() flow with a PM state check may be helpful. Any input from Tiwai or Mark ?

[ranj063]

@lgirdwood we recently changed the ASoC core to allow the sof-pci-dev/acpi-dev to be removed, but that created the nasty side effect that you can now remove those devices while playing. It's really the whole model of having the machine driver created as a child of the pci/acpi driver that wrecks everything.

Ok, so I'm guessing here we need the machine driver to be the PCI/ACPI device directly ? IIRC, the DAI link binding increased the module reference count in the past, I think we need a solution that also needs to be able to deal with kcontrol IO too. It seems we are already reference counting for runtime PM today so maybe hooking this into the machine driver remove() flow with a PM state check may be helpful. Any input from Tiwai or Mark ?

@lgirdwood the problem is that when you do an rmmod , it checks for the module refcnt before proceeding with the remove.
In our case, when a pcm is open, the module refcnt for the machine driver is > 0. So rmmod will return an error saying the module is in use. But rmmod sof_pci_dev will not because its refcnt is 0.

We could check the machinr drv ref cnt in the remove() method and return an error but it turns out to be messy and it not as graceful as rmmod checking for the refcnt itself.

[plbossart]

@lgirdwood 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. The problem is that when we play the PCM is owned by the card device, not the PCI one, so it makes sense to increase the parent module count on open/close. What I am not clear on is the relationship, if any, between module count and pm.use_count.

[lgirdwood]

@plbossart nice art !, make sense . My suggestion for PM was there would always be alignment with the card/dev PM state and whether the module could be safely removed.

[ranj063]

@plbossart I had an idea to solve this problem. How about we use the ignore_module_refcount field you added to the component driver to increment and decrement the refcount in soc_pcm_open and soc_pcm_close instead?
What this would mean is that we dont increment/decrement during probe and remove which would hold the until the component is removed. Instead, it would increment and decrement when the pcm opn/close is called for the component driver.

[plbossart]

@ranj063 so this would work for the cAVS/SST driver as well then? if yes it'd be a good fix.

[ranj063]

@ranj063 so this would work for the cAVS/SST driver as well then? if yes it'd be a good fix.

Yes, I think so. The cavs driver needs a bit of TLC. I looked into fixing the SKL driver and it has a this skl clk driver module which seems to register a couple of component drivers and it holds the refcount for the snd_soc_skl device preventing it from being removed. I am working with Sathya to test the cavs driver. I'll update you on Monday if I am able to succeed.

[ranj063]

@zhuyingjiang @plbossart #765 proposes a more comprehensive fix for module loading/unloading that covers the issue handles too.

[plbossart]

replaced by Ranjani's solution