[PATCH 0/2] ath10k: pci: Two PCI related fixups

[PATCH 0/2] ath10k: pci: Two PCI related fixups

[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

[PATCH 0/2] ath10k: pci: Two PCI related fixups

[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


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH 1/2] ath10k: pci: Only dump ATH10K_MEM_REGION_TYPE_IOREG when safe

[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

[PATCH 1/2] ath10k: pci: Only dump ATH10K_MEM_REGION_TYPE_IOREG when safe

[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


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH 2/2] ath10k: pci: Fix comment on ath10k_pci_dump_memory_sram

[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

[PATCH 2/2] ath10k: pci: Fix comment on ath10k_pci_dump_memory_sram

[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


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 1/2] ath10k: pci: Only dump ATH10K_MEM_REGION_TYPE_IOREG when safe

[Kalle Valo]

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

Re: [PATCH 1/2] ath10k: pci: Only dump ATH10K_MEM_REGION_TYPE_IOREG when safe

[Kalle Valo]

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

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 1/2] ath10k: pci: Only dump ATH10K_MEM_REGION_TYPE_IOREG when safe

[Bryan O'Donoghue]

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

Re: [PATCH 1/2] ath10k: pci: Only dump ATH10K_MEM_REGION_TYPE_IOREG when safe

[Bryan O'Donoghue]

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

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 1/2] ath10k: pci: Only dump ATH10K_MEM_REGION_TYPE_IOREG when safe

[Kalle Valo]

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

Re: [PATCH 1/2] ath10k: pci: Only dump ATH10K_MEM_REGION_TYPE_IOREG when safe

[Kalle Valo]

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

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 1/2] ath10k: pci: Only dump ATH10K_MEM_REGION_TYPE_IOREG when safe

[Bryan O'Donoghue]

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

Re: [PATCH 1/2] ath10k: pci: Only dump ATH10K_MEM_REGION_TYPE_IOREG when safe

[Bryan O'Donoghue]

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


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH 1/2] ath10k: pci: Only dump ATH10K_MEM_REGION_TYPE_IOREG when safe

[Kalle Valo]

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

Re: [PATCH 1/2] ath10k: pci: Only dump ATH10K_MEM_REGION_TYPE_IOREG when safe

[Kalle Valo]

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

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k