Re: [PATCH v4 02/13] soundwire: amd: return response on IMM status cleanup timeout

["Mukunda,Vijendar" <vijendar.mukunda@amd.com>]

On 10/26/26 17:43, Mukunda,Vijendar wrote:
>
>
> On 10/26/26 14:38, sashiko-bot@kernel.org wrote:
>> [You don't often get email from sashiko-bot@kernel.org. Learn why 
>> this is important at https://aka.ms/LearnAboutSenderIdentification ]
>>
>> Thank you for your contribution! Sashiko AI review found 1 potential 
>> issue(s) to consider:
>> - [High] soundwire: amd: unpolled IMM_RES_VALID clear in 
>> amd_sdw_send_cmd_get_resp()
>> -- 
>>
>> commit 3a55a761f23cc08cf1a64f142c04e1aa78cc244d
>> Author: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
>>
>> soundwire: amd: return response on IMM status cleanup timeout
>>
>> This commit changes amd_sdw_send_cmd_get_resp() to log an error, 
>> store the
>> read response, and return 0 when polling for AMD_SDW_IMM_RES_VALID to 
>> clear
>> times out. It relies on the bad-state check at the start of the next
>> invocation to clear the remaining AMD_SDW_IMM_RES_VALID bit.
>>
>>> diff --git a/drivers/soundwire/amd_manager.c 
>>> b/drivers/soundwire/amd_manager.c
>>> index 307cbfdb94422..0b83126b5ce6c 100644
>>> --- a/drivers/soundwire/amd_manager.c
>>> +++ b/drivers/soundwire/amd_manager.c
>>> @@ -285,11 +285,9 @@ static int amd_sdw_send_cmd_get_resp(struct 
>>> amd_sdw_manager *amd_manager, u32 lo
>>>        writel(AMD_SDW_IMM_RES_VALID, amd_manager->mmio + 
>>> ACP_SW_IMM_CMD_STS);
>>>        ret = readl_poll_timeout(amd_manager->mmio + 
>>> ACP_SW_IMM_CMD_STS, sts,
>>>                                 !(sts & AMD_SDW_IMM_RES_VALID), 
>>> ACP_DELAY_US, AMD_SDW_TIMEOUT);
>>> -     if (ret) {
>>> +     if (ret)
>>>                dev_err(amd_manager->dev, "SDW%x cmd status retry 
>>> failed\n",
>>>                        amd_manager->instance);
>>> -             return ret;
>>> -     }
>>>        *response = ((u64)upper_resp << 32) | lower_resp;
>>>        return 0;
>>>   }
>> [Severity: High]
>> When this cleanup poll for !(sts & AMD_SDW_IMM_RES_VALID) times out and
>> amd_sdw_send_cmd_get_resp() in drivers/soundwire/amd_manager.c 
>> returns 0,
>> can back-to-back commands read stale response data from
>> ACP_SW_IMM_RESP_UPPER_WORD and ACP_SW_IMM_RESP_LOWER_QWORD?
++
> No. The response for the current command has already been latched and 
> read
> before the cleanup poll is performed.
>
> The first poll waits for AMD_SDW_IMM_RES_VALID to be set, indicating that
> the response is available. A timeout there is still returned as an error
> and this behavior is unchanged.
>
> The trailing poll only waits for AMD_SDW_IMM_RES_VALID to be cleared 
> after
> the response has been read. Therefore, a timeout during this cleanup does
> not invalidate the response that was already obtained.
>>
>> Callers such as amd_program_scp_addr() and amd_sdw_xfer_msg() issue 
>> multiple
>> commands in sequence:
>>
>> sdw_transfer()
>>    do_transfer()
>>      amd_sdw_xfer_msg()
>>        amd_prep_msg()
>>          amd_program_scp_addr()
>>            amd_sdw_send_cmd_get_resp()
>>        _amd_sdw_xfer_msg()
>>          amd_sdw_send_cmd_get_resp()
>>
>> On the immediate next call to amd_sdw_send_cmd_get_resp(), the bad-state
>> guard writes AMD_SDW_IMM_RES_VALID to ACP_SW_IMM_CMD_STS without 
>> polling for
>> !(sts & AMD_SDW_IMM_RES_VALID) before issuing the next command and 
>> polling
>> for (sts & AMD_SDW_IMM_RES_VALID):
>>
>>          if (sts & AMD_SDW_IMM_RES_VALID) {
>>                  dev_err(amd_manager->dev, "SDW%x manager is in bad 
>> state\n", amd_manager->instance);
>>                  writel(AMD_SDW_IMM_RES_VALID, amd_manager->mmio + 
>> ACP_SW_IMM_CMD_STS);
>>          }
>>          writel(upper_data, amd_manager->mmio + 
>> ACP_SW_IMM_CMD_UPPER_WORD);
>>          writel(lower_data, amd_manager->mmio + 
>> ACP_SW_IMM_CMD_LOWER_QWORD);
>>
>>          ret = readl_poll_timeout(amd_manager->mmio + 
>> ACP_SW_IMM_CMD_STS, sts,
>>                                   (sts & AMD_SDW_IMM_RES_VALID), 
>> ACP_DELAY_US, AMD_SDW_TIMEOUT);
>>
>> Because clearing AMD_SDW_IMM_RES_VALID requires an asynchronous hardware
>> handshake that already failed to complete within AMD_SDW_TIMEOUT on the
>> previous command, won't AMD_SDW_IMM_RES_VALID still be set when
>> readl_poll_timeout() immediately checks (sts & AMD_SDW_IMM_RES_VALID),
>> causing it to succeed on the stale bit and read the previous command's
>> response registers? Should the bad-state guard also poll until
>> !(sts & AMD_SDW_IMM_RES_VALID) before writing the new command words?
> No. If AMD_SDW_IMM_RES_VALID is still set on the next invocation, the
> bad-state path clears it by writing AMD_SDW_IMM_RES_VALID to
> ACP_SW_IMM_CMD_STS before programming the next command.
>
> This behavior is also consistent with the hardware reference
> implementation, where the bad-state path uses the same write-1-to-clear
> sequence without polling again for the status bit to clear.
> Therefore, no additional change is required for this scenario.
>
>>
>> -- 
>> Sashiko AI review · 
>> https://sashiko.dev/#/patchset/20261026044507.2310694-1-Vijendar.Mukunda@amd.com?part=2
>