Re: ahci(4): Add support for JMicron JMB585 chipset
Stuart Henderson writes: > That FreeBSD commit prevents using their "hw.ahci.force" tunable on the > device, it's used for attaching as AHCI to certain known chips even if > they're set in legacy IDE mode. > > Does it work to just add the vid/pid to the ahci_devices[] array > without a specific attach function? (like PCI_PRODUCT_ASMEDIA_ASM1061_SATA). After a little discussion off-list with Jonathan Matthew, I think what I've experienced here is actually an SSD that is on it's way to the great silicon graveyard. What I was experiencing was brief periods (sometimes a couple of seconds) of what seemed like I/O related pauses when trying to access files from that disk (the machine remained responsive the whole time to ssh, tmux etc but whatever process was trying to access that disk would hiccup briefly). However, I experienced it *only* with one particular drive (and it's quite an older one) - other drives did not do this whether regardless of NCQ being on or off. I had not experienced this with that drive in other systems previously. But after doing some testing again today I've now experienced the same hang on a completely different machine with a completely different AHCI controller with the drive getting somewhat warm to the touch when idle - so given that the SSD in question is ~7 years old, I'm going to put it out to pasture. I selected NCQ because IIRC it's the first feature knockout mask listed in the header - it just happened to seem like it worked. Me Googling and misreading the FreeBSD commit wasn't the trigger for selecting NCQ anyway - I was just looking for confirmation I wasn't going crazy but apparently I am. :-) Based on that I *think* attaching it just like the ASMedia device will work, but let me test that a little more thoroughly before I send an updated diff. Thanks for the review and sorry for the confusion, Ash
Re: ahci(4): Add support for JMicron JMB585 chipset
> Date: Sun, 25 Jul 2021 12:29:21 +0100 > From: Stuart Henderson > > On 2021/07/25 13:25, Mark Kettenis wrote: > > > Date: Sun, 25 Jul 2021 12:08:09 +0100 > > > From: Stuart Henderson > > > > > > On 2021/07/25 14:55, Jonathan Matthew wrote: > > > > On Thu, Jul 22, 2021 at 10:45:17PM -0400, Ashton Fagg wrote: > > > > > I have two devices here based on the JMicron JMB585 chipset. This diff > > > > > adds the required pcidev IDs and sets disables native command queuing > > > > > in > > > > > the driver. FreeBSD does something similar for this device: > > > > > > > > > > https://github.com/freebsd/freebsd-src/commit/16b766eed443043f4216d50e40ba283e74f992c2 > > > > > > > > Can you explain how you came to the conclusion that you'd need to > > > > disable NCQ? The FreeBSD commit you link to doesn't appear to do that > > > > as they're not applying the AHCI_Q_NONCQ flag to these devices. > > > > Does it not work with NCQ enabled? > > > > > > > > > > That FreeBSD commit prevents using their "hw.ahci.force" tunable on the > > > device, it's used for attaching as AHCI to certain known chips even if > > > they're set in legacy IDE mode. > > > > > > Does it work to just add the vid/pid to the ahci_devices[] array > > > without a specific attach function? (like > > > PCI_PRODUCT_ASMEDIA_ASM1061_SATA). > > > > Hmm, that suggests that the right fix might actually be to add > > pciide(4) on riscv64. > > The FreeBSD commit is "do not allow hw.ahci.force to work with this > device" so kind-of the opposite of that. Actually, the commit doesn't set the AHCI_Q_NOFORCE flag for this particular JMicron device, which seems to indicate that they do allow hw.ahci.force to work with this device. And that means that adding support for it in ahci(4) is fine (and given the 64-bit DMA issue, the most desirable option).
Re: ahci(4): Add support for JMicron JMB585 chipset
> Date: Sun, 25 Jul 2021 13:25:49 +0200 (CEST) > From: Mark Kettenis > > > Date: Sun, 25 Jul 2021 12:08:09 +0100 > > From: Stuart Henderson > > > > On 2021/07/25 14:55, Jonathan Matthew wrote: > > > On Thu, Jul 22, 2021 at 10:45:17PM -0400, Ashton Fagg wrote: > > > > I have two devices here based on the JMicron JMB585 chipset. This diff > > > > adds the required pcidev IDs and sets disables native command queuing in > > > > the driver. FreeBSD does something similar for this device: > > > > > > > > https://github.com/freebsd/freebsd-src/commit/16b766eed443043f4216d50e40ba283e74f992c2 > > > > > > Can you explain how you came to the conclusion that you'd need to > > > disable NCQ? The FreeBSD commit you link to doesn't appear to do that > > > as they're not applying the AHCI_Q_NONCQ flag to these devices. > > > Does it not work with NCQ enabled? > > > > > > > That FreeBSD commit prevents using their "hw.ahci.force" tunable on the > > device, it's used for attaching as AHCI to certain known chips even if > > they're set in legacy IDE mode. > > > > Does it work to just add the vid/pid to the ahci_devices[] array > > without a specific attach function? (like PCI_PRODUCT_ASMEDIA_ASM1061_SATA). > > Hmm, that suggests that the right fix might actually be to add > pciide(4) on riscv64. However, I'm not sure if we want to do that since legacy IDE mode doesn't support 64-bit DMA.
Re: ahci(4): Add support for JMicron JMB585 chipset
On 2021/07/25 13:25, Mark Kettenis wrote: > > Date: Sun, 25 Jul 2021 12:08:09 +0100 > > From: Stuart Henderson > > > > On 2021/07/25 14:55, Jonathan Matthew wrote: > > > On Thu, Jul 22, 2021 at 10:45:17PM -0400, Ashton Fagg wrote: > > > > I have two devices here based on the JMicron JMB585 chipset. This diff > > > > adds the required pcidev IDs and sets disables native command queuing in > > > > the driver. FreeBSD does something similar for this device: > > > > > > > > https://github.com/freebsd/freebsd-src/commit/16b766eed443043f4216d50e40ba283e74f992c2 > > > > > > Can you explain how you came to the conclusion that you'd need to > > > disable NCQ? The FreeBSD commit you link to doesn't appear to do that > > > as they're not applying the AHCI_Q_NONCQ flag to these devices. > > > Does it not work with NCQ enabled? > > > > > > > That FreeBSD commit prevents using their "hw.ahci.force" tunable on the > > device, it's used for attaching as AHCI to certain known chips even if > > they're set in legacy IDE mode. > > > > Does it work to just add the vid/pid to the ahci_devices[] array > > without a specific attach function? (like PCI_PRODUCT_ASMEDIA_ASM1061_SATA). > > Hmm, that suggests that the right fix might actually be to add > pciide(4) on riscv64. The FreeBSD commit is "do not allow hw.ahci.force to work with this device" so kind-of the opposite of that.
Re: ahci(4): Add support for JMicron JMB585 chipset
> Date: Sun, 25 Jul 2021 12:08:09 +0100 > From: Stuart Henderson > > On 2021/07/25 14:55, Jonathan Matthew wrote: > > On Thu, Jul 22, 2021 at 10:45:17PM -0400, Ashton Fagg wrote: > > > I have two devices here based on the JMicron JMB585 chipset. This diff > > > adds the required pcidev IDs and sets disables native command queuing in > > > the driver. FreeBSD does something similar for this device: > > > > > > https://github.com/freebsd/freebsd-src/commit/16b766eed443043f4216d50e40ba283e74f992c2 > > > > Can you explain how you came to the conclusion that you'd need to > > disable NCQ? The FreeBSD commit you link to doesn't appear to do that > > as they're not applying the AHCI_Q_NONCQ flag to these devices. > > Does it not work with NCQ enabled? > > > > That FreeBSD commit prevents using their "hw.ahci.force" tunable on the > device, it's used for attaching as AHCI to certain known chips even if > they're set in legacy IDE mode. > > Does it work to just add the vid/pid to the ahci_devices[] array > without a specific attach function? (like PCI_PRODUCT_ASMEDIA_ASM1061_SATA). Hmm, that suggests that the right fix might actually be to add pciide(4) on riscv64.
Re: ahci(4): Add support for JMicron JMB585 chipset
On 2021/07/25 14:55, Jonathan Matthew wrote: > On Thu, Jul 22, 2021 at 10:45:17PM -0400, Ashton Fagg wrote: > > I have two devices here based on the JMicron JMB585 chipset. This diff > > adds the required pcidev IDs and sets disables native command queuing in > > the driver. FreeBSD does something similar for this device: > > > > https://github.com/freebsd/freebsd-src/commit/16b766eed443043f4216d50e40ba283e74f992c2 > > Can you explain how you came to the conclusion that you'd need to > disable NCQ? The FreeBSD commit you link to doesn't appear to do that > as they're not applying the AHCI_Q_NONCQ flag to these devices. > Does it not work with NCQ enabled? > That FreeBSD commit prevents using their "hw.ahci.force" tunable on the device, it's used for attaching as AHCI to certain known chips even if they're set in legacy IDE mode. Does it work to just add the vid/pid to the ahci_devices[] array without a specific attach function? (like PCI_PRODUCT_ASMEDIA_ASM1061_SATA).
Re: ahci(4): Add support for JMicron JMB585 chipset
On Thu, Jul 22, 2021 at 10:45:17PM -0400, Ashton Fagg wrote: > I have two devices here based on the JMicron JMB585 chipset. This diff > adds the required pcidev IDs and sets disables native command queuing in > the driver. FreeBSD does something similar for this device: > > https://github.com/freebsd/freebsd-src/commit/16b766eed443043f4216d50e40ba283e74f992c2 Can you explain how you came to the conclusion that you'd need to disable NCQ? The FreeBSD commit you link to doesn't appear to do that as they're not applying the AHCI_Q_NONCQ flag to these devices. Does it not work with NCQ enabled?
ahci(4): Add support for JMicron JMB585 chipset
I have two devices here based on the JMicron JMB585 chipset. This diff
adds the required pcidev IDs and sets disables native command queuing in
the driver. FreeBSD does something similar for this device:
https://github.com/freebsd/freebsd-src/commit/16b766eed443043f4216d50e40ba283e74f992c2
I've tested both devices on amd64 and riscv64 and it seems to be working well.
On my amd64 machine dmesg shows:
ahci0 at pci1 dev 0 function 0 "JMicron JMB585 SATA/AHCI" rev 0x00: msi,
AHCI 1.3.1
Thanks,
Ash
Index: sys/dev/pci/pcidevs
===
RCS file: /cvs/src/sys/dev/pci/pcidevs,v
retrieving revision 1.1975
diff -u -p -u -p -r1.1975 pcidevs
--- sys/dev/pci/pcidevs 18 Jul 2021 05:02:08 - 1.1975
+++ sys/dev/pci/pcidevs 23 Jul 2021 01:27:53 -
@@ -6400,6 +6400,7 @@ product ITT ITT3204 0x0002 ITT3204 MPEG
/* JMicron */
product JMICRON JMC250 0x0250 JMC250
product JMICRON JMC260 0x0260 JMC260
+product JMICRON JMB585 0x0585 JMB585 SATA/AHCI
product JMICRON JMB360 0x2360 JMB360 SATA
product JMICRON JMB361 0x2361 JMB361 IDE/SATA
product JMICRON JMB362 0x2362 JMB362 SATA
Index: sys/dev/pci/ahci_pci.c
===
RCS file: /cvs/src/sys/dev/pci/ahci_pci.c,v
retrieving revision 1.15
diff -u -p -u -p -r1.15 ahci_pci.c
--- sys/dev/pci/ahci_pci.c 3 Aug 2018 22:18:13 - 1.15
+++ sys/dev/pci/ahci_pci.c 23 Jul 2021 01:28:02 -
@@ -76,6 +76,8 @@ int ahci_amd_hudson2_attach(struct ahc
struct pci_attach_args *);
intahci_intel_attach(struct ahci_softc *,
struct pci_attach_args *);
+intahci_jmicron_jmb58x_attach(struct ahci_softc *,
+ struct pci_attach_args *);
intahci_samsung_attach(struct ahci_softc *,
struct pci_attach_args *);
@@ -147,6 +149,9 @@ static const struct ahci_device ahci_dev
{ PCI_VENDOR_INTEL, PCI_PRODUCT_INTEL_EP80579_AHCI,
NULL, ahci_intel_attach },
+ { PCI_VENDOR_JMICRON, PCI_PRODUCT_JMICRON_JMB585,
+ NULL, ahci_jmicron_jmb58x_attach },
+
{ PCI_VENDOR_SAMSUNG2, PCI_PRODUCT_SAMSUNG2_S4LN053X01,
NULL, ahci_samsung_attach },
{ PCI_VENDOR_SAMSUNG2, PCI_PRODUCT_SAMSUNG2_XP941,
@@ -288,6 +293,14 @@ ahci_samsung_attach(struct ahci_softc *s
* https://bugzilla.kernel.org/show_bug.cgi?id=89171
*/
sc->sc_flags |= AHCI_F_NO_MSI;
+
+ return (0);
+}
+
+int
+ahci_jmicron_jmb58x_attach(struct ahci_softc *sc, struct pci_attach_args *pa)
+{
+ sc->sc_flags |= AHCI_F_NO_NCQ;
return (0);
}
