On Thu, 30 Nov 2023 19:43:15 +0800 Justin Lai wrote:

quoted

+#include <linux/crc32.h>
+#include <linux/dma-mapping.h>
+#include <linux/etherdevice.h>
+#include <linux/if_vlan.h>
+#include <linux/in.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/ip.h>
+#include <linux/ipv6.h>
+#include <linux/mdio.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>

I don't see module params, please trim the includes.

Thanks, I will remove it.

quoted

+#include <linux/netdevice.h>
+#include <linux/pci.h>
+#include <linux/pm_runtime.h>
+#include <linux/prefetch.h>
+#include <linux/rtnetlink.h>
+#include <linux/tcp.h>
+#include <asm/irq.h>
+#include <net/ip6_checksum.h>
+#include <net/page_pool/helpers.h>
+#include <net/pkt_cls.h>

quoted

+static void rtase_get_mac_address(struct net_device *dev) {
+     struct rtase_private *tp = netdev_priv(dev);
+     u8 mac_addr[ETH_ALEN] __aligned(2) = {};
+     u32 i;
+
+     for (i = 0; i < ETH_ALEN; i++)
+             mac_addr[i] = rtase_r8(tp, RTASE_MAC0 + i);
+
+     if (!is_valid_ether_addr(mac_addr)) {
+             eth_random_addr(mac_addr);
+             dev->addr_assign_type = NET_ADDR_RANDOM;

eth_hw_addr_random()

I will use this function instead for this part.

quoted

+             netdev_warn(dev, "Random ether addr %pM\n",

mac_addr);

quoted

+     }
+
+     eth_hw_addr_set(dev, mac_addr);
+     rtase_rar_set(tp, mac_addr);
+
+     ether_addr_copy(dev->perm_addr, dev->dev_addr);

Should it be perm if it's random?

No, I will modify this part.

quoted

+}
+
+static void rtase_reset_interrupt(struct pci_dev *pdev,
+                               const struct rtase_private *tp) {
+     if (tp->sw_flag & SWF_MSIX_ENABLED)
+             pci_disable_msix(pdev);
+     else
+             pci_disable_msi(pdev);
+}
+
+static int rtase_alloc_msix(struct pci_dev *pdev, struct
+rtase_private *tp) {
+     int ret;
+     u16 i;
+
+     memset(tp->msix_entry, 0x0, RTASE_NUM_MSIX * sizeof(struct
+ msix_entry));
+
+     for (i = 0; i < RTASE_NUM_MSIX; i++)
+             tp->msix_entry[i].entry = i;
+
+     ret = pci_enable_msix_range(pdev, tp->msix_entry, tp->int_nums,
+                                 tp->int_nums);

pci_enable_msix_exact()

Thanks, I will use this function instead.

quoted

+     if (ret == tp->int_nums) {
+             for (i = 0; i < tp->int_nums; i++) {
+                     tp->int_vector[i].irq = pci_irq_vector(pdev, i);
+                     tp->int_vector[i].status = 1;
+             }
+     }
+
+     return ret;
+}

quoted

+     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)))

quoted

+             goto err_out_free_res;
+     else
+             dev_info(&pdev->dev, "DMA_BIT_MASK: 32\n");

This dance is unnecessary, see https://lkml.org/lkml/2021/6/7/398

Thank you for providing this information, I will remove the 32-bit part.

quoted

+     dev->tstats = netdev_alloc_pcpu_stats(struct pcpu_sw_netstats);
+     if (!dev->tstats)
+             goto err_out_1;

Please use dev->pcpu_stat_type
Set it before register and core will allocate stats for you.

Thanks for your suggestion, I will allocate stats by setting dev->pcpu_stat_type.

quoted

+     ret = register_netdev(dev);
+     if (ret != 0)
+             goto err_out;
+
+     netdev_dbg(dev, "%pM, IRQ %d\n", dev->dev_addr, dev->irq);
+
+     netif_carrier_off(dev);

Should be before register_netdev().

 
Ok, I will do that.

quoted

+     goto out;

Just return 0...

I will modify it.

quoted

+static void rtase_remove_one(struct pci_dev *pdev) {
+     struct net_device *dev = pci_get_drvdata(pdev);
+     struct rtase_private *tp = netdev_priv(dev);
+     struct rtase_int_vector *ivec;
+     u32 i;
+
+     for (i = 0; i < tp->int_nums; i++) {
+             ivec = &tp->int_vector[i];
+             netif_napi_del(&ivec->napi);

NAPI instances should be added on ndo_open()

Do you want me to call netif_napi_add() in the .ndo_open function, and netif_napi_del() in the .ndo_stop function?
However, I saw that many drivers do it in probe and remove. What is the purpose of doing this in .ndo_open and .ndo_stop?