On 2022-06-15 3:27 AM, Pierre-Louis Bossart wrote:
On 6/13/22 04:15, Cezary Rojewski wrote:
Make use of card->remove() rather than pdev->remove() to unassign jack during card unbind procedure.
To reduce code size, define unified jack setter in form of bdw_rt286_set_jack() and invoke it during remove(), suspend_pre() and resume_port().
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com Reviewed-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com
This seems to have rather negative side effects in our modprobe/modprobe -r tests?
The pattern of disabling the jack in the platform device .remove is fairly common, I don't recall having seen a machine driver doing this in the card .remove step. Are you sure this is equivalent?
Reverting this patch removes the kernel oops.
I don't have time to debug further - but this adds to my point of minimizing risk on legacy code, doesn't it? suspend-resume is difficult to get right, and easy to break. I have done the latter more often that the former.
if you want to reproduce the issue, see https://github.com/thesofproject/linux/pull/3696
and use sof-test: /root/sof-test/test-case/check-kmod-load-unload.sh -l 1
Thanks for the report. Indeed, the latest "optimization" broke the card->remove() path.
Jacks are often initialized during dai_link initialization which is completely out of platform_device area. This report made me think further - if we assign jack in dai_link->init(), we should be able to drop it in dai_link->exit().
Not exactly! ->init() is done once card components are already accounted for (available for use) but snd_soc_link_exit() is called during snd_soc_remove_pcm_runtime() when card components are available no longer - soc_remove_link_components().
TLDR: teardown path is not symmetric with its counterpart, perhaps a problem yet to be addressed. I'll see if moving the jack-NULLing to codec's DAI ->remove() won't be a better temporary (?) solution than reverting to platform_device->remove() usage.
Mark,
Is it fine to leave v2 series as is, just ignoring this single 16/17 patch? Or should I resend entire series as v3 without this very patch? I'd like to address the problem via a separate change.
Regards, Czarek