Re: [RFC PATCH 5/5] staging: vt6655: implement allocation failure handling

[Dan Carpenter]

On Thu, Sep 15, 2022 at 10:29:36PM +0200, Nam Cao wrote:
> The function device_rx_srv does not handle allocation failure very well.
> Currently, it performs these steps:
>         - Unmap DMA buffer and hand over the buffer to mac80211

Does the unmapping happens in vnt_receive_frame();?

>         - Allocate and dma-map new buffer

Is the new buffer for the next frame?  So in your patch if we don't
have space for the next frame then we do not process the current frame?
(Rhetorical questions are a bad idea on development lists.  I genuinely
don't know the answers to these questions).

>         - If allocation fails, abort
> 
> The problem is that, it aborts while still marking the buffer as
> OWNED_BY_HOST. So when this function is called again in the future, it
> incorrectly perceives the same buffer as valid and dma-unmap and hand
> over this buffer to mac80211 again.

Where is it supposed to get marked as OWNED_BY_HOST?

> 
> Re-implement this function to do things in a different order:
>         - Allocate and dma-map new buffer
>         - If allocation fails, abort and give up the ownership of the
>           buffer (so that the device can re-use this buffer)
>         - If allocation does not fail: unmap dma buffer and hand over
>           the buffer to mac80211
> 
> Thus, when the driver cannot allocate new buffer, it simply discards the
> received data and re-use the current buffer.
> 
> Signed-off-by: Nam Cao <namcaov@xxxxxxxxx>
> ---
>  drivers/staging/vt6655/device_main.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/vt6655/device_main.c b/drivers/staging/vt6655/device_main.c
> index ca6c4266f010..8ae4ecca2ee3 100644
> --- a/drivers/staging/vt6655/device_main.c
> +++ b/drivers/staging/vt6655/device_main.c
> @@ -826,6 +826,7 @@ static void device_free_td1_ring(struct vnt_private *priv)
>  static int device_rx_srv(struct vnt_private *priv, unsigned int idx)
>  {
>  	struct vnt_rx_desc *rd;
> +	struct vnt_rd_info new_info;
>  	int works = 0;
>  
>  	for (rd = priv->pCurrRD[idx];
> @@ -837,16 +838,18 @@ static int device_rx_srv(struct vnt_private *priv, unsigned int idx)
>  		if (!rd->rd_info->skb)
>  			break;
>  
> -		vnt_receive_frame(priv, rd);
> -
> -		if (!device_alloc_rx_buf(priv, rd->rd_info)) {
> +		if (!device_alloc_rx_buf(priv, &new_info)) {
>  			dev_err(&priv->pcid->dev,
>  				"can not allocate rx buf\n");
> +			rd->rd0.owner = OWNED_BY_NIC;
>  			break;
> -		} else {
> -			device_init_rx_desc(priv, rd);
>  		}
>  
> +		vnt_receive_frame(priv, rd);
> +
> +		memcpy(rd->rd_info, &new_info, sizeof(new_info));
> +		device_init_rx_desc(priv, rd);
> +
>  		rd->rd0.owner = OWNED_BY_NIC;
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

The device_init_rx_desc() function sets it to OWNED_BY_NIC so this line
can be deleted.

regards,
dan carpenter