Re: [PATCH V6 8/8] soundwire: amd: add pm_prepare callback and pm ops support

[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@xxxxxxx
>>>>
>>>> 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?