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

On 8/14/26 06:27, [email protected] wrote:
> From: Reza Jelveh <[email protected]>
>
> 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 <[email protected]>
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;
>   }