[RFC] brcmsmac: check return from dma_map_single

[RFC] brcmsmac: check return from dma_map_single

[John W. Linville]

From: "John W. Linville" <linville@tuxdriver.com>

dma_map_single can fail, so it's return value needs to be checked and
handled accordingly.  Failure to do so is akin to failing to check the
return from a kmalloc.

http://linuxdriverproject.org/mediawiki/index.php/DMA_Mapping_Error_Analysis

Signed-off-by: John W. Linville <linville@tuxdriver.com>
Cc: Arend van Spriel <arend@broadcom.com>
---
I'm not too sure about the bail-out in dma_rxfill.  Perhaps the experts
can suggest something better?

 drivers/net/wireless/brcm80211/brcmsmac/dma.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/brcm80211/brcmsmac/dma.c b/drivers/net/wireless/brcm80211/brcmsmac/dma.c
index 5e53305..fa24c58 100644
--- a/drivers/net/wireless/brcm80211/brcmsmac/dma.c
+++ b/drivers/net/wireless/brcm80211/brcmsmac/dma.c
@@ -1079,6 +1079,10 @@ bool dma_rxfill(struct dma_pub *pub)
 
 		pa = dma_map_single(di->dmadev, p->data, di->rxbufsize,
 				    DMA_FROM_DEVICE);
+		if (dma_mapping_error(di->dmadev, pa)) {
+			brcmu_pkt_buf_free_skb(p);
+			break;
+		}
 
 		/* save the free packet pointer */
 		di->rxp[rxout] = p;
@@ -1293,13 +1297,15 @@ int dma_txfast(struct dma_pub *pub, struct sk_buff *p, bool commit)
 	if (len == 0)
 		return 0;
 
+	/* get physical address of buffer start */
+	pa = dma_map_single(di->dmadev, data, len, DMA_TO_DEVICE);
+	if (dma_mapping_error(di->dmadev, pa))
+		return -1;
+
 	/* return nonzero if out of tx descriptors */
 	if (nexttxd(di, txout) == di->txin)
 		goto outoftxd;
 
-	/* get physical address of buffer start */
-	pa = dma_map_single(di->dmadev, data, len, DMA_TO_DEVICE);
-
 	/* With a DMA segment list, Descriptor table is filled
 	 * using the segment list instead of looping over
 	 * buffers in multi-chain DMA. Therefore, EOF for SGLIST
-- 
1.7.11.4

Re: [RFC] brcmsmac: check return from dma_map_single

[Arend van Spriel]

On 10/02/2012 08:46 PM, John W. Linville wrote:
> From: "John W. Linville" <linville@tuxdriver.com>
>
> dma_map_single can fail, so it's return value needs to be checked and
> handled accordingly.  Failure to do so is akin to failing to check the
> return from a kmalloc.
>
> http://linuxdriverproject.org/mediawiki/index.php/DMA_Mapping_Error_Analysis
>
> Signed-off-by: John W. Linville <linville@tuxdriver.com>
> Cc: Arend van Spriel <arend@broadcom.com>
> ---
> I'm not too sure about the bail-out in dma_rxfill.  Perhaps the experts
> can suggest something better?
>
>   drivers/net/wireless/brcm80211/brcmsmac/dma.c | 12 +++++++++---
>   1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/wireless/brcm80211/brcmsmac/dma.c b/drivers/net/wireless/brcm80211/brcmsmac/dma.c
> index 5e53305..fa24c58 100644
> --- a/drivers/net/wireless/brcm80211/brcmsmac/dma.c
> +++ b/drivers/net/wireless/brcm80211/brcmsmac/dma.c
> @@ -1079,6 +1079,10 @@ bool dma_rxfill(struct dma_pub *pub)
>
>   		pa = dma_map_single(di->dmadev, p->data, di->rxbufsize,
>   				    DMA_FROM_DEVICE);
> +		if (dma_mapping_error(di->dmadev, pa)) {
> +			brcmu_pkt_buf_free_skb(p);
> +			break;
> +		}

I noticed the calling function is ignoring any error returned by 
dma_rxfill(). I am not sure what to do here when this mapping error 
happens after being halfway the for loop filling the dma with receive 
buffers. The function is being called on mac80211 .start() callback so 
we have to pass the failure back to that.

>   		/* save the free packet pointer */
>   		di->rxp[rxout] = p;
> @@ -1293,13 +1297,15 @@ int dma_txfast(struct dma_pub *pub, struct sk_buff *p, bool commit)
>   	if (len == 0)
>   		return 0;
>
> +	/* get physical address of buffer start */
> +	pa = dma_map_single(di->dmadev, data, len, DMA_TO_DEVICE);
> +	if (dma_mapping_error(di->dmadev, pa))
> +		return -1;
> +

Same for dma_txfast() and from DMA-API.txt I understand the buffer needs 
to be freed:

- checking the returned dma_addr_t of dma_map_single and dma_map_page
   by using dma_mapping_error():

         dma_addr_t dma_handle;

         dma_handle = dma_map_single(dev, addr, size, direction);
         if (dma_mapping_error(dev, dma_handle)) {
                 /*
                  * reduce current DMA mapping usage,
                  * delay and try again later or
                  * reset driver.
                  */
         }

Networking drivers must call dev_kfree_skb to free the socket buffer
and return NETDEV_TX_OK if the DMA mapping fails on the transmit hook
(ndo_start_xmit). This means that the socket buffer is just dropped in
the failure case.

>   	/* return nonzero if out of tx descriptors */
>   	if (nexttxd(di, txout) == di->txin)
>   		goto outoftxd;
>
> -	/* get physical address of buffer start */
> -	pa = dma_map_single(di->dmadev, data, len, DMA_TO_DEVICE);
> -
>   	/* With a DMA segment list, Descriptor table is filled
>   	 * using the segment list instead of looping over
>   	 * buffers in multi-chain DMA. Therefore, EOF for SGLIST
>

I will discuss how we should handle the error flows given the options 
mentioned in the DMA-API pseudo-code above.

Gr. AvS