[PATCH] accel/amdxdna: Keep PCI power state in sync with runtime PM

[PATCH] accel/amdxdna: Keep PCI power state in sync with runtime PM

[reza.jelveh]

From: Reza Jelveh <reza.jelveh+git@gmail.com>

The platform powers the NPU down as soon as the driver stops its
firmware, but aie2_hw_stop() never recorded that transition. The PCI
core kept tracking the device as D0, so on the next system suspend the
noirq phase attempted a D0-to-D3hot transition on a device whose config
space returns 0xffffffff:

  Unable to change power state from D0 to D3hot, device inaccessible

The platform's power state was then out of sync with the kernel's, the
SMU idle condition was never met, and suspend-to-idle did not reach the
deepest state. On resume, aie2_hw_start() re-enabled the device without
requesting D0 first and failed reading the SMU mailbox (0xffffffff,
-EINVAL), leaving the NPU dead until reboot.

Record D3hot at the end of aie2_hw_stop(), while the device is still
reachable, and request D0 at the beginning of aie2_hw_start(), so the
kernel's PCI power state always matches the platform state. Skip the
firmware stop in aie2_hw_suspend() when runtime PM has already stopped
the device, and demote the "device is already stopped" message to debug
level.

Signed-off-by: Reza Jelveh <reza.jelveh+git@gmail.com>
---
 drivers/accel/amdxdna/aie2_pci.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/accel/amdxdna/aie2_pci.c b/drivers/accel/amdxdna/aie2_pci.c
index a7b923005..ad14e4fc6 100644
--- a/drivers/accel/amdxdna/aie2_pci.c
+++ b/drivers/accel/amdxdna/aie2_pci.c
@@ -288,7 +288,7 @@ static void aie2_hw_stop(struct amdxdna_dev *xdna)
 	struct amdxdna_dev_hdl *ndev = xdna->dev_handle;
 
 	if (ndev->dev_status <= AIE2_DEV_INIT) {
-		XDNA_ERR(xdna, "device is already stopped");
+		XDNA_DBG(xdna, "device is already stopped");
 		return;
 	}
 
@@ -301,6 +301,12 @@ static void aie2_hw_stop(struct amdxdna_dev *xdna)
 	aie2_smu_fini(ndev);
 	aie2_error_async_events_free(ndev);
 	pci_disable_device(pdev);
+	/*
+	 * The platform powers the NPU down once the firmware is stopped.
+	 * Record D3hot while the device is still reachable, so the noirq
+	 * suspend path does not attempt a stale D0 transition.
+	 */
+	pci_set_power_state(pdev, PCI_D3hot);
 
 	ndev->dev_status = AIE2_DEV_INIT;
 }
@@ -318,6 +324,13 @@ static int aie2_hw_start(struct amdxdna_dev *xdna)
 		return 0;
 	}
 
+	/* The platform powers the device down when the firmware is stopped. */
+	ret = pci_set_power_state(pdev, PCI_D0);
+	if (ret) {
+		XDNA_ERR(xdna, "failed to power up device, ret %d", ret);
+		return ret;
+	}
+
 	ret = pci_enable_device(pdev);
 	if (ret) {
 		XDNA_ERR(xdna, "failed to enable device, ret %d", ret);
@@ -427,12 +440,15 @@ static int aie2_hw_start(struct amdxdna_dev *xdna)
 
 static int aie2_hw_suspend(struct amdxdna_dev *xdna)
 {
+	struct amdxdna_dev_hdl *ndev = xdna->dev_handle;
 	struct amdxdna_client *client;
 
 	list_for_each_entry(client, &xdna->client_list, node)
 		aie2_hwctx_suspend(client);
 
-	aie2_hw_stop(xdna);
+	/* Runtime PM may already have stopped the device. */
+	if (ndev->dev_status > AIE2_DEV_INIT)
+		aie2_hw_stop(xdna);
 
 	return 0;
 }
-- 
2.55.0

Re: [PATCH] accel/amdxdna: Keep PCI power state in sync with runtime PM

[Lizhi Hou]

On 8/14/26 06:27, reza.jelveh@gmail.com wrote:
> From: Reza Jelveh <reza.jelveh+git@gmail.com>
>
> The platform powers the NPU down as soon as the driver stops its
> firmware, but aie2_hw_stop() never recorded that transition. The PCI
> core kept tracking the device as D0, so on the next system suspend the
> noirq phase attempted a D0-to-D3hot transition on a device whose config
> space returns 0xffffffff:
>
>    Unable to change power state from D0 to D3hot, device inaccessible
>
> The platform's power state was then out of sync with the kernel's, the
> SMU idle condition was never met, and suspend-to-idle did not reach the
> deepest state. On resume, aie2_hw_start() re-enabled the device without
> requesting D0 first and failed reading the SMU mailbox (0xffffffff,
> -EINVAL), leaving the NPU dead until reboot.
>
> Record D3hot at the end of aie2_hw_stop(), while the device is still
> reachable, and request D0 at the beginning of aie2_hw_start(), so the
> kernel's PCI power state always matches the platform state. Skip the
> firmware stop in aie2_hw_suspend() when runtime PM has already stopped
> the device, and demote the "device is already stopped" message to debug
> level.
>
> Signed-off-by: Reza Jelveh <reza.jelveh+git@gmail.com>
Thanks for providing the fix. Could you add a 'Fixes' tag?
> ---
>   drivers/accel/amdxdna/aie2_pci.c | 20 ++++++++++++++++++--
>   1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/accel/amdxdna/aie2_pci.c b/drivers/accel/amdxdna/aie2_pci.c
> index a7b923005..ad14e4fc6 100644
> --- a/drivers/accel/amdxdna/aie2_pci.c
> +++ b/drivers/accel/amdxdna/aie2_pci.c
> @@ -288,7 +288,7 @@ static void aie2_hw_stop(struct amdxdna_dev *xdna)
>   	struct amdxdna_dev_hdl *ndev = xdna->dev_handle;
>   
>   	if (ndev->dev_status <= AIE2_DEV_INIT) {
> -		XDNA_ERR(xdna, "device is already stopped");
> +		XDNA_DBG(xdna, "device is already stopped");
>   		return;
>   	}
>   
> @@ -301,6 +301,12 @@ static void aie2_hw_stop(struct amdxdna_dev *xdna)
>   	aie2_smu_fini(ndev);
>   	aie2_error_async_events_free(ndev);
>   	pci_disable_device(pdev);
> +	/*
> +	 * The platform powers the NPU down once the firmware is stopped.
> +	 * Record D3hot while the device is still reachable, so the noirq
> +	 * suspend path does not attempt a stale D0 transition.
> +	 */
> +	pci_set_power_state(pdev, PCI_D3hot);

Need to add

     pci_save_state(pdev);

before pci_disable_device(pdev) ? (and pci_restore_state after 
recovering to D0)

>   
>   	ndev->dev_status = AIE2_DEV_INIT;
>   }
> @@ -318,6 +324,13 @@ static int aie2_hw_start(struct amdxdna_dev *xdna)
>   		return 0;
>   	}
>   
> +	/* The platform powers the device down when the firmware is stopped. */
> +	ret = pci_set_power_state(pdev, PCI_D0);
> +	if (ret) {
> +		XDNA_ERR(xdna, "failed to power up device, ret %d", ret);
> +		return ret;
> +	}
> +
>   	ret = pci_enable_device(pdev);
>   	if (ret) {
>   		XDNA_ERR(xdna, "failed to enable device, ret %d", ret);
> @@ -427,12 +440,15 @@ static int aie2_hw_start(struct amdxdna_dev *xdna)
>   
>   static int aie2_hw_suspend(struct amdxdna_dev *xdna)
>   {
> +	struct amdxdna_dev_hdl *ndev = xdna->dev_handle;
>   	struct amdxdna_client *client;
>   
>   	list_for_each_entry(client, &xdna->client_list, node)
>   		aie2_hwctx_suspend(client);
>   
> -	aie2_hw_stop(xdna);
> +	/* Runtime PM may already have stopped the device. */
> +	if (ndev->dev_status > AIE2_DEV_INIT)
> +		aie2_hw_stop(xdna);

This is redundant check. aie2_hw_stop() already checks it and returns early.

Thanks,

Lizhi

>   
>   	return 0;
>   }