Re: [PATCH v3] ata: libahci: re-enable FBS after error handling on a port multiplier

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ata: ahci_enable_fbs() modifies PxFBS.EN without clearing PxCMD.FRE (AHCI spec violation)
--

commit 9f8000187fcdaf491c78af72f7fe5bba9f1d33a6
Author: Kyrylo Biryuk <illofspeed@gmail.com>

ata: libahci: re-enable FBS after error handling on a port multiplier

This commit re-enables FBS at the end of the error handler for port
multipliers. It addresses an issue where a softreset failure escalates
to a PMP hardreset, which leaves FBS disabled until the next boot.

> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -2221,6 +2221,23 @@ void ahci_error_handler(struct ata_port *ap)
>  
>  	sata_pmp_error_handler(ap);
>  
> +	/*
> +	 * ahci_do_softreset() turns FBS off before resetting a fan-out link
> +	 * and turns it back on only if that reset succeeds. When it fails
> +	 * (e.g. a hot-plugged drive is still spinning up), EH escalates to a
> +	 * PMP hardreset, which never re-enables FBS, so the port would keep
> +	 * command-based switching until the PMP is attached again (reboot).
> +	 * Restore FBS once recovery is done and the PMP is still attached.
> +	 */
> +	if (sata_pmp_attached(ap) && ata_dev_enabled(ap->link.device)) {
> +		struct ahci_port_priv *pp = ap->private_data;
> +
> +		if (pp->fbs_supported && !pp->fbs_enabled) {
> +			ata_port_info(ap, "re-enabling FBS after error handling\n");
> +			ahci_enable_fbs(ap);

[Severity: Medium]
This problem wasn't introduced by this patch, but this new call to
ahci_enable_fbs() exposes a preexisting AHCI spec violation.

AHCI Spec 1.2+ section 9.3.6 requires that PxFBS.EN must not be set to '1'
while PxCMD.ST or PxCMD.FRE are '1'.

In ahci_enable_fbs(), we call hpriv->stop_engine():

drivers/ata/libahci.c:ahci_enable_fbs() {
    ...
	rc = hpriv->stop_engine(ap);
	if (rc)
		return;

	writel(fbs | PORT_FBS_EN, port_mmio + PORT_FBS);
    ...
}

However, stop_engine() only clears PxCMD.ST (PORT_CMD_START). It does not
clear PxCMD.FRE (PORT_CMD_FIS_RX).

Since ahci_stop_fis_rx() is not called here to clear PORT_CMD_FIS_RX, toggling
PORT_FBS_EN modifies PxFBS.EN while PxCMD.FRE is still '1'.

Does this run the risk of causing controller instability or command failures
on strict hardware implementations?

> +		}
> +	}
> +
>  	if (!ata_dev_enabled(ap->link.device))
>  		hpriv->stop_engine(ap);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011180236.735-1-illofspeed@gmail.com?part=1