[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.