Message-ID: <9399110b-bbba-f96e-85ef-a317e8f4d518@linux.intel.com>
Date:   Tue, 7 Mar 2023 15:08:01 -0600
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.7.1
Subject: Re: [PATCH V6 8/8] soundwire: amd: add pm_prepare callback and pm ops
 support
Content-Language: en-US
To:     "Mukunda,Vijendar" <vijendar.mukunda@amd.com>, vkoul@kernel.org
Cc:     alsa-devel@alsa-project.org, Basavaraj.Hiregoudar@amd.com,
        Sunil-kumar.Dommati@amd.com, Mario.Limonciello@amd.com,
        amadeuszx.slawinski@linux.intel.com, Mastan.Katragadda@amd.com,
        Arungopal.kondaveeti@amd.com, claudiu.beznea@microchip.com,
        Bard Liao <yung-chuan.liao@linux.intel.com>,
        Sanyog Kale <sanyog.r.kale@intel.com>,
        open list <linux-kernel@vger.kernel.org>
References: <20230307133135.545952-1-Vijendar.Mukunda@amd.com>
 <20230307133135.545952-9-Vijendar.Mukunda@amd.com>
 <4330af6a-ce97-53ed-f675-6d3d0ac8f32f@linux.intel.com>
 <d5a75826-d762-27fc-5820-6826debdecd9@amd.com>
From:   Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
In-Reply-To: <d5a75826-d762-27fc-5820-6826debdecd9@amd.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


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.

> 
> 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;
>>> +}
>