linux-kernel - 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: <0bf45c1a-96ec-4a9d-9c41-fcb3d366d6a3@lunn.ch>
Date: Thu, 3 Jul 2025 18:25:21 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Dong Yibo <dong100@...se.com>
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

> --- 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.

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

> +#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.

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

Why do you need this as a string?

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

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

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

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