ath10k: Correct error check of dma_map_single()
diff mbox series

Message ID 20191010162653.141303-1-bjorn.andersson@linaro.org
State New
Headers show
Series
  • ath10k: Correct error check of dma_map_single()
Related show

Commit Message

Bjorn Andersson Oct. 10, 2019, 4:26 p.m. UTC
The return value of dma_map_single() should be checked for errors using
dma_mapping_error(), rather than testing for NULL. Correct this.

Fixes: 1807da49733e ("ath10k: wmi: add management tx by reference support over wmi")
Cc: stable@vger.kernel.org
Reported-by: Niklas Cassel <niklas.cassel@linaro.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/net/wireless/ath/ath10k/mac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Kalle Valo Oct. 11, 2019, 11:57 a.m. UTC | #1
Bjorn Andersson <bjorn.andersson@linaro.org> wrote:

> The return value of dma_map_single() should be checked for errors using
> dma_mapping_error(), rather than testing for NULL. Correct this.
> 
> Fixes: 1807da49733e ("ath10k: wmi: add management tx by reference support over wmi")
> Cc: stable@vger.kernel.org
> Reported-by: Niklas Cassel <niklas.cassel@linaro.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Did this fix any real bug? Or is this just something found during code review?
Bjorn Andersson Oct. 11, 2019, 5:16 p.m. UTC | #2
On Fri 11 Oct 04:57 PDT 2019, Kalle Valo wrote:

> Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
> 
> > The return value of dma_map_single() should be checked for errors using
> > dma_mapping_error(), rather than testing for NULL. Correct this.
> > 
> > Fixes: 1807da49733e ("ath10k: wmi: add management tx by reference support over wmi")
> > Cc: stable@vger.kernel.org
> > Reported-by: Niklas Cassel <niklas.cassel@linaro.org>
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> Did this fix any real bug? Or is this just something found during code review?
> 

CONFIG_DMA_API_DEBUG screamed at us for calling dma_unmap_single()
without ever having called dma_mapping_error() on the return value.

But Govind just pointed out to me that I hastily missed the fact that
this code path leaks the dequeued skb. So I'll respin the patch to fix
both issues at once.

Regards,
Bjorn

> -- 
> https://patchwork.kernel.org/patch/11183923/
> 
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
> 
> 
> _______________________________________________
> ath10k mailing list
> ath10k@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/ath10k
Kalle Valo Oct. 14, 2019, 8:32 a.m. UTC | #3
Bjorn Andersson <bjorn.andersson@linaro.org> writes:

> On Fri 11 Oct 04:57 PDT 2019, Kalle Valo wrote:
>
>> Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
>> 
>> > The return value of dma_map_single() should be checked for errors using
>> > dma_mapping_error(), rather than testing for NULL. Correct this.
>> > 
>> > Fixes: 1807da49733e ("ath10k: wmi: add management tx by reference
>> > support over wmi")
>> > Cc: stable@vger.kernel.org
>> > Reported-by: Niklas Cassel <niklas.cassel@linaro.org>
>> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>> 
>> Did this fix any real bug? Or is this just something found during code review?
>> 
>
> CONFIG_DMA_API_DEBUG screamed at us for calling dma_unmap_single()
> without ever having called dma_mapping_error() on the return value.

Ok, I'll add something about to the commit log in v2 so that the
background is also documented.

Patch
diff mbox series

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 3d2c8fcba952..a01868938692 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -3904,7 +3904,7 @@  void ath10k_mgmt_over_wmi_tx_work(struct work_struct *work)
 			     ar->running_fw->fw_file.fw_features)) {
 			paddr = dma_map_single(ar->dev, skb->data,
 					       skb->len, DMA_TO_DEVICE);
-			if (!paddr)
+			if (dma_mapping_error(ar->dev, paddr))
 				continue;
 			ret = ath10k_wmi_mgmt_tx_send(ar, skb, paddr);
 			if (ret) {