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

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

[Kyrylo Biryuk]

ahci_do_softreset() disables FBS before resetting a device behind a port
multiplier, as required by AHCI 1.2 section 9.3.9, and re-enables it only
when that softreset succeeds. If it fails, for example because a
hot-plugged drive is still spinning up ("softreset failed (device not
ready)"), EH escalates to a hardreset of the PMP link, and nothing turns
FBS back on: only ahci_pmp_attach() does, which runs at the next boot.
The port keeps working with command-based switching until then.

Seen on a Marvell 88SE9235 (1b4b:9235) with Marvell 88SM9705 port
multipliers (1b4b:9705, in a Synology DX1222 expansion unit): after
hot-plugging a drive, the kernel logged "FBS is disabled" followed by
"softreset failed (device not ready)" and a PMP hardreset, and never
"FBS is enabled" again (twice, on two different ports).

At the end of ahci_error_handler(), re-enable FBS if a PMP is still
attached and FBS is supported but disabled.

Tested on 7.2.6 with the patch built as an out-of-tree libahci (the stock
ahci module binds to it, symbol CRCs are unchanged). Hot-plugging a drive
into a free port of the multiplier then logged "FBS is disabled",
"softreset failed (device not ready)" and a PMP hardreset, followed by
"re-enabling FBS after error handling" and "FBS is enabled". Afterwards
two other drives behind the same multiplier read 259 + 259 MB/s in
parallel (O_DIRECT, 4 MiB blocks), against 268 MB/s for one drive alone,
so their commands overlap again.

Fixes: 89dafa20f3da ("ahci: disabled FBS prior to issuing software reset")
Assisted-by: LLM
Signed-off-by: Kyrylo Biryuk <illofspeed@gmail.com>
---

Notes:
    v3: no code changes. From and Signed-off-by now carry my full name
    instead of the GitHub handle "illofspeed" used for v2 (same author).
    
    The controller is passed through (VFIO) to a KVM guest; the tested kernel is the
    guest's. The two earlier occurrences without the patch were on ata8 and ata10 of the
    same controller; this test was on ata7.

 drivers/ata/libahci.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 08ab56e9556..6f374fd79f5 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -2233,6 +2233,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);
+		}
+	}
+
 	if (!ata_dev_enabled(ap->link.device))
 		hpriv->stop_engine(ap);
 }
-- 
2.43.0

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

[Kyrylo Biryuk]

An automated review (Sashiko) of this patch pointed out that
ahci_enable_fbs() writes PxFBS.EN after hpriv->stop_engine(), which
clears PxCMD.ST but leaves PxCMD.FRE set, and asked whether that could
upset strict controllers.

For the record: this patch only adds a third caller of the existing
helper. ahci_pmp_attach() (every boot with a port multiplier, after
ahci_start_port() has enabled FIS reception) and ahci_do_softreset()
(after a successful fan-out softreset) already call ahci_enable_fbs()
with the port in the same state, so the patch does not change when or
how PxFBS.EN is written. If the helper should also stop FIS reception
around the write, that would be a separate change to ahci_enable_fbs()
and ahci_disable_fbs(); I am happy to look into it if the maintainers
think it is worth doing.

Tested on the 88SE9235 + 88SM9705 setup from the commit message: the
hot-plug now ends with "re-enabling FBS after error handling" and
"FBS is enabled", and parallel reads behind the multiplier scale again.

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

[Niklas Cassel]

On Sun, Oct 11, 2026 at 08:02:36PM +0200, Kyrylo Biryuk wrote:
> ahci_do_softreset() disables FBS before resetting a device behind a port
> multiplier, as required by AHCI 1.2 section 9.3.9, and re-enables it only
> when that softreset succeeds. If it fails, for example because a
> hot-plugged drive is still spinning up ("softreset failed (device not
> ready)"), EH escalates to a hardreset of the PMP link, and nothing turns
> FBS back on: only ahci_pmp_attach() does, which runs at the next boot.
> The port keeps working with command-based switching until then.
> 
> Seen on a Marvell 88SE9235 (1b4b:9235) with Marvell 88SM9705 port
> multipliers (1b4b:9705, in a Synology DX1222 expansion unit): after
> hot-plugging a drive, the kernel logged "FBS is disabled" followed by
> "softreset failed (device not ready)" and a PMP hardreset, and never
> "FBS is enabled" again (twice, on two different ports).
> 
> At the end of ahci_error_handler(), re-enable FBS if a PMP is still
> attached and FBS is supported but disabled.
> 
> Tested on 7.2.6 with the patch built as an out-of-tree libahci (the stock
> ahci module binds to it, symbol CRCs are unchanged). Hot-plugging a drive
> into a free port of the multiplier then logged "FBS is disabled",
> "softreset failed (device not ready)" and a PMP hardreset, followed by
> "re-enabling FBS after error handling" and "FBS is enabled". Afterwards
> two other drives behind the same multiplier read 259 + 259 MB/s in
> parallel (O_DIRECT, 4 MiB blocks), against 268 MB/s for one drive alone,
> so their commands overlap again.

This whole testing paragraph belongs after the '---'.
Having specific information about exact kernel version, or if the module
was built as an out of tree module, or built-in, is not information that
should be in the commit log itself.


> 
> Fixes: 89dafa20f3da ("ahci: disabled FBS prior to issuing software reset")
> Assisted-by: LLM
> Signed-off-by: Kyrylo Biryuk <illofspeed@gmail.com>
> ---
> 
> Notes:
>     v3: no code changes. From and Signed-off-by now carry my full name
>     instead of the GitHub handle "illofspeed" used for v2 (same author).
>     
>     The controller is passed through (VFIO) to a KVM guest; the tested kernel is the
>     guest's. The two earlier occurrences without the patch were on ata8 and ata10 of the
>     same controller; this test was on ata7.
> 
>  drivers/ata/libahci.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index 08ab56e9556..6f374fd79f5 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -2233,6 +2233,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");

I would drop this print.


Kind regards,
Niklas

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

[Niklas Cassel]

On Sun, Oct 11, 2026 at 08:31:26PM +0200, Kyrylo Biryuk wrote:
> An automated review (Sashiko) of this patch pointed out that
> ahci_enable_fbs() writes PxFBS.EN after hpriv->stop_engine(), which
> clears PxCMD.ST but leaves PxCMD.FRE set, and asked whether that could
> upset strict controllers.
> 
> For the record: this patch only adds a third caller of the existing
> helper. ahci_pmp_attach() (every boot with a port multiplier, after
> ahci_start_port() has enabled FIS reception) and ahci_do_softreset()
> (after a successful fan-out softreset) already call ahci_enable_fbs()
> with the port in the same state, so the patch does not change when or
> how PxFBS.EN is written. If the helper should also stop FIS reception
> around the write, that would be a separate change to ahci_enable_fbs()
> and ahci_disable_fbs(); I am happy to look into it if the maintainers
> think it is worth doing.
> 
> Tested on the 88SE9235 + 88SM9705 setup from the commit message: the
> hot-plug now ends with "re-enabling FBS after error handling" and
> "FBS is enabled", and parallel reads behind the multiplier scale again.

Looking at:
3.3.16 Offset 40h: PxFBS: Port x FIS-based Switching Control

Enable (EN): When set to ‘1’, a Port Multiplier is attached and the HBA shall
use FIS-based switching to communicate with it. When cleared to ‘0’, FIS-based
switching is not being used. Software shall only change the value of the EN bit
when PxCMD.ST is cleared to ‘0’


PxCMD.ST will be cleared to 0 by the hpriv->stop_engine() in ahci_enable_fbs().

I don't see any mention that PxCMD.FRE needs to be disabled in addition to
PxCMD.ST.

Presumably, if PxCMD.ST is disabled, why would you receive any FIS?

Sure the spec says that PxCMD.FRE should be set before PxCMD.ST,
but I can't find anywhere in the spec where it says that you need to
clear PxCMD.FRE before each time you re-start the engine (set PxCMD.ST).

So I think Sashiko is hallucinating here.


Kind regards,
Niklas