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.
