On 2 August 2013 09:41, Kalle Valo [off-list ref] wrote:

Michal Kazior [off-list ref] writes:

quoted

This fixes interrupt-related issue when no
interfaces were running thus the device was
considered powered down.

The power_down() function isn't really powering
down the device. It simply assumed it won't
interrupt. This wasn't true in some cases and
could lead to paging failures upon FW indication
interrupt (i.e. FW crash) because some structures
aren't allocated in that device state.

One reason for that was that ar_pci->started
wasn't reset. The other is interrupts should've
been masked when teardown starts.

The patch reorganized interrupt setup and makes
sure ar_pci->started is reset accordingly.

Reported-by: Ben Greear <redacted>
Signed-off-by: Michal Kazior <redacted>
---
v2:
 * updated commit message
 * added Reported-By: Ben
 * added disable_irq() in hif_stop()
 * added ar_pci->started resetting
 * removed ar_pci->intr_started

Thanks, this looks much better now.

But I still have one question:

quoted

@@ -1742,6 +1761,12 @@ static int ath10k_pci_hif_power_up(struct ath10k *ar)
 {
      int ret;

+     ret = ath10k_pci_start_intr(ar);
+     if (ret) {
+             ath10k_err("could not start interrupt handling (%d)\n", ret);
+             goto err;
+     }

So now we call start_intr() during power_up(), which means that we do
the request_irq() calls during every interface up event. Does that cause
any meaningful overhead?

I don't think so.

For me it looks better to do all resource allocation in
ath10k_pci_probe(), like request_irq(), and free the resources in
ath10k_pci_remove(). But then we would need to immeadiately call
disable_irq() and then enable_irq() from power_up() so I'm not sure if
that's any better.

Not only that. Since disable/enable_irq must be balanced we'd need
some way to track whether we have irqs enabled/disabled - either with
an extra bool variable, additional ath10k_states or new pci-specific
states.

The patch assumes disable_irq is followed by free_irq (which it is)
and possibly request_irq later on.


Pozdrawiam / Best regards,
Michał Kazior.