From: "Pavel Knoblokh" <knoblokh@gmail.com>
Subject: [PATCH] ahci(4) Do not write to read-only registers
To: <tech@openbsd.org>
Cc: <dlg@openbsd.org>
Date: Sat, 03 Aug 2024 15:07:45 +1000

Hi,

This patch removes the code that likely never worked as intended. I came
across this while trying to understand how the AHCI driver works.

The commit that introduced the code says "Save BIOS configured
parameters over reset.  Always enable staggered spin-up.", but the code
obviously has a bug that clears all but one bit in the capabilities
register. If that had ever worked, it would have broken things.

So I checked the AHCI spec [1], and it says all capabilities bits are
read-only (section 3.1.1), but some are "loaded by the BIOS prior to OS
initialization". Just in case, I checked that it has always been the
case since the spec version 0.95 that was contemporary to the commit.

The same applies to the "ports implemented" register (section 3.1.4).

I have three different controllers to play with: Intel ADL-N AHCI,
JMicron JMB585 and ASMedia ASM1166.

It is not relevant to this patch, but just for visibility, I have a
timeout problem with the latter two that I am trying to investigate, but
the problem occurs at a later stage (and does not occur in Linux and
FreeBSD). I believe this is the same problem as [2].

So I tested the controllers, and they all follow the spec, that is, I
can change register values in the BIOS settings, but after the system
boots up and the registers are loaded by the BIOS, it is not possible to
change them programmatically.

I used the following code to test them:

---
printf("!!! caps to write: 0x%x\n", cap);
ahci_write(sc, AHCI_REG_CAP, cap);
delay(1000000);
cap = ahci_read(sc, AHCI_REG_CAP);
printf("!!! caps after restore: 0x%x\n", cap);

pi = 1;
printf("!!! ports implemented to write: 0x%x\n", pi);
ahci_write(sc, AHCI_REG_PI, pi);
delay(1000000);
pi = ahci_read(sc, AHCI_REG_PI);
printf("!!! ports implemented after restore: 0x%x\n", pi);
---

The results are below:

Intel ADL-N AHCI
!!! caps to write: 0x8000000
!!! caps after restore: 0xe534ff00
!!! ports implemented to write: 0x1
!!! ports implemented after restore: 0x0 (prior, I disabled the single
port in the BIOS settings)

JMicron JMB585
!!! caps to write: 0x8000000
!!! caps after restore: 0xef33ff84
!!! ports implemented to write: 0x1
!!! ports implemented after restore: 0x1f

ASMedia ASM1166
!!! caps to write: 0x8000000
!!! caps after restore: 0xee349f3f
!!! ports implemented to write: 0x1
!!! ports implemented after restore: 0xffffff3f

As can be seen, the read-only registers never store the written values.

[1] https://www.intel.com/content/www/us/en/io/serial-ata/serial-ata-ahci-spec-rev1-3-1.html
[2] https://marc.info/?l=openbsd-misc&m=172207535832024&w=2

Regards,
Pavel Knoblokh


Index: sys/dev/ic/ahci.c
===================================================================
RCS file: /cvs/src/sys/dev/ic/ahci.c,v
diff -u -p -u -p -r1.41 ahci.c
--- sys/dev/ic/ahci.c	28 May 2024 01:37:53 -0000	1.41
+++ sys/dev/ic/ahci.c	2 Aug 2024 13:29:04 -0000
@@ -397,18 +397,12 @@ ahci_activate(struct device *self, int a
 int
 ahci_init(struct ahci_softc *sc)
 {
-	u_int32_t			reg, cap, pi;
+	u_int32_t			reg;
 	const char			*revision;
 
 	DPRINTF(AHCI_D_VERBOSE, " GHC 0x%b", ahci_read(sc, AHCI_REG_GHC),
 	    AHCI_FMT_GHC);
 
-	/* save BIOS initialised parameters, enable staggered spin up */
-	cap = ahci_read(sc, AHCI_REG_CAP);
-	cap &= AHCI_REG_CAP_SMPS;
-	cap |= AHCI_REG_CAP_SSS;
-	pi = ahci_read(sc, AHCI_REG_PI);
-
 	if (ISSET(AHCI_REG_GHC_AE, ahci_read(sc, AHCI_REG_GHC))) {
 		/* reset the controller */
 		ahci_write(sc, AHCI_REG_GHC, AHCI_REG_GHC_HR);
@@ -421,10 +415,6 @@ ahci_init(struct ahci_softc *sc)
 
 	/* enable ahci (global interrupts disabled) */
 	ahci_write(sc, AHCI_REG_GHC, AHCI_REG_GHC_AE);
-
-	/* restore parameters */
-	ahci_write(sc, AHCI_REG_CAP, cap);
-	ahci_write(sc, AHCI_REG_PI, pi);
 
 	/* check the revision */
 	reg = ahci_read(sc, AHCI_REG_VS);