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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CD062DF313EAF6E1+20250704021036.GA26457@nic-Precision-5820-Tower>
Date: Fri, 4 Jul 2025 10:10:36 +0800
From: Yibo Dong <dong100@...se.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, horms@...nel.org, corbet@....net,
	andrew+netdev@...n.ch, gur.stavi@...wei.com, maddy@...ux.ibm.com,
	mpe@...erman.id.au, danishanwar@...com, lee@...ger.us,
	gongfan1@...wei.com, lorenzo@...nel.org, geert+renesas@...der.be,
	Parthiban.Veerasooran@...rochip.com, lukas.bulwahn@...hat.com,
	alexanderduyck@...com, netdev@...r.kernel.org,
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 01/15] net: rnpgbe: Add build support for rnpgbe

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@...ux-foundation.org>
> > -M:	Suren Baghdasaryan <surenb@...gle.com>
> > -M:	Liam R. Howlett <Liam.Howlett@...cle.com>
> > -M:	Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
> > -R:	Vlastimil Babka <vbabka@...e.cz>
> > -R:	Shakeel Butt <shakeel.butt@...ux.dev>
> > +M:	Suren Baghdasaryan <surenb@...gle.com> M:	Liam R. Howlett <Liam.Howlett@...cle.com> M:	Lorenzo Stoakes <lorenzo.stoakes@...cle.com> R:	Vlastimil Babka <vbabka@...e.cz> R:	Shakeel Butt <shakeel.butt@...ux.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.
> > +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.