On 08/03/23 19:53, Pierre-Louis Bossart wrote:
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?
Sorry its my bad. This would work fine. We will fix it and respin the patch series.
No, As explained, pm_request_resume() return value is 1 for active device.
As mentioned in an earlier thread, there are two possible solutions.
- check pm runtime suspend state and return 0 if it is not suspended
- 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.