Re: [PATCH v8 phy-next 02/31] ata: add <linux/pm_runtime.h> where missing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Callers of `ahci_rpm_get_port()` unconditionally ignore the return value of `pm_runtime_get_sync()`, potentially accessing suspended hardware.
- [High] AHCI interrupt handlers process interrupts without checking if the device is runtime-active, leading to spurious handling and MMIO reads on suspended devices.
--

commit 4c8bdcda88f27ad5b4d7ed5e7d48bab865cfb2dd
Author: Vladimir Oltean <vladimir.oltean@nxp.com>

ata: add <linux/pm_runtime.h> where missing

This patch adds the <linux/pm_runtime.h> header to several AHCI drivers
that use runtime PM operations but previously relied on an indirect
inclusion from <linux/phy/phy.h>.

> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 1d73a53370cf3..1396a53bd6df3 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -26,6 +26,7 @@
>  #include <linux/device.h>
>  #include <linux/dmi.h>
>  #include <linux/gfp.h>
> +#include <linux/pm_runtime.h>
>  #include <scsi/scsi_host.h>
>  #include <scsi/scsi_cmnd.h>
>  #include <linux/libata.h>

While reviewing the addition of runtime PM headers, I noticed a pre-existing
regression with how interrupts are handled in this driver.

Could ahci_single_level_irq_intr() be invoked for a runtime-suspended
device due to shared interrupts (IRQF_SHARED)?

If the device is powered down, would reading HOST_IRQ_STAT return 0xffffffff
and cause the handler to call ata_qc_complete_multiple() with fatal errors
(like PORT_IRQ_FREEZE and PORT_IRQ_ERROR), eventually triggering severe ATA
error handling and port resets?

Would using pm_runtime_get_if_active() before accessing the hardware registers
prevent this issue?

[ ... ]

> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index c79abdfcd7a9b..e0de4703a4f2d 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -30,6 +30,7 @@
>  #include <scsi/scsi_cmnd.h>
>  #include <linux/libata.h>
>  #include <linux/pci.h>
> +#include <linux/pm_runtime.h>
>  #include "ahci.h"
>  #include "libata.h"
>  

I also noticed another pre-existing regression in this driver.

Does ahci_rpm_get_port() need to have its return value checked by callers?

It looks like it directly returns the result of pm_runtime_get_sync(), but
callers like ahci_show_port_cmd(), ahci_read_em_buffer(), and
ahci_port_resume() seem to unconditionally ignore this return value.

If pm_runtime_get_sync() fails, could the callers proceed to access
memory-mapped I/O registers (such as PORT_CMD or HOST_EM_CTL) on a suspended
device, potentially causing a synchronous external abort on ARM or invalid
reads on x86?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260505100523.1922388-1-vladimir.oltean@nxp.com?part=2