From: "Pavel Knoblokh" <knoblokh@gmail.com>
Subject: Re: [PATCH] ahci(4) Do not write to read-only registers
To: "Pavel Knoblokh" <knoblokh@gmail.com>, <tech@openbsd.org>
Cc: <dlg@openbsd.org>, <kettenis@openbsd.org>
Date: Sun, 13 Oct 2024 14:00:05 +1000

Bumping this up after the release, so that we could have more time to
steep this code on dev machines. :) Ok?

I also checked that Linux doesn't try to do anything like this, and
Dragonfly fixed the logical error, though writing to r/o registers likely
never worked anyway.

Best regards,
Pavel Knoblokh

On Sat Aug 3, 2024 at 3:07 PM AEST, Pavel Knoblokh wrote:
> 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);