[RFC 0/2] ath10k:set tx credit to one and delay the unmaping

[RFC 0/2] ath10k:set tx credit to one and delay the unmaping

[Youghandhar Chintala]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=n, Size: 1139 bytes --]

Currently for WCN3990, the host driver can send two wmi commands at once, 
now it is limiting to one because if HOST wants to initiate 2 DMA transfers,
it is possible when copy complete interrupt for first DMA reaches HOST,  
CE has already updated SRRI for both DMA transfers and is in the middle 
of 2nd DMA. HOST uses SRRI to interpret how many DMA’s have been completed 
and tries to unmap/free both the DMA entries, but CE is still in the middle
of 2nd DMA which can cause SMMU issues or corruption.

We are seeing a corner case smmu fault issue where copy engine is still accessing
the memory though host unmaps it hence as work around we are delaying the 
unmapping the memory and tx credit to one.

Youghandhar Chintala (2):
  ath10k: Set tx credit to one for wcn3990 snoc based devices
  ath10k: Delay the unmapping of the buffer

 drivers/net/wireless/ath/ath10k/core.c | 30 ++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/htc.c  | 18 +++++++++++++---
 drivers/net/wireless/ath/ath10k/htc.h  |  1 +
 drivers/net/wireless/ath/ath10k/hw.h   |  6 ++++++
 4 files changed, 52 insertions(+), 3 deletions(-)

-- 
2.29.0

[RFC 1/2] ath10k: Set tx credit to one for wcn3990 snoc based devices

[Youghandhar Chintala]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=n, Size: 6768 bytes --]

Currently for WCN3990, the host driver can send two wmi commands at once,
now it is limiting to one because if HOST wants to initiate 2 DMA transfers,
it is possible when copy complete interrupt for first DMA reaches HOST,
CE has already updated SRRI for both DMA transfers and is in the middle
of 2nd DMA.HOST uses SRRI to interpret how many DMA’s have been completed
and tries to unmap/free both the DMA entries, but CE is  still in the middle
of 2nd DMA which can cause SMMU issues or corruption.

Tested-on: WLAN.HL.3.2.2.c10-00757-QCAHLSWMTPL-1
Signed-off-by: Youghandhar Chintala <quic_youghand@quicinc.com>
---
 drivers/net/wireless/ath/ath10k/core.c | 15 +++++++++++++++
 drivers/net/wireless/ath/ath10k/htc.c  | 13 ++++++++++---
 drivers/net/wireless/ath/ath10k/htc.h  |  1 +
 drivers/net/wireless/ath/ath10k/hw.h   |  3 +++
 4 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index f0e12556e7dce..fcaed7d176568 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -92,6 +92,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.tx_stats_over_pktlog = true,
 		.tx_mac_seq_by_fw = false,
 		.dynamic_sar_support = false,
+		.tx_credit_limit = false,
 	},
 	{
 		.id = QCA988X_HW_2_0_VERSION,
@@ -128,6 +129,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.tx_stats_over_pktlog = true,
 		.tx_mac_seq_by_fw = false,
 		.dynamic_sar_support = false,
+		.tx_credit_limit = false,
 	},
 	{
 		.id = QCA9887_HW_1_0_VERSION,
@@ -165,6 +167,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.tx_stats_over_pktlog = false,
 		.tx_mac_seq_by_fw = false,
 		.dynamic_sar_support = false,
+		.tx_credit_limit = false,
 	},
 	{
 		.id = QCA6174_HW_3_2_VERSION,
@@ -196,6 +199,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.tx_mac_seq_by_fw = false,
 		.bmi_large_size_download = true,
 		.dynamic_sar_support = true,
+		.tx_credit_limit = false,
 	},
 	{
 		.id = QCA6174_HW_2_1_VERSION,
@@ -232,6 +236,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.tx_stats_over_pktlog = false,
 		.tx_mac_seq_by_fw = false,
 		.dynamic_sar_support = false,
+		.tx_credit_limit = false,
 	},
 	{
 		.id = QCA6174_HW_2_1_VERSION,
@@ -268,6 +273,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.tx_stats_over_pktlog = false,
 		.tx_mac_seq_by_fw = false,
 		.dynamic_sar_support = false,
+		.tx_credit_limit = false,
 	},
 	{
 		.id = QCA6174_HW_3_0_VERSION,
@@ -304,6 +310,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.tx_stats_over_pktlog = false,
 		.tx_mac_seq_by_fw = false,
 		.dynamic_sar_support = false,
+		.tx_credit_limit = false,
 	},
 	{
 		.id = QCA6174_HW_3_2_VERSION,
@@ -343,6 +350,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.tx_stats_over_pktlog = false,
 		.tx_mac_seq_by_fw = false,
 		.dynamic_sar_support = true,
+		.tx_credit_limit = false,
 	},
 	{
 		.id = QCA99X0_HW_2_0_DEV_VERSION,
@@ -385,6 +393,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.tx_stats_over_pktlog = false,
 		.tx_mac_seq_by_fw = false,
 		.dynamic_sar_support = false,
+		.tx_credit_limit = false,
 	},
 	{
 		.id = QCA9984_HW_1_0_DEV_VERSION,
@@ -434,6 +443,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.tx_stats_over_pktlog = false,
 		.tx_mac_seq_by_fw = false,
 		.dynamic_sar_support = false,
+		.tx_credit_limit = false,
 	},
 	{
 		.id = QCA9888_HW_2_0_DEV_VERSION,
@@ -480,6 +490,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.tx_stats_over_pktlog = false,
 		.tx_mac_seq_by_fw = false,
 		.dynamic_sar_support = false,
+		.tx_credit_limit = false,
 	},
 	{
 		.id = QCA9377_HW_1_0_DEV_VERSION,
@@ -516,6 +527,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.tx_stats_over_pktlog = false,
 		.tx_mac_seq_by_fw = false,
 		.dynamic_sar_support = false,
+		.tx_credit_limit = false,
 	},
 	{
 		.id = QCA9377_HW_1_1_DEV_VERSION,
@@ -554,6 +566,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.tx_stats_over_pktlog = false,
 		.tx_mac_seq_by_fw = false,
 		.dynamic_sar_support = false,
+		.tx_credit_limit = false,
 	},
 	{
 		.id = QCA9377_HW_1_1_DEV_VERSION,
@@ -625,6 +638,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.tx_stats_over_pktlog = false,
 		.tx_mac_seq_by_fw = false,
 		.dynamic_sar_support = false,
+		.tx_credit_limit = false,
 	},
 	{
 		.id = WCN3990_HW_1_0_DEV_VERSION,
@@ -654,6 +668,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.tx_stats_over_pktlog = false,
 		.tx_mac_seq_by_fw = true,
 		.dynamic_sar_support = true,
+		.tx_credit_limit = true,
 	},
 };
 
diff --git a/drivers/net/wireless/ath/ath10k/htc.c b/drivers/net/wireless/ath/ath10k/htc.c
index 61ee413d902a7..108980476d654 100644
--- a/drivers/net/wireless/ath/ath10k/htc.c
+++ b/drivers/net/wireless/ath/ath10k/htc.c
@@ -630,13 +630,20 @@ int ath10k_htc_wait_target(struct ath10k_htc *htc)
 		return -ECOMM;
 	}
 
-	htc->total_transmit_credits = __le16_to_cpu(msg->ready.credit_count);
+	if (ar->hw_params.tx_credit_limit)
+		htc->total_transmit_credits =
+			__le16_to_cpu(HTC_HOST_MAX_CREDIT_COUNT);
+	else
+		htc->total_transmit_credits =
+			__le16_to_cpu(msg->ready.credit_count);
+
 	htc->target_credit_size = __le16_to_cpu(msg->ready.credit_size);
 
 	ath10k_dbg(ar, ATH10K_DBG_HTC,
-		   "Target ready! transmit resources: %d size:%d\n",
+		   "Target ready! transmit resources: %d size:%d actual credits:%d\n",
 		   htc->total_transmit_credits,
-		   htc->target_credit_size);
+		   htc->target_credit_size,
+		   msg->ready.credit_count);
 
 	if ((htc->total_transmit_credits == 0) ||
 	    (htc->target_credit_size == 0)) {
diff --git a/drivers/net/wireless/ath/ath10k/htc.h b/drivers/net/wireless/ath/ath10k/htc.h
index 14e5c3f712c11..a84d76f062d36 100644
--- a/drivers/net/wireless/ath/ath10k/htc.h
+++ b/drivers/net/wireless/ath/ath10k/htc.h
@@ -41,6 +41,7 @@ struct ath10k;
  */
 
 #define HTC_HOST_MAX_MSG_PER_RX_BUNDLE        32
+#define HTC_HOST_MAX_CREDIT_COUNT	1
 
 enum ath10k_htc_tx_flags {
 	ATH10K_HTC_FLAG_NEED_CREDIT_UPDATE = 0x01,
diff --git a/drivers/net/wireless/ath/ath10k/hw.h b/drivers/net/wireless/ath/ath10k/hw.h
index 0dec9f01cd4dd..0d96df311b026 100644
--- a/drivers/net/wireless/ath/ath10k/hw.h
+++ b/drivers/net/wireless/ath/ath10k/hw.h
@@ -625,6 +625,9 @@ struct ath10k_hw_params {
 	bool tx_mac_seq_by_fw;
 
 	bool dynamic_sar_support;
+
+	/* limit tx credit */
+	bool tx_credit_limit;
 };
 
 struct htt_rx_desc;
-- 
2.29.0

[RFC 2/2] ath10k: Delay the unmapping of the buffer

[Youghandhar Chintala]

We are seeing a corner case smmu fault issue where copy engine is still
accessing the memory though host unmaps it hence as work around
we are delaying the unmapping the memory.

Tested-on: WLAN.HL.3.2.2.c10-00757-QCAHLSWMTPL-1

Signed-off-by: Youghandhar Chintala <quic_youghand@quicinc.com>
---
 drivers/net/wireless/ath/ath10k/core.c | 15 +++++++++++++++
 drivers/net/wireless/ath/ath10k/htc.c  |  5 +++++
 drivers/net/wireless/ath/ath10k/hw.h   |  3 +++
 3 files changed, 23 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index fcaed7d176568..65f775fb5ccab 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -93,6 +93,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.tx_mac_seq_by_fw = false,
 		.dynamic_sar_support = false,
 		.tx_credit_limit = false,
+		.delay_unmap_buffer = false,
 	},
 	{
 		.id = QCA988X_HW_2_0_VERSION,
@@ -130,6 +131,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.tx_mac_seq_by_fw = false,
 		.dynamic_sar_support = false,
 		.tx_credit_limit = false,
+		.delay_unmap_buffer = false,
 	},
 	{
 		.id = QCA9887_HW_1_0_VERSION,
@@ -168,6 +170,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.tx_mac_seq_by_fw = false,
 		.dynamic_sar_support = false,
 		.tx_credit_limit = false,
+		.delay_unmap_buffer = false,
 	},
 	{
 		.id = QCA6174_HW_3_2_VERSION,
@@ -200,6 +203,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.bmi_large_size_download = true,
 		.dynamic_sar_support = true,
 		.tx_credit_limit = false,
+		.delay_unmap_buffer = false,
 	},
 	{
 		.id = QCA6174_HW_2_1_VERSION,
@@ -237,6 +241,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.tx_mac_seq_by_fw = false,
 		.dynamic_sar_support = false,
 		.tx_credit_limit = false,
+		.delay_unmap_buffer = false,
 	},
 	{
 		.id = QCA6174_HW_2_1_VERSION,
@@ -274,6 +279,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.tx_mac_seq_by_fw = false,
 		.dynamic_sar_support = false,
 		.tx_credit_limit = false,
+		.delay_unmap_buffer = false,
 	},
 	{
 		.id = QCA6174_HW_3_0_VERSION,
@@ -311,6 +317,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.tx_mac_seq_by_fw = false,
 		.dynamic_sar_support = false,
 		.tx_credit_limit = false,
+		.delay_unmap_buffer = false,
 	},
 	{
 		.id = QCA6174_HW_3_2_VERSION,
@@ -351,6 +358,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.tx_mac_seq_by_fw = false,
 		.dynamic_sar_support = true,
 		.tx_credit_limit = false,
+		.delay_unmap_buffer = false,
 	},
 	{
 		.id = QCA99X0_HW_2_0_DEV_VERSION,
@@ -394,6 +402,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.tx_mac_seq_by_fw = false,
 		.dynamic_sar_support = false,
 		.tx_credit_limit = false,
+		.delay_unmap_buffer = false,
 	},
 	{
 		.id = QCA9984_HW_1_0_DEV_VERSION,
@@ -444,6 +453,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.tx_mac_seq_by_fw = false,
 		.dynamic_sar_support = false,
 		.tx_credit_limit = false,
+		.delay_unmap_buffer = false,
 	},
 	{
 		.id = QCA9888_HW_2_0_DEV_VERSION,
@@ -491,6 +501,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.tx_mac_seq_by_fw = false,
 		.dynamic_sar_support = false,
 		.tx_credit_limit = false,
+		.delay_unmap_buffer = false,
 	},
 	{
 		.id = QCA9377_HW_1_0_DEV_VERSION,
@@ -528,6 +539,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.tx_mac_seq_by_fw = false,
 		.dynamic_sar_support = false,
 		.tx_credit_limit = false,
+		.delay_unmap_buffer = false,
 	},
 	{
 		.id = QCA9377_HW_1_1_DEV_VERSION,
@@ -567,6 +579,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.tx_mac_seq_by_fw = false,
 		.dynamic_sar_support = false,
 		.tx_credit_limit = false,
+		.delay_unmap_buffer = false,
 	},
 	{
 		.id = QCA9377_HW_1_1_DEV_VERSION,
@@ -639,6 +652,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.tx_mac_seq_by_fw = false,
 		.dynamic_sar_support = false,
 		.tx_credit_limit = false,
+		.delay_unmap_buffer = false,
 	},
 	{
 		.id = WCN3990_HW_1_0_DEV_VERSION,
@@ -669,6 +683,7 @@ static const struct ath10k_hw_params ath10k_hw_params_list[] = {
 		.tx_mac_seq_by_fw = true,
 		.dynamic_sar_support = true,
 		.tx_credit_limit = true,
+		.delay_unmap_buffer = true,
 	},
 };
 
diff --git a/drivers/net/wireless/ath/ath10k/htc.c b/drivers/net/wireless/ath/ath10k/htc.c
index 108980476d654..e2640f244918e 100644
--- a/drivers/net/wireless/ath/ath10k/htc.c
+++ b/drivers/net/wireless/ath/ath10k/htc.c
@@ -55,6 +55,11 @@ void ath10k_htc_notify_tx_completion(struct ath10k_htc_ep *ep,
 	ath10k_dbg(ar, ATH10K_DBG_HTC, "%s: ep %d skb %pK\n", __func__,
 		   ep->eid, skb);
 
+	/* WAR - Delay the unmapping of the buffer */
+	if (ar->hw_params.delay_unmap_buffer &&
+	    ep->ul_pipe_id == 3)
+		mdelay(2);
+
 	ath10k_htc_restore_tx_skb(ep->htc, skb);
 
 	if (!ep->ep_ops.ep_tx_complete) {
diff --git a/drivers/net/wireless/ath/ath10k/hw.h b/drivers/net/wireless/ath/ath10k/hw.h
index 0d96df311b026..3f7539e8da0fc 100644
--- a/drivers/net/wireless/ath/ath10k/hw.h
+++ b/drivers/net/wireless/ath/ath10k/hw.h
@@ -628,6 +628,9 @@ struct ath10k_hw_params {
 
 	/* limit tx credit */
 	bool tx_credit_limit;
+
+	/* WAR - Delay the unmapping of the buffer */
+	bool delay_unmap_buffer;
 };
 
 struct htt_rx_desc;
-- 
2.29.0

Re: [RFC 0/2] ath10k:set tx credit to one and delay the unmaping

[Doug Anderson]

Hi,

On Thu, Jan 20, 2022 at 10:44 PM Youghandhar Chintala
<quic_youghand@quicinc.com> wrote:
>
> Currently for WCN3990, the host driver can send two wmi commands at once,
> now it is limiting to one because if HOST wants to initiate 2 DMA transfers,
> it is possible when copy complete interrupt for first DMA reaches HOST,
> CE has already updated SRRI for both DMA transfers and is in the middle
> of 2nd DMA. HOST uses SRRI to interpret how many DMA’s have been completed
> and tries to unmap/free both the DMA entries, but CE is still in the middle
> of 2nd DMA which can cause SMMU issues or corruption.
>
> We are seeing a corner case smmu fault issue where copy engine is still accessing
> the memory though host unmaps it hence as work around we are delaying the
> unmapping the memory and tx credit to one.
>
> Youghandhar Chintala (2):
>   ath10k: Set tx credit to one for wcn3990 snoc based devices
>   ath10k: Delay the unmapping of the buffer
>
>  drivers/net/wireless/ath/ath10k/core.c | 30 ++++++++++++++++++++++++++
>  drivers/net/wireless/ath/ath10k/htc.c  | 18 +++++++++++++---
>  drivers/net/wireless/ath/ath10k/htc.h  |  1 +
>  drivers/net/wireless/ath/ath10k/hw.h   |  6 ++++++
>  4 files changed, 52 insertions(+), 3 deletions(-)

I don't understand the root cause here myself. I also don't have
enough skin in the game to say whether this is the best/cleanest way
to work around the problem. Thus I'll refrain from a Reviewed-by tag.
That being said, I did a bunch of testing of this solution and I also
am aware of others that have tested it. Thus I'm convinced that it at
least works around the problem that has been observed. Hence:

Tested-by: Douglas Anderson <dianders@chromium.org>

In my case I was on the same (or nearly the same) hardware as
Youghandhar, though I had a slightly older build. FWIW:

Tested-on: WLAN.HL.3.2.2.c10-00754-QCAHLSWMTPL-1

-Doug

Re: [RFC 1/2] ath10k: Set tx credit to one for wcn3990 snoc based devices

[Doug Anderson]

Hi,

On Thu, Jan 20, 2022 at 10:44 PM Youghandhar Chintala
<quic_youghand@quicinc.com> wrote:
>
> -       htc->total_transmit_credits = __le16_to_cpu(msg->ready.credit_count);
> +       if (ar->hw_params.tx_credit_limit)
> +               htc->total_transmit_credits =
> +                       __le16_to_cpu(HTC_HOST_MAX_CREDIT_COUNT);
> +       else
> +               htc->total_transmit_credits =
> +                       __le16_to_cpu(msg->ready.credit_count);

Apparently 0-day had a bit of a problem with the syntax above. See
<https://crrev.com/c/3435607>. Basically you don't need the
__le16_to_cpu() around the constant HTC_HOST_MAX_CREDIT_COUNT.

-Doug