[PATCH v2 0/5] wcn36xx: Fix DMA buffer allocation and free logic
Jeff Johnson
quic_jjohnson at quicinc.com
Tue Oct 19 08:59:28 PDT 2021
On 10/18/2021 4:17 PM, Bryan O'Donoghue wrote:
> V2:
> - Functionally decomposes the DXE reset in an additional patch.
> Since we call this logic more than once, it should be in a function.
>
> - Leaves as-is the DXE reset write.
>
> Johannes Berg asked me if we are sure by the time the write to the reset
> register completes that DXE transactions will be suitably quiesced.
>
> The answer is:
> 1. I believe these writes are non-posted writes
> 2. Downstream doesn't poll for DXE reset completion
>
> So on #2 I have no real data for or against a polling operation, my tests
> indicate the reset indication in the register is atomic and as far as I
> can discern that also means DMA transactions are terminated.
>
> V1:
> Digging around through some bugs reported from an extensive testing cycle
> we've found that wcn36xx has a number of unexplained RX related oopses.
>
> In at least one case we appear to have DMA'd data to an unmapped region.
> The written data appears to be a correctly formed DMA buffer descriptor - a
> DXE BD in WCNSS parlance, with an AP beacon inside of it.
>
> Reasoning about how such a situation might come about and reviewing the
> run-time code, there was no obvious path where we might free a BD or an
> skbuff pointed to by a BD, which DXE might not be aware of.
>
> However looking at the ieee80211_ops.start and ieee80211_ops.stop callbacks
> in wcn36xx we can see a number of bugs associated with BD allocation, error
> handling and leaving the DMA engine active, despite freeing SKBs on the MSM
> side.
>
> This last mention - failure to quiesce potential DMA from the downstream
> agent - WCNSS DXE despite freeing the memory @ the skbuffs is a decent
> candidate for our unexplained upstream DMA transaction to unmapped memory.
>
> Since wcn36xx_stop and wcn36xx_start can be called a number of times by the
> controlling upper layers it means there is a potential gap between
> wcn36xx_stop and wcn36xx_start which could leave WCNSS in a state where it
> will try to DMA to memory we have freed.
>
> This series addresses the obvious bugs that jump out on the start()/stop()
> path.
>
> Patch #1
> In order to make it easier to read the DXE code, I've moved all of the
> lock taking and freeing for DXE into dxe.c
>
> Patch #2
> Fixes a very obviously broken channel enable/disable cycle
>
> Patch #3
> Fixes a very obvious memory leak with dma_alloc_coherent()
>
> Patch #4
> Makes sure before we release skbuffs which we assigned to the RX channels
> that we ensure the DXE block is put into reset
>
> Bryan O'Donoghue (5):
> wcn36xx: Fix dxe lock layering violation
> wcn36xx: Fix DMA channel enable/disable cycle
> wcn36xx: Release DMA channel descriptor allocations
> wcn36xx: Functionally decompose DXE reset
> wcn36xx: Put DXE block into reset before freeing memory
>
> drivers/net/wireless/ath/wcn36xx/dxe.c | 83 +++++++++++++++++++++----
> drivers/net/wireless/ath/wcn36xx/dxe.h | 2 +
> drivers/net/wireless/ath/wcn36xx/txrx.c | 15 +----
> 3 files changed, 74 insertions(+), 26 deletions(-)
>
Reviewed-by: Jeff Johnson <quic_jjohnson at quicinc.com>
More information about the wcn36xx
mailing list