On 07/09/21(Tue) 15:48, Stefan Sperling wrote:
> 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().
That suggests the timeout fires before the wakeup(9). You can check
that by using wakeup_n() and print how many thread have been awaken.
> 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.
That suggests 100msec might be too small. Did you try with a bigger
value? Or is there something special happening during DVACT_WAKEUP?
> Am I missing something? Am I adding splnet() in the wrong place?
The splnet() you're adding ensure (with the KERNEL_LOCK()) no wakeup(9)
will happen until you go to sleep. I believe that's what you want.
> 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) {
>
