On Mon, Jul 21, 2025 at 04:55:02PM +0200, Andrew Lunn wrote:
> > +++ b/Documentation/networking/device_drivers/ethernet/index.rst
> > @@ -61,6 +61,7 @@ Contents:
> > wangxun/txgbevf
> > wangxun/ngbe
> > wangxun/ngbevf
> > + mucse/rnpgbe
>
> This list is sorted. Please keep with the order.
>
> Sorting happens all other the kernel. Please keep an eye out of it,
> and ensure you insert into the correct location.
>
Got it, I will fix this.
> > +++ b/drivers/net/ethernet/Kconfig
> > @@ -202,5 +202,6 @@ source "drivers/net/ethernet/wangxun/Kconfig"
> > source "drivers/net/ethernet/wiznet/Kconfig"
> > source "drivers/net/ethernet/xilinx/Kconfig"
> > source "drivers/net/ethernet/xircom/Kconfig"
> > +source "drivers/net/ethernet/mucse/Kconfig"
>
> Another sorted list.
>
Got it.
> > +#include <linux/types.h>
> > +#include <linux/module.h>
> > +#include <linux/pci.h>
> > +#include <linux/netdevice.h>
> > +#include <linux/string.h>
> > +#include <linux/etherdevice.h>
>
> It is also reasonably normal to sort includes.
>
Got it, I will also check all other files. But what rules should be
followed? General to specific?
> > +static int rnpgbe_add_adapter(struct pci_dev *pdev)
> > +{
> > + struct mucse *mucse = NULL;
> > + struct net_device *netdev;
> > + static int bd_number;
> > +
> > + netdev = alloc_etherdev_mq(sizeof(struct mucse), 1);
>
> If you only have one queue, you might as well use alloc_etherdev().
>
Ok, I got it.
> > + if (!netdev)
> > + return -ENOMEM;
> > +
> > + mucse = netdev_priv(netdev);
> > + mucse->netdev = netdev;
> > + mucse->pdev = pdev;
> > + mucse->bd_number = bd_number++;
> > + snprintf(mucse->name, sizeof(netdev->name), "%s%d",
> > + rnpgbe_driver_name, mucse->bd_number);
>
> That looks wrong. The point of the n in snprintf is to stop you
> overwriting the end of the destination buffer. Hence you should be
> passing the length of the destination buffer, not the source buffer.
>
> I've not looked at how mucse->name is used, but why do you need yet
> another name for the device? There is pdev->dev->name, and soon there
> will be netdev->name. Having yet another name just makes it confusing.
>
> Andrew
>
Yes, 'sizeof(netdev->name)' is wrong. Actually, mucse->name is not used,
I should remove it.
thanks for your feedback.
