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@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.
Noted. Thanks.
Best regards,
Nam
