MIME-Version: 1.0
References: <1548820940-15237-1-git-send-email-yhchuang@realtek.com>
 <1548820940-15237-4-git-send-email-yhchuang@realtek.com> <20190131223609.GA191502@google.com>
 <F7CD281DE3E379468C6D07993EA72F84D17A3A5E@RTITMBSVM04.realtek.com.tw>
In-Reply-To: <F7CD281DE3E379468C6D07993EA72F84D17A3A5E@RTITMBSVM04.realtek.com.tw>
From:   Brian Norris <briannorris@chromium.org>
Date:   Tue, 12 Feb 2019 14:04:02 -0800
Message-ID: <CA+ASDXMGtktTQtwoyCtaYmc9n5+OkXWMJn8FK-nGnoOLrbFe+w@mail.gmail.com>
Subject: Re: [PATCH v4 03/13] rtw88: hci files
To:     Tony Chuang <yhchuang@realtek.com>
Cc:     "kvalo@codeaurora.org" <kvalo@codeaurora.org>,
        "johannes@sipsolutions.net" <johannes@sipsolutions.net>,
        "Larry.Finger@lwfinger.net" <Larry.Finger@lwfinger.net>,
        Pkshih <pkshih@realtek.com>, Andy Huang <tehuang@realtek.com>,
        "sgruszka@redhat.com" <sgruszka@redhat.com>,
        "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On Mon, Feb 11, 2019 at 10:19 PM Tony Chuang <yhchuang@realtek.com> wrote:
> > From: Brian Norris [mailto:briannorris@chromium.org]
> > On Wed, Jan 30, 2019 at 12:02:10PM +0800, yhchuang@realtek.com wrote:
> > > diff --git a/drivers/net/wireless/realtek/rtw88/pci.c
> > b/drivers/net/wireless/realtek/rtw88/pci.c
> > > new file mode 100644
> > > index 0000000..ef3c9bb
> > > --- /dev/null
> > > +++ b/drivers/net/wireless/realtek/rtw88/pci.c
> > > @@ -0,0 +1,1210 @@
> >
> > ...
> >
> > > +static int rtw_pci_init_rx_ring(struct rtw_dev *rtwdev,
> > > +                           struct rtw_pci_rx_ring *rx_ring,
> > > +                           u8 desc_size, u32 len)
> > > +{
> > > +   struct pci_dev *pdev = to_pci_dev(rtwdev->dev);
> > > +   struct sk_buff *skb = NULL;
> > > +   dma_addr_t dma;
> > > +   u8 *head;
> > > +   int ring_sz = desc_size * len;
> > > +   int buf_sz = RTK_PCI_RX_BUF_SIZE;
> > > +   int i, allocated;
> > > +   int ret = 0;
> > > +
> > > +   head = pci_zalloc_consistent(pdev, ring_sz, &dma);
> > > +   if (!head) {
> > > +           rtw_err(rtwdev, "failed to allocate rx ring\n");
> > > +           return -ENOMEM;
> > > +   }
> > > +   rx_ring->r.head = head;
> > > +
> > > +   for (i = 0; i < len; i++) {
> > > +           skb = dev_alloc_skb(buf_sz);
> > > +           if (!skb) {
> > > +                   allocated = i;
> > > +                   ret = -ENOMEM;
> > > +                   goto err_out;
> > > +           }
> > > +
> > > +           memset(skb->data, 0, buf_sz);
> > > +           rx_ring->buf[i] = skb;
> > > +           ret = rtw_pci_reset_rx_desc(rtwdev, skb, rx_ring, i, desc_size);
> > > +           if (ret) {
> > > +                   allocated = i;
> > > +                   goto err_out;
> > > +           }
> > > +   }
> > > +
> > > +   rx_ring->r.dma = dma;
> > > +   rx_ring->r.len = len;
> > > +   rx_ring->r.desc_size = desc_size;
> > > +   rx_ring->r.wp = 0;
> > > +   rx_ring->r.rp = 0;
> > > +
> > > +   return 0;
> > > +
> > > +err_out:
> > > +   dev_kfree_skb_any(skb);
> >
> > Maybe I'm misreading but...shouldn't this line not be here? You properly
> > iterate over the allocated SKBs below, and this looks like you're just
> > going to be double-freeing (or, negative ref-counting).
>
> Yeah, I think I should move this line above after reset_rx_desc failed.

Ah yes, so it's not technically incorrect, it's just a bit strange to
read. Yes, moving it up to the failure location would make it clearer
IMO. Thanks.

> > > +   for (i = 0; i < allocated; i++) {
> > > +           skb = rx_ring->buf[i];
> > > +           if (!skb)
> > > +                   continue;
> > > +           dma = *((dma_addr_t *)skb->cb);
> > > +           pci_unmap_single(pdev, dma, buf_sz, PCI_DMA_FROMDEVICE);
> > > +           dev_kfree_skb_any(skb);
> > > +           rx_ring->buf[i] = NULL;
> > > +   }
> > > +   pci_free_consistent(pdev, ring_sz, head, dma);
> > > +
> > > +   rtw_err(rtwdev, "failed to init rx buffer\n");
> > > +
> > > +   return ret;
> > > +}
> > > +

...

> After some testing, this function I think can be removed.
> (rtw_pci_parse_configuration)

Why was it here in the first place? Were there any hardware quirks
that this was covering? Or, are you just saying that the default
values are already correct (plus, the PCI framework already brings you
out of D3)? I do also see that removing this function doesn't actually
have a net effect on the the PCI configuration, judging by lspci
-vvvxxxx.

Brian