Re: [PATCH V3 10/10] soundwire: amd: fix IRQ thread dropping STATUS_0TO7 on simultaneous wake

[Mukunda,Vijendar]

On 18/10/26 22:19, Pierre-Louis Bossart wrote:
> On 10/13/26 10:49, Vijendar Mukunda wrote:
>> When the wake bit and a device-state change arrive at the same time,
>> the IRQ thread returned early after handling wake, silently discarding
>> STATUS_0TO7. If the manager is already at D0, pm_request_resume() is
>> a no-op so the dropped ATTACHED event is never recovered and jack
>> detection stops working.
> this wording is very very hard to follow. Please consider rewording to
> describe the race condition in better ways...
I will reword it in the next version to clearly separate the dropped STATUS_0TO7
event
from the STATUS_8TO11 double-clear race.
>
>> Remove the early return on wake: call amd_sdw_process_wake_event(),
>> strip the wake bit from the local STATUS_8TO11 copy, and continue to
>> the normal slave-status processing path. Remove the STATUS_8TO11 clear
>> from amd_sdw_process_wake_event() so the single tail clear in
>> amd_sdw_irq_thread() is the only write to the register per invocation,
>> eliminating the double-clear race where a new event arriving between
>> two successive clears would be wiped before being read.
>>
>> Fixes: 66c878838f37 ("soundwire: amd: handle SoundWire wake enable interrupt")
>> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@xxxxxxx>
>> ---
>>  drivers/soundwire/amd_manager.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
>> index 1a252a81269a..beb982b17fa9 100644
>> --- a/drivers/soundwire/amd_manager.c
>> +++ b/drivers/soundwire/amd_manager.c
>> @@ -1006,7 +1006,6 @@ static void amd_sdw_process_wake_event(struct amd_sdw_manager *amd_manager)
>>  	dev_dbg(amd_manager->dev, "SoundWire Wake event reported\n");
>>  	pm_request_resume(amd_manager->dev);
> ... specifically why this would be a no-op and ...
When SoundWire manager is already in D0 state and if Wake STS interrupt 
is asserted in this case, it would be no-op. In this case the dropped STATUS_0TO7
event cannot be recovered through amd_resume_runtime().
Will clarify this in commit message.
>
>>  	writel(0x00, amd_manager->acp_mmio + ACP_SW_WAKE_EN(amd_manager->instance));
>> -	writel(0x00, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_8TO11);
>>  }
>>  
>>  static void amd_sdw_irq_thread(struct work_struct *work)
>> @@ -1023,8 +1022,11 @@ static void amd_sdw_irq_thread(struct work_struct *work)
>>  
>>  	dev_dbg(amd_manager->dev, "[SDW%d] SDW INT: 0to7=0x%x, 8to11=0x%x\n",
>>  		amd_manager->instance, status_change_0to7, status_change_8to11);
>> -	if (status_change_8to11 & AMD_SDW_WAKE_STAT_MASK)
>> -		return amd_sdw_process_wake_event(amd_manager);
>> +
>> +	if (status_change_8to11 & AMD_SDW_WAKE_STAT_MASK) {
>> +		amd_sdw_process_wake_event(amd_manager);
>> +		status_change_8to11 &= ~AMD_SDW_WAKE_STAT_MASK;
> ... why adding this mask solves the race
The AMD_SDW_WAKE_STAT_MASK masking does not fix the race itself. 
The race is addressed by removing the STATUS_8TO11 clear from
amd_sdw_process_wake_event(), leaving a single clear in
amd_sdw_irq_thread(). The mask only removes the already-handled wake
indication from the local status value before processing the remaining
interrupt status.
>> +	}
>>  
>>  	if (status_change_8to11 & AMD_SDW_PREQ_INTR_STAT) {
>>  		amd_sdw_read_and_process_ping_status(amd_manager);