This patch makes iwx(4) resume reliably for me.
There were missing splnet() calls which leads to an obvious race
between the interrupt handler and the code which triggers firmware
loading and then sleeps to wait for confirmation.
This patch adds the missing splnet().
However, even with splnet() protection added I need to add the
following two lines of code to make the patch work reliably:
/* wait for the firmware to load */
for (w = 0; !sc->sc_uc.uc_intr && w < 10; w++) {
err = tsleep_nsec(&sc->sc_uc, 0, "iwxuc", MSEC_TO_NSEC(100));
+ /* XXX This should not be needed, should it: */
+ if (err == EWOULDBLOCK && sc->sc_uc.uc_intr)
+ err = 0;
}
if (err || !sc->sc_uc.uc_ok)
printf("%s: could not load firmware, %d\n", DEVNAME(sc), err);
Which seems odd. I would expect tsleep to return EWOULDBLOCK only when
the interrupt handler did not already set uc_intr and call wakeup().
However, here tsleep returns with EWOULDBLOCK, after the interrupt did
occur and firmware has reported that it is alive, so both uc_intr and uc_ok
are set. This only seems to happen during resume (in a task scheduled
during DVACT_WAKEUP), but not during autoconf or regular ifconfig down/up.
Am I missing something? Am I adding splnet() in the wrong place?
Does this patch work reliably for anyone without the above change?
diff c05ef66598a004c1e173a5d0fd4cdf5403f2ad99 /usr/src
blob - 428a299e7a2d859aa68a934d4048d843be1835ce
file + sys/dev/pci/if_iwx.c
--- sys/dev/pci/if_iwx.c
+++ sys/dev/pci/if_iwx.c
@@ -3350,6 +3350,8 @@ iwx_load_firmware(struct iwx_softc *sc)
struct iwx_fw_sects *fws;
int err, w;
+ splassert(IPL_NET);
+
sc->sc_uc.uc_intr = 0;
fws = &sc->sc_fw.fw_sects[IWX_UCODE_TYPE_REGULAR];
@@ -3362,6 +3364,9 @@ iwx_load_firmware(struct iwx_softc *sc)
/* wait for the firmware to load */
for (w = 0; !sc->sc_uc.uc_intr && w < 10; w++) {
err = tsleep_nsec(&sc->sc_uc, 0, "iwxuc", MSEC_TO_NSEC(100));
+ /* XXX This should not be needed, should it: */
+ if (err == EWOULDBLOCK && sc->sc_uc.uc_intr)
+ err = 0;
}
if (err || !sc->sc_uc.uc_ok)
printf("%s: could not load firmware, %d\n", DEVNAME(sc), err);
@@ -3466,7 +3471,7 @@ iwx_run_init_mvm_ucode(struct iwx_softc *sc, int readn
struct iwx_init_extended_cfg_cmd init_cfg = {
.init_flags = htole32(IWX_INIT_NVM),
};
- int err;
+ int err, s;
if ((sc->sc_flags & IWX_FLAG_RFKILL) && !readnvm) {
printf("%s: radio is disabled by hardware switch\n",
@@ -3474,10 +3479,12 @@ iwx_run_init_mvm_ucode(struct iwx_softc *sc, int readn
return EPERM;
}
+ s = splnet();
sc->sc_init_complete = 0;
err = iwx_load_ucode_wait_alive(sc);
if (err) {
printf("%s: failed to load init firmware\n", DEVNAME(sc));
+ splx(s);
return err;
}
@@ -3487,22 +3494,28 @@ iwx_run_init_mvm_ucode(struct iwx_softc *sc, int readn
*/
err = iwx_send_cmd_pdu(sc, IWX_WIDE_ID(IWX_SYSTEM_GROUP,
IWX_INIT_EXTENDED_CFG_CMD), 0, sizeof(init_cfg), &init_cfg);
- if (err)
+ if (err) {
+ splx(s);
return err;
+ }
err = iwx_send_cmd_pdu(sc, IWX_WIDE_ID(IWX_REGULATORY_AND_NVM_GROUP,
IWX_NVM_ACCESS_COMPLETE), 0, sizeof(nvm_complete), &nvm_complete);
- if (err)
+ if (err) {
+ splx(s);
return err;
+ }
/* Wait for the init complete notification from the firmware. */
while ((sc->sc_init_complete & wait_flags) != wait_flags) {
err = tsleep_nsec(&sc->sc_init_complete, 0, "iwxinit",
SEC_TO_NSEC(2));
- if (err)
+ if (err) {
+ splx(s);
return err;
+ }
}
-
+ splx(s);
if (readnvm) {
err = iwx_nvm_get(sc);
if (err) {
