[PATCH v5 00/25] soundwire: amd: SoundWire manager driver bug fixes

[PATCH v5 00/25] soundwire: amd: SoundWire manager driver bug fixes

[Vijendar Mukunda]

This series collects bug fixes for the AMD SoundWire manager driver
(ACP6.3/7.0/7.1/7.2). The patches address correctness issues across
five areas. Patches 9, 14, 15, 17, 22, and 23 fix issues reported by
Lee Jones <lee@kernel.org>.

Wake interrupt handling (patches 1-8): fixes reported on OEM platforms
running ACP6.3/7.0/7.1/7.2. Consolidate pad register macros into
amd_init.h, add correct SCHMEN/OE programming for ACP 7.0/7.1/7.2,
introduce D0/D3 PM pad helpers, track full pad teardown across
suspend/resume, fix missing wake interrupt and group wake-arming calls
in runtime suspend, fix the IRQ thread dropping STATUS_0TO7 on
simultaneous wake and state-change events, remove the spurious
STATUS_8TO11 clear in amd_sdw_process_wake_event(), and fix a missing
wake interrupt for ACP6.3 in POWER_OFF suspend.

Command response handling (patches 9-13): fix silent widening of
-ETIMEDOUT to u64 (reported by Lee Jones), return valid hardware
response when only the IMM status cleanup poll times out, clarify the
bad-state guard poll omission in the IMM_RES_VALID path, cache the
last successful PING result to avoid spurious disconnect warnings on
transient timeouts, and add lockdep_assert_held() to enforce the
msg_lock contract.

Work-queue and driver lifecycle (patches 14-18): fix drain ordering in
the remove path and defer dev_set_drvdata() until after INIT_WORK() to
close a concurrent-remove window (both reported by Lee Jones), guard
pm_runtime_disable() against unpaired calls, guard IRQ work scheduling
with the interrupt_enabled flag (reported by Lee Jones), and delay
*sdw_ctx publication until after sdw_amd_startup() succeeds.

Power management (patches 19-22): propagate amd_init_sdw_manager()
errors on POWER_OFF resume, drop amd_deinit_sdw_manager() in POWER_OFF
suspend, replace the open-coded >= ACP70 comparison with an explicit
switch/case, and fix interrupt-gate and work-drain ordering in PM ops
(reported by Lee Jones).

Drvdata and register safety (patches 23-25): add a NULL drvdata guard
in sdw_amd_startup() (reported by Lee Jones), and fix shared-register
read-modify-write races in amd_sdw_set_device_state() and
amd_sdw_host_wake_enable() by holding acp_sdw_lock.

Changes since v4:
- Add patches 1-8 (OEM-reported wake interrupt fixes for
  ACP6.3/7.0/7.1/7.2): pad register macro consolidation, SCHMEN/OE
  programming for ACP 7.0/7.1/7.2, D0/D3 PM helpers, pad teardown
  tracking, missing wake interrupt in runtime suspend, STATUS_0TO7
  race fix (promoted from patch 12/13 in v4), STATUS_8TO11 fix
  reworked to a targeted removal of the clear in
  amd_sdw_process_wake_event(), and ACP6.3 POWER_OFF wake fix.
- Add patch 11: clarify bad-state guard poll omission in
  IMM_RES_VALID path.
- Add patch 15: defer dev_set_drvdata() until after INIT_WORK().
- Add patches 17-18: guard IRQ work with interrupt_enabled flag;
  delay *sdw_ctx publication until after startup succeeds
  (supersedes v4 patch 7 ctx-leak fix with a more robust approach).
- Add patches 23-25: NULL drvdata guard in sdw_amd_startup(), and
  shared-register RMW races in amd_sdw_set_device_state() and
  amd_sdw_host_wake_enable().
- Drop v4 patch 7 (fix ctx leak when sdw_amd_startup() fails):
  superseded by patch 18.
-Link to v4:
https://lore.kernel.org/all/20261026044507.2310694-1-Vijendar.Mukunda@amd.com/

Changes since v3:
- Add patch 12/13 to fix IRQ thread dropping STATUS_0TO7 when a
  wake event and a device-state change arrive simultaneously.
- Add patch 13/13 to fix STATUS_8TO11 double-clear race in the
  wake handler introduced by the simultaneous-wake fix.
- Add lockdep_assert_held() to amd_sdw_read_ping_status() (patch 4/13)
  to enforce the msg_lock contract machine-checkably.
- Add return response on IMM status cleanup timeout (patch 2/13).
- Update patch 11/13 (fix interrupt gate and work drain ordering in PM
  ops) to also fix amd_sdw_wake_enable() ordering in both CLK_STOP_MODE
  paths: move the call to after amd_enable_sdw_interrupts() so the
  unconditional interrupt mask write does not clobber the wake bit.

Changes since v2:
- Update comment for ping_slave_stat variable description in patch 3/13.
- Split the remove path work drain and pm ops guard patch into separate
  patches (patches 5/13 and 6/13) to improve clarity and maintainability.

Changes since v1:
- Rework patch 1 to fix the u64 return type properly via out-parameter.

Vijendar Mukunda (25):
  soundwire: amd: consolidate pad register macros into amd_init.h
  soundwire: amd: program pad SCHMEN and OE control for ACP 7.0/7.1/7.2
  soundwire: amd: add pad ctrl helpers for D0/D3 PM transitions
  soundwire: amd: track full pad teardown across suspend/resume
  soundwire: amd: fix missing wake interrupt and group wake-arming calls
    in runtime suspend
  soundwire: amd: fix IRQ thread dropping STATUS_0TO7 on simultaneous
    wake
  soundwire: amd: remove STATUS_8TO11 clear from
    amd_sdw_process_wake_event()
  soundwire: amd: fix missing wake interrupt for ACP6.3 in POWER_OFF
    suspend
  soundwire: amd: fix SDW command timeout return value handling
  soundwire: amd: return response on IMM status cleanup timeout
  soundwire: amd: clarify bad-state guard poll omission in IMM_RES_VALID
    path
  soundwire: amd: cache ping slave status to avoid spurious disconnect
    on timeout
  soundwire: amd: assert msg_lock is held in amd_sdw_read_ping_status()
  soundwire: amd: fix work drain ordering in remove path
  soundwire: amd: defer dev_set_drvdata() until after INIT_WORK()
  soundwire: amd: pm_runtime guard in remove path
  soundwire: amd: use interrupt_enabled flag to guard IRQ work
    scheduling
  soundwire: amd: delay *sdw_ctx publication until after startup
    succeeds
  soundwire: amd: propagate amd_init_sdw_manager() error on resume
  soundwire: amd: drop amd_deinit_sdw_manager() in POWER_OFF suspend
  soundwire: amd: replace >= ACP70 with explicit switch/case in PM paths
  soundwire: amd: fix interrupt gate and work drain ordering in PM ops
  soundwire: amd: check for NULL manager drvdata in sdw_amd_startup()
  soundwire: amd: fix shared-register RMW race in
    amd_sdw_set_device_state()
  soundwire: amd: fix shared-register RMW race in
    amd_sdw_host_wake_enable()

 drivers/soundwire/amd_init.c      |  69 ++++--
 drivers/soundwire/amd_init.h      |  18 ++
 drivers/soundwire/amd_manager.c   | 364 ++++++++++++++++++++++++------
 drivers/soundwire/amd_manager.h   |  10 -
 include/linux/soundwire/sdw_amd.h |  15 ++
 5 files changed, 369 insertions(+), 107 deletions(-)

-- 
2.53.0

[PATCH v5 01/25] soundwire: amd: consolidate pad register macros into amd_init.h

[Vijendar Mukunda]

Move ACP_PAD_PULLDOWN_CTRL and ACP_SW_PAD_KEEPER_EN from their duplicate
definitions in amd_init.c and amd_manager.h into amd_init.h, so a single
canonical definition is shared by both translation units.

Drop the eight stale AMD_SDW{0,1}_PAD_*_{ENABLE,DISABLE}_MASK definitions
from amd_manager.h; they are no longer referenced anywhere.

No functional change.

Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/soundwire/amd_init.c    |  2 --
 drivers/soundwire/amd_init.h    |  3 +++
 drivers/soundwire/amd_manager.h | 10 ----------
 3 files changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/soundwire/amd_init.c b/drivers/soundwire/amd_init.c
index 8e419ddfa516..b4f45bdf3c17 100644
--- a/drivers/soundwire/amd_init.c
+++ b/drivers/soundwire/amd_init.c
@@ -16,8 +16,6 @@
 
 #include "amd_init.h"
 
-#define ACP_PAD_PULLDOWN_CTRL				0x0001448
-#define ACP_SW_PAD_KEEPER_EN				0x0001454
 #define AMD_SDW0_PAD_CTRL_MASK				0x60
 #define AMD_SDW1_PAD_CTRL_MASK				5
 #define AMD_SDW_PAD_CTRL_MASK		(AMD_SDW0_PAD_CTRL_MASK | AMD_SDW1_PAD_CTRL_MASK)
diff --git a/drivers/soundwire/amd_init.h b/drivers/soundwire/amd_init.h
index 5e7b43836a37..106852464dc7 100644
--- a/drivers/soundwire/amd_init.h
+++ b/drivers/soundwire/amd_init.h
@@ -8,6 +8,9 @@
 
 #include <linux/soundwire/sdw_amd.h>
 
+#define ACP_PAD_PULLDOWN_CTRL				0x0001448
+#define ACP_SW_PAD_KEEPER_EN				0x0001454
+
 int amd_sdw_manager_start(struct amd_sdw_manager *amd_manager);
 
 static inline void amd_updatel(void __iomem *mmio, int offset, u32 mask, u32 val)
diff --git a/drivers/soundwire/amd_manager.h b/drivers/soundwire/amd_manager.h
index 88cf8a426a0c..98c814d5d7cd 100644
--- a/drivers/soundwire/amd_manager.h
+++ b/drivers/soundwire/amd_manager.h
@@ -11,8 +11,6 @@
 #define SDW_MANAGER_REG_OFFSET				0xc00
 #define AMD_SDW_DEFAULT_ROWS				50
 #define AMD_SDW_DEFAULT_COLUMNS				10
-#define ACP_PAD_PULLDOWN_CTRL				0x0001448
-#define ACP_SW_PAD_KEEPER_EN				0x0001454
 #define ACP_SW0_WAKE_EN					0x0001458
 #define ACP_EXTERNAL_INTR_CNTL0				0x0001a04
 #define ACP_EXTERNAL_INTR_STAT0				0x0001a0c
@@ -180,14 +178,6 @@
 #define AMD_DPN_CH_EN_LCTRL				GENMASK(2, 0)
 #define AMD_DPN_CH_EN_CHMASK				GENMASK(10, 3)
 #define AMD_SDW_STAT_MAX_RETRY_COUNT			100
-#define AMD_SDW0_PAD_PULLDOWN_CTRL_ENABLE_MASK		0x7f9f
-#define AMD_SDW1_PAD_PULLDOWN_CTRL_ENABLE_MASK		0x7ffa
-#define AMD_SDW0_PAD_PULLDOWN_CTRL_DISABLE_MASK		0x60
-#define AMD_SDW1_PAD_PULLDOWN_CTRL_DISABLE_MASK		5
-#define AMD_SDW0_PAD_KEEPER_EN_MASK			1
-#define AMD_SDW1_PAD_KEEPER_EN_MASK			0x10
-#define AMD_SDW0_PAD_KEEPER_DISABLE_MASK		0x1e
-#define AMD_SDW1_PAD_KEEPER_DISABLE_MASK		0xf
 #define AMD_SDW_PREQ_INTR_STAT				BIT(19)
 #define AMD_SDW_CLK_STOP_DONE				1
 #define AMD_SDW_CLK_RESUME_REQ				2
-- 
2.53.0

[PATCH v5 02/25] soundwire: amd: program pad SCHMEN and OE control for ACP 7.0/7.1/7.2

[Vijendar Mukunda]

ACP 7.0/7.1/7.2 hardware requires SCHMEN control and
output-enable (OE) registers to be programmed as part of the pad enable
sequence, in addition to the existing keeper and pulldown writes.

Update amd_enable_sdw_pads() to program ACP_PAD_SCHMEN_CTRL and
ACP_PAD_DISABLE_OE_CTRL for active pads. Pass acp_rev to
amd_enable_sdw_pads() and restrict these writes to ACP 7.0/7.1/7.2;
ACP 6.3 is unaffected.

Define per-register masks in amd_init.h replacing generic local defines
in amd_init.c.

Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
Fixes: cf0ddbc29dfa ("soundwire: amd: refactor soundwire pads enable")
---
 drivers/soundwire/amd_init.c | 48 ++++++++++++++++++++++--------------
 drivers/soundwire/amd_init.h | 15 +++++++++++
 2 files changed, 45 insertions(+), 18 deletions(-)

diff --git a/drivers/soundwire/amd_init.c b/drivers/soundwire/amd_init.c
index b4f45bdf3c17..032697ea3590 100644
--- a/drivers/soundwire/amd_init.c
+++ b/drivers/soundwire/amd_init.c
@@ -16,37 +16,49 @@
 
 #include "amd_init.h"
 
-#define AMD_SDW0_PAD_CTRL_MASK				0x60
-#define AMD_SDW1_PAD_CTRL_MASK				5
-#define AMD_SDW_PAD_CTRL_MASK		(AMD_SDW0_PAD_CTRL_MASK | AMD_SDW1_PAD_CTRL_MASK)
-#define AMD_SDW0_PAD_EN					1
-#define AMD_SDW1_PAD_EN					0x10
-#define AMD_SDW_PAD_EN			(AMD_SDW0_PAD_EN | AMD_SDW1_PAD_EN)
-
-static int amd_enable_sdw_pads(void __iomem *mmio, u32 link_mask, struct device *dev)
+static int amd_enable_sdw_pads(void __iomem *mmio, u32 link_mask, u32 acp_rev, struct device *dev)
 {
-	u32 pad_keeper_en, pad_pulldown_ctrl_mask;
+	u32 pad_keeper_mask, pad_pulldown_mask, pad_schmen_mask, pad_oe_mask;
 
 	switch (link_mask) {
 	case 1:
-		pad_keeper_en = AMD_SDW0_PAD_EN;
-		pad_pulldown_ctrl_mask = AMD_SDW0_PAD_CTRL_MASK;
+		pad_keeper_mask = AMD_SDW0_PAD_KEEPER_EN_MASK;
+		pad_pulldown_mask = AMD_SDW0_PAD_PULLDOWN_MASK;
+		pad_schmen_mask = AMD_SDW0_PAD_SCHMEN_MASK;
+		pad_oe_mask = AMD_SDW0_PAD_OE_CTRL_MASK;
 		break;
 	case 2:
-		pad_keeper_en = AMD_SDW1_PAD_EN;
-		pad_pulldown_ctrl_mask = AMD_SDW1_PAD_CTRL_MASK;
+		pad_keeper_mask = AMD_SDW1_PAD_KEEPER_EN_MASK;
+		pad_pulldown_mask = AMD_SDW1_PAD_PULLDOWN_MASK;
+		pad_schmen_mask = AMD_SDW1_PAD_SCHMEN_MASK;
+		pad_oe_mask = AMD_SDW1_PAD_OE_CTRL_MASK;
 		break;
 	case 3:
-		pad_keeper_en = AMD_SDW_PAD_EN;
-		pad_pulldown_ctrl_mask = AMD_SDW_PAD_CTRL_MASK;
+		pad_keeper_mask = AMD_SDW_PAD_KEEPER_EN_MASK;
+		pad_pulldown_mask = AMD_SDW_PAD_PULLDOWN_MASK;
+		pad_schmen_mask = AMD_SDW_PAD_SCHMEN_MASK;
+		pad_oe_mask = AMD_SDW_PAD_OE_CTRL_MASK;
 		break;
 	default:
 		dev_err(dev, "No SDW Links are enabled\n");
 		return -ENODEV;
 	}
 
-	amd_updatel(mmio, ACP_SW_PAD_KEEPER_EN, pad_keeper_en, pad_keeper_en);
-	amd_updatel(mmio, ACP_PAD_PULLDOWN_CTRL, pad_pulldown_ctrl_mask, 0);
+	amd_updatel(mmio, ACP_SW_PAD_KEEPER_EN, pad_keeper_mask, pad_keeper_mask);
+	amd_updatel(mmio, ACP_PAD_PULLDOWN_CTRL, pad_pulldown_mask, 0);
+	switch (acp_rev) {
+	case ACP70_PCI_REV_ID:
+	case ACP71_PCI_REV_ID:
+	case ACP72_PCI_REV_ID:
+		amd_updatel(mmio, ACP_PAD_SCHMEN_CTRL, pad_schmen_mask, pad_schmen_mask);
+		amd_updatel(mmio, ACP_PAD_DISABLE_OE_CTRL, pad_oe_mask, 0);
+		break;
+	case ACP63_PCI_REV_ID:
+		break;
+	default:
+		dev_err(dev, "unsupported ACP revision 0x%x\n", acp_rev);
+		return -EINVAL;
+	}
 
 	return 0;
 }
@@ -86,7 +98,7 @@ static struct sdw_amd_ctx *sdw_amd_probe_controller(struct sdw_amd_res *res)
 
 	count = res->count;
 	dev_dbg(&adev->dev, "Creating %d SDW Link devices\n", count);
-	ret = amd_enable_sdw_pads(res->mmio_base, res->link_mask, res->parent);
+	ret = amd_enable_sdw_pads(res->mmio_base, res->link_mask, res->acp_rev, res->parent);
 	if (ret)
 		return NULL;
 
diff --git a/drivers/soundwire/amd_init.h b/drivers/soundwire/amd_init.h
index 106852464dc7..8324bc7a147f 100644
--- a/drivers/soundwire/amd_init.h
+++ b/drivers/soundwire/amd_init.h
@@ -9,7 +9,22 @@
 #include <linux/soundwire/sdw_amd.h>
 
 #define ACP_PAD_PULLDOWN_CTRL				0x0001448
+#define ACP_PAD_SCHMEN_CTRL				0x0001450
 #define ACP_SW_PAD_KEEPER_EN				0x0001454
+#define ACP_PAD_DISABLE_OE_CTRL				0x0001468
+
+#define AMD_SDW0_PAD_PULLDOWN_MASK			(BIT(5) | BIT(6))
+#define AMD_SDW1_PAD_PULLDOWN_MASK			(BIT(0) | BIT(2))
+#define AMD_SDW_PAD_PULLDOWN_MASK	(AMD_SDW0_PAD_PULLDOWN_MASK | AMD_SDW1_PAD_PULLDOWN_MASK)
+#define AMD_SDW0_PAD_KEEPER_EN_MASK			BIT(0)
+#define AMD_SDW1_PAD_KEEPER_EN_MASK			BIT(4)
+#define AMD_SDW_PAD_KEEPER_EN_MASK	(AMD_SDW0_PAD_KEEPER_EN_MASK | AMD_SDW1_PAD_KEEPER_EN_MASK)
+#define AMD_SDW0_PAD_OE_CTRL_MASK			(BIT(5) | BIT(6))
+#define AMD_SDW1_PAD_OE_CTRL_MASK			(BIT(0) | BIT(2))
+#define AMD_SDW_PAD_OE_CTRL_MASK	(AMD_SDW0_PAD_OE_CTRL_MASK | AMD_SDW1_PAD_OE_CTRL_MASK)
+#define AMD_SDW0_PAD_SCHMEN_MASK			(BIT(5) | BIT(6))
+#define AMD_SDW1_PAD_SCHMEN_MASK			(BIT(0) | BIT(2))
+#define AMD_SDW_PAD_SCHMEN_MASK		(AMD_SDW0_PAD_SCHMEN_MASK | AMD_SDW1_PAD_SCHMEN_MASK)
 
 int amd_sdw_manager_start(struct amd_sdw_manager *amd_manager);
 
-- 
2.53.0

Re: [PATCH v5 02/25] soundwire: amd: program pad SCHMEN and OE control for ACP 7.0/7.1/7.2

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Low] soundwire: amd: Fixes tag cites ACP 6.3 refactor instead of ACP
  7.x commits
- [Low] soundwire: amd: amd_enable_sdw_pads() writes pad registers
  before acp_rev check
--

commit feea96d1feca02ca2e3cb0b4f0b3f33fab424dca
Author: Vijendar Mukunda <Vijendar.Mukunda@amd.com>

> soundwire: amd: program pad SCHMEN and OE control for ACP 7.0/7.1/7.2
>
> ACP 7.0/7.1/7.2 hardware requires SCHMEN control and
> output-enable (OE) registers to be programmed as part of the pad enable
> sequence, in addition to the existing keeper and pulldown writes.
>
> Update amd_enable_sdw_pads() to program ACP_PAD_SCHMEN_CTRL and
> ACP_PAD_DISABLE_OE_CTRL for active pads. Pass acp_rev to
> amd_enable_sdw_pads() and restrict these writes to ACP 7.0/7.1/7.2;
> ACP 6.3 is unaffected.
>
> Define per-register masks in amd_init.h replacing generic local defines
> in amd_init.c.
>
> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
> Fixes: cf0ddbc29dfa ("soundwire: amd: refactor soundwire pads enable")

[Severity: Low]
cf0ddbc29dfa ("soundwire: amd: refactor soundwire pads enable") only
supported ACP 6.3, which is unaffected by this change, and predates both
struct sdw_amd_res.acp_rev and the ACP 7.x definitions.  Should this cite
2c0ae8ef1e5e ("soundwire: amd: add support for ACP7.0 & ACP7.1 platforms")
and 34b1cb4ec286 ("soundwire: amd: Add support for acp7.2 platform")
instead so stable tooling does not attempt backports to ACP 6.3-only trees?

> diff --git a/drivers/soundwire/amd_init.c b/drivers/soundwire/amd_init.c
> index b4f45bdf3c171..032697ea35906 100644
> --- a/drivers/soundwire/amd_init.c
> +++ b/drivers/soundwire/amd_init.c
> @@ -16,37 +16,49 @@
>  
>  #include "amd_init.h"
>  
> -#define AMD_SDW0_PAD_CTRL_MASK				0x60
> -#define AMD_SDW1_PAD_CTRL_MASK				5
> -#define AMD_SDW_PAD_CTRL_MASK		(AMD_SDW0_PAD_CTRL_MASK | AMD_SDW1_PAD_CTRL_MASK)
> -#define AMD_SDW0_PAD_EN					1
> -#define AMD_SDW1_PAD_EN					0x10
> -#define AMD_SDW_PAD_EN			(AMD_SDW0_PAD_EN | AMD_SDW1_PAD_EN)
> -
> -static int amd_enable_sdw_pads(void __iomem *mmio, u32 link_mask, struct device *dev)
> +static int amd_enable_sdw_pads(void __iomem *mmio, u32 link_mask, u32 acp_rev, struct device *dev)
>  {
> -	u32 pad_keeper_en, pad_pulldown_ctrl_mask;
> +	u32 pad_keeper_mask, pad_pulldown_mask, pad_schmen_mask, pad_oe_mask;
>  
>  	switch (link_mask) {
>  	case 1:
> -		pad_keeper_en = AMD_SDW0_PAD_EN;
> -		pad_pulldown_ctrl_mask = AMD_SDW0_PAD_CTRL_MASK;
> +		pad_keeper_mask = AMD_SDW0_PAD_KEEPER_EN_MASK;
> +		pad_pulldown_mask = AMD_SDW0_PAD_PULLDOWN_MASK;
> +		pad_schmen_mask = AMD_SDW0_PAD_SCHMEN_MASK;
> +		pad_oe_mask = AMD_SDW0_PAD_OE_CTRL_MASK;
>  		break;
>  	case 2:
> -		pad_keeper_en = AMD_SDW1_PAD_EN;
> -		pad_pulldown_ctrl_mask = AMD_SDW1_PAD_CTRL_MASK;
> +		pad_keeper_mask = AMD_SDW1_PAD_KEEPER_EN_MASK;
> +		pad_pulldown_mask = AMD_SDW1_PAD_PULLDOWN_MASK;
> +		pad_schmen_mask = AMD_SDW1_PAD_SCHMEN_MASK;
> +		pad_oe_mask = AMD_SDW1_PAD_OE_CTRL_MASK;
>  		break;
>  	case 3:
> -		pad_keeper_en = AMD_SDW_PAD_EN;
> -		pad_pulldown_ctrl_mask = AMD_SDW_PAD_CTRL_MASK;
> +		pad_keeper_mask = AMD_SDW_PAD_KEEPER_EN_MASK;
> +		pad_pulldown_mask = AMD_SDW_PAD_PULLDOWN_MASK;
> +		pad_schmen_mask = AMD_SDW_PAD_SCHMEN_MASK;
> +		pad_oe_mask = AMD_SDW_PAD_OE_CTRL_MASK;
>  		break;
>  	default:
>  		dev_err(dev, "No SDW Links are enabled\n");
>  		return -ENODEV;
>  	}
>  
> -	amd_updatel(mmio, ACP_SW_PAD_KEEPER_EN, pad_keeper_mask, pad_keeper_mask);
> -	amd_updatel(mmio, ACP_PAD_PULLDOWN_CTRL, pad_pulldown_ctrl_mask, 0);
> +	amd_updatel(mmio, ACP_SW_PAD_KEEPER_EN, pad_keeper_mask, pad_keeper_mask);
> +	amd_updatel(mmio, ACP_PAD_PULLDOWN_CTRL, pad_pulldown_mask, 0);
> +	switch (acp_rev) {
> +	case ACP70_PCI_REV_ID:
> +	case ACP71_PCI_REV_ID:
> +	case ACP72_PCI_REV_ID:
> +		amd_updatel(mmio, ACP_PAD_SCHMEN_CTRL, pad_schmen_mask, pad_schmen_mask);
> +		amd_updatel(mmio, ACP_PAD_DISABLE_OE_CTRL, pad_oe_mask, 0);
> +		break;
> +	case ACP63_PCI_REV_ID:
> +		break;
> +	default:
> +		dev_err(dev, "unsupported ACP revision 0x%x\n", acp_rev);
> +		return -EINVAL;
> +	}

[Severity: Low]
Should acp_rev be checked before programming ACP_SW_PAD_KEEPER_EN and
ACP_PAD_PULLDOWN_CTRL so that an unsupported acp_rev (such as 0x7b or 0x7f
from amd_sof_acp7x_probe()) does not modify pad registers before returning
-EINVAL?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107154659.3358223-1-Vijendar.Mukunda@amd.com?part=2

Re: [PATCH v5 02/25] soundwire: amd: program pad SCHMEN and OE control for ACP 7.0/7.1/7.2

[Mukunda,Vijendar]

On 11/7/26 21:43, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issues to consider:
> - [Low] soundwire: amd: Fixes tag cites ACP 6.3 refactor instead of ACP
>    7.x commits
> - [Low] soundwire: amd: amd_enable_sdw_pads() writes pad registers
>    before acp_rev check
> --
>
> commit feea96d1feca02ca2e3cb0b4f0b3f33fab424dca
> Author: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
>
>> soundwire: amd: program pad SCHMEN and OE control for ACP 7.0/7.1/7.2
>>
>> ACP 7.0/7.1/7.2 hardware requires SCHMEN control and
>> output-enable (OE) registers to be programmed as part of the pad enable
>> sequence, in addition to the existing keeper and pulldown writes.
>>
>> Update amd_enable_sdw_pads() to program ACP_PAD_SCHMEN_CTRL and
>> ACP_PAD_DISABLE_OE_CTRL for active pads. Pass acp_rev to
>> amd_enable_sdw_pads() and restrict these writes to ACP 7.0/7.1/7.2;
>> ACP 6.3 is unaffected.
>>
>> Define per-register masks in amd_init.h replacing generic local defines
>> in amd_init.c.
>>
>> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
>> Fixes: cf0ddbc29dfa ("soundwire: amd: refactor soundwire pads enable")
> [Severity: Low]
> cf0ddbc29dfa ("soundwire: amd: refactor soundwire pads enable") only
> supported ACP 6.3, which is unaffected by this change, and predates both
> struct sdw_amd_res.acp_rev and the ACP 7.x definitions.  Should this cite
> 2c0ae8ef1e5e ("soundwire: amd: add support for ACP7.0 & ACP7.1 platforms")
> and 34b1cb4ec286 ("soundwire: amd: Add support for acp7.2 platform")
> instead so stable tooling does not attempt backports to ACP 6.3-only trees?
Agreed. The missing SCHMEN and OE register programming was a gap
from when ACP 7.0/7.1/7.2 support was first introduced. The Fixes tags
will be updated in next respin.
>
>> diff --git a/drivers/soundwire/amd_init.c b/drivers/soundwire/amd_init.c
>> index b4f45bdf3c171..032697ea35906 100644
>> --- a/drivers/soundwire/amd_init.c
>> +++ b/drivers/soundwire/amd_init.c
>> @@ -16,37 +16,49 @@
>>   
>>   #include "amd_init.h"
>>   
>> -#define AMD_SDW0_PAD_CTRL_MASK				0x60
>> -#define AMD_SDW1_PAD_CTRL_MASK				5
>> -#define AMD_SDW_PAD_CTRL_MASK		(AMD_SDW0_PAD_CTRL_MASK | AMD_SDW1_PAD_CTRL_MASK)
>> -#define AMD_SDW0_PAD_EN					1
>> -#define AMD_SDW1_PAD_EN					0x10
>> -#define AMD_SDW_PAD_EN			(AMD_SDW0_PAD_EN | AMD_SDW1_PAD_EN)
>> -
>> -static int amd_enable_sdw_pads(void __iomem *mmio, u32 link_mask, struct device *dev)
>> +static int amd_enable_sdw_pads(void __iomem *mmio, u32 link_mask, u32 acp_rev, struct device *dev)
>>   {
>> -	u32 pad_keeper_en, pad_pulldown_ctrl_mask;
>> +	u32 pad_keeper_mask, pad_pulldown_mask, pad_schmen_mask, pad_oe_mask;
>>   
>>   	switch (link_mask) {
>>   	case 1:
>> -		pad_keeper_en = AMD_SDW0_PAD_EN;
>> -		pad_pulldown_ctrl_mask = AMD_SDW0_PAD_CTRL_MASK;
>> +		pad_keeper_mask = AMD_SDW0_PAD_KEEPER_EN_MASK;
>> +		pad_pulldown_mask = AMD_SDW0_PAD_PULLDOWN_MASK;
>> +		pad_schmen_mask = AMD_SDW0_PAD_SCHMEN_MASK;
>> +		pad_oe_mask = AMD_SDW0_PAD_OE_CTRL_MASK;
>>   		break;
>>   	case 2:
>> -		pad_keeper_en = AMD_SDW1_PAD_EN;
>> -		pad_pulldown_ctrl_mask = AMD_SDW1_PAD_CTRL_MASK;
>> +		pad_keeper_mask = AMD_SDW1_PAD_KEEPER_EN_MASK;
>> +		pad_pulldown_mask = AMD_SDW1_PAD_PULLDOWN_MASK;
>> +		pad_schmen_mask = AMD_SDW1_PAD_SCHMEN_MASK;
>> +		pad_oe_mask = AMD_SDW1_PAD_OE_CTRL_MASK;
>>   		break;
>>   	case 3:
>> -		pad_keeper_en = AMD_SDW_PAD_EN;
>> -		pad_pulldown_ctrl_mask = AMD_SDW_PAD_CTRL_MASK;
>> +		pad_keeper_mask = AMD_SDW_PAD_KEEPER_EN_MASK;
>> +		pad_pulldown_mask = AMD_SDW_PAD_PULLDOWN_MASK;
>> +		pad_schmen_mask = AMD_SDW_PAD_SCHMEN_MASK;
>> +		pad_oe_mask = AMD_SDW_PAD_OE_CTRL_MASK;
>>   		break;
>>   	default:
>>   		dev_err(dev, "No SDW Links are enabled\n");
>>   		return -ENODEV;
>>   	}
>>   
>> -	amd_updatel(mmio, ACP_SW_PAD_KEEPER_EN, pad_keeper_mask, pad_keeper_mask);
>> -	amd_updatel(mmio, ACP_PAD_PULLDOWN_CTRL, pad_pulldown_ctrl_mask, 0);
>> +	amd_updatel(mmio, ACP_SW_PAD_KEEPER_EN, pad_keeper_mask, pad_keeper_mask);
>> +	amd_updatel(mmio, ACP_PAD_PULLDOWN_CTRL, pad_pulldown_mask, 0);
>> +	switch (acp_rev) {
>> +	case ACP70_PCI_REV_ID:
>> +	case ACP71_PCI_REV_ID:
>> +	case ACP72_PCI_REV_ID:
>> +		amd_updatel(mmio, ACP_PAD_SCHMEN_CTRL, pad_schmen_mask, pad_schmen_mask);
>> +		amd_updatel(mmio, ACP_PAD_DISABLE_OE_CTRL, pad_oe_mask, 0);
>> +		break;
>> +	case ACP63_PCI_REV_ID:
>> +		break;
>> +	default:
>> +		dev_err(dev, "unsupported ACP revision 0x%x\n", acp_rev);
>> +		return -EINVAL;
>> +	}
> [Severity: Low]
> Should acp_rev be checked before programming ACP_SW_PAD_KEEPER_EN and
> ACP_PAD_PULLDOWN_CTRL so that an unsupported acp_rev (such as 0x7b or 0x7f
> from amd_sof_acp7x_probe()) does not modify pad registers before returning
> -EINVAL?
Agreed. The current ordering writes pad registers before validating
acp_rev, leaving the hardware in a partially modified state on the
-EINVAL return path. Next respin will move the acp_rev validation
to the top of amd_enable_sdw_pads() before any register write, so an
unsupported revision returns -EINVAL without touching pad registers.
>

[PATCH v5 03/25] soundwire: amd: add pad ctrl helpers for D0/D3 PM transitions

[Vijendar Mukunda]

During POWER_OFF_MODE clock stop the SoundWire clock is expected to
transition High to Low cleanly. On some OEM platforms based on
ACP 7.0/7.1/7.2, a transient High pulse is observed after the initial
Low; the clock briefly follows a High -> Low -> High -> Low sequence
(High lasting a few milliseconds) instead of the expected High -> Low.
This spurious High edge interferes with wake interrupt detection,
causing wake events to be silently missed.

Hardware team analysis identified that the pad keeper, pulldown,
SCHMEN control and output-enable registers must be programmed as a
coordinated sequence during D3 entry and D0 re-entry to suppress the
transient bounce and restore the pads correctly on resume.

Introduce amd_sdw_pad_ctrl() to program PAD_KEEPER_EN,
PAD_PULLDOWN_CTRL, PAD_SCHMEN_CTRL and PAD_DISABLE_OE_CTRL in a
single call for D0 entry (enable=true) or D3 entry (enable=false).
Introduce amd_sdw_oe_ctrl() as a lighter helper that only toggles OE.
Introduce amd_sdw_pad_ctrl_supported() to consolidate the ACP revision
check; both helpers use it in place of duplicate switch statements.

Both helpers apply only to ACP 7.0/7.1/7.2; ACP 6.3 returns without
error and unknown revisions emit a dev_err and return.

Wire amd_sdw_pad_ctrl() into amd_suspend() and amd_resume_runtime().
Wire amd_sdw_oe_ctrl() into amd_suspend_runtime() to disable OE while
leaving keeper and schmen enabled; pads remain capable of detecting wake
events when wake is armed.

Fixes: 829c3e1cb4a3 ("soundwire: amd: set device power state during suspend/resume sequence")
Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/soundwire/amd_manager.c | 108 ++++++++++++++++++++++++++++++++
 1 file changed, 108 insertions(+)

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index a3316efdf8ac..b21691652490 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -152,6 +152,104 @@ static int amd_deinit_sdw_manager(struct amd_sdw_manager *amd_manager)
 	return amd_disable_sdw_manager(amd_manager);
 }
 
+static void __amd_sdw_oe_update(struct amd_sdw_manager *amd_manager,
+				u32 mask, bool enable)
+{
+	amd_updatel(amd_manager->acp_mmio, ACP_PAD_DISABLE_OE_CTRL,
+		    mask, enable ? 0 : mask);
+}
+
+static bool amd_sdw_pad_ctrl_supported(struct amd_sdw_manager *amd_manager)
+{
+	switch (amd_manager->acp_rev) {
+	case ACP70_PCI_REV_ID:
+	case ACP71_PCI_REV_ID:
+	case ACP72_PCI_REV_ID:
+		return true;
+	case ACP63_PCI_REV_ID:
+		return false;
+	default:
+		dev_err(amd_manager->dev, "unknown ACP revision 0x%x\n",
+			amd_manager->acp_rev);
+		return false;
+	}
+}
+
+static void amd_sdw_oe_ctrl(struct amd_sdw_manager *amd_manager, bool enable)
+{
+	u32 mask;
+
+	if (!amd_sdw_pad_ctrl_supported(amd_manager))
+		return;
+
+	switch (amd_manager->instance) {
+	case ACP_SDW0:
+		mask = AMD_SDW0_PAD_OE_CTRL_MASK;
+		break;
+	case ACP_SDW1:
+		mask = AMD_SDW1_PAD_OE_CTRL_MASK;
+		break;
+	default:
+		dev_err(amd_manager->dev, "invalid SDW instance %d\n",
+			amd_manager->instance);
+		return;
+	}
+
+	mutex_lock(amd_manager->acp_sdw_lock);
+	__amd_sdw_oe_update(amd_manager, mask, enable);
+	mutex_unlock(amd_manager->acp_sdw_lock);
+}
+
+static void amd_sdw_pad_ctrl(struct amd_sdw_manager *amd_manager, bool enable)
+{
+	u32 keeper_mask, pulldown_mask, schmen_mask, oe_mask;
+
+	if (!amd_sdw_pad_ctrl_supported(amd_manager))
+		return;
+
+	switch (amd_manager->instance) {
+	case ACP_SDW0:
+		keeper_mask   = AMD_SDW0_PAD_KEEPER_EN_MASK;
+		pulldown_mask = AMD_SDW0_PAD_PULLDOWN_MASK;
+		schmen_mask   = AMD_SDW0_PAD_SCHMEN_MASK;
+		oe_mask       = AMD_SDW0_PAD_OE_CTRL_MASK;
+		break;
+	case ACP_SDW1:
+		keeper_mask   = AMD_SDW1_PAD_KEEPER_EN_MASK;
+		pulldown_mask = AMD_SDW1_PAD_PULLDOWN_MASK;
+		schmen_mask   = AMD_SDW1_PAD_SCHMEN_MASK;
+		oe_mask       = AMD_SDW1_PAD_OE_CTRL_MASK;
+		break;
+	default:
+		dev_err(amd_manager->dev, "invalid SDW instance %d\n",
+			amd_manager->instance);
+		return;
+	}
+
+	mutex_lock(amd_manager->acp_sdw_lock);
+	if (enable) {
+		/* D0: set keeper, clear pulldown, set schmen */
+		amd_updatel(amd_manager->acp_mmio, ACP_SW_PAD_KEEPER_EN,
+			    keeper_mask, keeper_mask);
+		amd_updatel(amd_manager->acp_mmio, ACP_PAD_PULLDOWN_CTRL,
+			    pulldown_mask, 0);
+		amd_updatel(amd_manager->acp_mmio, ACP_PAD_SCHMEN_CTRL,
+			    schmen_mask, schmen_mask);
+	} else {
+		/* D3: clear keeper, set pulldown, clear schmen */
+		amd_updatel(amd_manager->acp_mmio, ACP_SW_PAD_KEEPER_EN,
+			    keeper_mask, 0);
+		amd_updatel(amd_manager->acp_mmio, ACP_PAD_PULLDOWN_CTRL,
+			    pulldown_mask, pulldown_mask);
+		amd_updatel(amd_manager->acp_mmio, ACP_PAD_SCHMEN_CTRL,
+			    schmen_mask, 0);
+	}
+
+	/* enable OE on D0 (clear disable bits), disable OE on D3 (set them) */
+	__amd_sdw_oe_update(amd_manager, oe_mask, enable);
+	mutex_unlock(amd_manager->acp_sdw_lock);
+}
+
 static void amd_sdw_set_frameshape(struct amd_sdw_manager *amd_manager)
 {
 	u32 frame_size;
@@ -1319,6 +1417,8 @@ static int __maybe_unused amd_suspend(struct device *dev)
 		ret = amd_deinit_sdw_manager(amd_manager);
 		if (ret)
 			return ret;
+		/* System suspend: tear down all pads */
+		amd_sdw_pad_ctrl(amd_manager, false);
 	}
 	if (amd_manager->acp_rev >= ACP70_PCI_REV_ID) {
 		ret = amd_sdw_set_device_state(amd_manager, AMD_SDW_DEVICE_STATE_D3);
@@ -1363,6 +1463,13 @@ static int __maybe_unused amd_suspend_runtime(struct device *dev)
 		ret = amd_deinit_sdw_manager(amd_manager);
 		if (ret)
 			return ret;
+		/*
+		 * For ACP 7.0/7.1/7.2: disable OE only; keeper and schmen remain
+		 * enabled unconditionally so that wake-capable devices can detect
+		 * wake events if wake is armed.
+		 * For ACP 6.3 this call is a no-op.
+		 */
+		amd_sdw_oe_ctrl(amd_manager, false);
 	}
 	if (amd_manager->acp_rev >= ACP70_PCI_REV_ID) {
 		ret = amd_sdw_set_device_state(amd_manager, AMD_SDW_DEVICE_STATE_D3);
@@ -1403,6 +1510,7 @@ static int __maybe_unused amd_resume_runtime(struct device *dev)
 				return ret;
 		}
 	} else if (amd_manager->power_mode_mask & AMD_SDW_POWER_OFF_MODE) {
+		amd_sdw_pad_ctrl(amd_manager, true);
 		writel(0x00, amd_manager->acp_mmio + ACP_SW_WAKE_EN(amd_manager->instance));
 		if (amd_manager->acp_rev >= ACP70_PCI_REV_ID) {
 			ret = amd_sdw_host_wake_enable(amd_manager, false);
-- 
2.53.0

[PATCH v5 04/25] soundwire: amd: track full pad teardown across suspend/resume

[Vijendar Mukunda]

amd_sdw_pad_ctrl() performs a full pad teardown (keeper, pulldown, schmen
and OE) for system suspend, while amd_sdw_oe_ctrl() disables only OE for
wake-armed runtime suspend. amd_resume_runtime() must distinguish between
these two cases to restore pads correctly: a full teardown requires
amd_sdw_pad_ctrl(true) on resume; a wake-armed runtime suspend requires
only amd_sdw_oe_ctrl(true).

Introduce pad_fully_torn_down flag to track when a full pad teardown was
performed. Set the flag after amd_sdw_pad_ctrl(false) in amd_suspend()
using amd_sdw_pad_ctrl_supported() to gate the assignment to revisions
that support pad control. Clear it after the full restore in
amd_resume_runtime(). On resume, restore all pads if the flag is set,
otherwise restore OE only; both paths delegate revision gating to the
respective helpers.

Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/soundwire/amd_manager.c   | 12 ++++++++++--
 include/linux/soundwire/sdw_amd.h |  7 +++++++
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index b21691652490..28f5dbe131dc 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -1418,7 +1418,10 @@ static int __maybe_unused amd_suspend(struct device *dev)
 		if (ret)
 			return ret;
 		/* System suspend: tear down all pads */
-		amd_sdw_pad_ctrl(amd_manager, false);
+		if (amd_sdw_pad_ctrl_supported(amd_manager)) {
+			amd_sdw_pad_ctrl(amd_manager, false);
+			amd_manager->pad_fully_torn_down = true;
+		}
 	}
 	if (amd_manager->acp_rev >= ACP70_PCI_REV_ID) {
 		ret = amd_sdw_set_device_state(amd_manager, AMD_SDW_DEVICE_STATE_D3);
@@ -1510,7 +1513,12 @@ static int __maybe_unused amd_resume_runtime(struct device *dev)
 				return ret;
 		}
 	} else if (amd_manager->power_mode_mask & AMD_SDW_POWER_OFF_MODE) {
-		amd_sdw_pad_ctrl(amd_manager, true);
+		if (amd_manager->pad_fully_torn_down) {
+			amd_sdw_pad_ctrl(amd_manager, true);
+			amd_manager->pad_fully_torn_down = false;
+		} else {
+			amd_sdw_oe_ctrl(amd_manager, true);
+		}
 		writel(0x00, amd_manager->acp_mmio + ACP_SW_WAKE_EN(amd_manager->instance));
 		if (amd_manager->acp_rev >= ACP70_PCI_REV_ID) {
 			ret = amd_sdw_host_wake_enable(amd_manager, false);
diff --git a/include/linux/soundwire/sdw_amd.h b/include/linux/soundwire/sdw_amd.h
index 470360a2723c..c959d0fe2a39 100644
--- a/include/linux/soundwire/sdw_amd.h
+++ b/include/linux/soundwire/sdw_amd.h
@@ -75,6 +75,12 @@ struct sdw_amd_dai_runtime {
  * @wake_en_mask: wake enable mask per SoundWire manager
  * @acp_rev: acp pci device revision id
  * @clk_stopped: flag set to true when clock is stopped
+ * @pad_fully_torn_down: true when system suspend performed a full pad
+ *   teardown (keeper, schmen and OE cleared, pulldown asserted). Cleared after
+ *   all pads are restored in amd_resume_runtime(). False on initial probe
+ *   (zero-initialized) and after a wake-armed runtime suspend in which
+ *   only OE was disabled; in the latter case amd_resume_runtime() restores
+ *   OE only, leaving keeper and schmen untouched.
  * @power_mode_mask: flag interprets amd SoundWire manager power mode
  * @dai_runtime_array: dai runtime array
  */
@@ -106,6 +112,7 @@ struct amd_sdw_manager {
 	u32 power_mode_mask;
 	u32 acp_rev;
 	bool clk_stopped;
+	bool pad_fully_torn_down;
 
 	struct sdw_amd_dai_runtime **dai_runtime_array;
 };
-- 
2.53.0

[PATCH v5 05/25] soundwire: amd: fix missing wake interrupt and group wake-arming calls in runtime suspend

[Vijendar Mukunda]

In amd_suspend_runtime() POWER_OFF_MODE, amd_sdw_wake_enable() was called
before amd_deinit_sdw_manager(), causing the wake IRQ mask to be silently
overwritten.

amd_deinit_sdw_manager() calls amd_disable_sdw_interrupts() which writes
0x00 to ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11, wiping
AMD_SDW_WAKE_INTR_MASK (BIT(16)) set by amd_sdw_wake_enable(). Move
amd_sdw_wake_enable() to after amd_deinit_sdw_manager() so the wake mask
is armed after the interrupt-disable sequence and cannot be clobbered.

Move amd_sdw_host_wake_enable() after amd_deinit_sdw_manager() as well,
grouping both wake-arming calls together after the interrupt-disable
sequence.

Fixes: 86a4371b7697 ("soundwire: amd: fix for handling slave alerts after link is down")
Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/soundwire/amd_manager.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index 28f5dbe131dc..3d60bea82792 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -1454,18 +1454,18 @@ static int __maybe_unused amd_suspend_runtime(struct device *dev)
 		if (ret)
 			return ret;
 	} else if (amd_manager->power_mode_mask & AMD_SDW_POWER_OFF_MODE) {
-		amd_sdw_wake_enable(amd_manager, true);
-		if (amd_manager->acp_rev >= ACP70_PCI_REV_ID) {
-			ret = amd_sdw_host_wake_enable(amd_manager, true);
-			if (ret)
-				return ret;
-		}
 		ret = amd_sdw_clock_stop(amd_manager);
 		if (ret)
 			return ret;
 		ret = amd_deinit_sdw_manager(amd_manager);
 		if (ret)
 			return ret;
+		amd_sdw_wake_enable(amd_manager, true);
+		if (amd_manager->acp_rev >= ACP70_PCI_REV_ID) {
+			ret = amd_sdw_host_wake_enable(amd_manager, true);
+			if (ret)
+				return ret;
+		}
 		/*
 		 * For ACP 7.0/7.1/7.2: disable OE only; keeper and schmen remain
 		 * enabled unconditionally so that wake-capable devices can detect
-- 
2.53.0

Re: [PATCH v5 05/25] soundwire: amd: fix missing wake interrupt and group wake-arming calls in runtime suspend

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] soundwire: amd: wrong Fixes trailer in amd_suspend_runtime()
  wake fix
--

commit 969fd0aabc344fd522f022214936cf6f2b366440
Author: Vijendar Mukunda <Vijendar.Mukunda@amd.com>

> soundwire: amd: fix missing wake interrupt and group wake-arming calls in runtime suspend
>
> In amd_suspend_runtime() POWER_OFF_MODE, amd_sdw_wake_enable() was called
> before amd_deinit_sdw_manager(), causing the wake IRQ mask to be silently
> overwritten.
>
> amd_deinit_sdw_manager() calls amd_disable_sdw_interrupts() which writes
> 0x00 to ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11, wiping
> AMD_SDW_WAKE_INTR_MASK (BIT(16)) set by amd_sdw_wake_enable(). Move
> amd_sdw_wake_enable() to after amd_deinit_sdw_manager() so the wake mask
> is armed after the interrupt-disable sequence and cannot be clobbered.
>
> Move amd_sdw_host_wake_enable() after amd_deinit_sdw_manager() as well,
> grouping both wake-arming calls together after the interrupt-disable
> sequence.
>
> Fixes: 86a4371b7697 ("soundwire: amd: fix for handling slave alerts after link is down")
> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>

[Severity: Low]
86a4371b7697 ("soundwire: amd: fix for handling slave alerts after link is
down") only added cancel_work_sync() calls to amd_suspend() and did not
touch amd_suspend_runtime() or wake interrupt masking.

The amd_sdw_wake_enable() call in the AMD_SDW_POWER_OFF_MODE branch of
amd_suspend_runtime() was placed before amd_deinit_sdw_manager() by
dcc48a73eae7 ("soundwire: amd: change the soundwire wake enable/disable
sequence").  Should the Fixes: tag point to that commit instead:

  Fixes: dcc48a73eae7 ("soundwire: amd: change the soundwire wake enable/disable sequence")

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107154659.3358223-1-Vijendar.Mukunda@amd.com?part=5

[PATCH v5 06/25] soundwire: amd: fix IRQ thread dropping STATUS_0TO7 on simultaneous wake

[Vijendar Mukunda]

When a wake interrupt and a device-state change arrived simultaneously,
amd_sdw_irq_thread() returned early after the wake path, silently
discarding the STATUS_0TO7 bits.

Remove the early return and fall through to the normal slave-status
processing path. Clear the wake bit from the local status_change_8to11
copy before proceeding so the remaining status checks are not confused
by the wake bit.

Fixes: 66c878838f37 ("soundwire: amd: handle SoundWire wake enable interrupt")
Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/soundwire/amd_manager.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index 3d60bea82792..65626ca590c9 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -1110,8 +1110,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;
+	}
 
 	if (status_change_8to11 & AMD_SDW_PREQ_INTR_STAT) {
 		amd_sdw_read_and_process_ping_status(amd_manager);
-- 
2.53.0

[PATCH v5 07/25] soundwire: amd: remove STATUS_8TO11 clear from amd_sdw_process_wake_event()

[Vijendar Mukunda]

amd_sdw_irq_thread() falls through after handling a wake event to
process any coalesced PREQ or peripheral status events, rather than
returning early. With the fall-through in place, both
amd_sdw_process_wake_event() and the tail of amd_sdw_irq_thread() write
0x00 to ACP_SW_STATE_CHANGE_STATUS_8TO11 in the same invocation.
Any status bit asserted by hardware between those two writes would be
lost before the next invocation reads the register.

Remove the writel() from amd_sdw_process_wake_event() so that
ACP_SW_STATE_CHANGE_STATUS_8TO11 is cleared exactly once per invocation
by the tail of amd_sdw_irq_thread().

Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
Fixes: 66c878838f37 ("soundwire: amd: handle SoundWire wake enable interrupt")
---
 drivers/soundwire/amd_manager.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index 65626ca590c9..438cf52a212c 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -1093,7 +1093,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);
 	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)
-- 
2.53.0

[PATCH v5 08/25] soundwire: amd: fix missing wake interrupt for ACP6.3 in POWER_OFF suspend

[Vijendar Mukunda]

On some OEM platforms based on ACP 6.3, with a SoundWire topology
where the UAJ peripheral is on one instance and the speaker peripheral
is on the other, wake events from the UAJ instance are silently lost
every time it enters POWER_OFF runtime suspend (D3) while speaker
playback keeps ACP in D0.

On ACP6.3, unlike ACP7.0/7.1/7.2, there is no dedicated host wake
interrupt path (no ACP_EXTERNAL_INTR_CNTL1 host wake mask bits and no
PME wake mechanism). The wake event from a suspended SoundWire instance
is delivered solely through the standard SoundWire interrupt bit in
ACP_EXTERNAL_INTR_CNTL.

In the POWER_OFF_MODE runtime suspend path, amd_deinit_sdw_manager()
calls amd_disable_sdw_interrupts() which clears the SoundWire instance
bit in ACP_EXTERNAL_INTR_CNTL. After this, the wake interrupt has no
delivery path to the CPU.

Fix this by re-enabling the SoundWire instance bit in
ACP_EXTERNAL_INTR_CNTL after amd_deinit_sdw_manager() and
amd_sdw_wake_enable() complete, when acp_rev is ACP63_PCI_REV_ID and
wake_en_mask is set. The acp_sdw_lock mutex is held around the
read-modify-write to prevent a race when both instances suspend or
resume concurrently.

Fixes: 86a4371b7697 ("soundwire: amd: fix for handling slave alerts after link is down")
Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/soundwire/amd_manager.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index 438cf52a212c..3f3fffbd529a 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -1467,6 +1467,13 @@ static int __maybe_unused amd_suspend_runtime(struct device *dev)
 			ret = amd_sdw_host_wake_enable(amd_manager, true);
 			if (ret)
 				return ret;
+		} else if ((amd_manager->acp_rev == ACP63_PCI_REV_ID) &&
+			   amd_manager->wake_en_mask) {
+			mutex_lock(amd_manager->acp_sdw_lock);
+			val = sdw_manager_reg_mask_array[amd_manager->instance];
+			amd_updatel(amd_manager->acp_mmio,
+				    ACP_EXTERNAL_INTR_CNTL(amd_manager->instance), val, val);
+			mutex_unlock(amd_manager->acp_sdw_lock);
 		}
 		/*
 		 * For ACP 7.0/7.1/7.2: disable OE only; keeper and schmen remain
-- 
2.53.0

Re: [PATCH v5 08/25] soundwire: amd: fix missing wake interrupt for ACP6.3 in POWER_OFF suspend

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] soundwire: amd: inaccurate Fixes: trailer in commit message
--

commit e0001eef73595d8c30441824baf09c46bbaf4fea
Author: Vijendar Mukunda <Vijendar.Mukunda@amd.com>

> soundwire: amd: fix missing wake interrupt for ACP6.3 in POWER_OFF suspend
>
> On some OEM platforms based on ACP 6.3, with a SoundWire topology
> where the UAJ peripheral is on one instance and the speaker peripheral
> is on the other, wake events from the UAJ instance are silently lost
> every time it enters POWER_OFF runtime suspend (D3) while speaker
> playback keeps ACP in D0.
>
> On ACP6.3, unlike ACP7.0/7.1/7.2, there is no dedicated host wake
> interrupt path (no ACP_EXTERNAL_INTR_CNTL1 host wake mask bits and no
> PME wake mechanism). The wake event from a suspended SoundWire instance
> is delivered solely through the standard SoundWire interrupt bit in
> ACP_EXTERNAL_INTR_CNTL.
>
> In the POWER_OFF_MODE runtime suspend path, amd_deinit_sdw_manager()
> calls amd_disable_sdw_interrupts() which clears the SoundWire instance
> bit in ACP_EXTERNAL_INTR_CNTL. After this, the wake interrupt has no
> delivery path to the CPU.
>
> Fix this by re-enabling the SoundWire instance bit in
> ACP_EXTERNAL_INTR_CNTL after amd_deinit_sdw_manager() and
> amd_sdw_wake_enable() complete, when acp_rev is ACP63_PCI_REV_ID and
> wake_en_mask is set. The acp_sdw_lock mutex is held around the
> read-modify-write to prevent a race when both instances suspend or
> resume concurrently.
>
> Fixes: 86a4371b7697 ("soundwire: amd: fix for handling slave alerts after link is down")
> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>

[Severity: Low]
Does the Fixes: tag point to the right commit?  86a4371b7697 ("soundwire:
amd: fix for handling slave alerts after link is down") only added
cancel_work_sync() in amd_suspend() and did not touch
amd_suspend_runtime() or wake interrupt handling.

The AMD_SDW_POWER_OFF_MODE runtime suspend path calling
amd_deinit_sdw_manager() was introduced in 81ff58ff71ad ("soundwire: amd:
add runtime pm ops for AMD SoundWire manager driver"), and wake enablement
for that path was added in dcc48a73eae7 ("soundwire: amd: change the
soundwire wake enable/disable sequence").

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107154659.3358223-1-Vijendar.Mukunda@amd.com?part=8

[PATCH v5 09/25] soundwire: amd: fix SDW command timeout return value handling

[Vijendar Mukunda]

amd_sdw_send_cmd_get_resp() returned u64 but was returning -ETIMEDOUT
(a negative int) on readl_poll_timeout() failures, which silently
widens to a large u64 value.

Change the return type to int and pass the raw u64 response through an
out-parameter. Timeout is detected directly from readl_poll_timeout()
return value.

Update all callers: amd_program_scp_addr(), _amd_sdw_xfer_msg(),
amd_sdw_read_and_process_ping_status(), and amd_sdw_read_ping_status()
to check the return value and handle timeout explicitly.

Fixes: d8f48fbdfd9a ("soundwire: amd: Add support for AMD Manager driver")
Reported-by: Lee Jones <lee@kernel.org>
Closes: https://sashiko.dev/#/bug/linux-c3a53ed5-eb8f-4982-a3b6-db189e009b68
Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/soundwire/amd_manager.c | 54 +++++++++++++++++++--------------
 1 file changed, 32 insertions(+), 22 deletions(-)

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index 3f3fffbd529a..f2ed0490aa82 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -348,10 +348,9 @@ static void amd_sdw_ctl_word_prep(u32 *lower_word, u32 *upper_word, struct sdw_m
 	*lower_word = lower_data;
 }
 
-static u64 amd_sdw_send_cmd_get_resp(struct amd_sdw_manager *amd_manager, u32 lower_data,
-				     u32 upper_data)
+static int amd_sdw_send_cmd_get_resp(struct amd_sdw_manager *amd_manager, u32 lower_data,
+				     u32 upper_data, u64 *response)
 {
-	u64 resp;
 	u32 lower_resp, upper_resp;
 	u32 sts;
 	int ret;
@@ -389,9 +388,8 @@ static u64 amd_sdw_send_cmd_get_resp(struct amd_sdw_manager *amd_manager, u32 lo
 			amd_manager->instance);
 		return ret;
 	}
-	resp = upper_resp;
-	resp = (resp << 32) | lower_resp;
-	return resp;
+	*response = ((u64)upper_resp << 32) | lower_resp;
+	return 0;
 }
 
 static enum sdw_command_response
@@ -407,19 +405,22 @@ amd_program_scp_addr(struct amd_sdw_manager *amd_manager, struct sdw_msg *msg)
 	scp_msg.buf = &msg->addr_page1;
 	scp_msg.flags = SDW_MSG_FLAG_WRITE;
 	amd_sdw_ctl_word_prep(&lower_data, &upper_data, &scp_msg, 0);
-	response_buf[0] = amd_sdw_send_cmd_get_resp(amd_manager, lower_data, upper_data);
+	if (amd_sdw_send_cmd_get_resp(amd_manager, lower_data, upper_data, &response_buf[0])) {
+		dev_err_ratelimited(amd_manager->dev,
+				    "SCP_addrpage command timeout for Slave %d\n", msg->dev_num);
+		return SDW_CMD_TIMEOUT;
+	}
 	scp_msg.addr = SDW_SCP_ADDRPAGE2;
 	scp_msg.buf = &msg->addr_page2;
 	amd_sdw_ctl_word_prep(&lower_data, &upper_data, &scp_msg, 0);
-	response_buf[1] = amd_sdw_send_cmd_get_resp(amd_manager, lower_data, upper_data);
+	if (amd_sdw_send_cmd_get_resp(amd_manager, lower_data, upper_data, &response_buf[1])) {
+		dev_err_ratelimited(amd_manager->dev,
+				    "SCP_addrpage command timeout for Slave %d\n", msg->dev_num);
+		return SDW_CMD_TIMEOUT;
+	}
 
 	for (index = 0; index < 2; index++) {
-		if (response_buf[index] == -ETIMEDOUT) {
-			dev_err_ratelimited(amd_manager->dev,
-					    "SCP_addrpage command timeout for Slave %d\n",
-					    msg->dev_num);
-			return SDW_CMD_TIMEOUT;
-		} else if (!(response_buf[index] & AMD_SDW_MCP_RESP_ACK)) {
+		if (!(response_buf[index] & AMD_SDW_MCP_RESP_ACK)) {
 			if (response_buf[index] & AMD_SDW_MCP_RESP_NACK) {
 				dev_err_ratelimited(amd_manager->dev,
 						    "SCP_addrpage NACKed for Slave %d\n",
@@ -464,11 +465,7 @@ static enum sdw_command_response amd_sdw_fill_msg_resp(struct amd_sdw_manager *a
 		if (msg->flags == SDW_MSG_FLAG_READ)
 			msg->buf[offset] = FIELD_GET(AMD_SDW_MCP_RESP_RDATA, response);
 	} else {
-		if (response == -ETIMEDOUT) {
-			dev_err_ratelimited(amd_manager->dev, "command timeout for Slave %d\n",
-					    msg->dev_num);
-			return SDW_CMD_TIMEOUT;
-		} else if (response & AMD_SDW_MCP_RESP_NACK) {
+		if (response & AMD_SDW_MCP_RESP_NACK) {
 			dev_err_ratelimited(amd_manager->dev,
 					    "command response NACK received for Slave %d\n",
 					    msg->dev_num);
@@ -488,7 +485,11 @@ static unsigned int _amd_sdw_xfer_msg(struct amd_sdw_manager *amd_manager, struc
 	u32 upper_data = 0, lower_data = 0;
 
 	amd_sdw_ctl_word_prep(&lower_data, &upper_data, msg, cmd_offset);
-	response = amd_sdw_send_cmd_get_resp(amd_manager, lower_data, upper_data);
+	if (amd_sdw_send_cmd_get_resp(amd_manager, lower_data, upper_data, &response)) {
+		dev_err_ratelimited(amd_manager->dev, "command timeout for Slave %d\n",
+				    msg->dev_num);
+		return SDW_CMD_TIMEOUT;
+	}
 	return amd_sdw_fill_msg_resp(amd_manager, msg, response, cmd_offset);
 }
 
@@ -544,7 +545,12 @@ static void amd_sdw_read_and_process_ping_status(struct amd_sdw_manager *amd_man
 	u64 response;
 
 	mutex_lock(&amd_manager->bus.msg_lock);
-	response = amd_sdw_send_cmd_get_resp(amd_manager, 0, 0);
+	if (amd_sdw_send_cmd_get_resp(amd_manager, 0, 0, &response)) {
+		mutex_unlock(&amd_manager->bus.msg_lock);
+		dev_err_ratelimited(amd_manager->dev, "SDW%x ping status timeout\n",
+				    amd_manager->instance);
+		return;
+	}
 	mutex_unlock(&amd_manager->bus.msg_lock);
 	amd_sdw_process_ping_status(response, amd_manager);
 }
@@ -555,7 +561,11 @@ static u32 amd_sdw_read_ping_status(struct sdw_bus *bus)
 	u64 response;
 	u32 slave_stat;
 
-	response = amd_sdw_send_cmd_get_resp(amd_manager, 0, 0);
+	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;
+	}
 	/* 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;
-- 
2.53.0

[PATCH v5 10/25] soundwire: amd: return response on IMM status cleanup timeout

[Vijendar Mukunda]

amd_sdw_send_cmd_get_resp() reads the hardware response after
IMM_RES_VALID is set, then writes IMM_RES_VALID to acknowledge and
polls until the bit clears (status cleanup handshake).

When the cleanup poll timed out, the code returned the error without
writing *response, causing callers to report SDW_CMD_TIMEOUT and
discard a valid response - upper_resp and lower_resp had already been
read before the cleanup write.

The cleanup timeout indicates the hardware was slow to acknowledge
the clear of IMM_RES_VALID, not that the command exchange failed.
Log the anomaly at warn level, then write *response and return 0 so
callers receive the valid data. The stuck IMM_RES_VALID bit will be
cleared at the next invocation by the existing bad-state guard,
provided IMM_CMD_BUSY is not also stuck at that point.

Fixes: d8f48fbdfd9a ("soundwire: amd: Add support for AMD Manager driver")
Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/soundwire/amd_manager.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index f2ed0490aa82..579b12946c0c 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -384,9 +384,12 @@ static int amd_sdw_send_cmd_get_resp(struct amd_sdw_manager *amd_manager, u32 lo
 	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) {
-		dev_err(amd_manager->dev, "SDW%x cmd status retry failed\n",
-			amd_manager->instance);
-		return ret;
+		/* Response already latched above; timeout here is non-fatal.
+		 * The residual IMM_RES_VALID bit will be cleared by the bad-state
+		 * guard on the next invocation, provided IMM_CMD_BUSY is not also stuck.
+		 */
+		dev_warn(amd_manager->dev, "SDW%x cmd status retry failed\n",
+			 amd_manager->instance);
 	}
 	*response = ((u64)upper_resp << 32) | lower_resp;
 	return 0;
-- 
2.53.0

Re: [PATCH v5 10/25] soundwire: amd: return response on IMM status cleanup timeout

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] soundwire: amd: unpolled IMM_RES_VALID clear in
  amd_sdw_send_cmd_get_resp()
--

commit 89df36f0150dc3c96912a2aa097eb2d1d5dc0e6d
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 a warning, write
*response, and return 0 when polling for AMD_SDW_IMM_RES_VALID to clear
times out after reading the response registers.

> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
> index f2ed0490aa82d..579b12946c0ca 100644
> --- a/drivers/soundwire/amd_manager.c
> +++ b/drivers/soundwire/amd_manager.c
> @@ -384,9 +384,12 @@ static int amd_sdw_send_cmd_get_resp(struct amd_sdw_manager *amd_manager, u32 lo
>  	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) {
> -		dev_err(amd_manager->dev, "SDW%x cmd status retry failed\n",
> -			amd_manager->instance);
> -		return ret;
> +		/* Response already latched above; timeout here is non-fatal.
> +		 * The residual IMM_RES_VALID bit will be cleared by the bad-state
> +		 * guard on the next invocation, provided IMM_CMD_BUSY is not also stuck.
> +		 */
> +		dev_warn(amd_manager->dev, "SDW%x cmd status retry failed\n",
> +			 amd_manager->instance);
>  	}
>  	*response = ((u64)upper_resp << 32) | lower_resp;
>  	return 0;

[Severity: High]
Can returning 0 when AMD_SDW_IMM_RES_VALID fails to clear cause the next
command to read stale response registers from ACP_SW_IMM_RESP_UPPER_WORD
and ACP_SW_IMM_RESP_LOWER_QWORD?

When a caller such as amd_sdw_xfer_msg() issues back-to-back commands, the
next call to amd_sdw_send_cmd_get_resp() hits the bad-state guard, which
writes AMD_SDW_IMM_RES_VALID without polling for it to clear before
issuing the new command and polling for (sts & AMD_SDW_IMM_RES_VALID):

drivers/soundwire/amd_manager.c:amd_sdw_send_cmd_get_resp() {
    ...
	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 readl_poll_timeout() reads ACP_SW_IMM_CMD_STS immediately before
sleeping and the clear handshake is asynchronous, the residual
AMD_SDW_IMM_RES_VALID bit can still be set on the first read, causing
amd_sdw_send_cmd_get_resp() to return the previous command's response
before the new SoundWire frame finishes on the bus.  Follow-up commit
"soundwire: amd: clarify bad-state guard poll omission in IMM_RES_VALID
path" documents the missing poll in the bad-state guard, but does not
prevent the subsequent response poll from matching the un-cleared bit.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107154659.3358223-1-Vijendar.Mukunda@amd.com?part=10

[PATCH v5 11/25] soundwire: amd: clarify bad-state guard poll omission in IMM_RES_VALID path

[Vijendar Mukunda]

The bad-state guard clears IMM_RES_VALID with a single write without
polling for the bit to clear before issuing the next command. Add a
comment to explain that the poll is skipped as an optimistic recovery
step: the driver proceeds to issue a fresh command regardless, which
will itself produce a new IMM_RES_VALID event.

Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/soundwire/amd_manager.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index 579b12946c0c..d8b533875b9a 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -365,6 +365,10 @@ static int amd_sdw_send_cmd_get_resp(struct amd_sdw_manager *amd_manager, u32 lo
 
 	if (sts & AMD_SDW_IMM_RES_VALID) {
 		dev_err(amd_manager->dev, "SDW%x manager is in bad state\n", amd_manager->instance);
+		/*
+		 * Skip poll: proceed regardless; the next command will
+		 * generate a fresh IMM_RES_VALID event.
+		 */
 		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);
-- 
2.53.0

[PATCH v5 12/25] soundwire: amd: cache ping slave status to avoid spurious disconnect on timeout

[Vijendar Mukunda]

amd_sdw_read_ping_status() returned 0 on a PING command timeout.
This was indistinguishable from a valid "no peripherals attached"
response, causing sdw_show_ping_status() to log a spurious disconnect
warning even when devices were actually present.

Add ping_slave_stat to struct amd_sdw_manager to cache the slave_stat
from the last successful ping response. On timeout, the cached value is
returned instead of 0, so sdw_show_ping_status() sees the last known
good state. The timeout itself is still reported via
dev_err_ratelimited() - the cache does not suppress the error.

ping_slave_stat is initialised to U32_MAX in probe and reset to
U32_MAX under msg_lock before any failable step in the POWER_OFF_MODE
resume path, so a partially-failed resume cannot leave stale
pre-suspend data behind for a subsequent ping timeout to report.

Fixes: d8f48fbdfd9a ("soundwire: amd: Add support for AMD Manager driver")
Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/soundwire/amd_manager.c   | 18 +++++++++++++++++-
 include/linux/soundwire/sdw_amd.h |  4 ++++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index d8b533875b9a..ebd3787ee6c8 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -558,6 +558,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);
 	mutex_unlock(&amd_manager->bus.msg_lock);
 	amd_sdw_process_ping_status(response, amd_manager);
 }
@@ -571,12 +574,13 @@ static u32 amd_sdw_read_ping_status(struct sdw_bus *bus)
 	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;
 }
 
@@ -1194,6 +1198,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);
@@ -1546,6 +1551,17 @@ static int __maybe_unused amd_resume_runtime(struct device *dev)
 			amd_sdw_oe_ctrl(amd_manager, true);
 		}
 		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 before any failable step 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 c959d0fe2a39..2d26f357d842 100644
--- a/include/linux/soundwire/sdw_amd.h
+++ b/include/linux/soundwire/sdw_amd.h
@@ -82,6 +82,9 @@ struct sdw_amd_dai_runtime {
  *   only OE was disabled; in the latter case amd_resume_runtime() restores
  *   OE only, leaving keeper and schmen untouched.
  * @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 {
@@ -111,6 +114,7 @@ struct amd_sdw_manager {
 	u32 wake_en_mask;
 	u32 power_mode_mask;
 	u32 acp_rev;
+	u32 ping_slave_stat;
 	bool clk_stopped;
 	bool pad_fully_torn_down;
 
-- 
2.53.0

[PATCH v5 13/25] soundwire: amd: assert msg_lock is held in amd_sdw_read_ping_status()

[Vijendar Mukunda]

amd_sdw_read_ping_status() is always called from sdw_show_ping_status()
which holds bus->msg_lock. Add lockdep_assert_held() to document this
locking requirement and catch any future callers that omit the lock.

Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/soundwire/amd_manager.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index ebd3787ee6c8..767bc5510dfe 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -571,6 +571,7 @@ static u32 amd_sdw_read_ping_status(struct sdw_bus *bus)
 	u64 response;
 	u32 slave_stat;
 
+	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);
-- 
2.53.0

[PATCH v5 14/25] soundwire: amd: fix work drain ordering in remove path

[Vijendar Mukunda]

amd_sdw_manager_remove() cancelled amd_sdw_work but not
amd_sdw_irq_thread. Since amd_sdw_irq_thread() calls
schedule_work(&amd_sdw_work), an in-flight irq_thread item can
re-queue amd_sdw_work after its cancel returns, defeating the
cancellation.

Fix by calling amd_disable_sdw_interrupts() first to quiesce the
hardware IRQ source, then cancel_work_sync() for amd_sdw_irq_thread,
then cancel_work_sync() for amd_sdw_work. The existing
cancel_work_sync(amd_sdw_work) is also moved to after
amd_disable_sdw_interrupts() so that any work item queued between the
old cancel position and the interrupt disable cannot escape draining.

synchronize_irq() is deliberately not used before the
cancel_work_sync() calls. Once SoundWire interrupts are masked, no new
IRQ deliveries can occur. An IRQ handler already in flight may still
queue amd_sdw_irq_thread, so cancel_work_sync() is used to drain both
amd_sdw_irq_thread and any amd_sdw_work items it may have scheduled.
This fully quiesces the driver workqueues, making synchronize_irq()
unnecessary.

Fixes: f93b697ed98e ("soundwire: amd: cancel pending slave status handling workqueue during remove sequence")
Reported-by: Lee Jones <lee@kernel.org>
Closes: https://sashiko.dev/#/bug/linux-9890e8dc-4b7d-44d3-bfb0-171dc421e28c
Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/soundwire/amd_manager.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index 767bc5510dfe..4083a3f0ec2f 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -1278,8 +1278,9 @@ static void amd_sdw_manager_remove(struct platform_device *pdev)
 	int ret;
 
 	pm_runtime_disable(&pdev->dev);
-	cancel_work_sync(&amd_manager->amd_sdw_work);
 	amd_disable_sdw_interrupts(amd_manager);
+	cancel_work_sync(&amd_manager->amd_sdw_irq_thread);
+	cancel_work_sync(&amd_manager->amd_sdw_work);
 	sdw_bus_master_delete(&amd_manager->bus);
 	ret = amd_disable_sdw_manager(amd_manager);
 	if (ret)
-- 
2.53.0

Re: [PATCH v5 14/25] soundwire: amd: fix work drain ordering in remove path

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] soundwire: amd: race with in-flight IRQ handler in
  amd_sdw_manager_remove()
--

commit 96eb0e85d023d4d06d5fe26f4e91328e3104ebef
Author: Vijendar Mukunda <Vijendar.Mukunda@amd.com>

soundwire: amd: fix work drain ordering in remove path

This commit reorders work cancellation in amd_sdw_manager_remove() to disable
SoundWire interrupts first and then cancel amd_sdw_irq_thread and amd_sdw_work.

> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
> index 767bc5510dfef..4083a3f0ec2f8 100644
> --- a/drivers/soundwire/amd_manager.c
> +++ b/drivers/soundwire/amd_manager.c
> @@ -1278,8 +1278,9 @@ static void amd_sdw_manager_remove(struct platform_device *pdev)
>  	int ret;
>  
>  	pm_runtime_disable(&pdev->dev);
> -	cancel_work_sync(&amd_manager->amd_sdw_work);
>  	amd_disable_sdw_interrupts(amd_manager);
> +	cancel_work_sync(&amd_manager->amd_sdw_irq_thread);
> +	cancel_work_sync(&amd_manager->amd_sdw_work);
>  	sdw_bus_master_delete(&amd_manager->bus);
>  	ret = amd_disable_sdw_manager(amd_manager);
>  	if (ret)

[Severity: High]
Can an in-flight parent ACP interrupt handler still queue amd_sdw_irq_thread
after cancel_work_sync() returns?

Unlike disable_work_sync(), cancel_work_sync() only drains work that is
already queued or running and leaves the work item enabled before returning.
Without synchronize_irq() or disable_work_sync(), an interrupt asserted just
before amd_disable_sdw_interrupts() masks ACP_EXTERNAL_INTR_CNTL can already
be running acp63_irq_handler() on another CPU:

sound/soc/amd/ps/pci-ps.c:acp63_irq_handler() {
	...
	if (ext_intr_stat & ACP_SDW0_STAT) {
		writel(ACP_SDW0_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
		amd_manager = dev_get_drvdata(&adata->sdw->pdev[0]->dev);
		if (amd_manager)
			schedule_work(&amd_manager->amd_sdw_irq_thread);
	...
}

This allows schedule_work() to run after both cancel_work_sync() calls finish:

CPU0                                      CPU1 (<hardirq>)
amd_sdw_manager_remove()                  acp63_irq_handler()
                                            readl(... + ACP_EXTERNAL_INTR_STAT)
                                            amd_manager = dev_get_drvdata(...)
  amd_disable_sdw_interrupts()
  cancel_work_sync(&...->amd_sdw_irq_thread)
  cancel_work_sync(&...->amd_sdw_work)
                                            schedule_work(&...->amd_sdw_irq_thread)
  sdw_bus_master_delete(&amd_manager->bus)
  ...
  devres_release_all()
                                          amd_sdw_irq_thread()
                                            readl(amd_manager->mmio + ...)
                                              <-- use-after-free

Even with commit "soundwire: amd: use interrupt_enabled flag to guard IRQ
work scheduling" later in this series, interrupt_enabled is only checked
inside amd_sdw_irq_thread() before queueing amd_sdw_work, so the hardirq
handler still schedules amd_sdw_irq_thread unconditionally.

Also, because amd_disable_sdw_interrupts() now runs before
cancel_work_sync(&amd_manager->amd_sdw_work), an already running
amd_sdw_update_slave_status_work() with status[0] == SDW_SLAVE_ATTACHED can
write AMD_SDW_IRQ_MASK_0TO7 and AMD_SDW_IRQ_MASK_8TO11 back to
ACP_SW_STATE_CHANGE_STATUS_MASK_0TO7 and ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11,
overwriting the 0x00 masks written by amd_disable_sdw_interrupts().

Using disable_work_sync() instead of cancel_work_sync() (or synchronizing with
the parent IRQ after masking) and masking the state-change registers after the
work items are drained would prevent both races.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107154659.3358223-1-Vijendar.Mukunda@amd.com?part=14

Re: [PATCH v5 14/25] soundwire: amd: fix work drain ordering in remove path

[Mukunda,Vijendar]

On 11/7/26 21:44, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue to consider:
> - [High] soundwire: amd: race with in-flight IRQ handler in
>    amd_sdw_manager_remove()
> --
>
> commit 96eb0e85d023d4d06d5fe26f4e91328e3104ebef
> Author: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
>
> soundwire: amd: fix work drain ordering in remove path
>
> This commit reorders work cancellation in amd_sdw_manager_remove() to disable
> SoundWire interrupts first and then cancel amd_sdw_irq_thread and amd_sdw_work.
>
>> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
>> index 767bc5510dfef..4083a3f0ec2f8 100644
>> --- a/drivers/soundwire/amd_manager.c
>> +++ b/drivers/soundwire/amd_manager.c
>> @@ -1278,8 +1278,9 @@ static void amd_sdw_manager_remove(struct platform_device *pdev)
>>   	int ret;
>>   
>>   	pm_runtime_disable(&pdev->dev);
>> -	cancel_work_sync(&amd_manager->amd_sdw_work);
>>   	amd_disable_sdw_interrupts(amd_manager);
>> +	cancel_work_sync(&amd_manager->amd_sdw_irq_thread);
>> +	cancel_work_sync(&amd_manager->amd_sdw_work);
>>   	sdw_bus_master_delete(&amd_manager->bus);
>>   	ret = amd_disable_sdw_manager(amd_manager);
>>   	if (ret)
> [Severity: High]
> Can an in-flight parent ACP interrupt handler still queue amd_sdw_irq_thread
> after cancel_work_sync() returns?
>
> Unlike disable_work_sync(), cancel_work_sync() only drains work that is
> already queued or running and leaves the work item enabled before returning.
> Without synchronize_irq() or disable_work_sync(), an interrupt asserted just
> before amd_disable_sdw_interrupts() masks ACP_EXTERNAL_INTR_CNTL can already
> be running acp63_irq_handler() on another CPU:
>
> sound/soc/amd/ps/pci-ps.c:acp63_irq_handler() {
> 	...
> 	if (ext_intr_stat & ACP_SDW0_STAT) {
> 		writel(ACP_SDW0_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
> 		amd_manager = dev_get_drvdata(&adata->sdw->pdev[0]->dev);
> 		if (amd_manager)
> 			schedule_work(&amd_manager->amd_sdw_irq_thread);
> 	...
> }
The acp63_irq_handler() code shown above is the pre-patch state.
The companion ASoC patch posted to alsa-devel adds an
interrupt_enabled guard in acp63_irq_handler() before the
schedule_work() call:

   Link: 
https://lore.kernel.org/alsa-devel/20261107175339.3547918-2-Vijendar.Mukunda@amd.com

amd_disable_sdw_interrupts() clears interrupt_enabled = false before
writing to ACP_EXTERNAL_INTR_CNTL, so any in-flight IRQ handler
that reads the flag after that point sees false and skips
schedule_work(). The delivery gate (ACP_EXTERNAL_INTR_CNTL) then
prevents any further IRQ deliveries. cancel_work_sync() drains any
work that was queued before or during the disable, covering the
remaining in-flight window.
> This allows schedule_work() to run after both cancel_work_sync() calls finish:
>
> CPU0                                      CPU1 (<hardirq>)
> amd_sdw_manager_remove()                  acp63_irq_handler()
>                                              readl(... + ACP_EXTERNAL_INTR_STAT)
>                                              amd_manager = dev_get_drvdata(...)
>    amd_disable_sdw_interrupts()
>    cancel_work_sync(&...->amd_sdw_irq_thread)
>    cancel_work_sync(&...->amd_sdw_work)
>                                              schedule_work(&...->amd_sdw_irq_thread)
>    sdw_bus_master_delete(&amd_manager->bus)
>    ...
>    devres_release_all()
>                                            amd_sdw_irq_thread()
>                                              readl(amd_manager->mmio + ...)
>                                                <-- use-after-free
>
> Even with commit "soundwire: amd: use interrupt_enabled flag to guard IRQ
> work scheduling" later in this series, interrupt_enabled is only checked
> inside amd_sdw_irq_thread() before queueing amd_sdw_work, so the hardirq
> handler still schedules amd_sdw_irq_thread unconditionally.
>
> Also, because amd_disable_sdw_interrupts() now runs before
> cancel_work_sync(&amd_manager->amd_sdw_work), an already running
> amd_sdw_update_slave_status_work() with status[0] == SDW_SLAVE_ATTACHED can
> write AMD_SDW_IRQ_MASK_0TO7 and AMD_SDW_IRQ_MASK_8TO11 back to
> ACP_SW_STATE_CHANGE_STATUS_MASK_0TO7 and ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11,
> overwriting the 0x00 masks written by amd_disable_sdw_interrupts().
amd_disable_sdw_interrupts() clears ACP_EXTERNAL_INTR_CNTL, which
is the delivery gate that forwards the ACP IRQ line to the CPU.
Even if a concurrently running amd_sdw_work re-enables the
state-change mask registers (ACP_SW_STATE_CHANGE_STATUS_MASK_*),
no new SoundWire interrupt can reach the CPU while the delivery
gate is closed. The state-change masks only control which SoundWire
events set the pending-interrupt flag in the IP; ACP_EXTERNAL_INTR_CNTL
controls whether that signal is forwarded. With the gate cleared,
re-enabling the state-change masks has no effect on interrupt delivery.
> Using disable_work_sync() instead of cancel_work_sync() (or synchronizing with
> the parent IRQ after masking) and masking the state-change registers after the
> work items are drained would prevent both races.
disable_work_sync() is not used here because it permanently disables
the work item, preventing re-arming on resume. The ordering
(disable-gate → drain irq_thread → drain work) combined with the
interrupt_enabled guard in the companion ASoC patch fully quiesces
the workqueues without requiring synchronize_irq() or
disable_work_sync().
>

[PATCH v5 15/25] soundwire: amd: defer dev_set_drvdata() until after INIT_WORK()

[Vijendar Mukunda]

amd_sdw_manager_probe() called dev_set_drvdata() before initialising
the amd_sdw_irq_thread and amd_sdw_work structures with INIT_WORK().
An interrupt arriving in that window could call the IRQ handler which
enqueues amd_sdw_irq_thread before the work struct is initialised,
leading to undefined behaviour.

Move dev_set_drvdata() to after both INIT_WORK() calls to close the
race window.

Fixes: 65f93e4096a0 ("soundwire: amd: add SoundWire manager interrupt handling")
Reported-by: Lee Jones <lee@kernel.org>
Closes: https://sashiko.dev/#/bug/linux-9890e8dc-4b7d-44d3-bfb0-171dc421e28c
Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/soundwire/amd_manager.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index 4083a3f0ec2f..df2fdff18d3d 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -1266,9 +1266,9 @@ static int amd_sdw_manager_probe(struct platform_device *pdev)
 		sdw_bus_master_delete(&amd_manager->bus);
 		return ret;
 	}
-	dev_set_drvdata(dev, amd_manager);
 	INIT_WORK(&amd_manager->amd_sdw_irq_thread, amd_sdw_irq_thread);
 	INIT_WORK(&amd_manager->amd_sdw_work, amd_sdw_update_slave_status_work);
+	dev_set_drvdata(dev, amd_manager);
 	return 0;
 }
 
-- 
2.53.0

[PATCH v5 16/25] soundwire: amd: pm_runtime guard in remove path

[Vijendar Mukunda]

Guard pm_runtime_disable() so it is only called when runtime PM
was actually enabled. amd_sdw_manager_start() calls pm_runtime_enable()
only at the very end, after several fallible hardware init steps. If
sdw_amd_startup() fails mid-loop (one manager started, the next fails
before pm_runtime_enable()), sdw_amd_exit() triggers
platform_device_unregister() for all managers. Calling
pm_runtime_disable() on the partially-started manager finds
disable_depth already at its initial value of 1, silently increments it
to 2 and returns without a warning, so a later pm_runtime_enable() would
only bring it back to 1 and leave runtime PM disabled. Use
pm_runtime_enabled() to skip the call when it was never paired with an
enable.

Fixes: f93b697ed98e ("soundwire: amd: cancel pending slave status handling workqueue during remove sequence")
Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/soundwire/amd_manager.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index df2fdff18d3d..01aa051cf63f 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -1277,7 +1277,8 @@ static void amd_sdw_manager_remove(struct platform_device *pdev)
 	struct amd_sdw_manager *amd_manager = dev_get_drvdata(&pdev->dev);
 	int ret;
 
-	pm_runtime_disable(&pdev->dev);
+	if (pm_runtime_enabled(&pdev->dev))
+		pm_runtime_disable(&pdev->dev);
 	amd_disable_sdw_interrupts(amd_manager);
 	cancel_work_sync(&amd_manager->amd_sdw_irq_thread);
 	cancel_work_sync(&amd_manager->amd_sdw_work);
-- 
2.53.0

Re: [PATCH v5 16/25] soundwire: amd: pm_runtime guard in remove path

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] soundwire: amd: inaccurate Fixes tag in commit message for
  pm_runtime guard
--

commit 258769b3a1bdd79fbca74cfbf250109e984888bf
Author: Vijendar Mukunda <Vijendar.Mukunda@amd.com>

> soundwire: amd: pm_runtime guard in remove path
>
> Guard pm_runtime_disable() so it is only called when runtime PM
> was actually enabled. amd_sdw_manager_start() calls pm_runtime_enable()
> only at the very end, after several fallible hardware init steps. If
> sdw_amd_startup() fails mid-loop (one manager started, the next fails
> before pm_runtime_enable()), sdw_amd_exit() triggers
> platform_device_unregister() for all managers. Calling
> pm_runtime_disable() on the partially-started manager finds
> disable_depth already at its initial value of 1, silently increments it
> to 2 and returns without a warning, so a later pm_runtime_enable() would
> only bring it back to 1 and leave runtime PM disabled. Use
> pm_runtime_enabled() to skip the call when it was never paired with an
> enable.
>
> Fixes: f93b697ed98e ("soundwire: amd: cancel pending slave status handling workqueue during remove sequence")
> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>

[Severity: Low]
Is the Fixes tag pointing to the right commit?

Commit f93b697ed98e ("soundwire: amd: cancel pending slave status handling
workqueue during remove sequence") only added cancel_work_sync() to
amd_sdw_manager_remove().  The unconditional pm_runtime_disable() in
amd_sdw_manager_remove() was introduced in 81ff58ff71ad ("soundwire: amd: add
runtime pm ops for AMD SoundWire manager driver"), and the synchronous
sdw_amd_startup() / amd_sdw_manager_start() sequence described here was added
in ed5e8741b8db ("soundwire: amd: refactor amd soundwire manager device node
creation").

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107154659.3358223-1-Vijendar.Mukunda@amd.com?part=16

[PATCH v5 17/25] soundwire: amd: use interrupt_enabled flag to guard IRQ work scheduling

[Vijendar Mukunda]

Use a boolean interrupt_enabled flag to guard IRQ work scheduling and
prevent new work from being queued by an in-flight IRQ during the window
between starting to disable hardware interrupts and the masks taking
effect.

- Add interrupt_enabled to struct amd_sdw_manager.
- Set it to true at the end of amd_enable_sdw_interrupts(), after all
  hardware mask registers have been written.
- Clear it to false at the start of amd_disable_sdw_interrupts(), before
  touching any hardware registers, so that any IRQ that fires during the
  mask-register writes sees the flag as false and skips scheduling.
- Guard schedule_work(&amd_manager->amd_sdw_work) in
  amd_sdw_irq_thread() with the flag check.

Reported-by: Lee Jones <lee@kernel.org>
Closes: https://sashiko.dev/#/bug/linux-ab458b2e-f505-4a41-8238-a93ceb5d9c77
Fixes: 65f93e4096a0 ("soundwire: amd: add SoundWire manager interrupt handling")
Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/soundwire/amd_manager.c   | 12 +++++++++++-
 include/linux/soundwire/sdw_amd.h |  4 ++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index 01aa051cf63f..108553b0f32e 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -129,12 +129,22 @@ static void amd_enable_sdw_interrupts(struct amd_sdw_manager *amd_manager)
 	writel(AMD_SDW_IRQ_MASK_8TO11, amd_manager->mmio +
 		       ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11);
 	writel(AMD_SDW_IRQ_ERROR_MASK, amd_manager->mmio + ACP_SW_ERROR_INTR_MASK);
+	amd_manager->interrupt_enabled = true;
 }
 
 static void amd_disable_sdw_interrupts(struct amd_sdw_manager *amd_manager)
 {
 	u32 irq_mask;
 
+	/*
+	 * Clear the flag before updating hardware masks so that any IRQ
+	 * that fires during the mask-register writes sees the flag as false
+	 * and does not queue new work. Once the mask registers are written,
+	 * the ACP hardware will not assert any new SDW interrupt, so no
+	 * synchronize_irq() is required after this function returns.
+	 */
+	amd_manager->interrupt_enabled = false;
+
 	mutex_lock(amd_manager->acp_sdw_lock);
 	irq_mask = sdw_manager_reg_mask_array[amd_manager->instance];
 	amd_updatel(amd_manager->acp_mmio, ACP_EXTERNAL_INTR_CNTL(amd_manager->instance),
@@ -1143,7 +1153,7 @@ static void amd_sdw_irq_thread(struct work_struct *work)
 		/* Check for the updated status on peripheral device */
 		amd_sdw_update_slave_status(status_change_0to7, status_change_8to11, amd_manager);
 	}
-	if (status_change_8to11 || status_change_0to7)
+	if ((status_change_8to11 || status_change_0to7) && amd_manager->interrupt_enabled)
 		schedule_work(&amd_manager->amd_sdw_work);
 	writel(0x00, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_8TO11);
 	writel(0x00, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_0TO7);
diff --git a/include/linux/soundwire/sdw_amd.h b/include/linux/soundwire/sdw_amd.h
index 2d26f357d842..04b95379a724 100644
--- a/include/linux/soundwire/sdw_amd.h
+++ b/include/linux/soundwire/sdw_amd.h
@@ -85,6 +85,9 @@ struct sdw_amd_dai_runtime {
  * @ping_slave_stat: cached slave status from the last ping
  *		     transaction that returned a response;
  *		     initialised to U32_MAX ("no valid reading yet")
+ * @interrupt_enabled: set when hardware interrupts are enabled; cleared
+ *		     before disabling to prevent new work from being queued
+ *		     by an in-flight IRQ during teardown
  * @dai_runtime_array: dai runtime array
  */
 struct amd_sdw_manager {
@@ -117,6 +120,7 @@ struct amd_sdw_manager {
 	u32 ping_slave_stat;
 	bool clk_stopped;
 	bool pad_fully_torn_down;
+	bool interrupt_enabled;
 
 	struct sdw_amd_dai_runtime **dai_runtime_array;
 };
-- 
2.53.0

Re: [PATCH v5 17/25] soundwire: amd: use interrupt_enabled flag to guard IRQ work scheduling

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] soundwire: amd: lost IRQ in amd_enable_sdw_interrupts() due
  to late flag
- [Medium] soundwire: amd: unguarded IRQ work queueing in
  amd_disable_sdw_interrupts()
- [Medium] soundwire: amd: race re-enabling IRQ masks in
  amd_disable_sdw_interrupts()
--

commit c646d5fedeac998edd11296e03517aec14fbed88
Author: Vijendar Mukunda <Vijendar.Mukunda@amd.com>

soundwire: amd: use interrupt_enabled flag to guard IRQ work scheduling

This commit adds an interrupt_enabled boolean flag to struct
amd_sdw_manager to guard scheduling of amd_sdw_work when interrupts are
being disabled.

> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
> index 01aa051cf63fa..108553b0f32e8 100644
> --- a/drivers/soundwire/amd_manager.c
> +++ b/drivers/soundwire/amd_manager.c
> @@ -129,12 +129,22 @@ static void amd_enable_sdw_interrupts(struct amd_sdw_manager *amd_manager)
>  	writel(AMD_SDW_IRQ_MASK_8TO11, amd_manager->mmio +
>  		       ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11);
>  	writel(AMD_SDW_IRQ_ERROR_MASK, amd_manager->mmio + ACP_SW_ERROR_INTR_MASK);
> +	amd_manager->interrupt_enabled = true;
>  }

[Severity: Medium]
Can setting amd_manager->interrupt_enabled after unmasking the hardware
registers drop an initial peripheral status-change interrupt?

amd_init_sdw_manager() has already enabled the bus before
amd_enable_sdw_interrupts() runs, so a peripheral state-change interrupt
can fire as soon as the mask registers are written:

CPU0                                     CPU1 (kworker)
amd_enable_sdw_interrupts()
  amd_updatel(..., ACP_EXTERNAL_INTR_CNTL, ...)
  writel(..., ACP_SW_STATE_CHANGE_STATUS_MASK_0TO7)
  <IRQ fires, schedules amd_sdw_irq_thread>
                                         amd_sdw_irq_thread()
                                           status_change_0to7 = readl(...)
                                           amd_sdw_update_slave_status(...)
                                           if (... && amd_manager->interrupt_enabled)
                                             /* false: skips amd_sdw_work */
                                           writel(0x00, ..._STATUS_0TO7)
                                             <-- event lost
  amd_manager->interrupt_enabled = true

Because amd_sdw_irq_thread() skips schedule_work(&amd_manager->amd_sdw_work)
when interrupt_enabled is false but still clears
ACP_SW_STATE_CHANGE_STATUS_8TO11 and ACP_SW_STATE_CHANGE_STATUS_0TO7 to
0x00, the peripheral attachment or alert is permanently lost.  Setting
amd_manager->interrupt_enabled = true before unmasking the hardware
registers avoids this race.

>  static void amd_disable_sdw_interrupts(struct amd_sdw_manager *amd_manager)
>  {
>  	u32 irq_mask;
>  
> +	/*
> +	 * Clear the flag before updating hardware masks so that any IRQ
> +	 * that fires during the mask-register writes sees the flag as false
> +	 * and does not queue new work. Once the mask registers are written,
> +	 * the ACP hardware will not assert any new SDW interrupt, so no
> +	 * synchronize_irq() is required after this function returns.
> +	 */
> +	amd_manager->interrupt_enabled = false;
> +
>  	mutex_lock(amd_manager->acp_sdw_lock);
>  	irq_mask = sdw_manager_reg_mask_array[amd_manager->instance];
>  	amd_updatel(amd_manager->acp_mmio, ACP_EXTERNAL_INTR_CNTL(amd_manager->instance),

[Severity: Medium]
Does clearing amd_manager->interrupt_enabled and zeroing the mask
registers before cancel_work_sync(&amd_manager->amd_sdw_work) allow
amd_sdw_update_slave_status_work() to re-enable the status masks?

drivers/soundwire/amd_manager.c:amd_sdw_update_slave_status_work() {
    ...
	if (amd_manager->status[0] == SDW_SLAVE_ATTACHED) {
		if (retry_count++ < SDW_MAX_DEVICES) {
			writel(AMD_SDW_IRQ_MASK_0TO7, amd_manager->mmio +
			       ACP_SW_STATE_CHANGE_STATUS_MASK_0TO7);
			writel(AMD_SDW_IRQ_MASK_8TO11, amd_manager->mmio +
			       ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11);
    ...
}

CPU0                                     CPU1 (kworker)
                                         amd_sdw_update_slave_status_work()
                                           sdw_handle_slave_status(...)
amd_disable_sdw_interrupts()
  amd_manager->interrupt_enabled = false
  writel(0x00, ..._STATUS_MASK_0TO7)
  writel(0x00, ..._STATUS_MASK_8TO11)
                                           writel(AMD_SDW_IRQ_MASK_0TO7, ...)
                                           writel(AMD_SDW_IRQ_MASK_8TO11, ...)
                                             <-- masks re-enabled
cancel_work_sync(&amd_manager->amd_sdw_work)

During runtime suspend in AMD_SDW_POWER_OFF_MODE, amd_sdw_wake_enable()
then performs a read-modify-write on ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11
while ACP_EXTERNAL_INTR_CNTL is re-enabled for wake on ACP 6.3, leaving
non-wake state-change interrupts unmasked during suspend.

> @@ -1143,7 +1153,7 @@ static void amd_sdw_irq_thread(struct work_struct *work)
>  		/* Check for the updated status on peripheral device */
>  		amd_sdw_update_slave_status(status_change_0to7, status_change_8to11, amd_manager);
>  	}
> -	if (status_change_8to11 || status_change_0to7)
> +	if ((status_change_8to11 || status_change_0to7) && amd_manager->interrupt_enabled)
>  		schedule_work(&amd_manager->amd_sdw_work);
>  	writel(0x00, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_8TO11);
>  	writel(0x00, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_0TO7);

[Severity: Medium]
Does guarding only schedule_work(&amd_manager->amd_sdw_work) inside
amd_sdw_irq_thread() still leave amd_sdw_irq_thread itself unguarded
against an in-flight hard IRQ handler?

amd_sdw_irq_thread() is a work_struct callback rather than a threaded IRQ
handler.  The actual hard IRQ handlers (such as acp63_irq_handler() in
sound/soc/amd/ps/pci-ps.c and acp_irq_handler() in
sound/soc/sof/amd/acp.c) schedule amd_sdw_irq_thread without checking
amd_manager->interrupt_enabled:

sound/soc/amd/ps/pci-ps.c:acp63_irq_handler() {
    ...
	if (ext_intr_stat & ACP_SDW0_STAT) {
		writel(ACP_SDW0_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
		amd_manager = dev_get_drvdata(&adata->sdw->pdev[0]->dev);
		if (amd_manager)
			schedule_work(&amd_manager->amd_sdw_irq_thread);
    ...
}

Without synchronize_irq() or a check before schedule_work() in the hard
IRQ handler, an IRQ that fires just before ACP_EXTERNAL_INTR_CNTL is
masked can queue amd_sdw_irq_thread after cancel_work_sync() returns:

CPU0                                     CPU1 (<hardirq>)
                                         acp63_irq_handler()
                                           ext_intr_stat & ACP_SDW0_STAT
amd_sdw_manager_remove()
  amd_disable_sdw_interrupts()
    amd_manager->interrupt_enabled = false
    amd_updatel(..., ACP_EXTERNAL_INTR_CNTL, ...)
  cancel_work_sync(&amd_manager->amd_sdw_irq_thread)
  cancel_work_sync(&amd_manager->amd_sdw_work)
                                           schedule_work(&...->amd_sdw_irq_thread)
  /* devm frees amd_manager */
                                         amd_sdw_irq_thread()
                                           readl(amd_manager->mmio + ...)
                                             <-- use-after-free

Even when amd_sdw_irq_thread() runs with interrupt_enabled == false before
cancel_work_sync(), it still reads and writes MMIO registers and calls
amd_sdw_read_and_process_ping_status() before reaching the
interrupt_enabled check.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107154659.3358223-1-Vijendar.Mukunda@amd.com?part=17

Re: [PATCH v5 17/25] soundwire: amd: use interrupt_enabled flag to guard IRQ work scheduling

[Mukunda,Vijendar]

On 11/7/26 21:51, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issues to consider:
> - [Medium] soundwire: amd: lost IRQ in amd_enable_sdw_interrupts() due
>    to late flag
> - [Medium] soundwire: amd: unguarded IRQ work queueing in
>    amd_disable_sdw_interrupts()
> - [Medium] soundwire: amd: race re-enabling IRQ masks in
>    amd_disable_sdw_interrupts()
> --
>
> commit c646d5fedeac998edd11296e03517aec14fbed88
> Author: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
>
> soundwire: amd: use interrupt_enabled flag to guard IRQ work scheduling
>
> This commit adds an interrupt_enabled boolean flag to struct
> amd_sdw_manager to guard scheduling of amd_sdw_work when interrupts are
> being disabled.
>
>> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
>> index 01aa051cf63fa..108553b0f32e8 100644
>> --- a/drivers/soundwire/amd_manager.c
>> +++ b/drivers/soundwire/amd_manager.c
>> @@ -129,12 +129,22 @@ static void amd_enable_sdw_interrupts(struct amd_sdw_manager *amd_manager)
>>   	writel(AMD_SDW_IRQ_MASK_8TO11, amd_manager->mmio +
>>   		       ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11);
>>   	writel(AMD_SDW_IRQ_ERROR_MASK, amd_manager->mmio + ACP_SW_ERROR_INTR_MASK);
>> +	amd_manager->interrupt_enabled = true;
>>   }
> [Severity: Medium]
> Can setting amd_manager->interrupt_enabled after unmasking the hardware
> registers drop an initial peripheral status-change interrupt?
Agreed. Setting interrupt_enabled = true after the hardware unmask
creates a window where amd_sdw_irq_thread() runs with the flag false,
skips schedule_work(), and clears the status registers, permanently
losing the event. The fix is to set interrupt_enabled = true before
the first amd_updatel() call in amd_enable_sdw_interrupts(). This
will be corrected in v6.
> amd_init_sdw_manager() has already enabled the bus before
> amd_enable_sdw_interrupts() runs, so a peripheral state-change interrupt
> can fire as soon as the mask registers are written:
>
> CPU0                                     CPU1 (kworker)
> amd_enable_sdw_interrupts()
>    amd_updatel(..., ACP_EXTERNAL_INTR_CNTL, ...)
>    writel(..., ACP_SW_STATE_CHANGE_STATUS_MASK_0TO7)
>    <IRQ fires, schedules amd_sdw_irq_thread>
>                                           amd_sdw_irq_thread()
>                                             status_change_0to7 = readl(...)
>                                             amd_sdw_update_slave_status(...)
>                                             if (... && amd_manager->interrupt_enabled)
>                                               /* false: skips amd_sdw_work */
>                                             writel(0x00, ..._STATUS_0TO7)
>                                               <-- event lost
>    amd_manager->interrupt_enabled = true
>
> Because amd_sdw_irq_thread() skips schedule_work(&amd_manager->amd_sdw_work)
> when interrupt_enabled is false but still clears
> ACP_SW_STATE_CHANGE_STATUS_8TO11 and ACP_SW_STATE_CHANGE_STATUS_0TO7 to
> 0x00, the peripheral attachment or alert is permanently lost.  Setting
> amd_manager->interrupt_enabled = true before unmasking the hardware
> registers avoids this race.
>
>>   static void amd_disable_sdw_interrupts(struct amd_sdw_manager *amd_manager)
>>   {
>>   	u32 irq_mask;
>>   
>> +	/*
>> +	 * Clear the flag before updating hardware masks so that any IRQ
>> +	 * that fires during the mask-register writes sees the flag as false
>> +	 * and does not queue new work. Once the mask registers are written,
>> +	 * the ACP hardware will not assert any new SDW interrupt, so no
>> +	 * synchronize_irq() is required after this function returns.
>> +	 */
>> +	amd_manager->interrupt_enabled = false;
>> +
>>   	mutex_lock(amd_manager->acp_sdw_lock);
>>   	irq_mask = sdw_manager_reg_mask_array[amd_manager->instance];
>>   	amd_updatel(amd_manager->acp_mmio, ACP_EXTERNAL_INTR_CNTL(amd_manager->instance),
> [Severity: Medium]
> Does clearing amd_manager->interrupt_enabled and zeroing the mask
> registers before cancel_work_sync(&amd_manager->amd_sdw_work) allow
> amd_sdw_update_slave_status_work() to re-enable the status masks?
amd_disable_sdw_interrupts() clears ACP_EXTERNAL_INTR_CNTL, which
is the delivery gate that forwards the ACP IRQ line to the CPU.
Even if a concurrently running amd_sdw_work re-enables the
state-change mask registers, no new SoundWire interrupt can reach
the CPU while the delivery gate is closed.
> drivers/soundwire/amd_manager.c:amd_sdw_update_slave_status_work() {
>      ...
> 	if (amd_manager->status[0] == SDW_SLAVE_ATTACHED) {
> 		if (retry_count++ < SDW_MAX_DEVICES) {
> 			writel(AMD_SDW_IRQ_MASK_0TO7, amd_manager->mmio +
> 			       ACP_SW_STATE_CHANGE_STATUS_MASK_0TO7);
> 			writel(AMD_SDW_IRQ_MASK_8TO11, amd_manager->mmio +
> 			       ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11);
>      ...
> }
>
> CPU0                                     CPU1 (kworker)
>                                           amd_sdw_update_slave_status_work()
>                                             sdw_handle_slave_status(...)
> amd_disable_sdw_interrupts()
>    amd_manager->interrupt_enabled = false
>    writel(0x00, ..._STATUS_MASK_0TO7)
>    writel(0x00, ..._STATUS_MASK_8TO11)
>                                             writel(AMD_SDW_IRQ_MASK_0TO7, ...)
>                                             writel(AMD_SDW_IRQ_MASK_8TO11, ...)
>                                               <-- masks re-enabled
> cancel_work_sync(&amd_manager->amd_sdw_work)
>
> During runtime suspend in AMD_SDW_POWER_OFF_MODE, amd_sdw_wake_enable()
> then performs a read-modify-write on ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11
> while ACP_EXTERNAL_INTR_CNTL is re-enabled for wake on ACP 6.3, leaving
> non-wake state-change interrupts unmasked during suspend.
>
>> @@ -1143,7 +1153,7 @@ static void amd_sdw_irq_thread(struct work_struct *work)
>>   		/* Check for the updated status on peripheral device */
>>   		amd_sdw_update_slave_status(status_change_0to7, status_change_8to11, amd_manager);
>>   	}
>> -	if (status_change_8to11 || status_change_0to7)
>> +	if ((status_change_8to11 || status_change_0to7) && amd_manager->interrupt_enabled)
>>   		schedule_work(&amd_manager->amd_sdw_work);
>>   	writel(0x00, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_8TO11);
>>   	writel(0x00, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_0TO7);
> [Severity: Medium]
> Does guarding only schedule_work(&amd_manager->amd_sdw_work) inside
> amd_sdw_irq_thread() still leave amd_sdw_irq_thread itself unguarded
> against an in-flight hard IRQ handler?
The acp63_irq_handler() code shown above is the pre-patch state.
The companion ASoC series posted to alsa-devel adds an
interrupt_enabled guard in acp63_irq_handler() (pci-ps.c) and
acp_irq_handler() (acp.c) before the schedule_work() call,
covering the in-flight hard IRQ handler gap.

This finding is an artifact of the two series being reviewed in
separate trees. When both series are evaluated together as a single
patch set, Sashiko does not report this issue.
> amd_sdw_irq_thread() is a work_struct callback rather than a threaded IRQ
> handler.  The actual hard IRQ handlers (such as acp63_irq_handler() in
> sound/soc/amd/ps/pci-ps.c and acp_irq_handler() in
> sound/soc/sof/amd/acp.c) schedule amd_sdw_irq_thread without checking
> amd_manager->interrupt_enabled:
>
> sound/soc/amd/ps/pci-ps.c:acp63_irq_handler() {
>      ...
> 	if (ext_intr_stat & ACP_SDW0_STAT) {
> 		writel(ACP_SDW0_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
> 		amd_manager = dev_get_drvdata(&adata->sdw->pdev[0]->dev);
> 		if (amd_manager)
> 			schedule_work(&amd_manager->amd_sdw_irq_thread);
>      ...
> }
>
> Without synchronize_irq() or a check before schedule_work() in the hard
> IRQ handler, an IRQ that fires just before ACP_EXTERNAL_INTR_CNTL is
> masked can queue amd_sdw_irq_thread after cancel_work_sync() returns:
>
> CPU0                                     CPU1 (<hardirq>)
>                                           acp63_irq_handler()
>                                             ext_intr_stat & ACP_SDW0_STAT
> amd_sdw_manager_remove()
>    amd_disable_sdw_interrupts()
>      amd_manager->interrupt_enabled = false
>      amd_updatel(..., ACP_EXTERNAL_INTR_CNTL, ...)
>    cancel_work_sync(&amd_manager->amd_sdw_irq_thread)
>    cancel_work_sync(&amd_manager->amd_sdw_work)
>                                             schedule_work(&...->amd_sdw_irq_thread)
>    /* devm frees amd_manager */
>                                           amd_sdw_irq_thread()
>                                             readl(amd_manager->mmio + ...)
>                                               <-- use-after-free
>
> Even when amd_sdw_irq_thread() runs with interrupt_enabled == false before
> cancel_work_sync(), it still reads and writes MMIO registers and calls
> amd_sdw_read_and_process_ping_status() before reaching the
> interrupt_enabled check.
>

[PATCH v5 18/25] soundwire: amd: delay *sdw_ctx publication until after startup succeeds

[Vijendar Mukunda]

sdw_amd_probe() published *sdw_ctx immediately after probe_controller()
returned, before sdw_amd_startup() ran.  Two problems follow from this:

1. If sdw_amd_startup() fails, the error was returned directly, leaving
   *sdw_ctx pointing to an allocated context whose platform devices were
   already registered.  The caller has no way to distinguish this from a
   genuine probe success, so the context and devices leak.

2. A concurrent ACP IRQ handler on another CPU can read the non-NULL
   adata->sdw pointer between the sdw_amd_exit() teardown and the
   *sdw_ctx = NULL assignment, and dereference a freed context.

Fix both by using a local variable for the context, running
sdw_amd_startup() against it, and only writing *sdw_ctx after startup
succeeds.  On failure sdw_amd_exit() is called on the local pointer and
the function returns without touching *sdw_ctx, so callers never observe
a partially-initialised or freed context.

Fixes: ed5e8741b8db ("soundwire: amd: refactor amd soundwire manager device node creation")
Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/soundwire/amd_init.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/soundwire/amd_init.c b/drivers/soundwire/amd_init.c
index 032697ea3590..ce40b1e5dbf3 100644
--- a/drivers/soundwire/amd_init.c
+++ b/drivers/soundwire/amd_init.c
@@ -176,11 +176,20 @@ static int sdw_amd_startup(struct sdw_amd_ctx *ctx)
 
 int sdw_amd_probe(struct sdw_amd_res *res, struct sdw_amd_ctx **sdw_ctx)
 {
-	*sdw_ctx = sdw_amd_probe_controller(res);
-	if (!*sdw_ctx)
+	struct sdw_amd_ctx *ctx;
+	int ret;
+
+	ctx = sdw_amd_probe_controller(res);
+	if (!ctx)
 		return -ENODEV;
 
-	return sdw_amd_startup(*sdw_ctx);
+	ret = sdw_amd_startup(ctx);
+	if (ret) {
+		sdw_amd_exit(ctx);
+		return ret;
+	}
+	*sdw_ctx = ctx;
+	return 0;
 }
 EXPORT_SYMBOL_NS(sdw_amd_probe, "SOUNDWIRE_AMD_INIT");
 
-- 
2.53.0

Re: [PATCH v5 18/25] soundwire: amd: delay *sdw_ctx publication until after startup succeeds

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] soundwire: amd: NULL adata->sdw dereference in IRQ during
  sdw_amd_startup()
--

commit a9bb217bfdd227aefc49880481f23f5b560ad717
Author: Vijendar Mukunda <Vijendar.Mukunda@amd.com>

soundwire: amd: delay *sdw_ctx publication until after startup succeeds

This commit delays assigning *sdw_ctx in sdw_amd_probe() until
sdw_amd_startup() succeeds, cleaning up the local context with
sdw_amd_exit() on failure.

> diff --git a/drivers/soundwire/amd_init.c b/drivers/soundwire/amd_init.c
> index 032697ea35906..ce40b1e5dbf31 100644
> --- a/drivers/soundwire/amd_init.c
> +++ b/drivers/soundwire/amd_init.c
> @@ -176,11 +176,20 @@ static int sdw_amd_startup(struct sdw_amd_ctx *ctx)
>  
>  int sdw_amd_probe(struct sdw_amd_res *res, struct sdw_amd_ctx **sdw_ctx)
>  {
> -	*sdw_ctx = sdw_amd_probe_controller(res);
> -	if (!*sdw_ctx)
> +	struct sdw_amd_ctx *ctx;
> +	int ret;
> +
> +	ctx = sdw_amd_probe_controller(res);
> +	if (!ctx)
>  		return -ENODEV;
>  
> -	return sdw_amd_startup(*sdw_ctx);
> +	ret = sdw_amd_startup(ctx);
> +	if (ret) {
> +		sdw_amd_exit(ctx);
> +		return ret;
> +	}
> +	*sdw_ctx = ctx;
> +	return 0;
>  }
>  EXPORT_SYMBOL_NS(sdw_amd_probe, "SOUNDWIRE_AMD_INIT");

[Severity: High]
Can delaying the assignment of *sdw_ctx until after sdw_amd_startup()
cause a NULL pointer dereference of adata->sdw in the ACP IRQ handler?

The callers (snd_acp63_probe(), amd_sof_acp_probe(), and
amd_sof_acp7x_probe()) register their IRQ handlers and enable global ACP
external interrupts before calling sdw_amd_probe(&sdw_res, &adata->sdw).
Inside sdw_amd_startup(), amd_sdw_manager_start() unmasks SoundWire
interrupts and enables the bus on each link before *sdw_ctx is populated:

CPU0                              <hardirq>
sdw_amd_probe()
  sdw_amd_startup(ctx)
    amd_sdw_manager_start()
      amd_enable_sdw_interrupts()
      amd_enable_sdw_manager()
                                  acp63_irq_handler()
                                    adata->sdw->pdev[0]  <-- NULL deref
  *sdw_ctx = ctx;

Once the link is enabled and attached peripherals signal attachment while
amd_enable_sdw_manager() polls or while the second link starts up,
acp63_irq_handler() and acp_irq_handler() dereference adata->sdw without a
NULL check:

sound/soc/amd/ps/pci-ps.c:acp63_irq_handler() {
    ...
	if (ext_intr_stat & ACP_SDW0_STAT) {
		writel(ACP_SDW0_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
		amd_manager = dev_get_drvdata(&adata->sdw->pdev[0]->dev);
    ...
}

On ACP7x, acp7x_irq_handler() checks if (adata->sdw) and skips
sof_acp7x_handle_sdw_manager_irq() when it is NULL, leaving the SoundWire
interrupt status unacknowledged and returning IRQ_NONE.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107154659.3358223-1-Vijendar.Mukunda@amd.com?part=18

Re: [PATCH v5 18/25] soundwire: amd: delay *sdw_ctx publication until after startup succeeds

[Mukunda,Vijendar]

On 11/7/26 21:45, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue to consider:
> - [High] soundwire: amd: NULL adata->sdw dereference in IRQ during
>    sdw_amd_startup()
> --
>
> commit a9bb217bfdd227aefc49880481f23f5b560ad717
> Author: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
>
> soundwire: amd: delay *sdw_ctx publication until after startup succeeds
>
> This commit delays assigning *sdw_ctx in sdw_amd_probe() until
> sdw_amd_startup() succeeds, cleaning up the local context with
> sdw_amd_exit() on failure.
>
>> diff --git a/drivers/soundwire/amd_init.c b/drivers/soundwire/amd_init.c
>> index 032697ea35906..ce40b1e5dbf31 100644
>> --- a/drivers/soundwire/amd_init.c
>> +++ b/drivers/soundwire/amd_init.c
>> @@ -176,11 +176,20 @@ static int sdw_amd_startup(struct sdw_amd_ctx *ctx)
>>   
>>   int sdw_amd_probe(struct sdw_amd_res *res, struct sdw_amd_ctx **sdw_ctx)
>>   {
>> -	*sdw_ctx = sdw_amd_probe_controller(res);
>> -	if (!*sdw_ctx)
>> +	struct sdw_amd_ctx *ctx;
>> +	int ret;
>> +
>> +	ctx = sdw_amd_probe_controller(res);
>> +	if (!ctx)
>>   		return -ENODEV;
>>   
>> -	return sdw_amd_startup(*sdw_ctx);
>> +	ret = sdw_amd_startup(ctx);
>> +	if (ret) {
>> +		sdw_amd_exit(ctx);
>> +		return ret;
>> +	}
>> +	*sdw_ctx = ctx;
>> +	return 0;
>>   }
>>   EXPORT_SYMBOL_NS(sdw_amd_probe, "SOUNDWIRE_AMD_INIT");
> [Severity: High]
> Can delaying the assignment of *sdw_ctx until after sdw_amd_startup()
> cause a NULL pointer dereference of adata->sdw in the ACP IRQ handler?
The race is real. It is addressed by the companion ASoC series posted
to alsa-devel.
Link:
lore.kernel.org/alsa-devel/20261107175339.3547918-2-Vijendar.Mukunda@amd.com

Patches 1/4 and 2/4 add adata->sdw NULL checks in
acp63_irq_handler() (pci-ps.c) and acp_irq_handler() (acp.c) before
dereferencing ->pdev[0] and ->pdev[1]. The interrupt-status register
write that acknowledges the IRQ is kept outside the NULL guard so the
hardware bit is always cleared regardless of whether adata->sdw is live.

> The callers (snd_acp63_probe(), amd_sof_acp_probe(), and
> amd_sof_acp7x_probe()) register their IRQ handlers and enable global ACP
> external interrupts before calling sdw_amd_probe(&sdw_res, &adata->sdw).
> Inside sdw_amd_startup(), amd_sdw_manager_start() unmasks SoundWire
> interrupts and enables the bus on each link before *sdw_ctx is populated:
>
> CPU0                              <hardirq>
> sdw_amd_probe()
>    sdw_amd_startup(ctx)
>      amd_sdw_manager_start()
>        amd_enable_sdw_interrupts()
>        amd_enable_sdw_manager()
>                                    acp63_irq_handler()
>                                      adata->sdw->pdev[0]  <-- NULL deref
>    *sdw_ctx = ctx;
>
> Once the link is enabled and attached peripherals signal attachment while
> amd_enable_sdw_manager() polls or while the second link starts up,
> acp63_irq_handler() and acp_irq_handler() dereference adata->sdw without a
> NULL check:
>
> sound/soc/amd/ps/pci-ps.c:acp63_irq_handler() {
>      ...
> 	if (ext_intr_stat & ACP_SDW0_STAT) {
> 		writel(ACP_SDW0_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
> 		amd_manager = dev_get_drvdata(&adata->sdw->pdev[0]->dev);
>      ...
> }
>
> On ACP7x, acp7x_irq_handler() checks if (adata->sdw) and skips
> sof_acp7x_handle_sdw_manager_irq() when it is NULL, leaving the SoundWire
> interrupt status unacknowledged and returning IRQ_NONE.
>

[PATCH v5 19/25] soundwire: amd: propagate amd_init_sdw_manager() error on resume

[Vijendar Mukunda]

In amd_resume_runtime() POWER_OFF_MODE, amd_init_sdw_manager() was
called without capturing its return value. The function performs
hardware bus resets using readl_poll_timeout() and can return
-ETIMEDOUT if the hardware does not respond. When that happens,
execution fell through to amd_enable_sdw_interrupts() and
amd_enable_sdw_manager() on hardware that may still be in a broken
reset state.

Fix by capturing the return value and returning early on failure.

Fixes: 81ff58ff71ad ("soundwire: amd: add runtime pm ops for AMD SoundWire manager driver")
Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/soundwire/amd_manager.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index 108553b0f32e..71280745df8d 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -1596,7 +1596,9 @@ 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)
-- 
2.53.0

[PATCH v5 20/25] soundwire: amd: drop amd_deinit_sdw_manager() in POWER_OFF suspend

[Vijendar Mukunda]

amd_deinit_sdw_manager() was a small helper that bundled
amd_disable_sdw_interrupts() and amd_disable_sdw_manager() together.
Expand the two POWER_OFF_MODE call sites in amd_suspend() and
amd_suspend_runtime() to call those two functions directly, then remove
the helper. No functional change.

Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/soundwire/amd_manager.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index 71280745df8d..b6aa05f5f6c0 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -156,12 +156,6 @@ static void amd_disable_sdw_interrupts(struct amd_sdw_manager *amd_manager)
 	writel(0x00, amd_manager->mmio + ACP_SW_ERROR_INTR_MASK);
 }
 
-static int amd_deinit_sdw_manager(struct amd_sdw_manager *amd_manager)
-{
-	amd_disable_sdw_interrupts(amd_manager);
-	return amd_disable_sdw_manager(amd_manager);
-}
-
 static void __amd_sdw_oe_update(struct amd_sdw_manager *amd_manager,
 				u32 mask, bool enable)
 {
@@ -1451,7 +1445,8 @@ static int __maybe_unused amd_suspend(struct device *dev)
 		ret = amd_sdw_clock_stop(amd_manager);
 		if (ret)
 			return ret;
-		ret = amd_deinit_sdw_manager(amd_manager);
+		amd_disable_sdw_interrupts(amd_manager);
+		ret = amd_disable_sdw_manager(amd_manager);
 		if (ret)
 			return ret;
 		/* System suspend: tear down all pads */
@@ -1494,7 +1489,8 @@ static int __maybe_unused amd_suspend_runtime(struct device *dev)
 		ret = amd_sdw_clock_stop(amd_manager);
 		if (ret)
 			return ret;
-		ret = amd_deinit_sdw_manager(amd_manager);
+		amd_disable_sdw_interrupts(amd_manager);
+		ret = amd_disable_sdw_manager(amd_manager);
 		if (ret)
 			return ret;
 		amd_sdw_wake_enable(amd_manager, true);
-- 
2.53.0

[PATCH v5 21/25] soundwire: amd: replace >= ACP70 with explicit switch/case in PM paths

[Vijendar Mukunda]

Replace open-ended acp_rev >= ACP70_PCI_REV_ID comparisons in
amd_suspend(), amd_suspend_runtime(), and amd_resume_runtime() with
explicit per-revision handling enumerating ACP63, ACP70, ACP71, and
ACP72, so that every supported revision is handled explicitly.

The host wake enable step is guarded by acp_rev at six PM call sites.
Factor that repeated block into amd_sdw_host_wake_enable_by_rev() to
avoid duplicating the switch/case at every site. The remaining device
state transitions keep an inline switch/case as one of them carries
additional PME handling.

An unrecognised revision cannot reach these PM callbacks because
amd_sdw_manager_probe() already validates acp_rev with the same switch
and fails registration for anything unknown. The switch default therefore
emits dev_err() and returns -EINVAL to make the unexpected case visible
while keeping the error path consistent across all PM operations.

Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/soundwire/amd_manager.c | 100 +++++++++++++++++++++-----------
 1 file changed, 66 insertions(+), 34 deletions(-)

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index b6aa05f5f6c0..2b38b9bf97f5 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -1407,6 +1407,23 @@ static int __maybe_unused amd_pm_prepare(struct device *dev)
 	return 0;
 }
 
+static int __maybe_unused amd_sdw_host_wake_enable_by_rev(struct amd_sdw_manager *amd_manager,
+							  bool enable)
+{
+	switch (amd_manager->acp_rev) {
+	case ACP63_PCI_REV_ID:
+		break;
+	case ACP70_PCI_REV_ID:
+	case ACP71_PCI_REV_ID:
+	case ACP72_PCI_REV_ID:
+		return amd_sdw_host_wake_enable(amd_manager, enable);
+	default:
+		dev_err(amd_manager->dev, "unexpected acp_rev 0x%x\n", amd_manager->acp_rev);
+		return -EINVAL;
+	}
+	return 0;
+}
+
 static int __maybe_unused amd_suspend(struct device *dev)
 {
 	struct amd_sdw_manager *amd_manager = dev_get_drvdata(dev);
@@ -1422,22 +1439,18 @@ static int __maybe_unused amd_suspend(struct device *dev)
 	if (amd_manager->power_mode_mask & AMD_SDW_CLK_STOP_MODE) {
 		cancel_work_sync(&amd_manager->amd_sdw_work);
 		amd_sdw_wake_enable(amd_manager, false);
-		if (amd_manager->acp_rev >= ACP70_PCI_REV_ID) {
-			ret = amd_sdw_host_wake_enable(amd_manager, false);
-			if (ret)
-				return ret;
-		}
+		ret = amd_sdw_host_wake_enable_by_rev(amd_manager, false);
+		if (ret)
+			return ret;
 		ret = amd_sdw_clock_stop(amd_manager);
 		if (ret)
 			return ret;
 	} else if (amd_manager->power_mode_mask & AMD_SDW_POWER_OFF_MODE) {
 		cancel_work_sync(&amd_manager->amd_sdw_work);
 		amd_sdw_wake_enable(amd_manager, false);
-		if (amd_manager->acp_rev >= ACP70_PCI_REV_ID) {
-			ret = amd_sdw_host_wake_enable(amd_manager, false);
-			if (ret)
-				return ret;
-		}
+		ret = amd_sdw_host_wake_enable_by_rev(amd_manager, false);
+		if (ret)
+			return ret;
 		/*
 		 * As per hardware programming sequence on AMD platforms,
 		 * clock stop should be invoked first before powering-off
@@ -1455,10 +1468,19 @@ static int __maybe_unused amd_suspend(struct device *dev)
 			amd_manager->pad_fully_torn_down = true;
 		}
 	}
-	if (amd_manager->acp_rev >= ACP70_PCI_REV_ID) {
+	switch (amd_manager->acp_rev) {
+	case ACP63_PCI_REV_ID:
+		break;
+	case ACP70_PCI_REV_ID:
+	case ACP71_PCI_REV_ID:
+	case ACP72_PCI_REV_ID:
 		ret = amd_sdw_set_device_state(amd_manager, AMD_SDW_DEVICE_STATE_D3);
 		if (ret)
 			return ret;
+		break;
+	default:
+		dev_err(amd_manager->dev, "unexpected acp_rev 0x%x\n", amd_manager->acp_rev);
+		return -EINVAL;
 	}
 	return 0;
 }
@@ -1477,11 +1499,9 @@ static int __maybe_unused amd_suspend_runtime(struct device *dev)
 	}
 	if (amd_manager->power_mode_mask & AMD_SDW_CLK_STOP_MODE) {
 		amd_sdw_wake_enable(amd_manager, true);
-		if (amd_manager->acp_rev >= ACP70_PCI_REV_ID) {
-			ret = amd_sdw_host_wake_enable(amd_manager, true);
-			if (ret)
-				return ret;
-		}
+		ret = amd_sdw_host_wake_enable_by_rev(amd_manager, true);
+		if (ret)
+			return ret;
 		ret = amd_sdw_clock_stop(amd_manager);
 		if (ret)
 			return ret;
@@ -1494,12 +1514,10 @@ static int __maybe_unused amd_suspend_runtime(struct device *dev)
 		if (ret)
 			return ret;
 		amd_sdw_wake_enable(amd_manager, true);
-		if (amd_manager->acp_rev >= ACP70_PCI_REV_ID) {
-			ret = amd_sdw_host_wake_enable(amd_manager, true);
-			if (ret)
-				return ret;
-		} else if ((amd_manager->acp_rev == ACP63_PCI_REV_ID) &&
-			   amd_manager->wake_en_mask) {
+		ret = amd_sdw_host_wake_enable_by_rev(amd_manager, true);
+		if (ret)
+			return ret;
+		if (amd_manager->acp_rev == ACP63_PCI_REV_ID && amd_manager->wake_en_mask) {
 			mutex_lock(amd_manager->acp_sdw_lock);
 			val = sdw_manager_reg_mask_array[amd_manager->instance];
 			amd_updatel(amd_manager->acp_mmio,
@@ -1514,7 +1532,12 @@ static int __maybe_unused amd_suspend_runtime(struct device *dev)
 		 */
 		amd_sdw_oe_ctrl(amd_manager, false);
 	}
-	if (amd_manager->acp_rev >= ACP70_PCI_REV_ID) {
+	switch (amd_manager->acp_rev) {
+	case ACP63_PCI_REV_ID:
+		break;
+	case ACP70_PCI_REV_ID:
+	case ACP71_PCI_REV_ID:
+	case ACP72_PCI_REV_ID:
 		ret = amd_sdw_set_device_state(amd_manager, AMD_SDW_DEVICE_STATE_D3);
 		if (ret)
 			return ret;
@@ -1526,6 +1549,10 @@ static int __maybe_unused amd_suspend_runtime(struct device *dev)
 				dev_dbg(amd_manager->dev, "ACP_PME_EN:0x%x\n", val);
 			}
 		}
+		break;
+	default:
+		dev_err(amd_manager->dev, "unexpected acp_rev 0x%x\n", amd_manager->acp_rev);
+		return -EINVAL;
 	}
 	return 0;
 }
@@ -1547,11 +1574,9 @@ static int __maybe_unused amd_resume_runtime(struct device *dev)
 		ret = amd_sdw_clock_stop_exit(amd_manager);
 		if (ret)
 			return ret;
-		if (amd_manager->acp_rev >= ACP70_PCI_REV_ID) {
-			ret = amd_sdw_host_wake_enable(amd_manager, false);
-			if (ret)
-				return ret;
-		}
+		ret = amd_sdw_host_wake_enable_by_rev(amd_manager, false);
+		if (ret)
+			return ret;
 	} else if (amd_manager->power_mode_mask & AMD_SDW_POWER_OFF_MODE) {
 		if (amd_manager->pad_fully_torn_down) {
 			amd_sdw_pad_ctrl(amd_manager, true);
@@ -1571,11 +1596,9 @@ static int __maybe_unused amd_resume_runtime(struct device *dev)
 		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)
-				return ret;
-		}
+		ret = amd_sdw_host_wake_enable_by_rev(amd_manager, false);
+		if (ret)
+			return ret;
 		val = readl(amd_manager->mmio + ACP_SW_CLK_RESUME_CTRL);
 		if (val) {
 			val |= AMD_SDW_CLK_RESUME_REQ;
@@ -1601,10 +1624,19 @@ static int __maybe_unused amd_resume_runtime(struct device *dev)
 			return ret;
 		amd_sdw_set_frameshape(amd_manager);
 	}
-	if (amd_manager->acp_rev >= ACP70_PCI_REV_ID) {
+	switch (amd_manager->acp_rev) {
+	case ACP63_PCI_REV_ID:
+		break;
+	case ACP70_PCI_REV_ID:
+	case ACP71_PCI_REV_ID:
+	case ACP72_PCI_REV_ID:
 		ret = amd_sdw_set_device_state(amd_manager, AMD_SDW_DEVICE_STATE_D0);
 		if (ret)
 			return ret;
+		break;
+	default:
+		dev_err(amd_manager->dev, "unexpected acp_rev 0x%x\n", amd_manager->acp_rev);
+		return -EINVAL;
 	}
 	return 0;
 }
-- 
2.53.0

[PATCH v5 22/25] soundwire: amd: fix interrupt gate and work drain ordering in PM ops

[Vijendar Mukunda]

The CLK_STOP_MODE branches in amd_suspend() and amd_suspend_runtime()
did not close the ACP_EXTERNAL_INTR_CNTL delivery gate before draining
amd_sdw_irq_thread and amd_sdw_work. An ISR already running when the
drains completed could re-queue work after cancel_work_sync() returned,
racing amd_sdw_clock_stop().

In both CLK_STOP_MODE paths, amd_sdw_wake_enable() was also called
before amd_enable_sdw_interrupts(). Since amd_enable_sdw_interrupts()
writes the interrupt mask unconditionally, it clobbered the wake bit
set by amd_sdw_wake_enable().

For both amd_suspend() and amd_suspend_runtime():
 - CLK_STOP_MODE: add amd_disable_sdw_interrupts() and
   cancel_work_sync(irq_thread) before the work drain; reopen the gate
   with amd_enable_sdw_interrupts() after clock stop, then call
   amd_sdw_wake_enable() so the wake bit is not clobbered.
 - POWER_OFF_MODE: add amd_disable_sdw_interrupts() and
   cancel_work_sync(irq_thread) before the work drain; remove the
   redundant post-clock-stop amd_disable_sdw_interrupts() call.

Fixes: 86a4371b7697 ("soundwire: amd: fix for handling slave alerts after link is down")
Reported-by: Lee Jones <lee@kernel.org>
Closes: https://sashiko.dev/#/bug/linux-3b774ace-00cf-41e6-82eb-85cd43f5e1d6
Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/soundwire/amd_manager.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index 2b38b9bf97f5..4e151f7350c9 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -1437,15 +1437,20 @@ static int __maybe_unused amd_suspend(struct device *dev)
 	}
 
 	if (amd_manager->power_mode_mask & AMD_SDW_CLK_STOP_MODE) {
+		amd_disable_sdw_interrupts(amd_manager);
+		cancel_work_sync(&amd_manager->amd_sdw_irq_thread);
 		cancel_work_sync(&amd_manager->amd_sdw_work);
-		amd_sdw_wake_enable(amd_manager, false);
 		ret = amd_sdw_host_wake_enable_by_rev(amd_manager, false);
 		if (ret)
 			return ret;
 		ret = amd_sdw_clock_stop(amd_manager);
 		if (ret)
 			return ret;
+		amd_enable_sdw_interrupts(amd_manager);
+		amd_sdw_wake_enable(amd_manager, false);
 	} else if (amd_manager->power_mode_mask & AMD_SDW_POWER_OFF_MODE) {
+		amd_disable_sdw_interrupts(amd_manager);
+		cancel_work_sync(&amd_manager->amd_sdw_irq_thread);
 		cancel_work_sync(&amd_manager->amd_sdw_work);
 		amd_sdw_wake_enable(amd_manager, false);
 		ret = amd_sdw_host_wake_enable_by_rev(amd_manager, false);
@@ -1458,7 +1463,6 @@ static int __maybe_unused amd_suspend(struct device *dev)
 		ret = amd_sdw_clock_stop(amd_manager);
 		if (ret)
 			return ret;
-		amd_disable_sdw_interrupts(amd_manager);
 		ret = amd_disable_sdw_manager(amd_manager);
 		if (ret)
 			return ret;
@@ -1498,18 +1502,24 @@ static int __maybe_unused amd_suspend_runtime(struct device *dev)
 		return 0;
 	}
 	if (amd_manager->power_mode_mask & AMD_SDW_CLK_STOP_MODE) {
-		amd_sdw_wake_enable(amd_manager, true);
+		amd_disable_sdw_interrupts(amd_manager);
+		cancel_work_sync(&amd_manager->amd_sdw_irq_thread);
+		cancel_work_sync(&amd_manager->amd_sdw_work);
 		ret = amd_sdw_host_wake_enable_by_rev(amd_manager, true);
 		if (ret)
 			return ret;
 		ret = amd_sdw_clock_stop(amd_manager);
 		if (ret)
 			return ret;
+		amd_enable_sdw_interrupts(amd_manager);
+		amd_sdw_wake_enable(amd_manager, true);
 	} else if (amd_manager->power_mode_mask & AMD_SDW_POWER_OFF_MODE) {
+		amd_disable_sdw_interrupts(amd_manager);
+		cancel_work_sync(&amd_manager->amd_sdw_irq_thread);
+		cancel_work_sync(&amd_manager->amd_sdw_work);
 		ret = amd_sdw_clock_stop(amd_manager);
 		if (ret)
 			return ret;
-		amd_disable_sdw_interrupts(amd_manager);
 		ret = amd_disable_sdw_manager(amd_manager);
 		if (ret)
 			return ret;
-- 
2.53.0

Re: [PATCH v5 22/25] soundwire: amd: fix interrupt gate and work drain ordering in PM ops

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] soundwire: amd: unserialized hard IRQ race with work drain in
  amd_suspend()
- [Low] soundwire: amd: commit message describes prior patch revision in
  PM ops
--

commit f3586b88e19b93c1d283c25b97e67e7d9da93bab
Author: Vijendar Mukunda <Vijendar.Mukunda@amd.com>

> soundwire: amd: fix interrupt gate and work drain ordering in PM ops
> 
> The CLK_STOP_MODE branches in amd_suspend() and amd_suspend_runtime()
> did not close the ACP_EXTERNAL_INTR_CNTL delivery gate before draining
> amd_sdw_irq_thread and amd_sdw_work. An ISR already running when the
> drains completed could re-queue work after cancel_work_sync() returned,
> racing amd_sdw_clock_stop().
> 
> In both CLK_STOP_MODE paths, amd_sdw_wake_enable() was also called
> before amd_enable_sdw_interrupts(). Since amd_enable_sdw_interrupts()
> writes the interrupt mask unconditionally, it clobbered the wake bit
> set by amd_sdw_wake_enable().
> 
> For both amd_suspend() and amd_suspend_runtime():
>  - CLK_STOP_MODE: add amd_disable_sdw_interrupts() and
>    cancel_work_sync(irq_thread) before the work drain; reopen the gate
>    with amd_enable_sdw_interrupts() after clock stop, then call
>    amd_sdw_wake_enable() so the wake bit is not clobbered.
>  - POWER_OFF_MODE: add amd_disable_sdw_interrupts() and
>    cancel_work_sync(irq_thread) before the work drain; remove the
>    redundant post-clock-stop amd_disable_sdw_interrupts() call.
> 
> Fixes: 86a4371b7697 ("soundwire: amd: fix for handling slave alerts after link is down")
> Reported-by: Lee Jones <lee@kernel.org>
> Closes: https://sashiko.dev/#/bug/linux-3b774ace-00cf-41e6-82eb-85cd43f5e1d6
> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>

[Severity: Low]
Does the commit message describe an intermediate revision of this series
rather than the parent commit?

In the parent commit, amd_suspend_runtime() did not drain
amd_sdw_irq_thread or amd_sdw_work at all (this patch adds both
cancel_work_sync() calls there), and neither CLK_STOP_MODE path called
amd_enable_sdw_interrupts() before this patch.

> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
> index 2b38b9bf97f50..4e151f7350c96 100644
> --- a/drivers/soundwire/amd_manager.c
> +++ b/drivers/soundwire/amd_manager.c
> @@ -1437,15 +1437,20 @@ static int __maybe_unused amd_suspend(struct device *dev)
>  	}
>  
>  	if (amd_manager->power_mode_mask & AMD_SDW_CLK_STOP_MODE) {
> +		amd_disable_sdw_interrupts(amd_manager);
> +		cancel_work_sync(&amd_manager->amd_sdw_irq_thread);
>  		cancel_work_sync(&amd_manager->amd_sdw_work);
> -		amd_sdw_wake_enable(amd_manager, false);
>  		ret = amd_sdw_host_wake_enable_by_rev(amd_manager, false);
>  		if (ret)
>  			return ret;
>  		ret = amd_sdw_clock_stop(amd_manager);
>  		if (ret)
>  			return ret;
> +		amd_enable_sdw_interrupts(amd_manager);
> +		amd_sdw_wake_enable(amd_manager, false);

[Severity: Medium]
Can an in-flight parent ACP hard IRQ handler still queue
amd_sdw_irq_thread after cancel_work_sync() returns in amd_suspend() and
amd_suspend_runtime()?

None of the parent ACP hard IRQ handlers (such as acp63_irq_handler() in
sound/soc/amd/ps/pci-ps.c) check amd_manager->interrupt_enabled or acquire
acp_sdw_lock before calling schedule_work(&amd_manager->amd_sdw_irq_thread):

sound/soc/amd/ps/pci-ps.c:acp63_irq_handler() {
    ...
	ext_intr_stat = readl(adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
	if (ext_intr_stat & ACP_SDW0_STAT) {
		writel(ACP_SDW0_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
		amd_manager = dev_get_drvdata(&adata->sdw->pdev[0]->dev);
		if (amd_manager)
			schedule_work(&amd_manager->amd_sdw_irq_thread);
    ...
}

If the hard IRQ fires just before amd_disable_sdw_interrupts() masks
ACP_EXTERNAL_INTR_CNTL, the two contexts can interleave as follows:

CPU0                                  CPU1 (<hardirq>)
amd_suspend()
                                      acp63_irq_handler()
                                        readl(... + ACP_EXTERNAL_INTR_STAT)
  amd_disable_sdw_interrupts()
    interrupt_enabled = false
    amd_updatel(ACP_EXTERNAL_INTR_CNTL, ...)
  cancel_work_sync(&amd_sdw_irq_thread)
  cancel_work_sync(&amd_sdw_work)
  amd_sdw_clock_stop()
  amd_enable_sdw_interrupts()
    interrupt_enabled = true
                                        schedule_work(&amd_sdw_irq_thread)
                                      amd_sdw_irq_thread()
                                        schedule_work(&amd_sdw_work)
                                          <-- runs while clock is stopped

Even in AMD_SDW_POWER_OFF_MODE where interrupt_enabled remains false,
amd_sdw_irq_thread() still reads SoundWire MMIO registers and calls
amd_sdw_read_and_process_ping_status() without checking interrupt_enabled,
racing with amd_sdw_clock_stop(), amd_disable_sdw_manager(), and D3 entry.
In AMD_SDW_CLK_STOP_MODE, calling amd_enable_sdw_interrupts() right after
amd_sdw_clock_stop() also sets interrupt_enabled back to true while the
clock is stopped, allowing a late amd_sdw_irq_thread() to schedule
amd_sdw_work and trigger MIPI command timeouts (-ETIMEDOUT).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107154659.3358223-1-Vijendar.Mukunda@amd.com?part=22

Re: [PATCH v5 22/25] soundwire: amd: fix interrupt gate and work drain ordering in PM ops

[Mukunda,Vijendar]

On 11/7/26 21:55, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issues to consider:
> - [Medium] soundwire: amd: unserialized hard IRQ race with work drain in
>    amd_suspend()
> - [Low] soundwire: amd: commit message describes prior patch revision in
>    PM ops
> --
>
> commit f3586b88e19b93c1d283c25b97e67e7d9da93bab
> Author: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
>
>> soundwire: amd: fix interrupt gate and work drain ordering in PM ops
>>
>> The CLK_STOP_MODE branches in amd_suspend() and amd_suspend_runtime()
>> did not close the ACP_EXTERNAL_INTR_CNTL delivery gate before draining
>> amd_sdw_irq_thread and amd_sdw_work. An ISR already running when the
>> drains completed could re-queue work after cancel_work_sync() returned,
>> racing amd_sdw_clock_stop().
>>
>> In both CLK_STOP_MODE paths, amd_sdw_wake_enable() was also called
>> before amd_enable_sdw_interrupts(). Since amd_enable_sdw_interrupts()
>> writes the interrupt mask unconditionally, it clobbered the wake bit
>> set by amd_sdw_wake_enable().
>>
>> For both amd_suspend() and amd_suspend_runtime():
>>   - CLK_STOP_MODE: add amd_disable_sdw_interrupts() and
>>     cancel_work_sync(irq_thread) before the work drain; reopen the gate
>>     with amd_enable_sdw_interrupts() after clock stop, then call
>>     amd_sdw_wake_enable() so the wake bit is not clobbered.
>>   - POWER_OFF_MODE: add amd_disable_sdw_interrupts() and
>>     cancel_work_sync(irq_thread) before the work drain; remove the
>>     redundant post-clock-stop amd_disable_sdw_interrupts() call.
>>
>> Fixes: 86a4371b7697 ("soundwire: amd: fix for handling slave alerts after link is down")
>> Reported-by: Lee Jones <lee@kernel.org>
>> Closes: https://sashiko.dev/#/bug/linux-3b774ace-00cf-41e6-82eb-85cd43f5e1d6
>> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
> [Severity: Low]
> Does the commit message describe an intermediate revision of this series
> rather than the parent commit?
>
> In the parent commit, amd_suspend_runtime() did not drain
> amd_sdw_irq_thread or amd_sdw_work at all (this patch adds both
> cancel_work_sync() calls there), and neither CLK_STOP_MODE path called
> amd_enable_sdw_interrupts() before this patch.
Acknowledged. Will be fixed in v6.
>
>> diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
>> index 2b38b9bf97f50..4e151f7350c96 100644
>> --- a/drivers/soundwire/amd_manager.c
>> +++ b/drivers/soundwire/amd_manager.c
>> @@ -1437,15 +1437,20 @@ static int __maybe_unused amd_suspend(struct device *dev)
>>   	}
>>   
>>   	if (amd_manager->power_mode_mask & AMD_SDW_CLK_STOP_MODE) {
>> +		amd_disable_sdw_interrupts(amd_manager);
>> +		cancel_work_sync(&amd_manager->amd_sdw_irq_thread);
>>   		cancel_work_sync(&amd_manager->amd_sdw_work);
>> -		amd_sdw_wake_enable(amd_manager, false);
>>   		ret = amd_sdw_host_wake_enable_by_rev(amd_manager, false);
>>   		if (ret)
>>   			return ret;
>>   		ret = amd_sdw_clock_stop(amd_manager);
>>   		if (ret)
>>   			return ret;
>> +		amd_enable_sdw_interrupts(amd_manager);
>> +		amd_sdw_wake_enable(amd_manager, false);
> [Severity: Medium]
> Can an in-flight parent ACP hard IRQ handler still queue
> amd_sdw_irq_thread after cancel_work_sync() returns in amd_suspend() and
> amd_suspend_runtime()?
The acp63_irq_handler() shown is the pre-patch state. The companion
ASoC series adds an interrupt_enabled check in acp63_irq_handler()
(pci-ps.c) and acp_irq_handler() (acp.c) before schedule_work().
With that guard in place, triggering this race requires CPU1 to be
delayed between reading ACP_EXTERNAL_INTR_STAT and reading
interrupt_enabled for the entire duration of the suspend sequence on
CPU0. A hard IRQ handler cannot be preempted by normal execution so
this window does not arise in practice.

This finding is an artifact of the two series being reviewed in
separate trees.
> None of the parent ACP hard IRQ handlers (such as acp63_irq_handler() in
> sound/soc/amd/ps/pci-ps.c) check amd_manager->interrupt_enabled or acquire
> acp_sdw_lock before calling schedule_work(&amd_manager->amd_sdw_irq_thread):
>
> sound/soc/amd/ps/pci-ps.c:acp63_irq_handler() {
>      ...
> 	ext_intr_stat = readl(adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
> 	if (ext_intr_stat & ACP_SDW0_STAT) {
> 		writel(ACP_SDW0_STAT, adata->acp63_base + ACP_EXTERNAL_INTR_STAT);
> 		amd_manager = dev_get_drvdata(&adata->sdw->pdev[0]->dev);
> 		if (amd_manager)
> 			schedule_work(&amd_manager->amd_sdw_irq_thread);
>      ...
> }
>
> If the hard IRQ fires just before amd_disable_sdw_interrupts() masks
> ACP_EXTERNAL_INTR_CNTL, the two contexts can interleave as follows:
>
> CPU0                                  CPU1 (<hardirq>)
> amd_suspend()
>                                        acp63_irq_handler()
>                                          readl(... + ACP_EXTERNAL_INTR_STAT)
>    amd_disable_sdw_interrupts()
>      interrupt_enabled = false
>      amd_updatel(ACP_EXTERNAL_INTR_CNTL, ...)
>    cancel_work_sync(&amd_sdw_irq_thread)
>    cancel_work_sync(&amd_sdw_work)
>    amd_sdw_clock_stop()
>    amd_enable_sdw_interrupts()
>      interrupt_enabled = true
>                                          schedule_work(&amd_sdw_irq_thread)
>                                        amd_sdw_irq_thread()
>                                          schedule_work(&amd_sdw_work)
>                                            <-- runs while clock is stopped
>
> Even in AMD_SDW_POWER_OFF_MODE where interrupt_enabled remains false,
> amd_sdw_irq_thread() still reads SoundWire MMIO registers and calls
> amd_sdw_read_and_process_ping_status() without checking interrupt_enabled,
> racing with amd_sdw_clock_stop(), amd_disable_sdw_manager(), and D3 entry.
> In AMD_SDW_CLK_STOP_MODE, calling amd_enable_sdw_interrupts() right after
> amd_sdw_clock_stop() also sets interrupt_enabled back to true while the
> clock is stopped, allowing a late amd_sdw_irq_thread() to schedule
> amd_sdw_work and trigger MIPI command timeouts (-ETIMEDOUT).
>

[PATCH v5 23/25] soundwire: amd: check for NULL manager drvdata in sdw_amd_startup()

[Vijendar Mukunda]

sdw_amd_startup() calls dev_get_drvdata() to retrieve the per-manager
context and passes the result unconditionally to amd_sdw_manager_start().
If the platform device for a manager instance was registered successfully
but its probe failed (e.g. due to ENOMEM or a deferred probe),
dev_get_drvdata() returns NULL and amd_sdw_manager_start() immediately
dereferences it, causing a kernel panic.

Add a NULL check after dev_get_drvdata() and return -ENODEV so that a
failed manager probe surfaces as an error from sdw_amd_probe() rather
than a kernel crash.

Fixes: ed5e8741b8db ("soundwire: amd: refactor amd soundwire manager device node creation")
Reported-by: Lee Jones <lee@kernel.org>
Closes: https://sashiko.dev/#/bug/linux-6834a8a0-632f-4c73-9174-0fe7e06b5303
Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/soundwire/amd_init.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/soundwire/amd_init.c b/drivers/soundwire/amd_init.c
index ce40b1e5dbf3..e5e6c7415dc6 100644
--- a/drivers/soundwire/amd_init.c
+++ b/drivers/soundwire/amd_init.c
@@ -166,6 +166,10 @@ static int sdw_amd_startup(struct sdw_amd_ctx *ctx)
 		if (!(ctx->link_mask & BIT(i)))
 			continue;
 		amd_manager = dev_get_drvdata(&ctx->pdev[i]->dev);
+		if (!amd_manager) {
+			dev_err(&ctx->pdev[i]->dev, "SoundWire manager not probed\n");
+			return -ENODEV;
+		}
 		ret = amd_sdw_manager_start(amd_manager);
 		if (ret)
 			return ret;
-- 
2.53.0

[PATCH v5 24/25] soundwire: amd: fix shared-register RMW race in amd_sdw_set_device_state()

[Vijendar Mukunda]

AMD_SDW_DEVICE_STATE packs the D0/D3 state for SDW0 (bits [1:0]) and
SDW1 (bits [3:2]) in a single ACP-level register.
amd_sdw_set_device_state() performed a read-modify-write sequence on
this shared register without holding acp_sdw_lock, leaving it open to
a lost-update race when SDW0 and SDW1 execute concurrent PM transitions.

Acquire acp_sdw_lock around the read-modify-write sequence. Unlock
before returning -EINVAL from the default branch so the mutex is always
released on every exit path.

Fixes: 829c3e1cb4a3 ("soundwire: amd: set device power state during suspend/resume sequence")
Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/soundwire/amd_manager.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index 4e151f7350c9..495f0607bb0b 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -279,6 +279,7 @@ static int amd_sdw_set_device_state(struct amd_sdw_manager *amd_manager, u32 tar
 {
 	u32 sdw_dev_state;
 
+	mutex_lock(amd_manager->acp_sdw_lock);
 	sdw_dev_state = readl(amd_manager->acp_mmio + AMD_SDW_DEVICE_STATE);
 	switch (amd_manager->instance) {
 	case ACP_SDW0:
@@ -290,10 +291,12 @@ static int amd_sdw_set_device_state(struct amd_sdw_manager *amd_manager, u32 tar
 				  AMD_SDW1_DEVICE_STATE_MASK);
 		break;
 	default:
+		mutex_unlock(amd_manager->acp_sdw_lock);
 		return -EINVAL;
 	}
 	writel(sdw_dev_state, amd_manager->acp_mmio + AMD_SDW_DEVICE_STATE);
 	sdw_dev_state = readl(amd_manager->acp_mmio + AMD_SDW_DEVICE_STATE);
+	mutex_unlock(amd_manager->acp_sdw_lock);
 	dev_dbg(amd_manager->dev, "AMD_SDW_DEVICE_STATE:0x%x\n", sdw_dev_state);
 	return 0;
 }
-- 
2.53.0

[PATCH v5 25/25] soundwire: amd: fix shared-register RMW race in amd_sdw_host_wake_enable()

[Vijendar Mukunda]

ACP_EXTERNAL_INTR_CNTL(ACP_SDW1) holds host-wake interrupt bits for
both instances (BIT(22) for SDW0, BIT(23) for SDW1).
amd_sdw_host_wake_enable() performed a read-modify-write sequence on
this shared register without holding acp_sdw_lock, leaving it open to
a lost-update race when SDW0 and SDW1 execute concurrent PM transitions.

Acquire acp_sdw_lock around the read-modify-write sequence.

Fixes: 3df75289ddc2 ("soundwire: amd: add soundwire host wake interrupt enable/disable sequence")
Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com>
---
 drivers/soundwire/amd_manager.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/soundwire/amd_manager.c b/drivers/soundwire/amd_manager.c
index 495f0607bb0b..2c52fd181e3c 100644
--- a/drivers/soundwire/amd_manager.c
+++ b/drivers/soundwire/amd_manager.c
@@ -320,12 +320,14 @@ static int amd_sdw_host_wake_enable(struct amd_sdw_manager *amd_manager, bool en
 		return -EINVAL;
 	}
 
+	mutex_lock(amd_manager->acp_sdw_lock);
 	intr_cntl1 = readl(amd_manager->acp_mmio + ACP_EXTERNAL_INTR_CNTL(ACP_SDW1));
 	if (enable)
 		intr_cntl1 |= sdw_host_wake_irq_mask;
 	else
 		intr_cntl1 &= ~sdw_host_wake_irq_mask;
 	writel(intr_cntl1, amd_manager->acp_mmio + ACP_EXTERNAL_INTR_CNTL(ACP_SDW1));
+	mutex_unlock(amd_manager->acp_sdw_lock);
 	return 0;
 }
 
-- 
2.53.0