linux-kernel - Re: [PATCH V6 8/8] soundwire: amd: add pm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 8 Mar 2023 08:23:28 -0600
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     "Mukunda,Vijendar" <vijendar.mukunda@....com>, vkoul@...nel.org
Cc:     alsa-devel@...a-project.org, Basavaraj.Hiregoudar@....com,
        Sunil-kumar.Dommati@....com, Mario.Limonciello@....com,
        amadeuszx.slawinski@...ux.intel.com, Mastan.Katragadda@....com,
        Arungopal.kondaveeti@....com, claudiu.beznea@...rochip.com,
        Bard Liao <yung-chuan.liao@...ux.intel.com>,
        Sanyog Kale <sanyog.r.kale@...el.com>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V6 8/8] soundwire: amd: add pm_prepare callback and pm ops
 support


>>> device_for_each_child() will invoke amd_resume_child_device() function callback
>>> for each device which will try to resume the child device in this case.
>>> By definition, device_for_each_child() Iterate over @parent's child devices,
>>> and invokes the callback for each. We check the return of amd_resume_child_device()
>>> each time.
>>> If it returns anything other than 0, we break out and return that value.
>>>
>>> In current scenario, As AMP codec is not in runtime suspend state,
>>> pm_request_resume() will return a value as 1. This will break the
>>> sequence for resuming rest of the child devices(JACK codec in our case).
>> Well, yes, now that makes sense, thanks for the details.
>>
>> I think the reason why we didn't see the problem with the Intel code is
>> that both amplifiers are on the same dailink, so they are by
>> construction either both suspended or both active. We never had
>> different types of devices on the same link.
>>
>> I would however suggest this simpler alternative, where only negative
>> return values are returned:
>>
>> ret = pm_request_resume(dev);
>> if (ret < 0) {
>> 	dev_err(dev, "pm_request_resume failed: %d\n", ret);
>>         return ret;
>> }
>> return 0;
>>
>> this would work just fine, no?
>> No, As explained, pm_request_resume() return value is 1 for active device.
>>> As mentioned in an earlier thread, there are two possible solutions.
>>> 1. check pm runtime suspend state and return 0 if it is not suspended
>>> 2. simply always return 0 for amd_resume_child_device() function callback.
>>>
>>> We opted first one as solution.
>> My suggestion looks like your option 2. It's cleaner IMHO.
> To use option 2, we need to respin the patch series.
> Is it okay if we fix it as supplement patch?

I would vote for re-spinning a new version and ask others to review.