Re: [PATCH 01/15] net: rnpgbe: Add build support for rnpgbe

On Thu, Jul 03, 2025 at 06:25:21PM +0200, Andrew Lunn wrote:

quoted

--- a/MAINTAINERS
+++ b/MAINTAINERS

@@ -16001,11 +16001,7 @@ F:	tools/testing/vma/
 
 MEMORY MAPPING - LOCKING
 M:	Andrew Morton <akpm@linux-foundation.org>
-M:	Suren Baghdasaryan <surenb@google.com>
-M:	Liam R. Howlett <Liam.Howlett@oracle.com>
-M:	Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
-R:	Vlastimil Babka <vbabka@suse.cz>
-R:	Shakeel Butt <shakeel.butt@linux.dev>
+M:	Suren Baghdasaryan <surenb@google.com> M:	Liam R. Howlett <Liam.Howlett@oracle.com> M:	Lorenzo Stoakes <lorenzo.stoakes@oracle.com> R:	Vlastimil Babka <vbabka@suse.cz> R:	Shakeel Butt <shakeel.butt@linux.dev>

You clearly have not reviewed your own patch, or you would not be
changing this section of the MAINTAINERs file.

Sorry, I didn't review it carefully. I will correct this error and
review all the remaining patches.

quoted

+if NET_VENDOR_MUCSE
+
+config MGBE
+	tristate "Mucse(R) 1GbE PCI Express adapters support"
+        depends on PCI
+	select PAGE_POOL
+        help
+          This driver supports Mucse(R) 1GbE PCI Express family of
+          adapters.
+
+	  More specific information on configuring the driver is in
+	  <file:Documentation/networking/device_drivers/ethernet/mucse/rnpgbe.rst>.
+
+          To compile this driver as a module, choose M here. The module
+          will be called rnpgbe.

There is some odd indentation here.

I will correct this in v1.

quoted

+#include <linux/string.h>
+#include <linux/etherdevice.h>
+
+#include "rnpgbe.h"
+
+char rnpgbe_driver_name[] = "rnpgbe";
+static const char rnpgbe_driver_string[] =
+	"mucse 1 Gigabit PCI Express Network Driver";
+#define DRV_VERSION "1.0.0"
+const char rnpgbe_driver_version[] = DRV_VERSION;

Driver versions are pointless, since they never change, yet the kernel
around the driver changes all the time. Please drop.

OK, I got it.

quoted

+static const char rnpgbe_copyright[] =
+	"Copyright (c) 2020-2025 mucse Corporation.";

Why do you need this as a string?

I printed this in 'pr_info' before. Of course, I should remove this belong
with 'pr_info'.

quoted

+static int rnpgbe_add_adpater(struct pci_dev *pdev)
+{
+	struct mucse *mucse = NULL;
+	struct net_device *netdev;
+	static int bd_number;
+
+	pr_info("====  add rnpgbe queues:%d ====", RNPGBE_MAX_QUEUES);

If you are still debugging this driver, please wait until it is mostly
bug free before submitting. I would not expect a production quality
driver to have prints like this.

Got it, I will remove 'pr_info'.

quoted

+	netdev = alloc_etherdev_mq(sizeof(struct mucse), RNPGBE_MAX_QUEUES);
+	if (!netdev)
+		return -ENOMEM;
+
+	mucse = netdev_priv(netdev);
+	memset((char *)mucse, 0x00, sizeof(struct mucse));

priv is guaranteed to be zero'ed.

I will remove 'memset' here.

quoted

+static void rnpgbe_shutdown(struct pci_dev *pdev)
+{
+	bool wake = false;
+
+	__rnpgbe_shutdown(pdev, &wake);

Please avoid using __ function names. Those are supposed to be
reserved for the compiler. Sometimes you will see single _ for
functions which have an unlocked version and a locked version.

Got it, I will fix this.

quoted

+static int __init rnpgbe_init_module(void)
+{
+	int ret;
+
+	pr_info("%s - version %s\n", rnpgbe_driver_string,
+		rnpgbe_driver_version);
+	pr_info("%s\n", rnpgbe_copyright);

Please don't spam the log. Only print something on error.

	Andrew

I will remove the log, thanks for your feedback.