Linux-Wireless Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] ath10k:  Free beacon buf later in vdev teardown.
@ 2019-02-27 16:27 greearb
  2019-09-10 13:12 ` Kalle Valo
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: greearb @ 2019-02-27 16:27 UTC (permalink / raw)
  To: linux-wireless; +Cc: Ben Greear

From: Ben Greear <greearb@candelatech.com>

My wave-1 firmware often crashes when I am bringing down
AP vdevs, and sometimes at least some machines lockup hard
after spewing IOMMU errors.

I don't see the same issue in STA mode, so I suspect beacons
are the issue.

Moving the beacon buf deletion to later in the vdev teardown
logic appears to help this problem.  Firmware still crashes
often, but several iterations did not show IOMMU errors and
machine didn't hang.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 drivers/net/wireless/ath/ath10k/mac.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index f70c48ec9464..c38561788791 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -6142,10 +6142,6 @@ static void ath10k_remove_interface(struct ieee80211_hw *hw,
 
 	mutex_lock(&ar->conf_mutex);
 
-	spin_lock_bh(&ar->data_lock);
-	ath10k_mac_vif_beacon_cleanup(arvif);
-	spin_unlock_bh(&ar->data_lock);
-
 	ret = ath10k_spectral_vif_stop(arvif);
 	if (ret)
 		ath10k_warn(ar, "failed to stop spectral for vdev %i: %d\n",
@@ -6204,6 +6200,10 @@ static void ath10k_remove_interface(struct ieee80211_hw *hw,
 			peer->vif = NULL;
 		}
 	}
+
+	/* Clean this up late, less opportunity for firmware to access
+	   DMA memory we have deleted. */
+	ath10k_mac_vif_beacon_cleanup(arvif);
 	spin_unlock_bh(&ar->data_lock);
 
 	ath10k_peer_cleanup(ar, arvif->vdev_id);
-- 
2.20.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] ath10k:  Free beacon buf later in vdev teardown.
  2019-02-27 16:27 [PATCH] ath10k: Free beacon buf later in vdev teardown greearb
@ 2019-09-10 13:12 ` Kalle Valo
       [not found] ` <20190910131244.267AF602F2@smtp.codeaurora.org>
  2019-09-12 14:52 ` Kalle Valo
  2 siblings, 0 replies; 5+ messages in thread
From: Kalle Valo @ 2019-09-10 13:12 UTC (permalink / raw)
  To: greearb; +Cc: linux-wireless, Ben Greear

greearb@candelatech.com wrote:

> My wave-1 firmware often crashes when I am bringing down
> AP vdevs, and sometimes at least some machines lockup hard
> after spewing IOMMU errors.
> 
> I don't see the same issue in STA mode, so I suspect beacons
> are the issue.
> 
> Moving the beacon buf deletion to later in the vdev teardown
> logic appears to help this problem.  Firmware still crashes
> often, but several iterations did not show IOMMU errors and
> machine didn't hang.

I'm not really fond of fixing issues just by luck but after a quick look at the
code I can't see any harm in this change either. So I guess it's ok.

But can you provide the exact hardware and firmware you used for testing, I'll
add it to the commit log. I want to document that in every commit:

https://wireless.wiki.kernel.org/en/users/drivers/ath10k/submittingpatches#guidelines

Please also CC ath10k list when submitting patches.

-- 
https://patchwork.kernel.org/patch/10831867/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] ath10k: Free beacon buf later in vdev teardown.
       [not found] ` <20190910131244.267AF602F2@smtp.codeaurora.org>
@ 2019-09-10 13:19   ` Ben Greear
  2019-09-12 12:47     ` Kalle Valo
  0 siblings, 1 reply; 5+ messages in thread
From: Ben Greear @ 2019-09-10 13:19 UTC (permalink / raw)
  To: Kalle Valo; +Cc: linux-wireless



On 09/10/2019 06:12 AM, Kalle Valo wrote:
> greearb@candelatech.com wrote:
>
>> My wave-1 firmware often crashes when I am bringing down
>> AP vdevs, and sometimes at least some machines lockup hard
>> after spewing IOMMU errors.
>>
>> I don't see the same issue in STA mode, so I suspect beacons
>> are the issue.
>>
>> Moving the beacon buf deletion to later in the vdev teardown
>> logic appears to help this problem.  Firmware still crashes
>> often, but several iterations did not show IOMMU errors and
>> machine didn't hang.
>
> I'm not really fond of fixing issues just by luck but after a quick look at the
> code I can't see any harm in this change either. So I guess it's ok.
>
> But can you provide the exact hardware and firmware you used for testing, I'll
> add it to the commit log. I want to document that in every commit:

I used Compex WLE900VX hardware, and the firmware was ath10k-ct something or other,
it has been a long time, so I don't recall the exact firmware version.

The real fix for this is to have the firmware do proper beacon tx completion
logic so that the driver can know when it can safely free buffers.  I implemented
this in my ath10k-ct firmware/driver, but unless you want to start accepting patches for my
firmware, then that will not help upstream ath10k.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] ath10k: Free beacon buf later in vdev teardown.
  2019-09-10 13:19   ` Ben Greear
@ 2019-09-12 12:47     ` Kalle Valo
  0 siblings, 0 replies; 5+ messages in thread
From: Kalle Valo @ 2019-09-12 12:47 UTC (permalink / raw)
  To: Ben Greear; +Cc: linux-wireless

Ben Greear <greearb@candelatech.com> writes:

> On 09/10/2019 06:12 AM, Kalle Valo wrote:
>> greearb@candelatech.com wrote:
>>
>>> My wave-1 firmware often crashes when I am bringing down
>>> AP vdevs, and sometimes at least some machines lockup hard
>>> after spewing IOMMU errors.
>>>
>>> I don't see the same issue in STA mode, so I suspect beacons
>>> are the issue.
>>>
>>> Moving the beacon buf deletion to later in the vdev teardown
>>> logic appears to help this problem.  Firmware still crashes
>>> often, but several iterations did not show IOMMU errors and
>>> machine didn't hang.
>>
>> I'm not really fond of fixing issues just by luck but after a quick look at the
>> code I can't see any harm in this change either. So I guess it's ok.
>>
>> But can you provide the exact hardware and firmware you used for testing, I'll
>> add it to the commit log. I want to document that in every commit:
>
> I used Compex WLE900VX hardware, and the firmware was ath10k-ct something or other,
> it has been a long time, so I don't recall the exact firmware version.

Ok, I added QCA9880 and unspecified version of ath10k-ct firmware to the
commit log.

> The real fix for this is to have the firmware do proper beacon tx
> completion logic so that the driver can know when it can safely free
> buffers. I implemented this in my ath10k-ct firmware/driver, but
> unless you want to start accepting patches for my firmware, then that
> will not help upstream ath10k.

It's a challenge enough to support so many differerent QCOM firmware
branches and still try to avoid bloating the driver too much to keep it
maintainable. Adding support for a new firmware branch/fork with a
significantly different interface would make everything so much harder.

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] ath10k:  Free beacon buf later in vdev teardown.
  2019-02-27 16:27 [PATCH] ath10k: Free beacon buf later in vdev teardown greearb
  2019-09-10 13:12 ` Kalle Valo
       [not found] ` <20190910131244.267AF602F2@smtp.codeaurora.org>
@ 2019-09-12 14:52 ` Kalle Valo
  2 siblings, 0 replies; 5+ messages in thread
From: Kalle Valo @ 2019-09-12 14:52 UTC (permalink / raw)
  To: greearb; +Cc: linux-wireless, Ben Greear

greearb@candelatech.com wrote:

> My wave-1 firmware often crashes when I am bringing down
> AP vdevs, and sometimes at least some machines lockup hard
> after spewing IOMMU errors.
> 
> I don't see the same issue in STA mode, so I suspect beacons
> are the issue.
> 
> Moving the beacon buf deletion to later in the vdev teardown
> logic appears to help this problem.  Firmware still crashes
> often, but several iterations did not show IOMMU errors and
> machine didn't hang.
> 
> Tested hardware: QCA9880
> Tested firmware: ath10k-ct from beginning of 2019, exact version unknown
> 
> Signed-off-by: Ben Greear <greearb@candelatech.com>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>

Patch applied to ath-next branch of ath.git, thanks.

b3281c6cb768 ath10k: free beacon buf later in vdev teardown

-- 
https://patchwork.kernel.org/patch/10831867/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-27 16:27 [PATCH] ath10k: Free beacon buf later in vdev teardown greearb
2019-09-10 13:12 ` Kalle Valo
     [not found] ` <20190910131244.267AF602F2@smtp.codeaurora.org>
2019-09-10 13:19   ` Ben Greear
2019-09-12 12:47     ` Kalle Valo
2019-09-12 14:52 ` Kalle Valo

Linux-Wireless Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-wireless/0 linux-wireless/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-wireless linux-wireless/ https://lore.kernel.org/linux-wireless \
		linux-wireless@vger.kernel.org linux-wireless@archiver.kernel.org
	public-inbox-index linux-wireless


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-wireless


AGPL code for this site: git clone https://public-inbox.org/ public-inbox