Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 27 Sep 2022 13:54:27 +0200
From:   Nam Cao <namcaov@gmail.com>
To:     Dan Carpenter <dan.carpenter@oracle.com>
Cc:     forest@alittletooquiet.net, gregkh@linuxfoundation.org,
        philipp.g.hortmann@gmail.com, linux-kernel@vger.kernel.org,
        linux-staging@lists.linux.dev
Subject: Re: [RFC PATCH 5/5] staging: vt6655: implement allocation failure
 handling
Message-ID: <20220927115427.GC10468@nam-dell>
References: <cover.1663273218.git.namcaov@gmail.com>
 <dd63998765f1ea1bf507a94c2d49317b36b5f32c.1663273218.git.namcaov@gmail.com>
 <YyhAhiUlclk7HxE3@kadam>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YyhAhiUlclk7HxE3@kadam>
Precedence: bulk

On Mon, Sep 19, 2022 at 01:12:22PM +0300, Dan Carpenter wrote:
> 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();?

Yes.

> 
> >         - 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).

Almost correct: if we don't have space for next frame, we _drop_ the
current frame. Note that this is also how it is implemented in similar
drivers, such as:
  - adm8211_interrupt_rci() in drivers/net/wireless/admtek/adm8211.c
  - rtl8180_handle_rx() in drivers/net/wireless/realtek/rtl818x/rtl8180/dev.c

> 
> >         - 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?

By the device/hardware.

The basic idea how this driver works is that: the cpu allocates buffers
and marks them as OWNED_BY_NIC, basically telling the device that "here
are some buffers that you can use". When there is new frame, the device
looks for buffer marked with OWNED_BY_NIC and write data in there; then
it marks the buffer as OWNED_BY_HOST, basically saying "there is some
valid data in this buffer, you should read it".

> 
> > 
> > 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@gmail.com>
> > ---
> >  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.

Noted. Thanks.

Best regards,
Nam