* [PATCH 0/2] ath10k: pci: Two PCI related fixups @ 2019-12-19 13:15 Bryan O'Donoghue 2019-12-19 13:15 ` [PATCH 1/2] ath10k: pci: Only dump ATH10K_MEM_REGION_TYPE_IOREG when safe Bryan O'Donoghue 2019-12-19 13:15 ` [PATCH 2/2] ath10k: pci: Fix comment on ath10k_pci_dump_memory_sram Bryan O'Donoghue 0 siblings, 2 replies; 8+ messages in thread From: Bryan O'Donoghue @ 2019-12-19 13:15 UTC (permalink / raw) To: kvalo, akolli, ath10k; +Cc: linux-wireless, Bryan O'Donoghue This set addresses two issues one a contrived but real corner-case crash scenario and the other a simple typo. Debugging on a QCS405 which has an ath10k attached to PCIe its been found that a loop similar to the below [1] will cause. 1. A significant slow-down in the time it takes an individual ioread32() to complete. 2. A secure watchdog bite. This is as a result of the restart routine and the dump register routine running in parallel and a period of time during restart where dumping registers is unstable. The second patch is a simple fix to an apparent copy/paste error describing the behavior of a similar dump function. [1] Reset method while $1 do echo hw-restart > /sys/kernel/debug/ieee80211/phy0/ath10k/simulate_fw_crash echo hard > /sys/kernel/debug/ieee80211/phy0/ath10k/simulate_fw_crash done; Bryan O'Donoghue (2): ath10k: pci: Only dump ATH10K_MEM_REGION_TYPE_IOREG when safe ath10k: pci: Fix comment on ath10k_pci_dump_memory_sram drivers/net/wireless/ath/ath10k/pci.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) -- 2.24.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] ath10k: pci: Only dump ATH10K_MEM_REGION_TYPE_IOREG when safe 2019-12-19 13:15 [PATCH 0/2] ath10k: pci: Two PCI related fixups Bryan O'Donoghue @ 2019-12-19 13:15 ` Bryan O'Donoghue 2019-12-19 13:53 ` Kalle Valo 2020-01-26 10:23 ` Kalle Valo 2019-12-19 13:15 ` [PATCH 2/2] ath10k: pci: Fix comment on ath10k_pci_dump_memory_sram Bryan O'Donoghue 1 sibling, 2 replies; 8+ messages in thread From: Bryan O'Donoghue @ 2019-12-19 13:15 UTC (permalink / raw) To: kvalo, akolli, ath10k; +Cc: linux-wireless, Bryan O'Donoghue ath10k_pci_dump_memory_reg() will try to access memory of type ATH10K_MEM_REGION_TYPE_IOREG however, if a hardware restart is in progress this can crash a system. Individual ioread32() time has been observed to jump from 15-20 ticks to > 80k ticks followed by a secure-watchdog bite and a system reset. Work around this corner case by only issuing the read transaction when the driver state is ATH10K_STATE_ON. Fixes: 219cc084c6706 ("ath10k: add memory dump support QCA9984") Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- drivers/net/wireless/ath/ath10k/pci.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c index bb44f5a0941b..4822a65f6f3c 100644 --- a/drivers/net/wireless/ath/ath10k/pci.c +++ b/drivers/net/wireless/ath/ath10k/pci.c @@ -1604,11 +1604,22 @@ static int ath10k_pci_dump_memory_reg(struct ath10k *ar, { struct ath10k_pci *ar_pci = ath10k_pci_priv(ar); u32 i; + int ret; + + mutex_lock(&ar->conf_mutex); + if (ar->state != ATH10K_STATE_ON) { + ath10k_warn(ar, "Skipping pci_dump_memory_reg invalid state\n"); + ret = -EIO; + goto done; + } for (i = 0; i < region->len; i += 4) *(u32 *)(buf + i) = ioread32(ar_pci->mem + region->start + i); - return region->len; + ret = region->len; +done: + mutex_unlock(&ar->conf_mutex); + return ret; } /* if an error happened returns < 0, otherwise the length */ @@ -1704,7 +1715,11 @@ static void ath10k_pci_dump_memory(struct ath10k *ar, count = ath10k_pci_dump_memory_sram(ar, current_region, buf); break; case ATH10K_MEM_REGION_TYPE_IOREG: - count = ath10k_pci_dump_memory_reg(ar, current_region, buf); + ret = ath10k_pci_dump_memory_reg(ar, current_region, buf); + if (ret < 0) + break; + + count = ret; break; default: ret = ath10k_pci_dump_memory_generic(ar, current_region, buf); -- 2.24.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] ath10k: pci: Only dump ATH10K_MEM_REGION_TYPE_IOREG when safe 2019-12-19 13:15 ` [PATCH 1/2] ath10k: pci: Only dump ATH10K_MEM_REGION_TYPE_IOREG when safe Bryan O'Donoghue @ 2019-12-19 13:53 ` Kalle Valo 2019-12-19 14:15 ` Bryan O'Donoghue 2020-01-26 10:23 ` Kalle Valo 1 sibling, 1 reply; 8+ messages in thread From: Kalle Valo @ 2019-12-19 13:53 UTC (permalink / raw) To: Bryan O'Donoghue; +Cc: akolli, ath10k, linux-wireless Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: > ath10k_pci_dump_memory_reg() will try to access memory of type > ATH10K_MEM_REGION_TYPE_IOREG however, if a hardware restart is in progress > this can crash a system. > > Individual ioread32() time has been observed to jump from 15-20 ticks to > > 80k ticks followed by a secure-watchdog bite and a system reset. > > Work around this corner case by only issuing the read transaction when the > driver state is ATH10K_STATE_ON. > > Fixes: 219cc084c6706 ("ath10k: add memory dump support QCA9984") > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> What ath10k hardware and firmware did you test this on? I can add that to the commit log. -- https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] ath10k: pci: Only dump ATH10K_MEM_REGION_TYPE_IOREG when safe 2019-12-19 13:53 ` Kalle Valo @ 2019-12-19 14:15 ` Bryan O'Donoghue 2019-12-19 14:21 ` Kalle Valo 0 siblings, 1 reply; 8+ messages in thread From: Bryan O'Donoghue @ 2019-12-19 14:15 UTC (permalink / raw) To: Kalle Valo; +Cc: akolli, ath10k, linux-wireless On 19/12/2019 13:53, Kalle Valo wrote: > Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: > >> ath10k_pci_dump_memory_reg() will try to access memory of type >> ATH10K_MEM_REGION_TYPE_IOREG however, if a hardware restart is in progress >> this can crash a system. >> >> Individual ioread32() time has been observed to jump from 15-20 ticks to > >> 80k ticks followed by a secure-watchdog bite and a system reset. >> >> Work around this corner case by only issuing the read transaction when the >> driver state is ATH10K_STATE_ON. >> >> Fixes: 219cc084c6706 ("ath10k: add memory dump support QCA9984") >> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > > What ath10k hardware and firmware did you test this on? I can add that > to the commit log. > HW = QCA9988 FW = ?? Not quite sure how to find the firmware version TBH ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] ath10k: pci: Only dump ATH10K_MEM_REGION_TYPE_IOREG when safe 2019-12-19 14:15 ` Bryan O'Donoghue @ 2019-12-19 14:21 ` Kalle Valo 2019-12-19 14:29 ` Bryan O'Donoghue 0 siblings, 1 reply; 8+ messages in thread From: Kalle Valo @ 2019-12-19 14:21 UTC (permalink / raw) To: Bryan O'Donoghue; +Cc: akolli, ath10k, linux-wireless Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: > On 19/12/2019 13:53, Kalle Valo wrote: >> Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: >> >>> ath10k_pci_dump_memory_reg() will try to access memory of type >>> ATH10K_MEM_REGION_TYPE_IOREG however, if a hardware restart is in progress >>> this can crash a system. >>> >>> Individual ioread32() time has been observed to jump from 15-20 ticks to > >>> 80k ticks followed by a secure-watchdog bite and a system reset. >>> >>> Work around this corner case by only issuing the read transaction when the >>> driver state is ATH10K_STATE_ON. >>> >>> Fixes: 219cc084c6706 ("ath10k: add memory dump support QCA9984") >>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >> >> What ath10k hardware and firmware did you test this on? I can add that >> to the commit log. >> > > HW = QCA9988 > FW = ?? > > Not quite sure how to find the firmware version TBH 'dmesg | grep ath10k' should show it. -- https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] ath10k: pci: Only dump ATH10K_MEM_REGION_TYPE_IOREG when safe 2019-12-19 14:21 ` Kalle Valo @ 2019-12-19 14:29 ` Bryan O'Donoghue 0 siblings, 0 replies; 8+ messages in thread From: Bryan O'Donoghue @ 2019-12-19 14:29 UTC (permalink / raw) To: Kalle Valo; +Cc: akolli, ath10k, linux-wireless On 19/12/2019 14:21, Kalle Valo wrote: > 'dmesg | grep ath10k' should show it. [ 6.579772] ath10k_pci 0000:01:00.0: firmware ver 10.4-3.9.0.2-00044 api 5 features no-p2p,mfp,peer-flow-ctrl,btcoex-param,allows-mesh-bcast,no-ps crc32 c3e1b393 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] ath10k: pci: Only dump ATH10K_MEM_REGION_TYPE_IOREG when safe 2019-12-19 13:15 ` [PATCH 1/2] ath10k: pci: Only dump ATH10K_MEM_REGION_TYPE_IOREG when safe Bryan O'Donoghue 2019-12-19 13:53 ` Kalle Valo @ 2020-01-26 10:23 ` Kalle Valo 1 sibling, 0 replies; 8+ messages in thread From: Kalle Valo @ 2020-01-26 10:23 UTC (permalink / raw) To: Bryan O'Donoghue; +Cc: akolli, ath10k, linux-wireless, Bryan O'Donoghue Bryan O'Donoghue <bryan.odonoghue@linaro.org> wrote: > ath10k_pci_dump_memory_reg() will try to access memory of type > ATH10K_MEM_REGION_TYPE_IOREG however, if a hardware restart is in progress > this can crash a system. > > Individual ioread32() time has been observed to jump from 15-20 ticks to > > 80k ticks followed by a secure-watchdog bite and a system reset. > > Work around this corner case by only issuing the read transaction when the > driver state is ATH10K_STATE_ON. > > Tested-on: QCA9988 PCI 10.4-3.9.0.2-00044 > > Fixes: 219cc084c6706 ("ath10k: add memory dump support QCA9984") > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > Signed-off-by: Kalle Valo <kvalo@codeaurora.org> 2 patches applied to ath-next branch of ath.git, thanks. d239380196c4 ath10k: pci: Only dump ATH10K_MEM_REGION_TYPE_IOREG when safe 63ec5cbc31f7 ath10k: pci: Fix comment on ath10k_pci_dump_memory_sram -- https://patchwork.kernel.org/patch/11303563/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] ath10k: pci: Fix comment on ath10k_pci_dump_memory_sram 2019-12-19 13:15 [PATCH 0/2] ath10k: pci: Two PCI related fixups Bryan O'Donoghue 2019-12-19 13:15 ` [PATCH 1/2] ath10k: pci: Only dump ATH10K_MEM_REGION_TYPE_IOREG when safe Bryan O'Donoghue @ 2019-12-19 13:15 ` Bryan O'Donoghue 1 sibling, 0 replies; 8+ messages in thread From: Bryan O'Donoghue @ 2019-12-19 13:15 UTC (permalink / raw) To: kvalo, akolli, ath10k; +Cc: linux-wireless, Bryan O'Donoghue The description of ath10k_pci_dump_memory_sram() is inaccurate, an error can never be returned, it is always the length. Update the comment to reflect. Fixes: 219cc084c6706 ("ath10k: add memory dump support QCA9984") Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- drivers/net/wireless/ath/ath10k/pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c index 4822a65f6f3c..ded7a220a4aa 100644 --- a/drivers/net/wireless/ath/ath10k/pci.c +++ b/drivers/net/wireless/ath/ath10k/pci.c @@ -1578,7 +1578,7 @@ static int ath10k_pci_set_ram_config(struct ath10k *ar, u32 config) return 0; } -/* if an error happened returns < 0, otherwise the length */ +/* Always returns the length */ static int ath10k_pci_dump_memory_sram(struct ath10k *ar, const struct ath10k_mem_region *region, u8 *buf) -- 2.24.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-01-26 10:23 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-12-19 13:15 [PATCH 0/2] ath10k: pci: Two PCI related fixups Bryan O'Donoghue 2019-12-19 13:15 ` [PATCH 1/2] ath10k: pci: Only dump ATH10K_MEM_REGION_TYPE_IOREG when safe Bryan O'Donoghue 2019-12-19 13:53 ` Kalle Valo 2019-12-19 14:15 ` Bryan O'Donoghue 2019-12-19 14:21 ` Kalle Valo 2019-12-19 14:29 ` Bryan O'Donoghue 2020-01-26 10:23 ` Kalle Valo 2019-12-19 13:15 ` [PATCH 2/2] ath10k: pci: Fix comment on ath10k_pci_dump_memory_sram Bryan O'Donoghue
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).