[PATCH] ata: libata-eh: Fix link state check for IDE/PATA ports

[PATCH] ata: libata-eh: Fix link state check for IDE/PATA ports

[Damien Le Moal]

Commit 4371fe1ba400 ("ata: libata-eh: Avoid unnecessary resets when
revalidating devices") replaced the call to ata_phys_link_offline() in
ata_eh_revalidate_and_attach() with the new function
ata_eh_link_established() which relaxes the checks on a device link
state to account for low power mode transitions. However, this changed
assumed that the device port has a valid scr_read method to obstain the
SSTATUS register for the port. This is not always the case, especially
with older IDE/PATA adapters (e.g. as emulated with qemu). For such
adapter, ata_eh_link_established() will always return false, causing
ata_eh_revalidate_and_attach() to go into its error path and ultimately
to the device being disabled.

Avoid this by restoring the previous behavior, which is to assume that
the link is online if reading the port SSTATUS register fails.

Reported-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Fixes: 4371fe1ba400 ("ata: libata-eh: Avoid unnecessary resets when revalidating devices")
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/libata-eh.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 2946ae6d4b2c..354d2c0abcf3 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -2089,8 +2089,13 @@ static bool ata_eh_link_established(struct ata_link *link)
 	u32 sstatus;
 	u8 det, ipm;
 
+	/*
+	 * For old IDE/PATA adapters that do not have a valid scr_read method,
+	 * or if reading the SSTATUS register fails, assume that the device is
+	 * present. Device probe will determine if that is really the case.
+	 */
 	if (sata_scr_read(link, SCR_STATUS, &sstatus))
-		return false;
+		return true;
 
 	det = sstatus & 0x0f;
 	ipm = (sstatus >> 8) & 0x0f;
-- 
2.50.1

Re: [PATCH] ata: libata-eh: Fix link state check for IDE/PATA ports

[Shinichiro Kawasaki]

On Aug 13, 2025 / 18:27, Damien Le Moal wrote:
> Commit 4371fe1ba400 ("ata: libata-eh: Avoid unnecessary resets when
> revalidating devices") replaced the call to ata_phys_link_offline() in
> ata_eh_revalidate_and_attach() with the new function
> ata_eh_link_established() which relaxes the checks on a device link
> state to account for low power mode transitions. However, this changed
> assumed that the device port has a valid scr_read method to obstain the
> SSTATUS register for the port. This is not always the case, especially
> with older IDE/PATA adapters (e.g. as emulated with qemu). For such
> adapter, ata_eh_link_established() will always return false, causing
> ata_eh_revalidate_and_attach() to go into its error path and ultimately
> to the device being disabled.
> 
> Avoid this by restoring the previous behavior, which is to assume that
> the link is online if reading the port SSTATUS register fails.
> 
> Reported-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> Fixes: 4371fe1ba400 ("ata: libata-eh: Avoid unnecessary resets when revalidating devices")
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>

Thank you for the fix. I confirmed that this patch avoids the failure of
blktests test case scsi/006 with QEMU IDE/PATA device that I reported [1].

Tested-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>

[1] https://lore.kernel.org/linux-block/suhzith2uj75uiprq4m3cglvr7qwm3d7gi4tmjeohlxl6fcmv3@zu6zym6nmvun/T/#u

Re: [PATCH] ata: libata-eh: Fix link state check for IDE/PATA ports

[Niklas Cassel]

On Wed, Aug 13, 2025 at 06:27:07PM +0900, Damien Le Moal wrote:
> Commit 4371fe1ba400 ("ata: libata-eh: Avoid unnecessary resets when
> revalidating devices") replaced the call to ata_phys_link_offline() in
> ata_eh_revalidate_and_attach() with the new function
> ata_eh_link_established() which relaxes the checks on a device link
> state to account for low power mode transitions. However, this changed

s/changed/change/

> assumed that the device port has a valid scr_read method to obstain the

s/obstain/obtain/

> SSTATUS register for the port. This is not always the case, especially
> with older IDE/PATA adapters (e.g. as emulated with qemu). For such

s/emulated with qemu/PATA emulated with QEMU/

(QEMU also support AHCI emulation)

> adapter, ata_eh_link_established() will always return false, causing
> ata_eh_revalidate_and_attach() to go into its error path and ultimately
> to the device being disabled.
> 
> Avoid this by restoring the previous behavior, which is to assume that
> the link is online if reading the port SSTATUS register fails.
> 
> Reported-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> Fixes: 4371fe1ba400 ("ata: libata-eh: Avoid unnecessary resets when revalidating devices")
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>

Reviewed-by: Niklas Cassel <cassel@kernel.org>

Re: [PATCH] ata: libata-eh: Fix link state check for IDE/PATA ports

[Sergey Shtylyov]

On 8/13/25 12:27 PM, Damien Le Moal wrote:

> Commit 4371fe1ba400 ("ata: libata-eh: Avoid unnecessary resets when
> revalidating devices") replaced the call to ata_phys_link_offline() in
> ata_eh_revalidate_and_attach() with the new function
> ata_eh_link_established() which relaxes the checks on a device link
> state to account for low power mode transitions. However, this changed
> assumed that the device port has a valid scr_read method to obstain the
> SSTATUS register for the port. This is not always the case, especially

   Hum, another nit... Don't the SATA specs call this register SStatus?

> with older IDE/PATA adapters (e.g. as emulated with qemu). For such
> adapter, ata_eh_link_established() will always return false, causing
> ata_eh_revalidate_and_attach() to go into its error path and ultimately
> to the device being disabled.
> 
> Avoid this by restoring the previous behavior, which is to assume that
> the link is online if reading the port SSTATUS register fails.
> 
> Reported-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> Fixes: 4371fe1ba400 ("ata: libata-eh: Avoid unnecessary resets when revalidating devices")
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/ata/libata-eh.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 2946ae6d4b2c..354d2c0abcf3 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -2089,8 +2089,13 @@ static bool ata_eh_link_established(struct ata_link *link)
>  	u32 sstatus;
>  	u8 det, ipm;
>  
> +	/*
> +	 * For old IDE/PATA adapters that do not have a valid scr_read method,
> +	 * or if reading the SSTATUS register fails, assume that the device is

   Same comment here... 

> +	 * present. Device probe will determine if that is really the case.
> +	 */
>  	if (sata_scr_read(link, SCR_STATUS, &sstatus))
> -		return false;
> +		return true;
>  
>  	det = sstatus & 0x0f;
>  	ipm = (sstatus >> 8) & 0x0f;

MBR, Sergey