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

[Mukunda,Vijendar]

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).

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.
>
>> If we skip , pm_suspended check , it will not resume all
>> peripheral devices when any one of the peripheral device is active.
>>>> +	if (ret < 0)
>>>> +		dev_err(dev, "pm_request_resume failed: %d\n", ret);
>>>> +
>>>> +	return ret;
>>>> +}