| Message ID | 1375427747-9539-1-git-send-email-michal.kazior@tieto.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Michal Kazior <michal.kazior@tieto.com> writes: > 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 <greearb@candelatech.com> > Signed-off-by: Michal Kazior <michal.kazior@tieto.com> > --- > 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: > @@ -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? 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.
On 2 August 2013 09:41, Kalle Valo <kvalo@qca.qualcomm.com> wrote: > Michal Kazior <michal.kazior@tieto.com> writes: > >> 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 <greearb@candelatech.com> >> Signed-off-by: Michal Kazior <michal.kazior@tieto.com> >> --- >> 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: > >> @@ -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. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Michal Kazior <michal.kazior@tieto.com> writes: > On 2 August 2013 09:41, Kalle Valo <kvalo@qca.qualcomm.com> wrote: >> Michal Kazior <michal.kazior@tieto.com> writes: >> >>> @@ -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. Good. >> 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. Yeah, your v2 sounds much better. And if there's overhead or something else we can always change this later. I'll wait for comments from others and if I don't get any, I'll apply this.
Michal Kazior <michal.kazior@tieto.com> writes: > 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 <greearb@candelatech.com> > Signed-off-by: Michal Kazior <michal.kazior@tieto.com> Thanks, applied.
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c index c71b488..690a8b4 100644 --- a/drivers/net/wireless/ath/ath10k/pci.c +++ b/drivers/net/wireless/ath/ath10k/pci.c @@ -56,6 +56,8 @@ static void ath10k_pci_rx_pipe_cleanup(struct hif_ce_pipe_info *pipe_info); static void ath10k_pci_stop_ce(struct ath10k *ar); static void ath10k_pci_device_reset(struct ath10k *ar); static int ath10k_pci_reset_target(struct ath10k *ar); +static int ath10k_pci_start_intr(struct ath10k *ar); +static void ath10k_pci_stop_intr(struct ath10k *ar); static const struct ce_attr host_ce_config_wlan[] = { /* host->target HTC control and raw streams */ @@ -1254,10 +1256,25 @@ static void ath10k_pci_ce_deinit(struct ath10k *ar) } } +static void ath10k_pci_disable_irqs(struct ath10k *ar) +{ + struct ath10k_pci *ar_pci = ath10k_pci_priv(ar); + int i; + + for (i = 0; i < max(1, ar_pci->num_msi_intrs); i++) + disable_irq(ar_pci->pdev->irq + i); +} + static void ath10k_pci_hif_stop(struct ath10k *ar) { + struct ath10k_pci *ar_pci = ath10k_pci_priv(ar); + ath10k_dbg(ATH10K_DBG_PCI, "%s\n", __func__); + /* Irqs are never explicitly re-enabled. They are implicitly re-enabled + * by ath10k_pci_start_intr(). */ + ath10k_pci_disable_irqs(ar); + ath10k_pci_stop_ce(ar); /* At this point, asynchronous threads are stopped, the target should @@ -1267,6 +1284,8 @@ static void ath10k_pci_hif_stop(struct ath10k *ar) ath10k_pci_process_ce(ar); ath10k_pci_cleanup_ce(ar); ath10k_pci_buffer_cleanup(ar); + + ar_pci->started = 0; } static int ath10k_pci_hif_exchange_bmi_msg(struct ath10k *ar, @@ -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; + } + /* * Bring the target up cleanly. * @@ -1756,7 +1781,7 @@ static int ath10k_pci_hif_power_up(struct ath10k *ar) ret = ath10k_pci_reset_target(ar); if (ret) - goto err; + goto err_irq; if (ath10k_target_ps) { ath10k_dbg(ATH10K_DBG_PCI, "on-chip power save enabled\n"); @@ -1787,12 +1812,15 @@ err_ce: err_ps: if (!ath10k_target_ps) ath10k_do_pci_sleep(ar); +err_irq: + ath10k_pci_stop_intr(ar); err: return ret; } static void ath10k_pci_hif_power_down(struct ath10k *ar) { + ath10k_pci_stop_intr(ar); ath10k_pci_ce_deinit(ar); if (!ath10k_target_ps) ath10k_do_pci_sleep(ar); @@ -2358,22 +2386,14 @@ static int ath10k_pci_probe(struct pci_dev *pdev, ar_pci->cacheline_sz = dma_get_cache_alignment(); - ret = ath10k_pci_start_intr(ar); - if (ret) { - ath10k_err("could not start interrupt handling (%d)\n", ret); - goto err_iomap; - } - ret = ath10k_core_register(ar); if (ret) { ath10k_err("could not register driver core (%d)\n", ret); - goto err_intr; + goto err_iomap; } return 0; -err_intr: - ath10k_pci_stop_intr(ar); err_iomap: pci_iounmap(pdev, mem); err_master: @@ -2410,7 +2430,6 @@ static void ath10k_pci_remove(struct pci_dev *pdev) tasklet_kill(&ar_pci->msi_fw_err); ath10k_core_unregister(ar); - ath10k_pci_stop_intr(ar); pci_set_drvdata(pdev, NULL); pci_iounmap(pdev, ar_pci->mem);
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 <greearb@candelatech.com> Signed-off-by: Michal Kazior <michal.kazior@tieto.com> --- v2: * updated commit message * added Reported-By: Ben * added disable_irq() in hif_stop() * added ar_pci->started resetting * removed ar_pci->intr_started drivers/net/wireless/ath/ath10k/pci.c | 41 ++++++++++++++++++++++++--------- 1 file changed, 30 insertions(+), 11 deletions(-)