Re: [PATCH V3 02/10] soundwire: amd: cache ping slave status to avoid spurious disconnect on timeout

[Mukunda,Vijendar]

On 18/10/26 22:11, Pierre-Louis Bossart wrote:
> On 10/13/26 10:49, Vijendar Mukunda wrote:
>> Add ping_slave_stat to struct amd_sdw_manager to cache the slave_stat
>> from the last successful ping response. On timeout,
>> amd_sdw_read_ping_status() returns the cached value (or 0 if no
>> successful response yet) to avoid the spurious "no peripherals
>> attached" warning from sdw_show_ping_status().
> I read this description multiple times and I don't get
>
> a) in what cases you'd get a timeout on a PING command?
The AMD SoundWire manager uses the Immediate Command mechanism to issue a
PING command. A timeout occurs when the AMD_SDW_IMM_CMD_BUSY bit in
ACP_SW_IMM_CMD_STS does not clear within AMD_SDW_TIMEOUT.
>
> and
>
> b) why the cached value of the last ping is a relevant concealment?
> You'd still get an error message on a timeout...Maybe this makes the log
> "cleaner" but still there was an error in the first place.
You're right. The cache does not suppress the timeout error, and that is
not its purpose.

The cache prevents a PING timeout from being interpreted as a valid zero
status, which can result in sdw_show_ping_status() reporting that no
peripherals are attached. Instead, the last known valid status is
returned while dev_err_ratelimited() still reports the actual timeout.

I will make this distinction clear in the next version.
>
>> ping_slave_stat is initialised to U32_MAX in probe and reset to
>> U32_MAX under msg_lock on entry to the POWER_OFF_MODE resume path,
>> before any hardware step that can fail and return early, so that a
>> partially-failed resume cannot leave a stale pre-suspend value behind
>> for a subsequent ping timeout to report.
>>
>> Add lockdep_assert_held() in amd_sdw_read_ping_status() to document
>> that the caller must hold msg_lock.
> that lockdep_ add is a different improvement that should go in a
> separate patch - it's not related to the timeout management and the
> suggested use of a cached value.
Agreed. The lockdep_assert_held() change is independent of the PING
status caching and will be split into a separate patch in the next
version.
>
>> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@xxxxxxx>
>> ---
>>  drivers/soundwire/amd_manager.c   | 21 ++++++++++++++++++++-
>>  include/linux/soundwire/sdw_amd.h |  4 ++++
>>  2 files changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
>> index fa3f4e797edd..a57b59609bfe 100644
>> --- a/drivers/soundwire/amd_manager.c
>> +++ b/drivers/soundwire/amd_manager.c
>> @@ -453,6 +453,9 @@ static void amd_sdw_read_and_process_ping_status(struct amd_sdw_manager *amd_man
>>  				    amd_manager->instance);
>>  		return;
>>  	}
>> +	amd_manager->ping_slave_stat =
>> +		FIELD_GET(AMD_SDW_MCP_SLAVE_STAT_0_3, response) |
>> +		(FIELD_GET(AMD_SDW_MCP_SLAVE_STAT_4_11, response) << 8);
>>  	amd_sdw_process_ping_status(response, amd_manager);
>>  	mutex_unlock(&amd_manager->bus.msg_lock);
>>  }
>> @@ -463,15 +466,19 @@ static u32 amd_sdw_read_ping_status(struct sdw_bus *bus)
>>  	u64 response;
>>  	u32 slave_stat;
>>  
>> +	/* Called by sdw_show_ping_status() which holds msg_lock. */
>> +	lockdep_assert_held(&bus->msg_lock);
>> +
>>  	if (amd_sdw_send_cmd_get_resp(amd_manager, 0, 0, &response)) {
>>  		dev_err_ratelimited(amd_manager->dev, "SDW%x ping status timeout\n",
>>  				    amd_manager->instance);
>> -		return 0;
>> +		return amd_manager->ping_slave_stat == U32_MAX ? 0 : amd_manager->ping_slave_stat;
>>  	}
>>  	/* slave status from ping response */
>>  	slave_stat = FIELD_GET(AMD_SDW_MCP_SLAVE_STAT_0_3, response);
>>  	slave_stat |= FIELD_GET(AMD_SDW_MCP_SLAVE_STAT_4_11, response) << 8;
>>  	dev_dbg(amd_manager->dev, "slave_stat:0x%x\n", slave_stat);
>> +	amd_manager->ping_slave_stat = slave_stat;
>>  	return slave_stat;
>>  }
>>  
>> @@ -1087,6 +1094,7 @@ static int amd_sdw_manager_probe(struct platform_device *pdev)
>>  		dev_err(dev, "mmio not found\n");
>>  		return -ENOMEM;
>>  	}
>> +	amd_manager->ping_slave_stat = U32_MAX;
>>  	amd_manager->instance = pdata->instance;
>>  	amd_manager->mmio = amd_manager->acp_mmio +
>>  			    (amd_manager->instance * SDW_MANAGER_REG_OFFSET);
>> @@ -1414,6 +1422,17 @@ static int __maybe_unused amd_resume_runtime(struct device *dev)
>>  		}
>>  	} else if (amd_manager->power_mode_mask & AMD_SDW_POWER_OFF_MODE) {
>>  		writel(0x00, amd_manager->acp_mmio + ACP_SW_WAKE_EN(amd_manager->instance));
>> +		/*
>> +		 * POWER_OFF_MODE tears the bus down completely; invalidate the
>> +		 * cached ping status on entry, before any step that can fail and
>> +		 * return early, so that a partially-failed resume leaves the first
>> +		 * post-resume ping timeout returning 0 rather than stale
>> +		 * pre-suspend data. CLK_STOP_MODE leaves peripherals attached, so
>> +		 * the cache remains valid there.
>> +		 */
>> +		mutex_lock(&amd_manager->bus.msg_lock);
>> +		amd_manager->ping_slave_stat = U32_MAX;
>> +		mutex_unlock(&amd_manager->bus.msg_lock);
>>  		if (amd_manager->acp_rev >= ACP70_PCI_REV_ID) {
>>  			ret = amd_sdw_host_wake_enable(amd_manager, false);
>>  			if (ret)
>> diff --git a/include/linux/soundwire/sdw_amd.h b/include/linux/soundwire/sdw_amd.h
>> index 470360a2723c..d0e97007e3ff 100644
>> --- a/include/linux/soundwire/sdw_amd.h
>> +++ b/include/linux/soundwire/sdw_amd.h
>> @@ -76,6 +76,9 @@ struct sdw_amd_dai_runtime {
>>   * @acp_rev: acp pci device revision id
>>   * @clk_stopped: flag set to true when clock is stopped
>>   * @power_mode_mask: flag interprets amd SoundWire manager power mode
>> + * @ping_slave_stat: cached slave status from the last ping
>> + *		     transaction that returned a response;
>> + *                   initialised to U32_MAX ("no valid reading yet")
>>   * @dai_runtime_array: dai runtime array
>>   */
>>  struct amd_sdw_manager {
>> @@ -105,6 +108,7 @@ struct amd_sdw_manager {
>>  	u32 wake_en_mask;
>>  	u32 power_mode_mask;
>>  	u32 acp_rev;
>> +	u32 ping_slave_stat;
>>  	bool clk_stopped;
>>  
>>  	struct sdw_amd_dai_runtime **dai_runtime_array;