Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] Error paths in aie2_hw_start() leave the PCI power state out of sync Pre-existing issues: - [High] Missing PCI state save before D3hot transition causes kernel warnings and configuration corruption -- --- Patch [1]: [PATCH] accel/amdxdna: Keep PCI power state in sync with runtime PM --- commit f75387302cbbf4db35ad9c9ef5066d43bf266934 Author: Reza Jelveh <[email protected]> accel/amdxdna: Keep PCI power state in sync with runtime PM This patch attempts to keep the PCI core's power state tracking in sync with the NPU hardware by explicitly setting D3hot in aie2_hw_stop() and D0 in aie2_hw_start(). It aims to prevent errors during noirq suspend and resume due to reading unreachable config space. > diff --git a/drivers/accel/amdxdna/aie2_pci.c > b/drivers/accel/amdxdna/aie2_pci.c > index daec1f6b49079..831277c1eb744 100644 > --- a/drivers/accel/amdxdna/aie2_pci.c > +++ b/drivers/accel/amdxdna/aie2_pci.c [ ... ] > @@ -316,6 +316,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); [Severity: High] This is a pre-existing issue, but will setting the power state to D3hot here without first saving the PCI state trigger a PM warning and force a read of corrupted config state? If aie2_hw_stop() calls pci_set_power_state() without a preceding call to pci_save_state(), the suspend paths detect that the power state changed but the state is not saved, which triggers a "PCI PM: State of device not saved" warning. During pci_pm_suspend(), does this warning proceed to the noirq phase where pci_save_state() reads from the already-powered-off device? This could save 0xffffffff and restore it on resume, which would permanently break the device until a cold reboot. [ ... ] > @@ -333,6 +339,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; > + } > + [Severity: High] If hardware initialization fails later in aie2_hw_start(), do the error paths leave the PCI power state out of sync? For example, if aie2_mgmt_fw_init() fails, the code jumps to stop_fw and eventually disable_dev: stop_fw: aie2_suspend_fw(ndev); xdna_mailbox_stop_channel(ndev->aie.mgmt_chann); stop_psp: aie_psp_stop(ndev->aie.psp_hdl); fini_smu: aie2_smu_fini(ndev); free_channel: xdna_mailbox_free_channel(ndev->aie.mgmt_chann); ndev->aie.mgmt_chann = NULL; disable_dev: pci_disable_device(pdev); These error paths power down the hardware by stopping the firmware but fail to roll back the kernel's state tracking with pci_set_power_state(pdev, PCI_D3hot). Since the PM core continues to track the device as D0, won't the next suspend attempt try a D0-to-D3hot transition on the unreachable device and trigger the exact inaccessible warning this patch is meant to fix? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
