Message-ID: <B4DFD08F6CE2FA99+20250917013755.GA33236@nic-Precision-5820-Tower>
Date: Wed, 17 Sep 2025 09:37:55 +0800
From: Yibo Dong <dong100@...se.com>
To: Jörg Sommer <joerg@...so.de>
Cc: andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, horms@...nel.org,
	corbet@....net, 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, richardcochran@...il.com, kees@...nel.org,
	gustavoars@...nel.org, rdunlap@...radead.org,
	vadim.fedorenko@...ux.dev, netdev@...r.kernel.org,
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-hardening@...r.kernel.org, Andrew Lunn <andrew@...n.ch>
Subject: Re: [PATCH net-next v12 1/5] net: rnpgbe: Add build support for
 rnpgbe

On Tue, Sep 16, 2025 at 09:00:23PM +0200, Jörg Sommer wrote:
> Dong Yibo schrieb am Di 16. Sep, 19:29 (+0800):
> > diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> > new file mode 100644
> > index 000000000000..60bbc806f17b
> > --- /dev/null
> > +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> > @@ -0,0 +1,124 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright(c) 2020 - 2025 Mucse Corporation. */
> > +
> > +#include <linux/pci.h>
> > +
> > +#include "rnpgbe.h"
> > +
> > +static const char rnpgbe_driver_name[] = "rnpgbe";
> > +
> > +/* rnpgbe_pci_tbl - PCI Device ID Table
> > + *
> > + * { PCI_DEVICE(Vendor ID, Device ID),
> > + *   driver_data (used for different hw chip) }
> > + */
> > +static struct pci_device_id rnpgbe_pci_tbl[] = {
> > +	{ PCI_DEVICE(PCI_VENDOR_ID_MUCSE, PCI_DEVICE_ID_N500_QUAD_PORT),
> > +	  .driver_data = board_n500},
> > +	{ PCI_DEVICE(PCI_VENDOR_ID_MUCSE, PCI_DEVICE_ID_N500_DUAL_PORT),
> > +	  .driver_data = board_n500},
> > +	{ PCI_DEVICE(PCI_VENDOR_ID_MUCSE, PCI_DEVICE_ID_N210),
> > +	  .driver_data = board_n210},
> > +	{ PCI_DEVICE(PCI_VENDOR_ID_MUCSE, PCI_DEVICE_ID_N210L),
> > +	  .driver_data = board_n210},
> 
> Should there be a space before }?
> 

ixgbe_main.c has sapce before }, but no sapce after {.
ngbe_mainc. no sapce before }, but space after {.
mlx5/core/main.c has space both.
It seems not the same....
I will add sapce before }.

> > +	/* required last entry */
> > +	{0, },
> > +};
> > +
> > +/**
> > + * rnpgbe_probe - Device initialization routine
> > + * @pdev: PCI device information struct
> > + * @id: entry in rnpgbe_pci_tbl
> > + *
> > + * rnpgbe_probe initializes a PF adapter identified by a pci_dev
> > + * structure.
> > + *
> > + * Return: 0 on success, negative errno on failure
> > + **/
> > +static int rnpgbe_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > +{
> > +	int err;
> 
> In rnpgbe_mbx.c you use `int ret` for this pattern. I think you should unify
> this. But I'm more in favour of `err` than `ret`.
> 

I see, I will use err in rnpgbe_mbx.c

> > +
> > +	err = pci_enable_device_mem(pdev);
> > +	if (err)
> > +		return err;
> > +
> > +	err = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(56));
> > +	if (err) {
> > +		dev_err(&pdev->dev,
> > +			"No usable DMA configuration, aborting %d\n", err);
> > +		goto err_disable_dev;
> > +	}
> > +
> > +	err = pci_request_mem_regions(pdev, rnpgbe_driver_name);
> > +	if (err) {
> > +		dev_err(&pdev->dev,
> > +			"pci_request_selected_regions failed %d\n", err);
> > +		goto err_disable_dev;
> > +	}
> > +
> > +	pci_set_master(pdev);
> > +	err = pci_save_state(pdev);
> > +	if (err) {
> > +		dev_err(&pdev->dev, "pci_save_state failed %d\n", err);
> > +		goto err_free_regions;
> > +	}
> > +
> > +	return 0;
> > +err_free_regions:
> > +	pci_release_mem_regions(pdev);
> > +err_disable_dev:
> > +	pci_disable_device(pdev);
> > +	return err;
> > +}
> > +
> > +/**
> > + * rnpgbe_remove - Device removal routine
> > + * @pdev: PCI device information struct
> > + *
> > + * rnpgbe_remove is called by the PCI subsystem to alert the driver
> > + * that it should release a PCI device. This could be caused by a
> > + * Hot-Plug event, or because the driver is going to be removed from
> > + * memory.
> > + **/
> > +static void rnpgbe_remove(struct pci_dev *pdev)
> > +{
> > +	pci_release_mem_regions(pdev);
> > +	pci_disable_device(pdev);
> > +}
> > +
> > +/**
> > + * rnpgbe_dev_shutdown - Device shutdown routine
> > + * @pdev: PCI device information struct
> > + **/
> > +static void rnpgbe_dev_shutdown(struct pci_dev *pdev)
> > +{
> > +	pci_disable_device(pdev);
> > +}
> > +
> > +/**
> > + * rnpgbe_shutdown - Device shutdown routine
> > + * @pdev: PCI device information struct
> > + *
> > + * rnpgbe_shutdown is called by the PCI subsystem to alert the driver
> > + * that os shutdown. Device should setup wakeup state here.
> > + **/
> > +static void rnpgbe_shutdown(struct pci_dev *pdev)
> > +{
> > +	rnpgbe_dev_shutdown(pdev);
> 
> Is this the only user of rnpgbe_dev_shutdown?
> 

No, it maybe called by suspend callback in the future.
Device relative code will be added in rnpgbe_dev_shutdown, and power state
code in rnpgbe_shutdown. This is the same like other driver did
(ixgbe_main.c)

> > +}
> > +
> > +static struct pci_driver rnpgbe_driver = {
> > +	.name = rnpgbe_driver_name,
> > +	.id_table = rnpgbe_pci_tbl,
> > +	.probe = rnpgbe_probe,
> > +	.remove = rnpgbe_remove,
> > +	.shutdown = rnpgbe_shutdown,
> > +};
> > +
> > +module_pci_driver(rnpgbe_driver);
> > +
> > +MODULE_DEVICE_TABLE(pci, rnpgbe_pci_tbl);
> > +MODULE_AUTHOR("Mucse Corporation, <techsupport@...se.com>");
> > +MODULE_DESCRIPTION("Mucse(R) 1 Gigabit PCI Express Network Driver");
> > +MODULE_LICENSE("GPL");
> > -- 
> > 2.25.1
> > 
> > 
> 
> -- 
> Als deutscher Tourist im Ausland steht man vor der Frage, ob man sich
> anständig benehmen muss oder ob schon deutsche Touristen dagewesen sind.
>                                                 (Kurt Tucholsky)

Thanks for your feedback.

Powered by blists - more mailing lists