re: Aquantia AQC100 issues

[matthew green <mrg%eterna23.net@localhost>]

Rin Okuyama writes:
> Hi Andrius,
>
> If you still have this AQC100 in working condition, can you try this patch?
>
> https://gist.github.com/rokuyama/ab6ba1a0fac7fa15f243d63a99e14f33
>
> I've collected three fibre aq(4) variants (all rev 2), and link status
> interrupts work just fine for me. I think that link intr did not work for
> you, not due to fibre variant, but hardware revision. If this is correct,
> the patch above should work...

this reminded me that my aq(4) doesn't have working link and that
mlelstv suggested to me that the linux driver always uses a tick
timer to also check status, as well as interrupts.  i implemented
this recently and now my aq(4) has link status correctly:


aq(4): always poll for link status

some devices don't have working link status and rather than have
a likely incomplete list of issues, always poll as well as use
the interrupt if possible.

fixes link status on this device:

aq0 at pci5 dev 0 function 0: Aquantia AQC107 10 Gigabit Network Adapter (rev. 0x02)
aq0: Atlantic revision B1, F/W version 3.1.88

(was otherwise functional, just didn't report status, which likely
meant eg, dhcpcd would be upset?)

idea via mlelstv@ from linux.

remove sc_detect_linkstat and rename sc_poll_linkstat to
sc_no_link_intr, as the meaning has changed.  simplify the signature
for aq_setup_msix() and aq_establish_msix_intr(), removing forward
decls that aren't required.  obsolete AQ_FORCE_POLL_LINKSTAT.


Index: if_aq.c
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/if_aq.c,v
retrieving revision 1.45
diff -p -u -r1.45 if_aq.c
--- if_aq.c	29 May 2023 08:00:05 -0000	1.45
+++ if_aq.c	26 Oct 2023 06:55:28 -0000
@@ -1330,8 +1330,7 @@ struct aq_softc {
 	int sc_rx_irq[AQ_RSSQUEUE_MAX];
 	int sc_linkstat_irq;
 	bool sc_use_txrx_independent_intr;
-	bool sc_poll_linkstat;
-	bool sc_detect_linkstat;
+	bool sc_no_link_intr;
 
 #if NSYSMON_ENVSYS > 0
 	struct sysmon_envsys *sc_sme;
@@ -1443,11 +1442,9 @@ static int aq_match(device_t, cfdata_t, 
 static void aq_attach(device_t, device_t, void *);
 static int aq_detach(device_t, int);
 
-static int aq_setup_msix(struct aq_softc *, struct pci_attach_args *, int,
-    bool, bool);
+static int aq_setup_msix(struct aq_softc *, struct pci_attach_args *);
 static int aq_setup_legacy(struct aq_softc *, struct pci_attach_args *,
     pci_intr_type_t);
-static int aq_establish_msix_intr(struct aq_softc *, bool, bool);
 
 static int aq_ifmedia_change(struct ifnet * const);
 static void aq_ifmedia_status(struct ifnet * const, struct ifmediareq *);
@@ -1784,67 +1781,57 @@ aq_attach(device_t parent, device_t self
 	if (msixcount >= (sc->sc_nqueues * 2 + 1)) {
 		/* TX intrs + RX intrs + LINKSTAT intrs */
 		sc->sc_use_txrx_independent_intr = true;
-		sc->sc_poll_linkstat = false;
 		sc->sc_msix = true;
 	} else if (msixcount >= (sc->sc_nqueues * 2)) {
 		/* TX intrs + RX intrs */
 		sc->sc_use_txrx_independent_intr = true;
-		sc->sc_poll_linkstat = true;
 		sc->sc_msix = true;
 	} else
 #endif
 	if (msixcount >= (sc->sc_nqueues + 1)) {
 		/* TX/RX intrs LINKSTAT intrs */
 		sc->sc_use_txrx_independent_intr = false;
-		sc->sc_poll_linkstat = false;
 		sc->sc_msix = true;
 	} else if (msixcount >= sc->sc_nqueues) {
 		/* TX/RX intrs */
 		sc->sc_use_txrx_independent_intr = false;
-		sc->sc_poll_linkstat = true;
+		sc->sc_no_link_intr = true;
 		sc->sc_msix = true;
 	} else {
 		/* giving up using MSI-X */
 		sc->sc_msix = false;
 	}
 
-	/* on AQ1a0, AQ2, or FIBRE, linkstat interrupt doesn't work? */
-	if (aqp->aq_media_type == AQ_MEDIA_TYPE_FIBRE ||
-	    (HWTYPE_AQ1_P(sc) && FW_VERSION_MAJOR(sc) == 1) ||
-	    HWTYPE_AQ2_P(sc))
-		sc->sc_poll_linkstat = true;
-
-#ifdef AQ_FORCE_POLL_LINKSTAT
-	sc->sc_poll_linkstat = true;
-#endif
-
 	aprint_debug_dev(sc->sc_dev,
 	    "ncpu=%d, pci_msix_count=%d."
 	    " allocate %d interrupts for %d%s queues%s\n",
 	    ncpu, msixcount,
 	    (sc->sc_use_txrx_independent_intr ?
 	    (sc->sc_nqueues * 2) : sc->sc_nqueues) +
-	    (sc->sc_poll_linkstat ? 0 : 1),
+	    (sc->sc_no_link_intr ? 0 : 1),
 	    sc->sc_nqueues,
 	    sc->sc_use_txrx_independent_intr ? "*2" : "",
-	    sc->sc_poll_linkstat ? "" : ", and link status");
+	    (sc->sc_no_link_intr) ? "" : ", and link status");
 
 	if (sc->sc_msix)
-		error = aq_setup_msix(sc, pa, sc->sc_nqueues,
-		    sc->sc_use_txrx_independent_intr, !sc->sc_poll_linkstat);
+		error = aq_setup_msix(sc, pa);
 	else
 		error = ENODEV;
 
 	if (error != 0) {
 		/* if MSI-X failed, fallback to MSI with single queue */
 		sc->sc_use_txrx_independent_intr = false;
-		sc->sc_poll_linkstat = false;
 		sc->sc_msix = false;
 		sc->sc_nqueues = 1;
+		sc->sc_no_link_intr = false;
+		aprint_debug_dev(sc->sc_dev, "MSI-X failed: %d, trying MSI",
+		    error);
 		error = aq_setup_legacy(sc, pa, PCI_INTR_TYPE_MSI);
 	}
 	if (error != 0) {
 		/* if MSI failed, fallback to INTx */
+		aprint_debug_dev(sc->sc_dev, "MSI failed: %d, trying legacy",
+		    error);
 		error = aq_setup_legacy(sc, pa, PCI_INTR_TYPE_INTX);
 	}
 	if (error != 0)
@@ -1858,7 +1845,7 @@ aq_attach(device_t parent, device_t self
 	error = workqueue_create(&sc->sc_reset_wq, wqname,
 	    aq_handle_reset_work, sc, PRI_SOFTNET, IPL_SOFTCLOCK,
 	    WQ_MPSAFE);
-	if (error) {
+	if (error != 0) {
 		aprint_error_dev(sc->sc_dev,
 		    "unable to create reset workqueue\n");
 		goto attach_failure;
@@ -1979,6 +1966,8 @@ aq_attach(device_t parent, device_t self
 		if (sysmon_envsys_register(sc->sc_sme)) {
 			sysmon_envsys_destroy(sc->sc_sme);
 			sc->sc_sme = NULL;
+			aprint_debug_dev(sc->sc_dev, "failed to create envsys");
+			error = EINVAL;
 			goto attach_failure;
 		}
 
@@ -2025,6 +2014,7 @@ aq_attach(device_t parent, device_t self
 	return;
 
  attach_failure:
+	aprint_debug_dev(sc->sc_dev, "attach failed: %d", error);
 	aq_detach(self, 0);
 }
 
@@ -2152,8 +2142,7 @@ aq_establish_intr(struct aq_softc *sc, i
 }
 
 static int
-aq_establish_msix_intr(struct aq_softc *sc, bool txrx_independent,
-    bool linkintr)
+aq_establish_msix_intr(struct aq_softc *sc)
 {
 	kcpuset_t *affinity;
 	int error, intno, i;
@@ -2163,7 +2152,7 @@ aq_establish_msix_intr(struct aq_softc *
 
 	intno = 0;
 
-	if (txrx_independent) {
+	if (sc->sc_use_txrx_independent_intr) {
 		for (i = 0; i < sc->sc_nqueues; i++) {
 			snprintf(intr_xname, sizeof(intr_xname), "%s RX%d",
 			    device_xname(sc->sc_dev), i);
@@ -2195,7 +2184,7 @@ aq_establish_msix_intr(struct aq_softc *
 		}
 	}
 
-	if (linkintr) {
+	if (!sc->sc_no_link_intr) {
 		snprintf(intr_xname, sizeof(intr_xname), "%s LINK",
 		    device_xname(sc->sc_dev));
 		sc->sc_linkstat_irq = intno;
@@ -2221,9 +2210,11 @@ aq_establish_msix_intr(struct aq_softc *
 }
 
 static int
-aq_setup_msix(struct aq_softc *sc, struct pci_attach_args *pa, int nqueue,
-    bool txrx_independent, bool linkintr)
+aq_setup_msix(struct aq_softc *sc, struct pci_attach_args *pa)
 {
+	int nqueue = sc->sc_nqueues;
+	bool txrx_independent = sc->sc_use_txrx_independent_intr;
+	bool linkintr = !sc->sc_no_link_intr;
 	int error, nintr;
 
 	if (txrx_independent)
@@ -2241,7 +2232,7 @@ aq_setup_msix(struct aq_softc *sc, struc
 		goto fail;
 	}
 
-	error = aq_establish_msix_intr(sc, txrx_independent, linkintr);
+	error = aq_establish_msix_intr(sc);
 	if (error == 0) {
 		sc->sc_nintrs = nintr;
 	} else {
@@ -5043,10 +5034,7 @@ aq_tick(void *arg)
 		return;
 	}
 
-	if (sc->sc_poll_linkstat || sc->sc_detect_linkstat) {
-		sc->sc_detect_linkstat = false;
-		aq_update_link_status(sc);
-	}
+	aq_update_link_status(sc);
 
 #ifdef AQ_EVENT_COUNTERS
 	if (sc->sc_poll_statistics)
@@ -5094,7 +5082,6 @@ aq_legacy_intr(void *arg)
 
 	if (status & __BIT(sc->sc_linkstat_irq)) {
 		AQ_LOCK(sc);
-		sc->sc_detect_linkstat = true;
 		if (!sc->sc_stopping)
 			callout_schedule(&sc->sc_tick_ch, 0);
 		AQ_UNLOCK(sc);
@@ -5150,7 +5137,6 @@ aq_link_intr(void *arg)
 	status = AQ_READ_REG(sc, AQ_INTR_STATUS_REG);
 	if (status & __BIT(sc->sc_linkstat_irq)) {
 		AQ_LOCK(sc);
-		sc->sc_detect_linkstat = true;
 		if (!sc->sc_stopping)
 			callout_schedule(&sc->sc_tick_ch, 0);
 		AQ_UNLOCK(sc);