Re: [PATCH v2 16/17] ASoC: Intel: bdw_rt286: Refactor suspend/resume

[Cezary Rojewski <cezary.rojewski@intel.com>]

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