Message-ID: <edbc001d331944f69017af67f71990ef@realtek.com>
Date: Thu, 14 Sep 2023 14:42:23 +0000
From: Justin Lai <justinlai0215@...ltek.com>
To: Andrew Lunn <andrew@...n.ch>
CC: "kuba@...nel.org" <kuba@...nel.org>,
        "davem@...emloft.net"
	<davem@...emloft.net>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>,
        "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>
Subject: RE: [PATCH net-next v7 01/13] net:ethernet:realtek:rtase: Add pci table supported in this module


> > +/* the table of time unit
> > + * 4b'0000: 1.024us,    4b'0001: 2.048us,    4b'0010: 4.096us,
> > + * 4b'0011: 8.192us,    4b'0100: 16.384us,   4b'0101: 32.768us,
> > + * 4b'0110: 65.536us,   4b'0111: 131.072us,  4b'1000: 252.144us,
> > + * 4b'1001: 524.288us,  4b'1010: 1048.576us, 4b'1011: 2097.152us,
> > + * 4b'1100: 4194.304us, 4b'1101: 8388.608us, 4b'1110: 16777.216us,
> > + * 4b'1111: 33554.432us
> 
> This seems to be all comment. Where is the table?
> 
> > + *
> > + * the table of packet number unit
> > + * 2b'00: 1,
> > + * 2b'01: 2,
> > + * 2b'10: 4,
> > + * 2b'11: 16
> 
> Again, what use is this?
> 
> > + *
> > + * interrupt mitigation = count * unit
> > + * example: If want to set packet number mitigation be 64
> > + *          the number unit is set 3,
> > + *          and the number count is set 4
> > + *          If want to set time be 131.072us
> > + *          the time unit is set 4,
> > + *          and the time count is set 8
> > + */
> 
> Rather than a comment, how about a little function which does the calculation.
> Code can be just as good at explaining something as English text.

Hi, Andrew

This calculation is a bit complicated. We will rewrite this part as a fixed-unit calculation mitigation function in the next version. The time unit will be set to 16u, and the packet number unit will be set to 16.

> 
> > +     if (!dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)))
> > +             dev->features |= NETIF_F_HIGHDMA;
> > +     else if (dma_set_mask_and_coherent(&pdev->dev,
> DMA_BIT_MASK(32)))
> > +             goto err_out_free_res;
> > +     else
> > +             pr_info("DMA_BIT_MASK: 32\n");
> 
> dev_info(). Don't use pr_ functions if you have a struct device.
> 
> > +static int rtase_init_one(struct pci_dev *pdev,
> > +                       const struct pci_device_id *ent) {
> > +     struct net_device *dev = NULL;
> > +     void __iomem *ioaddr = NULL;
> > +     struct rtase_private *tp;
> > +     int ret;
> > +
> > +     if (!pdev->is_physfn && pdev->is_virtfn) {
> > +             pr_info("This module does not support a virtual function.");
> > +             return -EINVAL;
> > +     }
> > +
> > +     pr_info("Automotive Switch Ethernet driver loaded\n");
> 
> dev_dbg(), or nothing at all.
> 
> > +
> > +     ret = rtase_init_board(pdev, &dev, &ioaddr);
> > +     if (ret != 0)
> > +             return ret;
> > +
> > +     tp = netdev_priv(dev);
> > +     tp->mmio_addr = ioaddr;
> > +     tp->dev = dev;
> > +     tp->pdev = pdev;
> > +
> > +     /* identify chip attached to board */
> > +     if (!rtase_check_mac_version_valid(tp)) {
> > +             return dev_err_probe(&pdev->dev, -ENODEV,
> > +                                  "unknown chip version, contact
> rtase maintainers (see MAINTAINERS file)\n");
> > +     }
> > +
> > +     dev->tstats = netdev_alloc_pcpu_stats(struct pcpu_sw_netstats);
> > +     if (!dev->tstats)
> > +             goto err_out_1;
> > +
> > +     rtase_init_software_variable(tp);
> > +     rtase_init_hardware(tp);
> > +
> > +     ret = rtase_alloc_interrupt(pdev, tp);
> > +     if (ret < 0) {
> > +             pr_err("unable to alloc MSIX/MSI\n");
> > +             goto err_out_1;
> > +     }
> > +
> > +     rtase_init_netdev_ops(dev);
> > +
> > +     dev->features |= NETIF_F_HW_VLAN_CTAG_TX |
> > + NETIF_F_HW_VLAN_CTAG_RX;
> > +
> > +     dev->features |= NETIF_F_IP_CSUM;
> > +     dev->features |= NETIF_F_RXCSUM | NETIF_F_SG | NETIF_F_TSO;
> > +     dev->features |= NETIF_F_IPV6_CSUM | NETIF_F_TSO6;
> > +     dev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_TSO
> |
> > +                        NETIF_F_RXCSUM |
> NETIF_F_HW_VLAN_CTAG_TX |
> > +                        NETIF_F_HW_VLAN_CTAG_RX;
> > +     dev->hw_features |= NETIF_F_RXALL;
> > +     dev->hw_features |= NETIF_F_RXFCS;
> > +     dev->hw_features |= NETIF_F_IPV6_CSUM | NETIF_F_TSO6;
> > +     dev->vlan_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_TSO
> |
> > +                          NETIF_F_HIGHDMA;
> > +     dev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
> > +     netif_set_tso_max_size(dev, LSO_64K);
> > +     netif_set_tso_max_segs(dev, NIC_MAX_PHYS_BUF_COUNT_LSO2);
> > +
> > +     rtase_get_mac_address(dev);
> > +
> > +     tp->tally_vaddr = dma_alloc_coherent(&pdev->dev,
> > +
> sizeof(*tp->tally_vaddr),
> > +                                          &tp->tally_paddr,
> > +                                          GFP_KERNEL);
> > +     if (!tp->tally_vaddr) {
> > +             ret = -ENOMEM;
> > +             goto err_out;
> > +     }
> > +
> > +     rtase_tally_counter_clear(tp);
> > +
> > +     pci_set_drvdata(pdev, dev);
> > +
> > +     ret = register_netdev(dev);
> > +     if (ret != 0)
> > +             goto err_out;
> > +
> > +     netdev_info(dev, "%pM, IRQ %d\n", dev->dev_addr, dev->irq);
> 
> netdev_dbg(), or nothing at all.
> 
>         Andrew

Thank you for your suggestion, I will correct it.

Powered by blists - more mailing lists