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

[Yibo Dong]

On Thu, Jul 03, 2025 at 06:25:21PM +0200, Andrew Lunn wrote:
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -16001,11 +16001,7 @@ F:	tools/testing/vma/
> >  
> >  MEMORY MAPPING - LOCKING
> >  M:	Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> > -M:	Suren Baghdasaryan <surenb@xxxxxxxxxx>
> > -M:	Liam R. Howlett <Liam.Howlett@xxxxxxxxxx>
> > -M:	Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>
> > -R:	Vlastimil Babka <vbabka@xxxxxxx>
> > -R:	Shakeel Butt <shakeel.butt@xxxxxxxxx>
> > +M:	Suren Baghdasaryan <surenb@xxxxxxxxxx> M:	Liam R. Howlett <Liam.Howlett@xxxxxxxxxx> M:	Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx> R:	Vlastimil Babka <vbabka@xxxxxxx> R:	Shakeel Butt <shakeel.butt@xxxxxxxxx>
> 
> 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.
> > +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.
> > +#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.
> > +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'.
> > +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'.
> > +	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.
> > +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.
> > +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.