Messages in this thread | | | Date | Wed, 8 Mar 2023 19:49:03 +0530 | Subject | Re: [PATCH V6 8/8] soundwire: amd: add pm_prepare callback and pm ops support | From | "Mukunda,Vijendar" <> |
| |
On 08/03/23 19:28, Pierre-Louis Bossart wrote: > > On 3/7/23 22:32, Mukunda,Vijendar wrote: >> On 08/03/23 02:38, Pierre-Louis Bossart wrote: >>> On 3/7/23 14:25, Mukunda,Vijendar wrote: >>>> On 07/03/23 20:58, Pierre-Louis Bossart wrote: >>>>>> +static int amd_resume_child_device(struct device *dev, void *data) >>>>>> +{ >>>>>> + struct sdw_slave *slave = dev_to_sdw_dev(dev); >>>>>> + int ret; >>>>>> + >>>>>> + if (!slave->probed) { >>>>>> + dev_dbg(dev, "skipping device, no probed driver\n"); >>>>>> + return 0; >>>>>> + } >>>>>> + if (!slave->dev_num_sticky) { >>>>>> + dev_dbg(dev, "skipping device, never detected on bus\n"); >>>>>> + return 0; >>>>>> + } >>>>>> + if (!pm_runtime_suspended(dev)) >>>>>> + return 0; >>>>>> + ret = pm_request_resume(dev); >>>>> I still don't get why the test above was needed. It's racy and brings >>>>> limited benefits. >>>> As explained below thread, >>>> >>>> https://lore.kernel.org/lkml/acd3a560-1218-9f1d-06ec-19e4d3d4e2c9@amd.com >>>> >>>> Our scenario is multiple peripheral devices are connected >>>> over the same link. >>>> >>>> In our implementation, device_for_each_child() function invokes >>>> amd_resume_child_device callback for each child. >>>> When any one of the child device is active, It will break the >>>> iteration, which results in failure resuming all child devices. >>> Can you clarify the 'it will break the iteration' statement? >>> >>> Are you saying pm_request_resume() will return a negative error code if >>> the device is already active? >>> >>> We've used an unconditional pm_request_resume() in the Intel code for >>> quite some time, including with multiple amplifiers per link, and have >>> never observed the issue you report, so I'd like to get to the root >>> cause pretty please. You took the Intel code and added a test for AMD >>> platforms, and I'd really like to understand if the Intel code was wrong >>> in the first place, or if the test is not needed. Something does not add >>> up here. >> AMP Codec (In aggregate mode) + Jack Codec connected over the same >> link on our platform. >> Consider below, scenario. >> Active stream is running on AMP codec and Jack codec is already in runtime >> suspend state. >> If system level suspend is invoked, in prepare callback, we need to resume >> both the codec devices. >> >> 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?
| |