On Wed, Oct 03, 2018 at 04:02:19PM +0800, yhchuang@xxxxxxxxxxx wrote:
> +static inline u32
> +rtw_read_rf(struct rtw_dev *rtwdev, enum rtw_rf_path rf_path,
> + u32 addr, u32 mask)
> +{
> + unsigned long flags;
> + u32 val;
> +
> + spin_lock_irqsave(&rtwdev->rf_lock, flags);
> + val = rtwdev->chip->ops->read_rf(rtwdev, rf_path, addr, mask);
> + spin_unlock_irqrestore(&rtwdev->rf_lock, flags);
What for is rtwdev->rf_lock lock ? Is possible to call
rtw_read_rf() or rtw_write_rf() in some simultanious way ?
> +static int rtw_pci_reset_rx_desc(struct rtw_dev *rtwdev, struct sk_buff *skb,
> + struct rtw_pci_rx_ring *rx_ring,
> + u32 idx, u32 desc_sz)
> +{
> + struct pci_dev *pdev = to_pci_dev(rtwdev->dev);
> + struct rtw_pci_rx_buffer_desc *buf_desc;
> + int buf_sz = RTK_PCI_RX_BUF_SIZE;
> + dma_addr_t dma;
> +
> + if (!skb)
> + return -EINVAL;
Too late, see below.
> +
> + dma = pci_map_single(pdev, skb->data, buf_sz, PCI_DMA_FROMDEVICE);
> + if (pci_dma_mapping_error(pdev, dma))
> + return -EBUSY;
> +
> + *((dma_addr_t *)skb->cb) = dma;
> + buf_desc = (struct rtw_pci_rx_buffer_desc *)(rx_ring->r.head +
> + idx * desc_sz);
> + memset(buf_desc, 0, sizeof(*buf_desc));
> + buf_desc->buf_size = cpu_to_le16(8216);
Why the difference between RTK_PCI_RX_BUF_SIZE = 9100 and this 8216 ?
> +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);
> + memset(skb->data, 0, buf_sz);
No error check. Also I think you should use different version to
allow specify GPF_KERNEL flag, this is not atomic context.
> +static void rtw_pci_dma_check(struct rtw_dev *rtwdev,
> + struct rtw_pci_rx_ring *rx_ring,
> + u32 idx)
> +{
> + struct rtw_chip_info *chip = rtwdev->chip;
> + struct rtw_pci_rx_buffer_desc *buf_desc;
> + u32 desc_sz = chip->rx_buf_desc_sz;
> + u16 total_pkt_size;
> + int i;
> +
> + buf_desc = (struct rtw_pci_rx_buffer_desc *)(rx_ring->r.head +
> + idx * desc_sz);
> + for (i = 0; i < 20; i++) {
> + total_pkt_size = le16_to_cpu(buf_desc->total_pkt_size);
> + if (total_pkt_size)
> + return;
> + }
> +
> + if (i >= 20)
> + rtw_warn(rtwdev, "pci bus timeout, drop packet\n");
This is not right, most likely you need to use
dma_sync_single_for_cpu() .
> +static int rtw_pci_tx(struct rtw_dev *rtwdev,
> + struct rtw_tx_pkt_info *pkt_info,
> + struct sk_buff *skb)
> +{
> + struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
> + struct rtw_pci_tx_ring *ring;
> + u8 queue = rtw_hw_queue_mapping(skb);
> + int ret;
> +
> + ret = rtw_pci_xmit(rtwdev, pkt_info, skb, queue);
> + if (ret)
> + return ret;
> +
> + ring = &rtwpci->tx_rings[queue];
> + if (avail_desc(ring->r.wp, ring->r.rp, ring->r.len) < 2) {
> + ieee80211_stop_queue(rtwdev->hw, skb_get_queue_mapping(skb));
> + ring->queue_stopped = true;
I think here is race condition with below code that wakes queue...
> + if (ring->queue_stopped &&
> + avail_desc(ring->r.wp, ring->r.rp, ring->r.len) > 4) {
> + q_map = skb_get_queue_mapping(skb);
> + ieee80211_wake_queue(hw, q_map);
> + ring->queue_stopped = false;
... here. This should be somehow synchronized.
> + }
> +
> + info = IEEE80211_SKB_CB(skb);
> + ieee80211_tx_info_clear_status(info);
> + info->flags |= IEEE80211_TX_STAT_ACK;
> + ieee80211_tx_status_irqsafe(hw, skb);
Always report ACK ?
> +
> +static irqreturn_t rtw_pci_interrupt_handler(int irq, void *dev)
> +{
> + struct rtw_dev *rtwdev = dev;
> + struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
> + u32 irq_status[4];
> + unsigned long flags;
> +
> + spin_lock_irqsave(&rtwpci->irq_lock, flags);
flags not needed, interrupts are disabled in IRQ.
> + rtw_pci_disable_interrupt(rtwdev, rtwpci);
This seems to be not needed as well.
> +static void rtw_pci_init_aspm(struct rtw_dev *rtwdev)
> +{
> +}
Something is missing ;-)
> +static void rtw_dbi_write8(struct rtw_dev *rtwdev, u16 addr, u8 data)
> +{
> + u16 write_addr;
> + u16 remainder = addr & 0x3;
> + u8 flag;
> + u8 cnt = 20;
> +
> + write_addr = ((addr & 0x0ffc) | (BIT(0) << (remainder + 12)));
> + rtw_write8(rtwdev, REG_DBI_WDATA_V1 + remainder, data);
> + rtw_write16(rtwdev, REG_DBI_FLAG_V1, write_addr);
> + rtw_write8(rtwdev, REG_DBI_FLAG_V1 + 2, 0x01);
> +
> + flag = rtw_read8(rtwdev, REG_DBI_FLAG_V1 + 2);
> + while (flag && (cnt != 0)) {
> + udelay(10);
> + flag = rtw_read8(rtwdev, REG_DBI_FLAG_V1 + 2);
> + cnt--;
> + }
> +
> + WARN(flag, "DBI write fail");
We always print WARN, there is other return point in this function.
> +static void rtw_mdio_write(struct rtw_dev *rtwdev, u8 addr, u16 data, bool g1)
<snip>
> + WARN(wflag, "MDIO write fail");
The same.
> +struct rtw_hci_ops rtw_pci_ops = {
<snip>
> + .check_avail_desc = rtw_pci_check_avail_desc,
Not used ?
Thanks
Stanislaw
