Re: [PATCH] ata: libata: avoid long timeouts on hot-unplugged SATA DAS

[Damien Le Moal <dlemoal@kernel.org>]

On 11/20/25 4:35 PM, Henry Tseng wrote:
> When a SATA DAS enclosure is connected behind a Thunderbolt PCIe
> switch, hot-unplugging the whole enclosure causes pciehp to tear down
> the PCI hierarchy before the SCSI layer issues SYNCHRONIZE CACHE and
> START STOP UNIT for the disks.
> 
> libata still queues these commands and the AHCI driver tries to access
> the HBA registers even though the PCI channel is already offline. This
> results in a series of timeouts and error recovery attempts, e.g.:
> 
>   [  824.778346] pcieport 0000:00:07.0: pciehp: Slot(14): Link Down
>   [  891.612720] ata8.00: qc timeout after 5000 msecs (cmd 0xec)
>   [  902.876501] ata8.00: qc timeout after 10000 msecs (cmd 0xec)
>   [  934.107998] ata8.00: qc timeout after 30000 msecs (cmd 0xec)
>   [  936.206431] sd 7:0:0:0: [sda] Synchronize Cache(10) failed:
>       Result: hostbyte=DID_BAD_TARGET driverbyte=DRIVER_OK
>   ...
>   [ 1006.298356] ata1.00: qc timeout after 5000 msecs (cmd 0xec)
>   [ 1017.561926] ata1.00: qc timeout after 10000 msecs (cmd 0xec)
>   [ 1048.791790] ata1.00: qc timeout after 30000 msecs (cmd 0xec)
>   [ 1050.890035] sd 0:0:0:0: [sdb] Synchronize Cache(10) failed:
>       Result: hostbyte=DID_BAD_TARGET driverbyte=DRIVER_OK
> 
> With this patch applied, the same hot-unplug looks like:
> 
>   [   55.452526] pcieport 0000:00:07.0: pciehp: Slot(14): Link Down
>   [   55.496563] sd 7:0:0:0: [sda] Synchronize Cache(10) failed:
>       Result: hostbyte=DID_BAD_TARGET driverbyte=DRIVER_OK
>   [   55.617450] sd 0:0:0:0: [sdb] Synchronize Cache(10) failed:
>       Result: hostbyte=DID_BAD_TARGET driverbyte=DRIVER_OK
> 
> In this test setup with two disks, the hot-unplug sequence shrinks from
> about 226 seconds (~3.8 minutes) between the Link Down event and the
> last SYNCHRONIZE CACHE failure to under 200 ms. Without this patch the
> total delay grows roughly with the number of disks, because each disk
> gets its own SYNCHRONIZE CACHE and qc timeout series.
> 
> If the underlying PCI device is already gone, these commands cannot
> succeed anyway. Avoid issuing them by introducing
> ata_port_pci_channel_offline(), which checks pci_channel_offline() for
> PCI-based hosts and is used in both ata_scsi_queuecmd() and
> ata_qc_issue().
> 
> For SCSI-originated commands we now complete the request immediately
> with DID_BAD_TARGET instead of queuing it. For internal ATA commands
> we fail the qc with AC_ERR_SYSTEM early without touching the HBA
> registers.
> 
> With this change, SYNCHRONIZE CACHE issued during hot-unplug fails
> quickly with DID_BAD_TARGET, without qc timeout spam, and the whole
> unplug completes much faster.

Wao... a hot-pluggable AHCI adapter. That is a new one...
And super dangerous with libata & AHCI driver as is: if the drives connected to
this adapter are not also declared as removabe devices and their write cache
disabled, random unplug of the drives will corrupt data/FS on the drive.

I understand what you are trying to fix here, but do not expect this setup to
be reliable without more patches.

See comments below.


> Signed-off-by: Henry Tseng <henrytseng@qnap.com>
> ---
>  drivers/ata/libata-core.c |  3 +++
>  drivers/ata/libata-scsi.c |  3 ++-
>  drivers/ata/libata.h      | 16 ++++++++++++++++
>  3 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 2a210719c4ce..49a59a90e8eb 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5060,6 +5060,9 @@ void ata_qc_issue(struct ata_queued_cmd *qc)
>  		if (ata_sg_setup(qc))
>  			goto sys_err;
>  
> +	if (ata_port_pci_channel_offline(ap))
> +		goto sys_err;
> +
>  	/* if device is sleeping, schedule reset and abort the link */
>  	if (unlikely(qc->dev->flags & ATA_DFLAG_SLEEPING)) {
>  		link->eh_info.action |= ATA_EH_RESET;
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index b43a3196e2be..a49fd4bbf321 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -4388,7 +4388,8 @@ int ata_scsi_queuecmd(struct Scsi_Host *shost, struct scsi_cmnd *cmd)
>  	spin_lock_irqsave(ap->lock, irq_flags);
>  
>  	dev = ata_scsi_find_dev(ap, scsidev);
> -	if (likely(dev))
> +
> +	if (likely(dev) && !ata_port_pci_channel_offline(ap))

libata-scsi implements a SAT (SCSI-to-ATA translation). It should *not* be
bothered with checking the underlying adapter state. So this is the wrong place
to put this.

Strictly speaking, the check for the PCI channel offline should go into
libahci, or any other PCI ATA adapter driver. But since that would be repeating
the code, I think it is acceptable to put this check in libata-core.c, in
ata_qc_issue().

>  		rc = __ata_scsi_queuecmd(cmd, dev);
>  	else {
>  		cmd->result = (DID_BAD_TARGET << 16);
> diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
> index e5b977a8d3e1..ff2eb824e51b 100644
> --- a/drivers/ata/libata.h
> +++ b/drivers/ata/libata.h
> @@ -12,6 +12,8 @@
>  #ifndef __LIBATA_H__
>  #define __LIBATA_H__
>  
> +#include <linux/pci.h>

No. This should not be here. If you do as suggested above, libata-core.c
already includes linux/pci.h. It has functions such as ata_dev_check_adapter()
which already look at PCI devices. So your function
ata_port_pci_channel_offline() should be changed into something more generic,
like this:

static inline bool ata_adapter_is_online(struct ata_port *ap)
{
	struct device *dev;

	if (!ap || !ap->host)
		return false;

	dev = ap->host->dev;
	if (!dev)
		return false;

	if (dev_is_pci(dev) &&
	    pci_channel_offline(to_pci_dev(dev)))
		return false;

	return true;
}

and then, at the top of ata_qc_issue(), add something like:

	if (!ata_adapter_is_online(ap))
		goto sys_err;

> +
>  #define DRV_NAME	"libata"
>  #define DRV_VERSION	"3.00"	/* must be exactly four chars */
>  
> @@ -56,6 +58,20 @@ static inline bool ata_port_eh_scheduled(struct ata_port *ap)
>  	return ap->pflags & (ATA_PFLAG_EH_PENDING | ATA_PFLAG_EH_IN_PROGRESS);
>  }
>  
> +static inline bool ata_port_pci_channel_offline(struct ata_port *ap)
> +{
> +	struct device *dev;
> +
> +	if (!ap || !ap->host)
> +		return false;
> +
> +	dev = ap->host->dev;
> +	if (!dev || !dev_is_pci(dev))
> +		return false;
> +
> +	return pci_channel_offline(to_pci_dev(dev));
> +}

This is too big a function to be inline. Make it a static function in
libata-core.c as suggested above.

> +
>  #ifdef CONFIG_ATA_FORCE
>  extern void ata_force_cbl(struct ata_port *ap);
>  #else


-- 
Damien Le Moal
Western Digital Research