| Message ID | 20140325091505.16651.15554.stgit@potku.adurom.net (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 25 March 2014 10:15, Kalle Valo <kvalo@qca.qualcomm.com> wrote: > ath10k_pci_wait_for_target_init() did really follow the style used elsewhere in > ath10k. Use ath10k_pci_read/write() wrappers, simplify the while loop and > improve warning messages. > > Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com> > --- > drivers/net/wireless/ath/ath10k/pci.c | 33 ++++++++++++++++++++------------- > 1 file changed, 20 insertions(+), 13 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c > index 9d242d801d9d..0425c76daf3f 100644 > --- a/drivers/net/wireless/ath/ath10k/pci.c > +++ b/drivers/net/wireless/ath/ath10k/pci.c > @@ -2385,30 +2385,37 @@ static int ath10k_pci_deinit_irq(struct ath10k *ar) > static int ath10k_pci_wait_for_target_init(struct ath10k *ar) > { > struct ath10k_pci *ar_pci = ath10k_pci_priv(ar); > - int wait_limit = 300; /* 3 sec */ > + const int wait = 3000; /* ms */ We probably should have a #define for this. > + unsigned long timeout; > int ret; > + u32 val; > > ret = ath10k_pci_wake(ar); > if (ret) { > - ath10k_err("failed to wake up target: %d\n", ret); > + ath10k_err("failed to wake up target for init: %d\n", ret); > return ret; > } > > - while (wait_limit-- && > - !(ioread32(ar_pci->mem + FW_INDICATOR_ADDRESS) & > - FW_IND_INITIALIZED)) { > + timeout = jiffies + msecs_to_jiffies(wait); > + > + do { > + val = ath10k_pci_read32(ar, FW_INDICATOR_ADDRESS); > + if (val == FW_IND_INITIALIZED) > + break; > + It might be worth to add: if (val == 0xFFFFFFFF) return -EIO; > if (ar_pci->num_msi_intrs == 0) > /* Fix potential race by repeating CORE_BASE writes */ > - iowrite32(PCIE_INTR_FIRMWARE_MASK | > - PCIE_INTR_CE_MASK_ALL, > - ar_pci->mem + (SOC_CORE_BASE_ADDRESS | > - PCIE_INTR_ENABLE_ADDRESS)); > + ath10k_pci_soc_write32(ar, PCIE_INTR_ENABLE_ADDRESS, > + PCIE_INTR_FIRMWARE_MASK | > + PCIE_INTR_CE_MASK_ALL); > + > mdelay(10); > - } > + } while (time_before(jiffies, timeout)); > > - if (wait_limit < 0) { > - ath10k_err("target stalled\n"); > - ret = -EIO; > + if (val != FW_IND_INITIALIZED) { > + ath10k_err("failed to receive initialized event from target after %d ms: %d\n", > + wait, val); `val` is u32 so it shouldn't be %d. %08x makes most sense I guess. Micha? -- 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 25 March 2014 10:15, Kalle Valo <kvalo@qca.qualcomm.com> wrote: >> ath10k_pci_wait_for_target_init() did really follow the style used elsewhere in >> ath10k. Use ath10k_pci_read/write() wrappers, simplify the while loop and >> improve warning messages. >> >> Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com> >> --- >> drivers/net/wireless/ath/ath10k/pci.c | 33 ++++++++++++++++++++------------- >> 1 file changed, 20 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c >> index 9d242d801d9d..0425c76daf3f 100644 >> --- a/drivers/net/wireless/ath/ath10k/pci.c >> +++ b/drivers/net/wireless/ath/ath10k/pci.c >> @@ -2385,30 +2385,37 @@ static int ath10k_pci_deinit_irq(struct ath10k *ar) >> static int ath10k_pci_wait_for_target_init(struct ath10k *ar) >> { >> struct ath10k_pci *ar_pci = ath10k_pci_priv(ar); >> - int wait_limit = 300; /* 3 sec */ >> + const int wait = 3000; /* ms */ > > We probably should have a #define for this. Yeah, I'll change that. >> - while (wait_limit-- && >> - !(ioread32(ar_pci->mem + FW_INDICATOR_ADDRESS) & >> - FW_IND_INITIALIZED)) { >> + timeout = jiffies + msecs_to_jiffies(wait); >> + >> + do { >> + val = ath10k_pci_read32(ar, FW_INDICATOR_ADDRESS); >> + if (val == FW_IND_INITIALIZED) >> + break; >> + > > It might be worth to add: > > if (val == 0xFFFFFFFF) > return -EIO; What does receiving 0xFFFFFFFF mean here? PCI bus kaput? Do we really want to stop trying after receiving that? What harm would it cause to keep on trying? We would return an error anyway after the timeout, right? >> + } while (time_before(jiffies, timeout)); >> >> - if (wait_limit < 0) { >> - ath10k_err("target stalled\n"); >> - ret = -EIO; >> + if (val != FW_IND_INITIALIZED) { >> + ath10k_err("failed to receive initialized event from target after %d ms: %d\n", >> + wait, val); > > `val` is u32 so it shouldn't be %d. %08x makes most sense I guess. I'll change it. Thanks for the review!
On 26 March 2014 10:28, Kalle Valo <kvalo@qca.qualcomm.com> wrote: > Michal Kazior <michal.kazior@tieto.com> writes: [...] >>> - while (wait_limit-- && >>> - !(ioread32(ar_pci->mem + FW_INDICATOR_ADDRESS) & >>> - FW_IND_INITIALIZED)) { >>> + timeout = jiffies + msecs_to_jiffies(wait); >>> + >>> + do { >>> + val = ath10k_pci_read32(ar, FW_INDICATOR_ADDRESS); >>> + if (val == FW_IND_INITIALIZED) >>> + break; Ah, for some reason I've missed this earlier. You seem to replace & with ==. I wonder if that's okay? FW indicator may contain FW_IND_EVENT_PENDING (i.e. firmware crashed bit) too. It might be a good idea to check for that too and return a different errno? >>> + >> >> It might be worth to add: >> >> if (val == 0xFFFFFFFF) >> return -EIO; > > What does receiving 0xFFFFFFFF mean here? PCI bus kaput? A simple grepping (find drivers/net/ethernet/ -type f -name '*.[ch]' | xargs grep -nC5 ioread32 | grep -C5 -i 0xffffffff) turns out results like this: drivers/net/ethernet/cisco/enic/vnic_rq.c-200- } drivers/net/ethernet/cisco/enic/vnic_rq.c-201- drivers/net/ethernet/cisco/enic/vnic_rq.c-202- /* Use current fetch_index as the ring starting point */ drivers/net/ethernet/cisco/enic/vnic_rq.c:203: fetch_index = ioread32(&rq->ctrl->fetch_index); drivers/net/ethernet/cisco/enic/vnic_rq.c-204- drivers/net/ethernet/cisco/enic/vnic_rq.c-205- if (fetch_index == 0xFFFFFFFF) { /* check for hardware gone */ drivers/net/ethernet/cisco/enic/vnic_rq.c-206- /* Hardware surprise removal: reset fetch_index */ drivers/net/ethernet/cisco/enic/vnic_rq.c-207- fetch_index = 0; drivers/net/ethernet/cisco/enic/vnic_rq.c-208- } > Do we really want to stop trying after receiving that? What harm would > it cause to keep on trying? We would return an error anyway after the > timeout, right? Yes. It's harmless but having the check allows to discern a real timeout (target doesn't wake up for some reason) and a pci-e link issue. Micha? -- 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 26 March 2014 10:28, Kalle Valo <kvalo@qca.qualcomm.com> wrote: >> Michal Kazior <michal.kazior@tieto.com> writes: > > [...] > >>>> - while (wait_limit-- && >>>> - !(ioread32(ar_pci->mem + FW_INDICATOR_ADDRESS) & >>>> - FW_IND_INITIALIZED)) { >>>> + timeout = jiffies + msecs_to_jiffies(wait); >>>> + >>>> + do { >>>> + val = ath10k_pci_read32(ar, FW_INDICATOR_ADDRESS); >>>> + if (val == FW_IND_INITIALIZED) >>>> + break; > > Ah, for some reason I've missed this earlier. You seem to replace & > with ==. I wonder if that's okay? No, it's not. Good catch, I'll change it back to '&'. > FW indicator may contain FW_IND_EVENT_PENDING (i.e. firmware crashed > bit) too. I'm guessing that FW_IND_INITIALIZED should be the only bit set if initialisation has happened correctly, but we cannot be certain (right now). Hence I'll change it back to '&'. > It might be a good idea to check for that too and return a different > errno? Maybe. But to keep this patch simple, I don't add that right now. We can create a new patch for that. >>> It might be worth to add: >>> >>> if (val == 0xFFFFFFFF) >>> return -EIO; >> >> What does receiving 0xFFFFFFFF mean here? PCI bus kaput? > > A simple grepping (find drivers/net/ethernet/ -type f -name '*.[ch]' | > xargs grep -nC5 ioread32 | grep -C5 -i 0xffffffff) turns out results > like this: > > drivers/net/ethernet/cisco/enic/vnic_rq.c-200- } > drivers/net/ethernet/cisco/enic/vnic_rq.c-201- > drivers/net/ethernet/cisco/enic/vnic_rq.c-202- /* Use current > fetch_index as the ring starting point */ > drivers/net/ethernet/cisco/enic/vnic_rq.c:203: fetch_index = > ioread32(&rq->ctrl->fetch_index); > drivers/net/ethernet/cisco/enic/vnic_rq.c-204- > drivers/net/ethernet/cisco/enic/vnic_rq.c-205- if (fetch_index == > 0xFFFFFFFF) { /* check for hardware gone */ > drivers/net/ethernet/cisco/enic/vnic_rq.c-206- /* Hardware > surprise removal: reset fetch_index */ > drivers/net/ethernet/cisco/enic/vnic_rq.c-207- fetch_index = 0; > drivers/net/ethernet/cisco/enic/vnic_rq.c-208- } > > >> Do we really want to stop trying after receiving that? What harm would >> it cause to keep on trying? We would return an error anyway after the >> timeout, right? > > Yes. It's harmless but having the check allows to discern a real > timeout (target doesn't wake up for some reason) and a pci-e link > issue. Ok, I'll add the test you suggested.
diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c index 9d242d801d9d..0425c76daf3f 100644 --- a/drivers/net/wireless/ath/ath10k/pci.c +++ b/drivers/net/wireless/ath/ath10k/pci.c @@ -2385,30 +2385,37 @@ static int ath10k_pci_deinit_irq(struct ath10k *ar) static int ath10k_pci_wait_for_target_init(struct ath10k *ar) { struct ath10k_pci *ar_pci = ath10k_pci_priv(ar); - int wait_limit = 300; /* 3 sec */ + const int wait = 3000; /* ms */ + unsigned long timeout; int ret; + u32 val; ret = ath10k_pci_wake(ar); if (ret) { - ath10k_err("failed to wake up target: %d\n", ret); + ath10k_err("failed to wake up target for init: %d\n", ret); return ret; } - while (wait_limit-- && - !(ioread32(ar_pci->mem + FW_INDICATOR_ADDRESS) & - FW_IND_INITIALIZED)) { + timeout = jiffies + msecs_to_jiffies(wait); + + do { + val = ath10k_pci_read32(ar, FW_INDICATOR_ADDRESS); + if (val == FW_IND_INITIALIZED) + break; + if (ar_pci->num_msi_intrs == 0) /* Fix potential race by repeating CORE_BASE writes */ - iowrite32(PCIE_INTR_FIRMWARE_MASK | - PCIE_INTR_CE_MASK_ALL, - ar_pci->mem + (SOC_CORE_BASE_ADDRESS | - PCIE_INTR_ENABLE_ADDRESS)); + ath10k_pci_soc_write32(ar, PCIE_INTR_ENABLE_ADDRESS, + PCIE_INTR_FIRMWARE_MASK | + PCIE_INTR_CE_MASK_ALL); + mdelay(10); - } + } while (time_before(jiffies, timeout)); - if (wait_limit < 0) { - ath10k_err("target stalled\n"); - ret = -EIO; + if (val != FW_IND_INITIALIZED) { + ath10k_err("failed to receive initialized event from target after %d ms: %d\n", + wait, val); + ret = -ETIMEDOUT; goto out; }
ath10k_pci_wait_for_target_init() did really follow the style used elsewhere in ath10k. Use ath10k_pci_read/write() wrappers, simplify the while loop and improve warning messages. Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com> --- drivers/net/wireless/ath/ath10k/pci.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) -- 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