On Mon, Feb 11, 2019 at 10:19 PM Tony Chuang <yhchuang@xxxxxxxxxxx> wrote: > > From: Brian Norris [mailto:briannorris@xxxxxxxxxxxx] > > On Wed, Jan 30, 2019 at 12:02:10PM +0800, yhchuang@xxxxxxxxxxx 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
