On Thu, Sep 09, 2021 at 03:15:07PM -0700, Mike Larkin wrote:
> On Wed, Sep 08, 2021 at 03:25:20PM +0200, Stefan Sperling wrote:
> > On Wed, Sep 08, 2021 at 02:19:00PM +0200, Stefan Sperling wrote:
> > > This patch applies on top of all the other iwx(4) diffs I've sent today.
> > > It makes iwx(4) initialize the device completely in the acpi thread.
> > >
> > > We now prepare the device for loading firmware during DVACT_RESUME,
> > > and load firmware from host memory into the device during DVACT_WAKEUP.
> > >
> > > Previously, DVACT_WAKEUP would schedule the init_task which resets the
> > > device, undoing work done during DVACT_RESUME, and starts all over again.
> > >
> > > ok?
> >
> > The previous version had a bug: It resumed the device even while the
> > interface was marked down. Fixed patch below.
> >
>
> It looks like DVACT_RESUME invokes iwx_resume which does a not-trivial amount
> of chip repair/bringup. If you are satisfied this is safe, ok mlarkin@
(There was some off-list discussion with Mike and Theo.)
I like your idea of moving all the hw init code to the WAKEUP stage.
In case we add support for new devices we might have to debug the
suspend/resume path again. Running it in WAKEUP means the screen is
more likely to work, so let's do it that way.
Theo pointed out some bogus splnet() calls that wrap simple code
paths that don't sleep. Let's remove them.
Also, sync a comment with Linux to clarify why are teaking the PCI
retry timeout register.
All those changes are shown below. My device is still happy with this.
I will reply with a new full diff against -current next.
diff b42d4d802af3b16524c2bf3d31eac8ecc7a511d1 /usr/src
blob - 26f8a7fa85aa48a054d79e7a175e35bfe96a447b
file + sys/dev/pci/if_iwx.c
--- sys/dev/pci/if_iwx.c
+++ sys/dev/pci/if_iwx.c
@@ -1914,11 +1914,8 @@ int
iwx_check_rfkill(struct iwx_softc *sc)
{
uint32_t v;
- int s;
int rv;
- s = splnet();
-
/*
* "documentation" is not really helpful here:
* 27: HW_RF_KILL_SW
@@ -1934,7 +1931,6 @@ iwx_check_rfkill(struct iwx_softc *sc)
sc->sc_flags &= ~IWX_FLAG_RFKILL;
}
- splx(s);
return rv;
}
@@ -1987,8 +1983,6 @@ iwx_restore_interrupts(struct iwx_softc *sc)
void
iwx_disable_interrupts(struct iwx_softc *sc)
{
- int s = splnet();
-
if (!sc->sc_msix) {
IWX_WRITE(sc, IWX_CSR_INT_MASK, 0);
@@ -2001,8 +1995,6 @@ iwx_disable_interrupts(struct iwx_softc *sc)
IWX_WRITE(sc, IWX_CSR_MSIX_HW_INT_MASK_AD,
sc->sc_hw_init_mask);
}
-
- splx(s);
}
void
@@ -9282,7 +9274,10 @@ iwx_attach(struct device *parent, struct device *self,
return;
}
- /* Clear device-specific "PCI retry timeout" register (41h). */
+ /*
+ * We disable the RETRY_TIMEOUT register (0x41) to keep
+ * PCI Tx retries from interfering with C3 CPU state.
+ */
reg = pci_conf_read(sc->sc_pct, sc->sc_pcitag, 0x40);
pci_conf_write(sc->sc_pct, sc->sc_pcitag, 0x40, reg & ~0xff00);
@@ -9574,7 +9569,10 @@ iwx_resume(struct iwx_softc *sc)
{
pcireg_t reg;
- /* Clear device-specific "PCI retry timeout" register (41h). */
+ /*
+ * We disable the RETRY_TIMEOUT register (0x41) to keep
+ * PCI Tx retries from interfering with C3 CPU state.
+ */
reg = pci_conf_read(sc->sc_pct, sc->sc_pcitag, 0x40);
pci_conf_write(sc->sc_pct, sc->sc_pcitag, 0x40, reg & ~0xff00);
@@ -9590,7 +9588,7 @@ iwx_resume(struct iwx_softc *sc)
iwx_disable_interrupts(sc);
- return iwx_start_hw(sc);
+ return 0;
}
int
@@ -9602,6 +9600,10 @@ iwx_wakeup(struct iwx_softc *sc)
refcnt_init(&sc->task_refs);
+ err = iwx_start_hw(sc);
+ if (err)
+ return err;
+
err = iwx_init_hw(sc);
if (err)
return err;
