Re: New "ahcisata0 port 1: device present" messages with NetBSD 9

[Jaromír Doleček <jaromir.dolecek%gmail.com@localhost>]

All right, let's do that. Penalising quirky old gear makes more sense
than penalising new gear.

I didn't like that the controller approach means we penalise ALL
drives on the controller. However, it's better than penalising EVO
drives everywhere, when we have evidence it works without errors on
other controllers.

Please go ahead.

Jaromir

Le ven. 17 janv. 2020 à 10:24, Simon Burge <simonb%netbsd.org@localhost> a écrit :
>
> matthew green wrote (in kern/54855):
>
> > most people with 860 EVOs are successful with NCQ, i'm one
> > of them with a 500GB model, with netbsd-9 for months now,
> > and they're a wildly successful line i'm sure many others
> > also have working happily with NCQ.
> >
> > we don't know what the real issue is.  perhaps it is a
> > problem that the controller had that these drives trigger,
> > rather than a problem with them, so perhaps we should
> > disable it on the controller instead of the drive, if
> > we're going to pick one of the two triggers here.
>
> Izumi Tsutsui wrote:
>
> > Note mine is:
> > >> ahcisata0 at pci0 dev 18 function 0: vendor 1002 product 4380 (rev. 0x00)
> >  [ ... ]
> > >> 000:18:0: ATI Technologies SB600 SATA Controller (SATA mass storage, AHCI 1.0)
> >
> > Note it already has AHCI_QUIRK_BADPM, but the Samsung EVO 860 NCQ problem
> > should be an independent quirk?
> >  https://nxr.netbsd.org/xref/src/sys/dev/ic/ahcisatavar.h?r=1.23#57
> >  https://nxr.netbsd.org/xref/src/sys/dev/pci/ahcisata_pci.c?r=1.56#59
>
> Using the approach suggested by mrg, where we "penalise" the (older)
> AMD chipsets that exhibit the problem rather than the more recent and
> popular Samsung EVO 860 disks, I've added a new AHCI_QUIRK_BADNCQ quirk
> and enabled it for the same AMD chipsets that had the AHCI_QUIRK_BADPM
> quirk.  I do wonder, out of caution, if maybe this quirk should be
> enabled for PCI_PRODUCT_ATI_SB600_SATA_1 as well?
>
> Before patch:
>
> wd0(ahcisata0:0:0): using PIO mode 4, DMA mode 2, Ultra-DMA mode 6 (Ultra/133) (using DMA), NCQ (31 tags)
>
> After patch:
>
> wd0(ahcisata0:0:0): using PIO mode 4, DMA mode 2, Ultra-DMA mode 6 (Ultra/133) (using DMA), WRITE DMA FUA EXT
>
> Jaromir - are you happy with this alternate approach?  If you are
> happy with this I'll commit this patch and revert your EVO 860 quirk
> and request a pull-up to the netbsd-9 branch.
>
> tsutsui - are you able to test the patch too?
>
> Cheers,
> Simon.
>
> --
> Index: sys/dev/ic/ahcisata_core.c
> ===================================================================
> RCS file: /cvsroot/src/sys/dev/ic/ahcisata_core.c,v
> retrieving revision 1.75.4.2
> diff -d -p -u -r1.75.4.2 ahcisata_core.c
> --- sys/dev/ic/ahcisata_core.c  24 Dec 2019 17:34:33 -0000      1.75.4.2
> +++ sys/dev/ic/ahcisata_core.c  17 Jan 2020 08:45:41 -0000
> @@ -276,6 +276,11 @@ ahci_attach(struct ahci_softc *sc)
>                     "ignoring broken port multiplier support\n");
>                 sc->sc_ahci_cap &= ~AHCI_CAP_SPM;
>         }
> +       if (sc->sc_ahci_quirks & AHCI_QUIRK_BADNCQ) {
> +               aprint_verbose_dev(sc->sc_atac.atac_dev,
> +                   "ignoring broken NCQ support\n");
> +               sc->sc_ahci_cap &= ~AHCI_CAP_NCQ;
> +       }
>         sc->sc_atac.atac_nchannels = (sc->sc_ahci_cap & AHCI_CAP_NPMASK) + 1;
>         sc->sc_ncmds = ((sc->sc_ahci_cap & AHCI_CAP_NCS) >> 8) + 1;
>         ahci_rev = AHCI_READ(sc, AHCI_VS);
> Index: sys/dev/ic/ahcisatavar.h
> ===================================================================
> RCS file: /cvsroot/src/sys/dev/ic/ahcisatavar.h,v
> retrieving revision 1.22
> diff -d -p -u -r1.22 ahcisatavar.h
> --- sys/dev/ic/ahcisatavar.h    14 Jan 2019 21:29:56 -0000      1.22
> +++ sys/dev/ic/ahcisatavar.h    17 Jan 2020 08:45:41 -0000
> @@ -59,6 +59,7 @@ struct ahci_softc {
>  #define AHCI_PCI_QUIRK_BAD64   __BIT(1)  /* broken 64-bit DMA */
>  #define AHCI_QUIRK_BADPMP      __BIT(2)  /* broken PMP support, ignore */
>  #define AHCI_QUIRK_SKIP_RESET  __BIT(4)  /* skip drive reset sequence */
> +#define AHCI_QUIRK_BADNCQ      __BIT(5)  /* possibly broken NCQ support, ignore */
>
>         uint32_t sc_ahci_cap;   /* copy of AHCI_CAP */
>         int sc_ncmds; /* number of command slots */
> Index: sys/dev/pci/ahcisata_pci.c
> ===================================================================
> RCS file: /cvsroot/src/sys/dev/pci/ahcisata_pci.c,v
> retrieving revision 1.55.4.1
> diff -d -p -u -r1.55.4.1 ahcisata_pci.c
> --- sys/dev/pci/ahcisata_pci.c  23 Oct 2019 18:09:18 -0000      1.55.4.1
> +++ sys/dev/pci/ahcisata_pci.c  17 Jan 2020 08:45:41 -0000
> @@ -179,17 +179,17 @@ static const struct ahci_pci_quirk ahci_
>             AHCI_PCI_QUIRK_FORCE },
>         /* ATI SB600 AHCI 64-bit DMA only works on some boards/BIOSes */
>         { PCI_VENDOR_ATI, PCI_PRODUCT_ATI_SB600_SATA_1,
> -           AHCI_PCI_QUIRK_BAD64 | AHCI_QUIRK_BADPMP },
> +           AHCI_PCI_QUIRK_BAD64 | AHCI_QUIRK_BADPMP | AHCI_QUIRK_BADNCQ },
>         { PCI_VENDOR_ATI, PCI_PRODUCT_ATI_SB700_SATA_AHCI,
> -           AHCI_QUIRK_BADPMP },
> +           AHCI_QUIRK_BADPMP | AHCI_QUIRK_BADNCQ },
>         { PCI_VENDOR_ATI, PCI_PRODUCT_ATI_SB700_SATA_RAID,
> -           AHCI_QUIRK_BADPMP },
> +           AHCI_QUIRK_BADPMP | AHCI_QUIRK_BADNCQ },
>         { PCI_VENDOR_ATI, PCI_PRODUCT_ATI_SB700_SATA_RAID5,
> -           AHCI_QUIRK_BADPMP },
> +           AHCI_QUIRK_BADPMP | AHCI_QUIRK_BADNCQ },
>         { PCI_VENDOR_ATI, PCI_PRODUCT_ATI_SB700_SATA_AHCI2,
> -           AHCI_QUIRK_BADPMP },
> +           AHCI_QUIRK_BADPMP | AHCI_QUIRK_BADNCQ },
>         { PCI_VENDOR_ATI, PCI_PRODUCT_ATI_SB700_SATA_STORAGE,
> -           AHCI_QUIRK_BADPMP },
> +           AHCI_QUIRK_BADPMP | AHCI_QUIRK_BADNCQ },
>         { PCI_VENDOR_VIATECH, PCI_PRODUCT_VIATECH_VT8237R_SATA,
>             AHCI_QUIRK_BADPMP },
>         { PCI_VENDOR_VIATECH, PCI_PRODUCT_VIATECH_VT8251_SATA,