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

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.

---

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(+)

index 08ab56e9556..6f374fd79f5 100644
@@ -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

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

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.