[PATCH v2] wifi: mt76: fix potential memory leakage

[Sujuan Chen (陈素娟)]

On Wed, 2022-12-21 at 11:26 +0100, lorenzo at kernel.org wrote:
> [...]
> 
> > Hi Lore,
> 
> Hi Sujuan,
> 
> > 
> > we love your patch, but we have another patch to avoid memory
> > fragmentation by duplicating the rx skb after mt76_dma_dequeue().
> > it
> > requires mt76_get_rxwi() be placed before page_frag_alloc().
> > 
> > the below patch(need rebase) will be sent after the current patch
> > is
> > accepted.
> > 
> > --- a/drivers/net/wireless/mediatek/mt76/dma.c
> > +++ b/drivers/net/wireless/mediatek/mt76/dma.c
> > @@ -386,9 +386,11 @@ mt76_dma_get_buf(struct mt76_dev *dev, struct
> > mt76_queue *q, int idx,
> >                                  SKB_WITH_OVERHEAD(q->buf_size),
> >                                  DMA_FROM_DEVICE);
> > 
> > -               buf = t->ptr;
> > +               buf = page_frag_alloc(&q->rx_page, q->buf_size,
> > GFP_ATOMIC);
> > +               if (!buf)
> > +                       return NULL;
> > +               memcpy(buf, t->ptr, SKB_WITH_OVERHEAD(q-
> > >buf_size));
> 
> We this approach we still need to allocate the buffer (in
> mt76_dma_get_buf()
> instead of mt76_dma_rx_fill()) but we need even to copy the full
> buffer that
> can be pretty expensive instead of relying on the DMA, so I would
> avoid this
> approach.
> 

Hi Lore,

Yes, it is so expensive, but if no memcopy, it will casue memory
fragmentation (we hit this issue in internal SQC).

as we know, wed needs to exchange rx pkt(belongs to wed rx buffer
manager) with wifi driver(dma rx data queue) by exchanging wfdma dmad
to ensure the free wed rx buffer.

it is possiable that a large number of buffer has been exchanged to wed
and can not come back to wlan driver. So, the memory from the same 32K
page cache is unable to be released, and it will be failed at
page_frag_alloc in mt76_dma_rx_fill.

Any ideas but memcopy？

Regards,
Sujuan

> Regards,
> Lorenzo
> 
> >                 t->dma_addr = 0;
> > -               t->ptr = NULL;
> > 
> >                 mt76_put_rxwi(dev, t);
> > 
> > @@ -569,6 +571,7 @@ mt76_dma_rx_fill(struct mt76_dev *dev, struct
> > mt76_queue *q)
> >         while (q->queued < q->ndesc - 1) {
> >                 struct mt76_txwi_cache *t = NULL;
> >                 struct mt76_queue_buf qbuf;
> > +               bool skip_alloc = false;
> >                 void *buf = NULL;
> > 
> >                 if ((q->flags & MT_QFLAG_WED) &&
> > @@ -576,11 +579,19 @@ mt76_dma_rx_fill(struct mt76_dev *dev, struct
> > mt76_queue *q)
> >                         t = mt76_get_rxwi(dev);
> >                         if (!t)
> >                                 break;
> > +
> > +                       if (t->ptr) {
> > +                               skip_alloc = true;
> > +                               buf = t->ptr;
> > +                       }
> >                 }
> > 
> > -               buf = page_frag_alloc(&q->rx_page, q->buf_size,
> > GFP_ATOMIC);
> > -               if (!buf)
> > -                       break;
> > +               if (!skip_alloc) {
> > +                       buf = page_frag_alloc(&q->rx_page, q-
> > >buf_size,
> > +                                             GFP_ATOMIC);
> > +                       if (!buf)
> > +                               break;
> > +               }
> > 
> >                 addr = dma_map_single(dev->dma_dev, buf, len,
> > DMA_FROM_DEVICE);
> >                 if (unlikely(dma_mapping_error(dev->dma_dev,
> > addr))) {
> > --
> > 2.18.0
> > 
> > > > -- 
> > > > 2.18.0
> > > >