Re: [PATCH 21/21] soundwire: amd: Disable interrupts if enabling manager fails

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

On 10/30/26 18:28, Mukunda,Vijendar wrote:
>
>
> On 10/30/26 16:17, Lee Jones wrote:
>> In amd_sdw_manager_start() and amd_resume_runtime(),
>> amd_enable_sdw_interrupts() is called before amd_enable_sdw_manager().
>> If amd_enable_sdw_manager() fails, both functions return immediately
>> with SoundWire interrupts left unmasked.  In addition,
>> amd_resume_runtime() ignores the return value of
>> amd_init_sdw_manager().
>>
>> Check the return value of amd_init_sdw_manager() in
>> amd_resume_runtime(), and disable interrupts if amd_enable_sdw_manager()
>> fails in amd_sdw_manager_start() and amd_resume_runtime().
> ++
> The unchecked return value of amd_init_sdw_manager() in
> amd_resume_runtime() is already addressed in our v3 patch series:
>
> https://lore.kernel.org/alsa-devel/20261013085111.2009061-6-Vijendar.Mukunda@amd.com/T/#m7741edcedb1264764edc7f8559e39af8b23433fb 
>
>
> The missing amd_disable_sdw_interrupts() on amd_enable_sdw_manager()
> failure is a valid observation and will be addressed in our upcoming
> v4 patch series. Please hold this patch until v4 is on-list to
> avoid conflicts.
Correction to the above. After a more detailed analysis of the ACP
hardware interrupt routing, the fix is not required. Please drop
this change and disregard our earlier statement.

The AMD ACP IP is the parent device that dispatches SoundWire
interrupts per manager instance. It only forwards an SDW interrupt
to the CPU when the corresponding SoundWire manager is active.
If acp_enable_sdw_manager() times out the ACP treats that manager
instance as inactive and will not assert any SDW interrupt on its
behalf, regardless of the mask register state.

Two independent hardware guarantees prevent any interrupt from
firing after a manager-enable failure:

1. The masks written by acp_enable_sdw_interrupts() cover
    packet-based SoundWire status events. These require the
    SoundWire clock to be running, which in turn requires the
    manager to be enabled. Without an active manager there is
    no clock, no SoundWire packet can be received, and no
    interrupt is generated.

2. Wake interrupt masks are only functional once the SoundWire
    manager is enabled in the D0 sequence. Additionally, wake
    is explicitly disabled at the very start of the POWER_OFF_MODE
    resume path, well before acp_enable_sdw_manager() is reached:

      writel(0x00, acp_mmio + ACP_SW_WAKE_EN(instance));
      amd_sdw_host_wake_enable_by_rev(amd_manager, false);
      ...
      acp_enable_sdw_interrupts(amd_manager);
      ret = acp_enable_sdw_manager(amd_manager);  /* may fail here */

v5 will not include a fix for this case.
>>
>> Fixes: 81ff58ff71ad ("soundwire: amd: add runtime pm ops for AMD 
>> SoundWire manager driver")
>> Closes: 
>> https://sashiko.dev/#/bug/linux-309820e5-f754-4d3d-80aa-7f70d59f3e8e
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Lee Jones <lee@kernel.org>
>> ---
>>   drivers/soundwire/amd_manager.c | 12 +++++++++---
>>   1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/soundwire/amd_manager.c 
>> b/drivers/soundwire/amd_manager.c
>> index faff3e9eefad..f64e8fea7e16 100644
>> --- a/drivers/soundwire/amd_manager.c
>> +++ b/drivers/soundwire/amd_manager.c
>> @@ -1109,8 +1109,10 @@ int amd_sdw_manager_start(struct 
>> amd_sdw_manager *amd_manager)
>>               return ret;
>>           amd_enable_sdw_interrupts(amd_manager);
>>           ret = amd_enable_sdw_manager(amd_manager);
>> -        if (ret)
>> +        if (ret) {
>> +            amd_disable_sdw_interrupts(amd_manager);
>>               return ret;
>> +        }
>>           amd_sdw_set_frameshape(amd_manager);
>>       }
>>       /* Enable runtime PM */
>> @@ -1503,11 +1505,15 @@ static int __maybe_unused 
>> amd_resume_runtime(struct device *dev)
>>           ret = amd_sdw_clk_init_ctrl(amd_manager);
>>           if (ret)
>>               return ret;
>> -        amd_init_sdw_manager(amd_manager);
>> +        ret = amd_init_sdw_manager(amd_manager);
>> +        if (ret)
>> +            return ret;
>>           amd_enable_sdw_interrupts(amd_manager);
>>           ret = amd_enable_sdw_manager(amd_manager);
>> -        if (ret)
>> +        if (ret) {
>> +            amd_disable_sdw_interrupts(amd_manager);
>>               return ret;
>> +        }
>>           amd_sdw_set_frameshape(amd_manager);
>>       }
>>       if (amd_manager->acp_rev >= ACP70_PCI_REV_ID) {
>