[PATCH net-next v7 0/5] Add driver for 1Gbe network chips from MUCSE

linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH net-next v7 0/5] Add driver for 1Gbe network chips from MUCSE
@ 2025-08-22  2:34 Dong Yibo
  2025-08-22  2:34 ` [PATCH net-next v7 1/5] net: rnpgbe: Add build support for rnpgbe Dong Yibo
                   ` (4 more replies)
  0 siblings, 5 replies; 34+ messages in thread
From: Dong Yibo @ 2025-08-22  2:34 UTC (permalink / raw)
  To: andrew+netdev, davem, edumazet, kuba, pabeni, horms, corbet,
	gur.stavi, maddy, mpe, danishanwar, lee, gongfan1, lorenzo,
	geert+renesas, Parthiban.Veerasooran, lukas.bulwahn,
	alexanderduyck, richardcochran, kees, gustavoars
  Cc: netdev, linux-doc, linux-kernel, linux-hardening, dong100

Hi maintainers,

This patch series is v7 to introduce support for MUCSE N500/N210 1Gbps
Ethernet controllers. I divide codes into multiple series, this is the
first one which only register netdev without true tx/rx functions.

Changelog:
v6 -> v7:
  [patch 1/5]:
  1. Use module_pci_driver instead 'module_init' and 'module_exit'.
  [patch 2/5]:
  1. Remove total_queue_pair_cnts in struct rnpgbe_info.
  2. Remove no-used functions in this patch series.
  [patch 3/5]:
  1. Move 'MBX_FW2PF_COUNTER' to 'mucse_mbx_get_ack'.
  2. Call 'mucse_write_mbx_pf' directly.
  [patch 4/5]:
  1. Add comment for 'wait_event_timeout'.
  [patch 5/5]:
  1. Rewrite function 'rnpgbe_get_permanent_mac'.

links:
v6: https://lore.kernel.org/netdev/20250820092154.1643120-1-dong100@mucse.com/
v5: https://lore.kernel.org/netdev/20250818112856.1446278-1-dong100@mucse.com/
v4: https://lore.kernel.org/netdev/20250814073855.1060601-1-dong100@mucse.com/
v3: https://lore.kernel.org/netdev/20250812093937.882045-1-dong100@mucse.com/
v2: https://lore.kernel.org/netdev/20250721113238.18615-1-dong100@mucse.com/
v1: https://lore.kernel.org/netdev/20250703014859.210110-1-dong100@mucse.com/

Dong Yibo (5):
  net: rnpgbe: Add build support for rnpgbe
  net: rnpgbe: Add n500/n210 chip support
  net: rnpgbe: Add basic mbx ops support
  net: rnpgbe: Add basic mbx_fw support
  net: rnpgbe: Add register_netdev

 .../device_drivers/ethernet/index.rst         |   1 +
 .../device_drivers/ethernet/mucse/rnpgbe.rst  |  21 +
 MAINTAINERS                                   |   8 +
 drivers/net/ethernet/Kconfig                  |   1 +
 drivers/net/ethernet/Makefile                 |   1 +
 drivers/net/ethernet/mucse/Kconfig            |  34 ++
 drivers/net/ethernet/mucse/Makefile           |   7 +
 drivers/net/ethernet/mucse/rnpgbe/Makefile    |  11 +
 drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h    |  99 +++++
 .../net/ethernet/mucse/rnpgbe/rnpgbe_chip.c   | 153 +++++++
 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h |  18 +
 .../net/ethernet/mucse/rnpgbe/rnpgbe_main.c   | 282 +++++++++++++
 .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c    | 395 ++++++++++++++++++
 .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h    |  25 ++
 .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c | 333 +++++++++++++++
 .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h | 152 +++++++
 16 files changed, 1541 insertions(+)
 create mode 100644 Documentation/networking/device_drivers/ethernet/mucse/rnpgbe.rst
 create mode 100644 drivers/net/ethernet/mucse/Kconfig
 create mode 100644 drivers/net/ethernet/mucse/Makefile
 create mode 100644 drivers/net/ethernet/mucse/rnpgbe/Makefile
 create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
 create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
 create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
 create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
 create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
 create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h
 create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
 create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h

-- 
2.25.1


^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH net-next v7 1/5] net: rnpgbe: Add build support for rnpgbe
  2025-08-22  2:34 [PATCH net-next v7 0/5] Add driver for 1Gbe network chips from MUCSE Dong Yibo
@ 2025-08-22  2:34 ` Dong Yibo
  2025-08-22  4:32   ` Parthiban.Veerasooran
  2025-08-22  2:34 ` [PATCH net-next v7 2/5] net: rnpgbe: Add n500/n210 chip support Dong Yibo
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 34+ messages in thread
From: Dong Yibo @ 2025-08-22  2:34 UTC (permalink / raw)
  To: andrew+netdev, davem, edumazet, kuba, pabeni, horms, corbet,
	gur.stavi, maddy, mpe, danishanwar, lee, gongfan1, lorenzo,
	geert+renesas, Parthiban.Veerasooran, lukas.bulwahn,
	alexanderduyck, richardcochran, kees, gustavoars
  Cc: netdev, linux-doc, linux-kernel, linux-hardening, dong100

Add build options and doc for mucse.
Initialize pci device access for MUCSE devices.

Signed-off-by: Dong Yibo <dong100@mucse.com>
---
 .../device_drivers/ethernet/index.rst         |   1 +
 .../device_drivers/ethernet/mucse/rnpgbe.rst  |  21 +++
 MAINTAINERS                                   |   8 ++
 drivers/net/ethernet/Kconfig                  |   1 +
 drivers/net/ethernet/Makefile                 |   1 +
 drivers/net/ethernet/mucse/Kconfig            |  34 +++++
 drivers/net/ethernet/mucse/Makefile           |   7 +
 drivers/net/ethernet/mucse/rnpgbe/Makefile    |   8 ++
 drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h    |  24 ++++
 .../net/ethernet/mucse/rnpgbe/rnpgbe_main.c   | 120 ++++++++++++++++++
 10 files changed, 225 insertions(+)
 create mode 100644 Documentation/networking/device_drivers/ethernet/mucse/rnpgbe.rst
 create mode 100644 drivers/net/ethernet/mucse/Kconfig
 create mode 100644 drivers/net/ethernet/mucse/Makefile
 create mode 100644 drivers/net/ethernet/mucse/rnpgbe/Makefile
 create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
 create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c

diff --git a/Documentation/networking/device_drivers/ethernet/index.rst b/Documentation/networking/device_drivers/ethernet/index.rst
index 0b0a3eef6aae..41ff2152b7aa 100644
--- a/Documentation/networking/device_drivers/ethernet/index.rst
+++ b/Documentation/networking/device_drivers/ethernet/index.rst
@@ -47,6 +47,7 @@ Contents:
    mellanox/mlx5/index
    meta/fbnic
    microsoft/netvsc
+   mucse/rnpgbe
    neterion/s2io
    netronome/nfp
    pensando/ionic
diff --git a/Documentation/networking/device_drivers/ethernet/mucse/rnpgbe.rst b/Documentation/networking/device_drivers/ethernet/mucse/rnpgbe.rst
new file mode 100644
index 000000000000..7562fb6b8f61
--- /dev/null
+++ b/Documentation/networking/device_drivers/ethernet/mucse/rnpgbe.rst
@@ -0,0 +1,21 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+===========================================================
+Linux Base Driver for MUCSE(R) Gigabit PCI Express Adapters
+===========================================================
+
+MUCSE Gigabit Linux driver.
+Copyright (c) 2020 - 2025 MUCSE Co.,Ltd.
+
+Identifying Your Adapter
+========================
+The driver is compatible with devices based on the following:
+
+ * MUCSE(R) Ethernet Controller N500 series
+ * MUCSE(R) Ethernet Controller N210 series
+
+Support
+=======
+ If you have problems with the software or hardware, please contact our
+ customer support team via email at techsupport@mucse.com or check our
+ website at https://www.mucse.com/en/
diff --git a/MAINTAINERS b/MAINTAINERS
index bce96dd254b8..00b73e3631b4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17276,6 +17276,14 @@ T:	git git://linuxtv.org/media.git
 F:	Documentation/devicetree/bindings/media/i2c/aptina,mt9v111.yaml
 F:	drivers/media/i2c/mt9v111.c
 
+MUCSE ETHERNET DRIVER
+M:	Yibo Dong <dong100@mucse.com>
+L:	netdev@vger.kernel.org
+S:	Maintained
+W:	https://www.mucse.com/en/
+F:	Documentation/networking/device_drivers/ethernet/mucse/
+F:	drivers/net/ethernet/mucse/
+
 MULTIFUNCTION DEVICES (MFD)
 M:	Lee Jones <lee@kernel.org>
 S:	Maintained
diff --git a/drivers/net/ethernet/Kconfig b/drivers/net/ethernet/Kconfig
index f86d4557d8d7..167388f9c744 100644
--- a/drivers/net/ethernet/Kconfig
+++ b/drivers/net/ethernet/Kconfig
@@ -129,6 +129,7 @@ source "drivers/net/ethernet/microchip/Kconfig"
 source "drivers/net/ethernet/mscc/Kconfig"
 source "drivers/net/ethernet/microsoft/Kconfig"
 source "drivers/net/ethernet/moxa/Kconfig"
+source "drivers/net/ethernet/mucse/Kconfig"
 source "drivers/net/ethernet/myricom/Kconfig"
 
 config FEALNX
diff --git a/drivers/net/ethernet/Makefile b/drivers/net/ethernet/Makefile
index 67182339469a..1b8c4df3f594 100644
--- a/drivers/net/ethernet/Makefile
+++ b/drivers/net/ethernet/Makefile
@@ -65,6 +65,7 @@ obj-$(CONFIG_NET_VENDOR_MICREL) += micrel/
 obj-$(CONFIG_NET_VENDOR_MICROCHIP) += microchip/
 obj-$(CONFIG_NET_VENDOR_MICROSEMI) += mscc/
 obj-$(CONFIG_NET_VENDOR_MOXART) += moxa/
+obj-$(CONFIG_NET_VENDOR_MUCSE) += mucse/
 obj-$(CONFIG_NET_VENDOR_MYRI) += myricom/
 obj-$(CONFIG_FEALNX) += fealnx.o
 obj-$(CONFIG_NET_VENDOR_NATSEMI) += natsemi/
diff --git a/drivers/net/ethernet/mucse/Kconfig b/drivers/net/ethernet/mucse/Kconfig
new file mode 100644
index 000000000000..be0fdf268484
--- /dev/null
+++ b/drivers/net/ethernet/mucse/Kconfig
@@ -0,0 +1,34 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Mucse network device configuration
+#
+
+config NET_VENDOR_MUCSE
+	bool "Mucse devices"
+	default y
+	help
+	  If you have a network (Ethernet) card from Mucse(R), say Y.
+
+	  Note that the answer to this question doesn't directly affect the
+	  kernel: saying N will just cause the configurator to skip all
+	  the questions about Mucse(R) cards. If you say Y, you will
+	  be asked for your specific card in the following questions.
+
+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.
+
+endif # NET_VENDOR_MUCSE
+
diff --git a/drivers/net/ethernet/mucse/Makefile b/drivers/net/ethernet/mucse/Makefile
new file mode 100644
index 000000000000..675173fa05f7
--- /dev/null
+++ b/drivers/net/ethernet/mucse/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+# Copyright(c) 2020 - 2025 MUCSE Corporation.
+#
+# Makefile for the MUCSE(R) network device drivers
+#
+
+obj-$(CONFIG_MGBE) += rnpgbe/
diff --git a/drivers/net/ethernet/mucse/rnpgbe/Makefile b/drivers/net/ethernet/mucse/rnpgbe/Makefile
new file mode 100644
index 000000000000..9df536f0d04c
--- /dev/null
+++ b/drivers/net/ethernet/mucse/rnpgbe/Makefile
@@ -0,0 +1,8 @@
+# SPDX-License-Identifier: GPL-2.0
+# Copyright(c) 2020 - 2025 MUCSE Corporation.
+#
+# Makefile for the MUCSE(R) 1GbE PCI Express ethernet driver
+#
+
+obj-$(CONFIG_MGBE) += rnpgbe.o
+rnpgbe-objs := rnpgbe_main.o
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
new file mode 100644
index 000000000000..64b2c093bc6e
--- /dev/null
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright(c) 2020 - 2025 Mucse Corporation. */
+
+#ifndef _RNPGBE_H
+#define _RNPGBE_H
+
+enum rnpgbe_boards {
+	board_n500,
+	board_n210,
+	board_n210L,
+};
+
+struct mucse {
+	struct net_device *netdev;
+	struct pci_dev *pdev;
+};
+
+/* Device IDs */
+#define PCI_VENDOR_ID_MUCSE 0x8848
+#define PCI_DEVICE_ID_N500_QUAD_PORT 0x8308
+#define PCI_DEVICE_ID_N500_DUAL_PORT 0x8318
+#define PCI_DEVICE_ID_N210 0x8208
+#define PCI_DEVICE_ID_N210L 0x820a
+#endif /* _RNPGBE_H */
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..b4a9c5c66af6
--- /dev/null
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
@@ -0,0 +1,120 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright(c) 2020 - 2025 Mucse Corporation. */
+
+#include <linux/types.h>
+#include <linux/module.h>
+#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_n210L},
+	/* 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 on failure
+ **/
+static int rnpgbe_probe(struct pci_dev *pdev, const struct pci_device_id *id)
+{
+	int err;
+
+	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_dma;
+	}
+
+	err = pci_request_mem_regions(pdev, rnpgbe_driver_name);
+	if (err) {
+		dev_err(&pdev->dev,
+			"pci_request_selected_regions failed 0x%x\n", err);
+		goto err_dma;
+	}
+
+	pci_set_master(pdev);
+	pci_save_state(pdev);
+
+	return 0;
+err_dma:
+	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);
+}
+
+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@mucse.com>");
+MODULE_DESCRIPTION("Mucse(R) 1 Gigabit PCI Express Network Driver");
+MODULE_LICENSE("GPL");
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH net-next v7 2/5] net: rnpgbe: Add n500/n210 chip support
  2025-08-22  2:34 [PATCH net-next v7 0/5] Add driver for 1Gbe network chips from MUCSE Dong Yibo
  2025-08-22  2:34 ` [PATCH net-next v7 1/5] net: rnpgbe: Add build support for rnpgbe Dong Yibo
@ 2025-08-22  2:34 ` Dong Yibo
  2025-08-22  2:34 ` [PATCH net-next v7 3/5] net: rnpgbe: Add basic mbx ops support Dong Yibo
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 34+ messages in thread
From: Dong Yibo @ 2025-08-22  2:34 UTC (permalink / raw)
  To: andrew+netdev, davem, edumazet, kuba, pabeni, horms, corbet,
	gur.stavi, maddy, mpe, danishanwar, lee, gongfan1, lorenzo,
	geert+renesas, Parthiban.Veerasooran, lukas.bulwahn,
	alexanderduyck, richardcochran, kees, gustavoars
  Cc: netdev, linux-doc, linux-kernel, linux-hardening, dong100

Initialize n500/n210 chip bar resource map and
dma, eth, mbx ... info for future use.

Signed-off-by: Dong Yibo <dong100@mucse.com>
---
 drivers/net/ethernet/mucse/rnpgbe/Makefile    |  3 +-
 drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h    | 34 ++++++++
 .../net/ethernet/mucse/rnpgbe/rnpgbe_chip.c   | 68 +++++++++++++++
 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h | 16 ++++
 .../net/ethernet/mucse/rnpgbe/rnpgbe_main.c   | 87 +++++++++++++++++++
 5 files changed, 207 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
 create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h

diff --git a/drivers/net/ethernet/mucse/rnpgbe/Makefile b/drivers/net/ethernet/mucse/rnpgbe/Makefile
index 9df536f0d04c..42c359f459d9 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/Makefile
+++ b/drivers/net/ethernet/mucse/rnpgbe/Makefile
@@ -5,4 +5,5 @@
 #
 
 obj-$(CONFIG_MGBE) += rnpgbe.o
-rnpgbe-objs := rnpgbe_main.o
+rnpgbe-objs := rnpgbe_main.o\
+	       rnpgbe_chip.o
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
index 64b2c093bc6e..9a86e67d6395 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
@@ -4,15 +4,49 @@
 #ifndef _RNPGBE_H
 #define _RNPGBE_H
 
+#include <linux/types.h>
+
+extern const struct rnpgbe_info rnpgbe_n500_info;
+extern const struct rnpgbe_info rnpgbe_n210_info;
+extern const struct rnpgbe_info rnpgbe_n210L_info;
+
 enum rnpgbe_boards {
 	board_n500,
 	board_n210,
 	board_n210L,
 };
 
+enum rnpgbe_hw_type {
+	rnpgbe_hw_n500 = 0,
+	rnpgbe_hw_n210,
+	rnpgbe_hw_n210L,
+	rnpgbe_hw_unknown
+};
+
+struct mucse_mbx_info {
+	/* fw <--> pf mbx */
+	u32 fw_pf_shm_base;
+	u32 pf2fw_mbox_ctrl;
+	u32 fw_pf_mbox_mask;
+	u32 fw2pf_mbox_vec;
+};
+
+struct mucse_hw {
+	void __iomem *hw_addr;
+	struct pci_dev *pdev;
+	enum rnpgbe_hw_type hw_type;
+	struct mucse_mbx_info mbx;
+};
+
 struct mucse {
 	struct net_device *netdev;
 	struct pci_dev *pdev;
+	struct mucse_hw hw;
+};
+
+struct rnpgbe_info {
+	enum rnpgbe_hw_type hw_type;
+	void (*init)(struct mucse_hw *hw);
 };
 
 /* Device IDs */
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
new file mode 100644
index 000000000000..179621ea09f3
--- /dev/null
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
@@ -0,0 +1,68 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright(c) 2020 - 2025 Mucse Corporation. */
+
+#include "rnpgbe.h"
+#include "rnpgbe_hw.h"
+
+/**
+ * rnpgbe_init_common - Setup common attribute
+ * @hw: hw information structure
+ **/
+static void rnpgbe_init_common(struct mucse_hw *hw)
+{
+	struct mucse_mbx_info *mbx = &hw->mbx;
+
+	mbx->pf2fw_mbox_ctrl = GBE_PF2FW_MBX_MASK_OFFSET;
+	mbx->fw_pf_mbox_mask = GBE_FWPF_MBX_MASK;
+}
+
+/**
+ * rnpgbe_init_n500 - Setup n500 hw info
+ * @hw: hw information structure
+ *
+ * rnpgbe_init_n500 initializes all private
+ * structure, such as dma, eth, mac and mbx base on
+ * hw->hw_addr for n500
+ **/
+static void rnpgbe_init_n500(struct mucse_hw *hw)
+{
+	struct mucse_mbx_info *mbx = &hw->mbx;
+
+	rnpgbe_init_common(hw);
+
+	mbx->fw2pf_mbox_vec = N500_FW2PF_MBX_VEC_OFFSET;
+	mbx->fw_pf_shm_base = N500_FWPF_SHM_BASE_OFFSET;
+}
+
+/**
+ * rnpgbe_init_n210 - Setup n210 hw info
+ * @hw: hw information structure
+ *
+ * rnpgbe_init_n210 initializes all private
+ * structure, such as dma, eth, mac and mbx base on
+ * hw->hw_addr for n210
+ **/
+static void rnpgbe_init_n210(struct mucse_hw *hw)
+{
+	struct mucse_mbx_info *mbx = &hw->mbx;
+
+	rnpgbe_init_common(hw);
+
+	mbx->fw2pf_mbox_vec = N210_FW2PF_MBX_VEC_OFFSET;
+	mbx->fw_pf_shm_base = N210_FWPF_SHM_BASE_OFFSET;
+}
+
+const struct rnpgbe_info rnpgbe_n500_info = {
+	.hw_type = rnpgbe_hw_n500,
+	.init = &rnpgbe_init_n500,
+};
+
+const struct rnpgbe_info rnpgbe_n210_info = {
+	.hw_type = rnpgbe_hw_n210,
+	.init = &rnpgbe_init_n210,
+};
+
+const struct rnpgbe_info rnpgbe_n210L_info = {
+	.hw_type = rnpgbe_hw_n210L,
+	.init = &rnpgbe_init_n210,
+};
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
new file mode 100644
index 000000000000..746dca78f1df
--- /dev/null
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright(c) 2020 - 2025 Mucse Corporation. */
+
+#ifndef _RNPGBE_HW_H
+#define _RNPGBE_HW_H
+
+/**************** MBX Resource ****************************/
+#define N500_FW2PF_MBX_VEC_OFFSET 0x28b00
+#define N500_FWPF_SHM_BASE_OFFSET 0x2d000
+#define GBE_PF2FW_MBX_MASK_OFFSET 0x5500
+#define GBE_FWPF_MBX_MASK 0x5700
+#define N210_FW2PF_MBX_VEC_OFFSET 0x29400
+#define N210_FWPF_SHM_BASE_OFFSET 0x2d900
+/**************** CHIP Resource ****************************/
+#define RNPGBE_MAX_QUEUES 8
+#endif /* _RNPGBE_HW_H */
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
index b4a9c5c66af6..6992a3c0e58a 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
@@ -4,10 +4,18 @@
 #include <linux/types.h>
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <linux/netdevice.h>
+#include <linux/etherdevice.h>
 
 #include "rnpgbe.h"
+#include "rnpgbe_hw.h"
 
 static const char rnpgbe_driver_name[] = "rnpgbe";
+static const struct rnpgbe_info *rnpgbe_info_tbl[] = {
+	[board_n500] = &rnpgbe_n500_info,
+	[board_n210] = &rnpgbe_n210_info,
+	[board_n210L] = &rnpgbe_n210L_info,
+};
 
 /* rnpgbe_pci_tbl - PCI Device ID Table
  *
@@ -27,6 +35,56 @@ static struct pci_device_id rnpgbe_pci_tbl[] = {
 	{0, },
 };
 
+/**
+ * rnpgbe_add_adapter - Add netdev for this pci_dev
+ * @pdev: PCI device information structure
+ * @info: chip info structure
+ *
+ * rnpgbe_add_adapter initializes a netdev for this pci_dev
+ * structure. Initializes Bar map, private structure, and a
+ * hardware reset occur.
+ *
+ * @return: 0 on success, negative on failure
+ **/
+static int rnpgbe_add_adapter(struct pci_dev *pdev,
+			      const struct rnpgbe_info *info)
+{
+	struct net_device *netdev;
+	void __iomem *hw_addr;
+	struct mucse *mucse;
+	struct mucse_hw *hw;
+	int err;
+
+	netdev = alloc_etherdev_mq(sizeof(struct mucse), RNPGBE_MAX_QUEUES);
+	if (!netdev)
+		return -ENOMEM;
+
+	SET_NETDEV_DEV(netdev, &pdev->dev);
+	mucse = netdev_priv(netdev);
+	mucse->netdev = netdev;
+	mucse->pdev = pdev;
+	pci_set_drvdata(pdev, mucse);
+
+	hw = &mucse->hw;
+	hw->hw_type = info->hw_type;
+	hw->pdev = pdev;
+	hw_addr = devm_ioremap(&pdev->dev,
+			       pci_resource_start(pdev, 2),
+			       pci_resource_len(pdev, 2));
+	if (!hw_addr) {
+		err = -EIO;
+		goto err_free_net;
+	}
+
+	hw->hw_addr = hw_addr;
+	info->init(hw);
+	return 0;
+
+err_free_net:
+	free_netdev(netdev);
+	return err;
+}
+
 /**
  * rnpgbe_probe - Device initialization routine
  * @pdev: PCI device information struct
@@ -39,6 +97,7 @@ static struct pci_device_id rnpgbe_pci_tbl[] = {
  **/
 static int rnpgbe_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
+	const struct rnpgbe_info *info = rnpgbe_info_tbl[id->driver_data];
 	int err;
 
 	err = pci_enable_device_mem(pdev);
@@ -61,13 +120,36 @@ static int rnpgbe_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	pci_set_master(pdev);
 	pci_save_state(pdev);
+	err = rnpgbe_add_adapter(pdev, info);
+	if (err)
+		goto err_regions;
 
 	return 0;
+err_regions:
+	pci_release_mem_regions(pdev);
 err_dma:
 	pci_disable_device(pdev);
 	return err;
 }
 
+/**
+ * rnpgbe_rm_adapter - Remove netdev for this mucse structure
+ * @pdev: PCI device information struct
+ *
+ * rnpgbe_rm_adapter remove a netdev for this mucse structure
+ **/
+static void rnpgbe_rm_adapter(struct pci_dev *pdev)
+{
+	struct mucse *mucse = pci_get_drvdata(pdev);
+	struct net_device *netdev;
+
+	if (!mucse)
+		return;
+	netdev = mucse->netdev;
+	mucse->netdev = NULL;
+	free_netdev(netdev);
+}
+
 /**
  * rnpgbe_remove - Device removal routine
  * @pdev: PCI device information struct
@@ -79,6 +161,7 @@ static int rnpgbe_probe(struct pci_dev *pdev, const struct pci_device_id *id)
  **/
 static void rnpgbe_remove(struct pci_dev *pdev)
 {
+	rnpgbe_rm_adapter(pdev);
 	pci_release_mem_regions(pdev);
 	pci_disable_device(pdev);
 }
@@ -89,6 +172,10 @@ static void rnpgbe_remove(struct pci_dev *pdev)
  **/
 static void rnpgbe_dev_shutdown(struct pci_dev *pdev)
 {
+	struct mucse *mucse = pci_get_drvdata(pdev);
+	struct net_device *netdev = mucse->netdev;
+
+	netif_device_detach(netdev);
 	pci_disable_device(pdev);
 }
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH net-next v7 3/5] net: rnpgbe: Add basic mbx ops support
  2025-08-22  2:34 [PATCH net-next v7 0/5] Add driver for 1Gbe network chips from MUCSE Dong Yibo
  2025-08-22  2:34 ` [PATCH net-next v7 1/5] net: rnpgbe: Add build support for rnpgbe Dong Yibo
  2025-08-22  2:34 ` [PATCH net-next v7 2/5] net: rnpgbe: Add n500/n210 chip support Dong Yibo
@ 2025-08-22  2:34 ` Dong Yibo
  2025-08-22  4:41   ` Parthiban.Veerasooran
  2025-08-22  2:34 ` [PATCH net-next v7 4/5] net: rnpgbe: Add basic mbx_fw support Dong Yibo
  2025-08-22  2:34 ` [PATCH net-next v7 5/5] net: rnpgbe: Add register_netdev Dong Yibo
  4 siblings, 1 reply; 34+ messages in thread
From: Dong Yibo @ 2025-08-22  2:34 UTC (permalink / raw)
  To: andrew+netdev, davem, edumazet, kuba, pabeni, horms, corbet,
	gur.stavi, maddy, mpe, danishanwar, lee, gongfan1, lorenzo,
	geert+renesas, Parthiban.Veerasooran, lukas.bulwahn,
	alexanderduyck, richardcochran, kees, gustavoars
  Cc: netdev, linux-doc, linux-kernel, linux-hardening, dong100

Initialize basic mbx function.

Signed-off-by: Dong Yibo <dong100@mucse.com>
---
 drivers/net/ethernet/mucse/rnpgbe/Makefile    |   3 +-
 drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h    |  17 +
 .../net/ethernet/mucse/rnpgbe/rnpgbe_chip.c   |   3 +
 .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c    | 395 ++++++++++++++++++
 .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h    |  25 ++
 5 files changed, 442 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
 create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h

diff --git a/drivers/net/ethernet/mucse/rnpgbe/Makefile b/drivers/net/ethernet/mucse/rnpgbe/Makefile
index 42c359f459d9..5fc878ada4b1 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/Makefile
+++ b/drivers/net/ethernet/mucse/rnpgbe/Makefile
@@ -6,4 +6,5 @@
 
 obj-$(CONFIG_MGBE) += rnpgbe.o
 rnpgbe-objs := rnpgbe_main.o\
-	       rnpgbe_chip.o
+	       rnpgbe_chip.o\
+	       rnpgbe_mbx.o
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
index 9a86e67d6395..67e28a4667e7 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
@@ -5,6 +5,7 @@
 #define _RNPGBE_H
 
 #include <linux/types.h>
+#include <linux/mutex.h>
 
 extern const struct rnpgbe_info rnpgbe_n500_info;
 extern const struct rnpgbe_info rnpgbe_n210_info;
@@ -23,7 +24,23 @@ enum rnpgbe_hw_type {
 	rnpgbe_hw_unknown
 };
 
+struct mucse_mbx_stats {
+	u32 msgs_tx;
+	u32 msgs_rx;
+	u32 acks;
+	u32 reqs;
+};
+
 struct mucse_mbx_info {
+	struct mucse_mbx_stats stats;
+	u32 timeout;
+	u32 usec_delay;
+	u16 size;
+	u16 fw_req;
+	u16 fw_ack;
+	/* lock for only one use mbx */
+	struct mutex lock;
+	bool irq_enabled;
 	/* fw <--> pf mbx */
 	u32 fw_pf_shm_base;
 	u32 pf2fw_mbox_ctrl;
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
index 179621ea09f3..f38daef752a3 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
@@ -1,8 +1,11 @@
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright(c) 2020 - 2025 Mucse Corporation. */
 
+#include <linux/string.h>
+
 #include "rnpgbe.h"
 #include "rnpgbe_hw.h"
+#include "rnpgbe_mbx.h"
 
 /**
  * rnpgbe_init_common - Setup common attribute
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
new file mode 100644
index 000000000000..e02563b994c2
--- /dev/null
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
@@ -0,0 +1,395 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright(c) 2022 - 2025 Mucse Corporation. */
+
+#include <linux/pci.h>
+#include <linux/errno.h>
+#include <linux/delay.h>
+#include <linux/iopoll.h>
+
+#include "rnpgbe.h"
+#include "rnpgbe_mbx.h"
+#include "rnpgbe_hw.h"
+
+/**
+ * mbx_data_rd32  - Reads reg with base mbx->fw_pf_shm_base
+ * @mbx: pointer to the MBX structure
+ * @reg: register offset
+ *
+ * @return: register value
+ **/
+static u32 mbx_data_rd32(struct mucse_mbx_info *mbx, u32 reg)
+{
+	struct mucse_hw *hw = container_of(mbx, struct mucse_hw, mbx);
+
+	return readl(hw->hw_addr + mbx->fw_pf_shm_base + reg);
+}
+
+/**
+ * mbx_data_wr32  - Writes value to reg with base mbx->fw_pf_shm_base
+ * @mbx: pointer to the MBX structure
+ * @reg: register offset
+ * @value: value to be written
+ *
+ **/
+static void mbx_data_wr32(struct mucse_mbx_info *mbx, u32 reg, u32 value)
+{
+	struct mucse_hw *hw = container_of(mbx, struct mucse_hw, mbx);
+
+	writel(value, hw->hw_addr + mbx->fw_pf_shm_base + reg);
+}
+
+/**
+ * mbx_ctrl_rd32  - Reads reg with base mbx->fw2pf_mbox_vec
+ * @mbx: pointer to the MBX structure
+ * @reg: register offset
+ *
+ * @return: register value
+ **/
+static u32 mbx_ctrl_rd32(struct mucse_mbx_info *mbx, u32 reg)
+{
+	struct mucse_hw *hw = container_of(mbx, struct mucse_hw, mbx);
+
+	return readl(hw->hw_addr + mbx->fw2pf_mbox_vec + reg);
+}
+
+/**
+ * mbx_ctrl_wr32  - Writes value to reg with base mbx->fw2pf_mbox_vec
+ * @mbx: pointer to the MBX structure
+ * @reg: register offset
+ * @value: value to be written
+ *
+ **/
+static void mbx_ctrl_wr32(struct mucse_mbx_info *mbx, u32 reg, u32 value)
+{
+	struct mucse_hw *hw = container_of(mbx, struct mucse_hw, mbx);
+
+	writel(value, hw->hw_addr + mbx->fw2pf_mbox_vec + reg);
+}
+
+/**
+ * mucse_mbx_get_fwreq - Read fw req from reg
+ * @mbx: pointer to the mbx structure
+ *
+ * @return: the req value
+ **/
+static u16 mucse_mbx_get_fwreq(struct mucse_mbx_info *mbx)
+{
+	return mbx_data_rd32(mbx, MBX_FW2PF_COUNTER) & GENMASK_U32(15, 0);
+}
+
+/**
+ * mucse_mbx_get_fwack - Read fw ack from reg
+ * @mbx: pointer to the MBX structure
+ *
+ * @return: the ack value
+ **/
+static u16 mucse_mbx_get_fwack(struct mucse_mbx_info *mbx)
+{
+	return (mbx_data_rd32(mbx, MBX_FW2PF_COUNTER) >> 16);
+}
+
+/**
+ * mucse_mbx_inc_pf_req - Increase req
+ * @hw: pointer to the HW structure
+ *
+ * mucse_mbx_inc_pf_req read pf_req from hw, then write
+ * new value back after increase
+ **/
+static void mucse_mbx_inc_pf_req(struct mucse_hw *hw)
+{
+	struct mucse_mbx_info *mbx = &hw->mbx;
+	u16 req;
+	u32 v;
+
+	v = mbx_data_rd32(mbx, MBX_PF2FW_COUNTER);
+	req = (v & GENMASK_U32(15, 0));
+	req++;
+	v &= GENMASK_U32(31, 16);
+	v |= req;
+	mbx_data_wr32(mbx, MBX_PF2FW_COUNTER, v);
+	hw->mbx.stats.msgs_tx++;
+}
+
+/**
+ * mucse_mbx_inc_pf_ack - Increase ack
+ * @hw: pointer to the HW structure
+ *
+ * mucse_mbx_inc_pf_ack read pf_ack from hw, then write
+ * new value back after increase
+ **/
+static void mucse_mbx_inc_pf_ack(struct mucse_hw *hw)
+{
+	struct mucse_mbx_info *mbx = &hw->mbx;
+	u16 ack;
+	u32 v;
+
+	v = mbx_data_rd32(mbx, MBX_PF2FW_COUNTER);
+	ack = (v >> 16) & GENMASK_U32(15, 0);
+	ack++;
+	v &= GENMASK_U32(15, 0);
+	v |= (ack << 16);
+	mbx_data_wr32(mbx, MBX_PF2FW_COUNTER, v);
+	hw->mbx.stats.msgs_rx++;
+}
+
+/**
+ * mucse_check_for_msg_pf - Check to see if the fw has sent mail
+ * @hw: pointer to the HW structure
+ *
+ * @return: 0 if the fw has set the Status bit or else
+ * -EIO
+ **/
+static int mucse_check_for_msg_pf(struct mucse_hw *hw)
+{
+	struct mucse_mbx_info *mbx = &hw->mbx;
+	u16 hw_req_count = 0;
+
+	hw_req_count = mucse_mbx_get_fwreq(mbx);
+	/* chip's register is reset to 0 when rc send reset
+	 * mbx command. This causes 'hw_req_count != hw->mbx.fw_req'
+	 * be TRUE before fw really reply. Driver must wait fw reset
+	 * done reply before using chip, we must check no-zero.
+	 **/
+	if (hw_req_count != 0 && hw_req_count != hw->mbx.fw_req) {
+		hw->mbx.stats.reqs++;
+		return 0;
+	}
+
+	return -EIO;
+}
+
+/**
+ * mucse_poll_for_msg - Wait for message notification
+ * @hw: pointer to the HW structure
+ *
+ * @return: 0 on success, negative on failure
+ **/
+static int mucse_poll_for_msg(struct mucse_hw *hw)
+{
+	struct mucse_mbx_info *mbx = &hw->mbx;
+	int countdown = mbx->timeout;
+	int val;
+
+	return read_poll_timeout(mucse_check_for_msg_pf,
+				 val, val == 0, mbx->usec_delay,
+				 countdown * mbx->usec_delay,
+				 false, hw);
+}
+
+/**
+ * mucse_check_for_ack_pf - Check to see if the VF has ACKed
+ * @hw: pointer to the HW structure
+ *
+ * @return: 0 if the fw has set the Status bit or else
+ * -EIO
+ **/
+static int mucse_check_for_ack_pf(struct mucse_hw *hw)
+{
+	struct mucse_mbx_info *mbx = &hw->mbx;
+	u16 hw_fw_ack;
+
+	hw_fw_ack = mucse_mbx_get_fwack(mbx);
+	/* chip's register is reset to 0 when rc send reset
+	 * mbx command. This causes 'hw_fw_ack != hw->mbx.fw_ack'
+	 * be TRUE before fw really reply. Driver must wait fw reset
+	 * done reply before using chip, we must check no-zero.
+	 **/
+	if (hw_fw_ack != 0 && hw_fw_ack != hw->mbx.fw_ack) {
+		hw->mbx.stats.acks++;
+		return 0;
+	}
+
+	return -EIO;
+}
+
+/**
+ * mucse_poll_for_ack - Wait for message acknowledgment
+ * @hw: pointer to the HW structure
+ *
+ * @return: 0 if it successfully received a message acknowledgment
+ **/
+static int mucse_poll_for_ack(struct mucse_hw *hw)
+{
+	struct mucse_mbx_info *mbx = &hw->mbx;
+	int countdown = mbx->timeout;
+	int val;
+
+	return read_poll_timeout(mucse_check_for_ack_pf,
+				 val, val == 0, mbx->usec_delay,
+				 countdown * mbx->usec_delay,
+				 false, hw);
+}
+
+/**
+ * mucse_obtain_mbx_lock_pf - Obtain mailbox lock
+ * @hw: pointer to the HW structure
+ *
+ * This function maybe used in an irq handler.
+ *
+ * @return: 0 if we obtained the mailbox lock
+ **/
+static int mucse_obtain_mbx_lock_pf(struct mucse_hw *hw)
+{
+	struct mucse_mbx_info *mbx = &hw->mbx;
+	int try_cnt = 5000;
+	u32 reg;
+
+	reg = PF2FW_MBOX_CTRL(mbx);
+	while (try_cnt-- > 0) {
+		mbx_ctrl_wr32(mbx, reg, MBOX_PF_HOLD);
+		/* force write back before check */
+		wmb();
+		if (mbx_ctrl_rd32(mbx, reg) & MBOX_PF_HOLD)
+			return 0;
+		udelay(100);
+	}
+	return -EIO;
+}
+
+/**
+ * mucse_read_mbx_pf - Read a message from the mailbox
+ * @hw: pointer to the HW structure
+ * @msg: the message buffer
+ * @size: length of buffer
+ *
+ * This function copies a message from the mailbox buffer to the caller's
+ * memory buffer. The presumption is that the caller knows that there was
+ * a message due to a fw request so no polling for message is needed.
+ *
+ * @return: 0 on success, negative on failure
+ **/
+static int mucse_read_mbx_pf(struct mucse_hw *hw, u32 *msg, u16 size)
+{
+	struct mucse_mbx_info *mbx = &hw->mbx;
+	int size_inwords = size / 4;
+	u32 ctrl_reg;
+	int ret;
+	int i;
+
+	ctrl_reg = PF2FW_MBOX_CTRL(mbx);
+
+	ret = mucse_obtain_mbx_lock_pf(hw);
+	if (ret)
+		return ret;
+	for (i = 0; i < size_inwords; i++)
+		msg[i] = mbx_data_rd32(mbx, MBX_FW_PF_SHM_DATA + 4 * i);
+	/* Hw need write data_reg at last */
+	mbx_data_wr32(mbx, MBX_FW_PF_SHM_DATA, 0);
+	hw->mbx.fw_req = mucse_mbx_get_fwreq(mbx);
+	mucse_mbx_inc_pf_ack(hw);
+	mbx_ctrl_wr32(mbx, ctrl_reg, 0);
+
+	return 0;
+}
+
+/**
+ * mucse_read_posted_mbx - Wait for message notification and receive message
+ * @hw: pointer to the HW structure
+ * @msg: the message buffer
+ * @size: length of buffer
+ *
+ * @return: 0 if it successfully received a message notification and
+ * copied it into the receive buffer.
+ **/
+int mucse_read_posted_mbx(struct mucse_hw *hw, u32 *msg, u16 size)
+{
+	int ret;
+
+	ret = mucse_poll_for_msg(hw);
+	if (ret)
+		return ret;
+
+	return mucse_read_mbx_pf(hw, msg, size);
+}
+
+/**
+ * mucse_mbx_reset - Reset mbx info, sync info from regs
+ * @hw: pointer to the HW structure
+ *
+ * This function reset all mbx variables to default.
+ **/
+static void mucse_mbx_reset(struct mucse_hw *hw)
+{
+	struct mucse_mbx_info *mbx = &hw->mbx;
+	u32 v;
+
+	v = mbx_data_rd32(mbx, MBX_FW2PF_COUNTER);
+	hw->mbx.fw_req = v & GENMASK_U32(15, 0);
+	hw->mbx.fw_ack = (v >> 16) & GENMASK_U32(15, 0);
+	mbx_ctrl_wr32(mbx, PF2FW_MBOX_CTRL(mbx), 0);
+	mbx_ctrl_wr32(mbx, FW_PF_MBOX_MASK(mbx), GENMASK_U32(31, 16));
+}
+
+/**
+ * mucse_init_mbx_params_pf - Set initial values for pf mailbox
+ * @hw: pointer to the HW structure
+ *
+ * Initializes the hw->mbx struct to correct values for pf mailbox
+ */
+void mucse_init_mbx_params_pf(struct mucse_hw *hw)
+{
+	struct mucse_mbx_info *mbx = &hw->mbx;
+
+	mbx->usec_delay = 100;
+	mbx->timeout = (4 * USEC_PER_SEC) / mbx->usec_delay;
+	mbx->stats.msgs_tx = 0;
+	mbx->stats.msgs_rx = 0;
+	mbx->stats.reqs = 0;
+	mbx->stats.acks = 0;
+	mbx->size = MUCSE_MAILBOX_BYTES;
+	mutex_init(&mbx->lock);
+	mucse_mbx_reset(hw);
+}
+
+/**
+ * mucse_write_mbx_pf - Place a message in the mailbox
+ * @hw: pointer to the HW structure
+ * @msg: the message buffer
+ * @size: length of buffer
+ *
+ * This function maybe used in an irq handler.
+ *
+ * @return: 0 if it successfully copied message into the buffer
+ **/
+int mucse_write_mbx_pf(struct mucse_hw *hw, u32 *msg, u16 size)
+{
+	struct mucse_mbx_info *mbx = &hw->mbx;
+	int size_inwords = size / 4;
+	u32 ctrl_reg;
+	int ret;
+	int i;
+
+	ctrl_reg = PF2FW_MBOX_CTRL(mbx);
+	ret = mucse_obtain_mbx_lock_pf(hw);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < size_inwords; i++)
+		mbx_data_wr32(mbx, MBX_FW_PF_SHM_DATA + i * 4, msg[i]);
+
+	/* flush msg and acks as we are overwriting the message buffer */
+	hw->mbx.fw_ack = mucse_mbx_get_fwack(mbx);
+	mucse_mbx_inc_pf_req(hw);
+	mbx_ctrl_wr32(mbx, ctrl_reg, MBOX_CTRL_REQ);
+
+	return 0;
+}
+
+/**
+ * mucse_write_posted_mbx - Write a message to the mailbox, wait for ack
+ * @hw: pointer to the HW structure
+ * @msg: the message buffer
+ * @size: length of buffer
+ *
+ * @return: 0 if it successfully copied message into the buffer and
+ * received an ack to that message within delay * timeout period
+ **/
+int mucse_write_posted_mbx(struct mucse_hw *hw, u32 *msg, u16 size)
+{
+	int ret;
+
+	ret = mucse_write_mbx_pf(hw, msg, size);
+	if (ret)
+		return ret;
+	return mucse_poll_for_ack(hw);
+}
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h
new file mode 100644
index 000000000000..110c1ee025ba
--- /dev/null
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright(c) 2020 - 2025 Mucse Corporation. */
+
+#ifndef _RNPGBE_MBX_H
+#define _RNPGBE_MBX_H
+
+#include "rnpgbe.h"
+
+#define MUCSE_MAILBOX_BYTES 56
+#define MBX_FW2PF_COUNTER 0
+#define MBX_PF2FW_COUNTER 4
+#define MBX_FW_PF_SHM_DATA 8
+#define FW2PF_MBOX_VEC 0
+#define PF2FW_MBOX_CTRL(mbx) ((mbx)->pf2fw_mbox_ctrl)
+#define FW_PF_MBOX_MASK(mbx) ((mbx)->fw_pf_mbox_mask)
+#define MBOX_CTRL_REQ BIT(0)
+#define MBOX_PF_HOLD BIT(3)
+#define MBOX_IRQ_EN 0
+#define MBOX_IRQ_DISABLE 1
+
+int mucse_write_mbx_pf(struct mucse_hw *hw, u32 *msg, u16 size);
+int mucse_write_posted_mbx(struct mucse_hw *hw, u32 *msg, u16 size);
+void mucse_init_mbx_params_pf(struct mucse_hw *hw);
+int mucse_read_posted_mbx(struct mucse_hw *hw, u32 *msg, u16 size);
+#endif /* _RNPGBE_MBX_H */
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH net-next v7 4/5] net: rnpgbe: Add basic mbx_fw support
  2025-08-22  2:34 [PATCH net-next v7 0/5] Add driver for 1Gbe network chips from MUCSE Dong Yibo
                   ` (2 preceding siblings ...)
  2025-08-22  2:34 ` [PATCH net-next v7 3/5] net: rnpgbe: Add basic mbx ops support Dong Yibo
@ 2025-08-22  2:34 ` Dong Yibo
  2025-08-22  4:49   ` Parthiban.Veerasooran
                     ` (3 more replies)
  2025-08-22  2:34 ` [PATCH net-next v7 5/5] net: rnpgbe: Add register_netdev Dong Yibo
  4 siblings, 4 replies; 34+ messages in thread
From: Dong Yibo @ 2025-08-22  2:34 UTC (permalink / raw)
  To: andrew+netdev, davem, edumazet, kuba, pabeni, horms, corbet,
	gur.stavi, maddy, mpe, danishanwar, lee, gongfan1, lorenzo,
	geert+renesas, Parthiban.Veerasooran, lukas.bulwahn,
	alexanderduyck, richardcochran, kees, gustavoars
  Cc: netdev, linux-doc, linux-kernel, linux-hardening, dong100

Initialize basic mbx_fw ops, such as get_capability, reset phy
and so on.

Signed-off-by: Dong Yibo <dong100@mucse.com>
---
 drivers/net/ethernet/mucse/rnpgbe/Makefile    |   3 +-
 drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h    |   1 +
 .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c | 333 ++++++++++++++++++
 .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h | 152 ++++++++
 4 files changed, 488 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
 create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h

diff --git a/drivers/net/ethernet/mucse/rnpgbe/Makefile b/drivers/net/ethernet/mucse/rnpgbe/Makefile
index 5fc878ada4b1..de8bcb7772ab 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/Makefile
+++ b/drivers/net/ethernet/mucse/rnpgbe/Makefile
@@ -7,4 +7,5 @@
 obj-$(CONFIG_MGBE) += rnpgbe.o
 rnpgbe-objs := rnpgbe_main.o\
 	       rnpgbe_chip.o\
-	       rnpgbe_mbx.o
+	       rnpgbe_mbx.o\
+	       rnpgbe_mbx_fw.o
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
index 67e28a4667e7..a32419a34d75 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
@@ -52,6 +52,7 @@ struct mucse_hw {
 	void __iomem *hw_addr;
 	struct pci_dev *pdev;
 	enum rnpgbe_hw_type hw_type;
+	u8 pfvfnum;
 	struct mucse_mbx_info mbx;
 };
 
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
new file mode 100644
index 000000000000..84570763cf79
--- /dev/null
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
@@ -0,0 +1,333 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright(c) 2020 - 2025 Mucse Corporation. */
+
+#include <linux/pci.h>
+#include <linux/if_ether.h>
+
+#include "rnpgbe.h"
+#include "rnpgbe_hw.h"
+#include "rnpgbe_mbx.h"
+#include "rnpgbe_mbx_fw.h"
+
+/**
+ * mucse_fw_send_cmd_wait - Send cmd req and wait for response
+ * @hw: pointer to the HW structure
+ * @req: pointer to the cmd req structure
+ * @reply: pointer to the fw reply structure
+ *
+ * mucse_fw_send_cmd_wait sends req to pf-fw mailbox and wait
+ * reply from fw.
+ *
+ * @return: 0 on success, negative on failure
+ **/
+static int mucse_fw_send_cmd_wait(struct mucse_hw *hw,
+				  struct mbx_fw_cmd_req *req,
+				  struct mbx_fw_cmd_reply *reply)
+{
+	int len = le16_to_cpu(req->datalen);
+	int retry_cnt = 3;
+	int err;
+
+	err = mutex_lock_interruptible(&hw->mbx.lock);
+	if (err)
+		return err;
+	err = mucse_write_posted_mbx(hw, (u32 *)req, len);
+	if (err)
+		goto out;
+	do {
+		err = mucse_read_posted_mbx(hw, (u32 *)reply,
+					    sizeof(*reply));
+		if (err)
+			goto out;
+		/* mucse_write_posted_mbx return 0 means fw has
+		 * received request, wait for the expect opcode
+		 * reply with 'retry_cnt' times.
+		 */
+	} while (--retry_cnt >= 0 && reply->opcode != req->opcode);
+out:
+	mutex_unlock(&hw->mbx.lock);
+	if (!err && retry_cnt < 0)
+		return -ETIMEDOUT;
+	if (!err && reply->error_code)
+		return -EIO;
+	return err;
+}
+
+/**
+ * build_phy_abilities_req - build req with get_phy_ability opcode
+ * @req: pointer to the cmd req structure
+ **/
+static void build_phy_abilities_req(struct mbx_fw_cmd_req *req)
+{
+	req->flags = 0;
+	req->opcode = cpu_to_le16(GET_PHY_ABILITY);
+	req->datalen = cpu_to_le16(MBX_REQ_HDR_LEN);
+	req->reply_lo = 0;
+	req->reply_hi = 0;
+}
+
+/**
+ * mucse_fw_get_capability - Get hw abilities from fw
+ * @hw: pointer to the HW structure
+ * @abil: pointer to the hw_abilities structure
+ *
+ * mucse_fw_get_capability tries to get hw abilities from
+ * hw.
+ *
+ * @return: 0 on success, negative on failure
+ **/
+static int mucse_fw_get_capability(struct mucse_hw *hw,
+				   struct hw_abilities *abil)
+{
+	struct mbx_fw_cmd_reply reply = {};
+	struct mbx_fw_cmd_req req = {};
+	int err;
+
+	build_phy_abilities_req(&req);
+	err = mucse_fw_send_cmd_wait(hw, &req, &reply);
+	if (!err)
+		memcpy(abil, &reply.hw_abilities, sizeof(*abil));
+	return err;
+}
+
+/**
+ * mucse_mbx_get_capability - Get hw abilities from fw
+ * @hw: pointer to the HW structure
+ *
+ * mucse_mbx_get_capability tries to get capabities from
+ * hw. Many retrys will do if it is failed.
+ *
+ * @return: 0 on success, negative on failure
+ **/
+int mucse_mbx_get_capability(struct mucse_hw *hw)
+{
+	struct hw_abilities ability = {};
+	int try_cnt = 3;
+	int err = -EIO;
+
+	while (try_cnt--) {
+		err = mucse_fw_get_capability(hw, &ability);
+		if (err)
+			continue;
+		hw->pfvfnum = le16_to_cpu(ability.pfnum) & GENMASK_U16(7, 0);
+		return 0;
+	}
+	return err;
+}
+
+/**
+ * mbx_cookie_zalloc - Alloc a cookie structure
+ * @priv_len: private length for this cookie
+ *
+ * @return: cookie structure on success
+ **/
+static struct mbx_req_cookie *mbx_cookie_zalloc(int priv_len)
+{
+	struct mbx_req_cookie *cookie;
+
+	cookie = kzalloc(struct_size(cookie, priv, priv_len), GFP_KERNEL);
+	if (cookie) {
+		cookie->timeout_jiffies = 30 * HZ;
+		cookie->magic = COOKIE_MAGIC;
+		cookie->priv_len = priv_len;
+	}
+	return cookie;
+}
+
+/**
+ * mucse_mbx_fw_post_req - Posts a mbx req to firmware and wait reply
+ * @hw: pointer to the HW structure
+ * @req: pointer to the cmd req structure
+ * @cookie: pointer to the req cookie
+ *
+ * mucse_mbx_fw_post_req posts a mbx req to firmware and wait for the
+ * reply. cookie->wait will be set in irq handler.
+ *
+ * @return: 0 on success, negative on failure
+ **/
+static int mucse_mbx_fw_post_req(struct mucse_hw *hw,
+				 struct mbx_fw_cmd_req *req,
+				 struct mbx_req_cookie *cookie)
+{
+	int len = le16_to_cpu(req->datalen);
+	int err;
+
+	cookie->errcode = 0;
+	cookie->done = 0;
+	init_waitqueue_head(&cookie->wait);
+	err = mutex_lock_interruptible(&hw->mbx.lock);
+	if (err)
+		return err;
+	err = mucse_write_mbx_pf(hw, (u32 *)req, len);
+	if (err)
+		goto out;
+	/* if write succeeds, we must wait for firmware response or
+	 * timeout to avoid using the already freed cookie->wait
+	 */
+	err = wait_event_timeout(cookie->wait,
+				 cookie->done == 1,
+				 cookie->timeout_jiffies);
+
+	if (!err)
+		err = -ETIMEDOUT;
+	else
+		err = 0;
+	if (!err && cookie->errcode)
+		err = cookie->errcode;
+out:
+	mutex_unlock(&hw->mbx.lock);
+	return err;
+}
+
+/**
+ * build_ifinsmod - build req with insmod opcode
+ * @req: pointer to the cmd req structure
+ * @status: true for insmod, false for rmmod
+ **/
+static void build_ifinsmod(struct mbx_fw_cmd_req *req,
+			   int status)
+{
+	req->flags = 0;
+	req->opcode = cpu_to_le16(DRIVER_INSMOD);
+	req->datalen = cpu_to_le16(sizeof(req->ifinsmod) +
+				   MBX_REQ_HDR_LEN);
+	req->cookie = NULL;
+	req->reply_lo = 0;
+	req->reply_hi = 0;
+#define FIXED_VERSION 0xFFFFFFFF
+	req->ifinsmod.version = cpu_to_le32(FIXED_VERSION);
+	req->ifinsmod.status = cpu_to_le32(status);
+}
+
+/**
+ * mucse_mbx_ifinsmod - Echo driver insmod status to hw
+ * @hw: pointer to the HW structure
+ * @status: true for insmod, false for rmmod
+ *
+ * @return: 0 on success, negative on failure
+ **/
+int mucse_mbx_ifinsmod(struct mucse_hw *hw, int status)
+{
+	struct mbx_fw_cmd_req req = {};
+	int len;
+	int err;
+
+	build_ifinsmod(&req, status);
+	len = le16_to_cpu(req.datalen);
+	err = mutex_lock_interruptible(&hw->mbx.lock);
+	if (err)
+		return err;
+
+	if (status) {
+		err = mucse_write_posted_mbx(hw, (u32 *)&req,
+					     len);
+	} else {
+		err = mucse_write_mbx_pf(hw, (u32 *)&req,
+					 len);
+	}
+
+	mutex_unlock(&hw->mbx.lock);
+	return err;
+}
+
+/**
+ * build_reset_phy_req - build req with reset_phy opcode
+ * @req: pointer to the cmd req structure
+ * @cookie: pointer of cookie for this cmd
+ **/
+static void build_reset_phy_req(struct mbx_fw_cmd_req *req,
+				void *cookie)
+{
+	req->flags = 0;
+	req->opcode = cpu_to_le16(RESET_PHY);
+	req->datalen = cpu_to_le16(MBX_REQ_HDR_LEN);
+	req->reply_lo = 0;
+	req->reply_hi = 0;
+	req->cookie = cookie;
+}
+
+/**
+ * mucse_mbx_fw_reset_phy - Posts a mbx req to reset hw
+ * @hw: pointer to the HW structure
+ *
+ * mucse_mbx_fw_reset_phy posts a mbx req to firmware to reset hw.
+ * It uses mucse_fw_send_cmd_wait if no irq, and mucse_mbx_fw_post_req
+ * if other irq is registered.
+ *
+ * @return: 0 on success, negative on failure
+ **/
+int mucse_mbx_fw_reset_phy(struct mucse_hw *hw)
+{
+	struct mbx_fw_cmd_reply reply = {};
+	struct mbx_fw_cmd_req req = {};
+	int ret;
+
+	if (hw->mbx.irq_enabled) {
+		struct mbx_req_cookie *cookie = mbx_cookie_zalloc(0);
+
+		if (!cookie)
+			return -ENOMEM;
+
+		build_reset_phy_req(&req, cookie);
+		ret = mucse_mbx_fw_post_req(hw, &req, cookie);
+		kfree(cookie);
+		return ret;
+	}
+
+	build_reset_phy_req(&req, &req);
+	return mucse_fw_send_cmd_wait(hw, &req, &reply);
+}
+
+/**
+ * build_get_macaddress_req - build req with get_mac opcode
+ * @req: pointer to the cmd req structure
+ * @port_mask: port valid for this cmd
+ * @pfvfnum: pfvfnum for this cmd
+ * @cookie: pointer of cookie for this cmd
+ **/
+static void build_get_macaddress_req(struct mbx_fw_cmd_req *req,
+				     int port_mask, int pfvfnum,
+				     void *cookie)
+{
+	req->flags = 0;
+	req->opcode = cpu_to_le16(GET_MAC_ADDRES);
+	req->datalen = cpu_to_le16(sizeof(req->get_mac_addr) +
+				   MBX_REQ_HDR_LEN);
+	req->cookie = cookie;
+	req->reply_lo = 0;
+	req->reply_hi = 0;
+	req->get_mac_addr.port_mask = cpu_to_le32(port_mask);
+	req->get_mac_addr.pfvf_num = cpu_to_le32(pfvfnum);
+}
+
+/**
+ * mucse_fw_get_macaddr - Posts a mbx req to request macaddr
+ * @hw: pointer to the HW structure
+ * @pfvfnum: index of pf/vf num
+ * @mac_addr: pointer to store mac_addr
+ * @port: port index
+ *
+ * mucse_fw_get_macaddr posts a mbx req to firmware to get mac_addr.
+ * It uses mucse_fw_send_cmd_wait if no irq, and mucse_mbx_fw_post_req
+ * if other irq is registered.
+ *
+ * @return: 0 on success, negative on failure
+ **/
+int mucse_fw_get_macaddr(struct mucse_hw *hw, int pfvfnum,
+			 u8 *mac_addr,
+			 int port)
+{
+	struct mbx_fw_cmd_reply reply = {};
+	struct mbx_fw_cmd_req req = {};
+	int err;
+
+	build_get_macaddress_req(&req, BIT(port), pfvfnum, &req);
+	err = mucse_fw_send_cmd_wait(hw, &req, &reply);
+	if (err)
+		return err;
+	if (le32_to_cpu(reply.mac_addr.ports) & BIT(port))
+		memcpy(mac_addr, reply.mac_addr.addrs[port].mac, ETH_ALEN);
+	else
+		return -ENODATA;
+	return 0;
+}
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h
new file mode 100644
index 000000000000..b73238d0e848
--- /dev/null
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h
@@ -0,0 +1,152 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright(c) 2020 - 2025 Mucse Corporation. */
+
+#ifndef _RNPGBE_MBX_FW_H
+#define _RNPGBE_MBX_FW_H
+
+#include <linux/types.h>
+#include <linux/errno.h>
+#include <linux/wait.h>
+
+#include "rnpgbe.h"
+
+#define MBX_REQ_HDR_LEN 24
+
+struct mbx_fw_cmd_reply;
+typedef void (*cookie_cb)(struct mbx_fw_cmd_reply *reply, void *priv);
+
+struct mbx_req_cookie {
+	int magic;
+#define COOKIE_MAGIC 0xCE
+	cookie_cb cb;
+	int timeout_jiffies;
+	int errcode;
+	wait_queue_head_t wait;
+	int done;
+	int priv_len;
+	char priv[] __counted_by(priv_len);
+};
+
+enum MUCSE_FW_CMD {
+	GET_PHY_ABILITY = 0x0601,
+	GET_MAC_ADDRES = 0x0602,
+	RESET_PHY = 0x0603,
+	DRIVER_INSMOD = 0x0803,
+};
+
+struct hw_abilities {
+	u8 link_stat;
+	u8 port_mask;
+	__le32 speed;
+	__le16 phy_type;
+	__le16 nic_mode;
+	__le16 pfnum;
+	__le32 fw_version;
+	__le32 axi_mhz;
+	union {
+		u8 port_id[4];
+		__le32 port_ids;
+	};
+	__le32 bd_uid;
+	__le32 phy_id;
+	__le32 wol_status;
+	union {
+		__le32 ext_ability;
+		struct {
+			u32 valid : 1;
+			u32 wol_en : 1;
+			u32 pci_preset_runtime_en : 1;
+			u32 smbus_en : 1;
+			u32 ncsi_en : 1;
+			u32 rpu_en : 1;
+			u32 v2 : 1;
+			u32 pxe_en : 1;
+			u32 mctp_en : 1;
+			u32 yt8614 : 1;
+			u32 pci_ext_reset : 1;
+			u32 rpu_availble : 1;
+			u32 fw_lldp_ability : 1;
+			u32 lldp_enabled : 1;
+			u32 only_1g : 1;
+			u32 force_down_en: 1;
+		} e_host;
+	};
+} __packed;
+
+/* FW stores extended ability information in 'ext_ability' as a 32-bit
+ * little-endian value. To make these flags easily accessible in the
+ * kernel (via named 'bitfields' instead of raw bitmask operations),
+ * we use the union's 'e_host' struct, which provides named bits
+ * (e.g., 'wol_en', 'smbus_en')
+ */
+static inline void ability_update_host_endian(struct hw_abilities *abi)
+{
+	u32 host_val = le32_to_cpu(abi->ext_ability);
+
+	abi->e_host = *(typeof(abi->e_host) *)&host_val;
+}
+
+#define FLAGS_DD BIT(0)
+#define FLAGS_ERR BIT(2)
+
+struct mbx_fw_cmd_req {
+	__le16 flags;
+	__le16 opcode;
+	__le16 datalen;
+	__le16 ret_value;
+	union {
+		struct {
+			__le32 cookie_lo;
+			__le32 cookie_hi;
+		};
+
+		void *cookie;
+	};
+	__le32 reply_lo;
+	__le32 reply_hi;
+	union {
+		u8 data[32];
+		struct {
+			__le32 version;
+			__le32 status;
+		} ifinsmod;
+		struct {
+			__le32 port_mask;
+			__le32 pfvf_num;
+		} get_mac_addr;
+	};
+} __packed;
+
+struct mbx_fw_cmd_reply {
+	__le16 flags;
+	__le16 opcode;
+	__le16 error_code;
+	__le16 datalen;
+	union {
+		struct {
+			__le32 cookie_lo;
+			__le32 cookie_hi;
+		};
+		void *cookie;
+	};
+	union {
+		u8 data[40];
+		struct mac_addr {
+			__le32 ports;
+			struct _addr {
+				/* for macaddr:01:02:03:04:05:06
+				 * mac-hi=0x01020304 mac-lo=0x05060000
+				 */
+				u8 mac[8];
+			} addrs[4];
+		} mac_addr;
+		struct hw_abilities hw_abilities;
+	};
+} __packed;
+
+int mucse_mbx_get_capability(struct mucse_hw *hw);
+int mucse_mbx_ifinsmod(struct mucse_hw *hw, int status);
+int mucse_mbx_fw_reset_phy(struct mucse_hw *hw);
+int mucse_fw_get_macaddr(struct mucse_hw *hw, int pfvfnum,
+			 u8 *mac_addr, int port);
+#endif /* _RNPGBE_MBX_FW_H */
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* [PATCH net-next v7 5/5] net: rnpgbe: Add register_netdev
  2025-08-22  2:34 [PATCH net-next v7 0/5] Add driver for 1Gbe network chips from MUCSE Dong Yibo
                   ` (3 preceding siblings ...)
  2025-08-22  2:34 ` [PATCH net-next v7 4/5] net: rnpgbe: Add basic mbx_fw support Dong Yibo
@ 2025-08-22  2:34 ` Dong Yibo
  4 siblings, 0 replies; 34+ messages in thread
From: Dong Yibo @ 2025-08-22  2:34 UTC (permalink / raw)
  To: andrew+netdev, davem, edumazet, kuba, pabeni, horms, corbet,
	gur.stavi, maddy, mpe, danishanwar, lee, gongfan1, lorenzo,
	geert+renesas, Parthiban.Veerasooran, lukas.bulwahn,
	alexanderduyck, richardcochran, kees, gustavoars
  Cc: netdev, linux-doc, linux-kernel, linux-hardening, dong100

Initialize get mac from hw, register the netdev.

Signed-off-by: Dong Yibo <dong100@mucse.com>
---
 drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h    | 23 ++++++
 .../net/ethernet/mucse/rnpgbe/rnpgbe_chip.c   | 82 +++++++++++++++++++
 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h |  2 +
 .../net/ethernet/mucse/rnpgbe/rnpgbe_main.c   | 75 +++++++++++++++++
 4 files changed, 182 insertions(+)

diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
index a32419a34d75..44b581b8d45c 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
@@ -6,6 +6,7 @@
 
 #include <linux/types.h>
 #include <linux/mutex.h>
+#include <linux/netdevice.h>
 
 extern const struct rnpgbe_info rnpgbe_n500_info;
 extern const struct rnpgbe_info rnpgbe_n210_info;
@@ -24,6 +25,10 @@ enum rnpgbe_hw_type {
 	rnpgbe_hw_unknown
 };
 
+struct mucse_dma_info {
+	void __iomem *dma_base_addr;
+};
+
 struct mucse_mbx_stats {
 	u32 msgs_tx;
 	u32 msgs_rx;
@@ -48,12 +53,27 @@ struct mucse_mbx_info {
 	u32 fw2pf_mbox_vec;
 };
 
+struct mucse_hw;
+
+struct mucse_hw_operations {
+	int (*reset_hw)(struct mucse_hw *hw);
+	void (*driver_status)(struct mucse_hw *hw, bool enable, int mode);
+};
+
+enum {
+	mucse_driver_insmod,
+};
+
 struct mucse_hw {
 	void __iomem *hw_addr;
 	struct pci_dev *pdev;
 	enum rnpgbe_hw_type hw_type;
 	u8 pfvfnum;
+	const struct mucse_hw_operations *ops;
+	struct mucse_dma_info dma;
 	struct mucse_mbx_info mbx;
+	int port;
+	u8 perm_addr[ETH_ALEN];
 };
 
 struct mucse {
@@ -73,4 +93,7 @@ struct rnpgbe_info {
 #define PCI_DEVICE_ID_N500_DUAL_PORT 0x8318
 #define PCI_DEVICE_ID_N210 0x8208
 #define PCI_DEVICE_ID_N210L 0x820a
+
+#define rnpgbe_dma_wr32(dma, reg, val) \
+	writel((val), (dma)->dma_base_addr + (reg))
 #endif /* _RNPGBE_H */
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
index f38daef752a3..40c29411fe09 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
@@ -1,11 +1,87 @@
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright(c) 2020 - 2025 Mucse Corporation. */
 
+#include <linux/pci.h>
 #include <linux/string.h>
+#include <linux/etherdevice.h>
 
 #include "rnpgbe.h"
 #include "rnpgbe_hw.h"
 #include "rnpgbe_mbx.h"
+#include "rnpgbe_mbx_fw.h"
+
+/**
+ * rnpgbe_get_permanent_mac - Get permanent mac
+ * @hw: hw information structure
+ * @mac_addr: pointer to store mac
+ *
+ * rnpgbe_get_permanent_mac tries to get mac from hw.
+ * It use eth_random_addr if failed.
+ *
+ * @return: 0 on success, negative on failure
+ **/
+static int rnpgbe_get_permanent_mac(struct mucse_hw *hw,
+				    u8 *mac_addr)
+{
+	struct device *dev = &hw->pdev->dev;
+	int err;
+
+	err = mucse_fw_get_macaddr(hw, hw->pfvfnum, mac_addr, hw->port);
+	if (err) {
+		dev_err(dev, "Failed to get MAC from FW %d\n", err);
+		return err;
+	}
+
+	if (!is_valid_ether_addr(mac_addr)) {
+		dev_err(dev, "Failed to get valid MAC from FW\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+/**
+ * rnpgbe_reset_hw_ops - Do a hardware reset
+ * @hw: hw information structure
+ *
+ * rnpgbe_reset_hw_ops calls fw to do a hardware
+ * reset, and cleans some regs to default.
+ *
+ * @return: 0 on success, negative on failure
+ **/
+static int rnpgbe_reset_hw_ops(struct mucse_hw *hw)
+{
+	struct mucse_dma_info *dma = &hw->dma;
+	int err;
+
+	rnpgbe_dma_wr32(dma, RNPGBE_DMA_AXI_EN, 0);
+	err = mucse_mbx_fw_reset_phy(hw);
+	if (err)
+		return err;
+	return rnpgbe_get_permanent_mac(hw, hw->perm_addr);
+}
+
+/**
+ * rnpgbe_driver_status_hw_ops - Echo driver status to hw
+ * @hw: hw information structure
+ * @enable: true or false status
+ * @mode: status mode
+ **/
+static void rnpgbe_driver_status_hw_ops(struct mucse_hw *hw,
+					bool enable,
+					int mode)
+{
+	switch (mode) {
+	case mucse_driver_insmod:
+		mucse_mbx_ifinsmod(hw, enable);
+		break;
+	}
+}
+
+static const struct mucse_hw_operations rnpgbe_hw_ops = {
+	.reset_hw = &rnpgbe_reset_hw_ops,
+	.driver_status = &rnpgbe_driver_status_hw_ops,
+};
 
 /**
  * rnpgbe_init_common - Setup common attribute
@@ -13,10 +89,16 @@
  **/
 static void rnpgbe_init_common(struct mucse_hw *hw)
 {
+	struct mucse_dma_info *dma = &hw->dma;
 	struct mucse_mbx_info *mbx = &hw->mbx;
 
+	dma->dma_base_addr = hw->hw_addr;
+
 	mbx->pf2fw_mbox_ctrl = GBE_PF2FW_MBX_MASK_OFFSET;
 	mbx->fw_pf_mbox_mask = GBE_FWPF_MBX_MASK;
+
+	hw->ops = &rnpgbe_hw_ops;
+	hw->port = 0;
 }
 
 /**
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
index 746dca78f1df..0ab2c328c9e9 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
@@ -11,6 +11,8 @@
 #define GBE_FWPF_MBX_MASK 0x5700
 #define N210_FW2PF_MBX_VEC_OFFSET 0x29400
 #define N210_FWPF_SHM_BASE_OFFSET 0x2d900
+/**************** DMA Registers ****************************/
+#define RNPGBE_DMA_AXI_EN 0x0010
 /**************** CHIP Resource ****************************/
 #define RNPGBE_MAX_QUEUES 8
 #endif /* _RNPGBE_HW_H */
diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
index 6992a3c0e58a..947d0ca2101d 100644
--- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
+++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
@@ -9,6 +9,8 @@
 
 #include "rnpgbe.h"
 #include "rnpgbe_hw.h"
+#include "rnpgbe_mbx.h"
+#include "rnpgbe_mbx_fw.h"
 
 static const char rnpgbe_driver_name[] = "rnpgbe";
 static const struct rnpgbe_info *rnpgbe_info_tbl[] = {
@@ -35,6 +37,55 @@ static struct pci_device_id rnpgbe_pci_tbl[] = {
 	{0, },
 };
 
+/**
+ * rnpgbe_open - Called when a network interface is made active
+ * @netdev: network interface device structure
+ *
+ * The open entry point is called when a network interface is made
+ * active by the system (IFF_UP).
+ *
+ * @return: 0 on success, negative value on failure
+ **/
+static int rnpgbe_open(struct net_device *netdev)
+{
+	return 0;
+}
+
+/**
+ * rnpgbe_close - Disables a network interface
+ * @netdev: network interface device structure
+ *
+ * The close entry point is called when an interface is de-activated
+ * by the OS.
+ *
+ * @return: 0, this is not allowed to fail
+ **/
+static int rnpgbe_close(struct net_device *netdev)
+{
+	return 0;
+}
+
+/**
+ * rnpgbe_xmit_frame - Send a skb to driver
+ * @skb: skb structure to be sent
+ * @netdev: network interface device structure
+ *
+ * @return: NETDEV_TX_OK or NETDEV_TX_BUSY
+ **/
+static netdev_tx_t rnpgbe_xmit_frame(struct sk_buff *skb,
+				     struct net_device *netdev)
+{
+	dev_kfree_skb_any(skb);
+	netdev->stats.tx_dropped++;
+	return NETDEV_TX_OK;
+}
+
+static const struct net_device_ops rnpgbe_netdev_ops = {
+	.ndo_open = rnpgbe_open,
+	.ndo_stop = rnpgbe_close,
+	.ndo_start_xmit = rnpgbe_xmit_frame,
+};
+
 /**
  * rnpgbe_add_adapter - Add netdev for this pci_dev
  * @pdev: PCI device information structure
@@ -78,6 +129,27 @@ static int rnpgbe_add_adapter(struct pci_dev *pdev,
 
 	hw->hw_addr = hw_addr;
 	info->init(hw);
+	mucse_init_mbx_params_pf(hw);
+	/* echo fw driver insmod to control hw */
+	hw->ops->driver_status(hw, true, mucse_driver_insmod);
+	err = mucse_mbx_get_capability(hw);
+	if (err) {
+		dev_err(&pdev->dev,
+			"mucse_mbx_get_capability failed! %d\n",
+			err);
+		goto err_free_net;
+	}
+	netdev->netdev_ops = &rnpgbe_netdev_ops;
+	netdev->watchdog_timeo = 5 * HZ;
+	err = hw->ops->reset_hw(hw);
+	if (err) {
+		dev_err(&pdev->dev, "Hw reset failed %d\n", err);
+		goto err_free_net;
+	}
+	eth_hw_addr_set(netdev, hw->perm_addr);
+	err = register_netdev(netdev);
+	if (err)
+		goto err_free_net;
 	return 0;
 
 err_free_net:
@@ -141,12 +213,15 @@ static int rnpgbe_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 static void rnpgbe_rm_adapter(struct pci_dev *pdev)
 {
 	struct mucse *mucse = pci_get_drvdata(pdev);
+	struct mucse_hw *hw = &mucse->hw;
 	struct net_device *netdev;
 
 	if (!mucse)
 		return;
 	netdev = mucse->netdev;
+	unregister_netdev(netdev);
 	mucse->netdev = NULL;
+	hw->ops->driver_status(hw, false, mucse_driver_insmod);
 	free_netdev(netdev);
 }
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 34+ messages in thread

* Re: [PATCH net-next v7 1/5] net: rnpgbe: Add build support for rnpgbe
  2025-08-22  2:34 ` [PATCH net-next v7 1/5] net: rnpgbe: Add build support for rnpgbe Dong Yibo
@ 2025-08-22  4:32   ` Parthiban.Veerasooran
  2025-08-22  5:23     ` Yibo Dong
  0 siblings, 1 reply; 34+ messages in thread
From: Parthiban.Veerasooran @ 2025-08-22  4:32 UTC (permalink / raw)
  To: dong100, andrew+netdev, davem, edumazet, kuba, pabeni, horms,
	corbet, gur.stavi, maddy, mpe, danishanwar, lee, gongfan1,
	lorenzo, geert+renesas, lukas.bulwahn, alexanderduyck,
	richardcochran, kees, gustavoars
  Cc: netdev, linux-doc, linux-kernel, linux-hardening

Hi,

On 22/08/25 8:04 am, Dong Yibo wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Add build options and doc for mucse.
> Initialize pci device access for MUCSE devices.
> 
> Signed-off-by: Dong Yibo <dong100@mucse.com>
> ---
>   .../device_drivers/ethernet/index.rst         |   1 +
>   .../device_drivers/ethernet/mucse/rnpgbe.rst  |  21 +++
>   MAINTAINERS                                   |   8 ++
>   drivers/net/ethernet/Kconfig                  |   1 +
>   drivers/net/ethernet/Makefile                 |   1 +
>   drivers/net/ethernet/mucse/Kconfig            |  34 +++++
>   drivers/net/ethernet/mucse/Makefile           |   7 +
>   drivers/net/ethernet/mucse/rnpgbe/Makefile    |   8 ++
>   drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h    |  24 ++++
>   .../net/ethernet/mucse/rnpgbe/rnpgbe_main.c   | 120 ++++++++++++++++++
>   10 files changed, 225 insertions(+)
>   create mode 100644 Documentation/networking/device_drivers/ethernet/mucse/rnpgbe.rst
>   create mode 100644 drivers/net/ethernet/mucse/Kconfig
>   create mode 100644 drivers/net/ethernet/mucse/Makefile
>   create mode 100644 drivers/net/ethernet/mucse/rnpgbe/Makefile
>   create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
>   create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> 
> diff --git a/Documentation/networking/device_drivers/ethernet/index.rst b/Documentation/networking/device_drivers/ethernet/index.rst
> index 0b0a3eef6aae..41ff2152b7aa 100644
> --- a/Documentation/networking/device_drivers/ethernet/index.rst
> +++ b/Documentation/networking/device_drivers/ethernet/index.rst
> @@ -47,6 +47,7 @@ Contents:
>      mellanox/mlx5/index
>      meta/fbnic
>      microsoft/netvsc
> +   mucse/rnpgbe
>      neterion/s2io
>      netronome/nfp
>      pensando/ionic
> diff --git a/Documentation/networking/device_drivers/ethernet/mucse/rnpgbe.rst b/Documentation/networking/device_drivers/ethernet/mucse/rnpgbe.rst
> new file mode 100644
> index 000000000000..7562fb6b8f61
> --- /dev/null
> +++ b/Documentation/networking/device_drivers/ethernet/mucse/rnpgbe.rst
> @@ -0,0 +1,21 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +===========================================================
> +Linux Base Driver for MUCSE(R) Gigabit PCI Express Adapters
> +===========================================================
> +
> +MUCSE Gigabit Linux driver.
> +Copyright (c) 2020 - 2025 MUCSE Co.,Ltd.
> +
> +Identifying Your Adapter
> +========================
> +The driver is compatible with devices based on the following:
> +
> + * MUCSE(R) Ethernet Controller N500 series
> + * MUCSE(R) Ethernet Controller N210 series
> +
> +Support
> +=======
> + If you have problems with the software or hardware, please contact our
> + customer support team via email at techsupport@mucse.com or check our
> + website at https://www.mucse.com/en/
> diff --git a/MAINTAINERS b/MAINTAINERS
> index bce96dd254b8..00b73e3631b4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -17276,6 +17276,14 @@ T:     git git://linuxtv.org/media.git
>   F:     Documentation/devicetree/bindings/media/i2c/aptina,mt9v111.yaml
>   F:     drivers/media/i2c/mt9v111.c
> 
> +MUCSE ETHERNET DRIVER
> +M:     Yibo Dong <dong100@mucse.com>
> +L:     netdev@vger.kernel.org
> +S:     Maintained
> +W:     https://www.mucse.com/en/
> +F:     Documentation/networking/device_drivers/ethernet/mucse/
> +F:     drivers/net/ethernet/mucse/
> +
>   MULTIFUNCTION DEVICES (MFD)
>   M:     Lee Jones <lee@kernel.org>
>   S:     Maintained
> diff --git a/drivers/net/ethernet/Kconfig b/drivers/net/ethernet/Kconfig
> index f86d4557d8d7..167388f9c744 100644
> --- a/drivers/net/ethernet/Kconfig
> +++ b/drivers/net/ethernet/Kconfig
> @@ -129,6 +129,7 @@ source "drivers/net/ethernet/microchip/Kconfig"
>   source "drivers/net/ethernet/mscc/Kconfig"
>   source "drivers/net/ethernet/microsoft/Kconfig"
>   source "drivers/net/ethernet/moxa/Kconfig"
> +source "drivers/net/ethernet/mucse/Kconfig"
>   source "drivers/net/ethernet/myricom/Kconfig"
> 
>   config FEALNX
> diff --git a/drivers/net/ethernet/Makefile b/drivers/net/ethernet/Makefile
> index 67182339469a..1b8c4df3f594 100644
> --- a/drivers/net/ethernet/Makefile
> +++ b/drivers/net/ethernet/Makefile
> @@ -65,6 +65,7 @@ obj-$(CONFIG_NET_VENDOR_MICREL) += micrel/
>   obj-$(CONFIG_NET_VENDOR_MICROCHIP) += microchip/
>   obj-$(CONFIG_NET_VENDOR_MICROSEMI) += mscc/
>   obj-$(CONFIG_NET_VENDOR_MOXART) += moxa/
> +obj-$(CONFIG_NET_VENDOR_MUCSE) += mucse/
>   obj-$(CONFIG_NET_VENDOR_MYRI) += myricom/
>   obj-$(CONFIG_FEALNX) += fealnx.o
>   obj-$(CONFIG_NET_VENDOR_NATSEMI) += natsemi/
> diff --git a/drivers/net/ethernet/mucse/Kconfig b/drivers/net/ethernet/mucse/Kconfig
> new file mode 100644
> index 000000000000..be0fdf268484
> --- /dev/null
> +++ b/drivers/net/ethernet/mucse/Kconfig
> @@ -0,0 +1,34 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# Mucse network device configuration
> +#
> +
> +config NET_VENDOR_MUCSE
> +       bool "Mucse devices"
> +       default y
> +       help
> +         If you have a network (Ethernet) card from Mucse(R), say Y.
> +
> +         Note that the answer to this question doesn't directly affect the
> +         kernel: saying N will just cause the configurator to skip all
> +         the questions about Mucse(R) cards. If you say Y, you will
> +         be asked for your specific card in the following questions.
> +
> +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.
> +
> +endif # NET_VENDOR_MUCSE
> +
> diff --git a/drivers/net/ethernet/mucse/Makefile b/drivers/net/ethernet/mucse/Makefile
> new file mode 100644
> index 000000000000..675173fa05f7
> --- /dev/null
> +++ b/drivers/net/ethernet/mucse/Makefile
> @@ -0,0 +1,7 @@
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright(c) 2020 - 2025 MUCSE Corporation.
> +#
> +# Makefile for the MUCSE(R) network device drivers
> +#
> +
> +obj-$(CONFIG_MGBE) += rnpgbe/
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/Makefile b/drivers/net/ethernet/mucse/rnpgbe/Makefile
> new file mode 100644
> index 000000000000..9df536f0d04c
> --- /dev/null
> +++ b/drivers/net/ethernet/mucse/rnpgbe/Makefile
> @@ -0,0 +1,8 @@
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright(c) 2020 - 2025 MUCSE Corporation.
> +#
> +# Makefile for the MUCSE(R) 1GbE PCI Express ethernet driver
> +#
> +
> +obj-$(CONFIG_MGBE) += rnpgbe.o
> +rnpgbe-objs := rnpgbe_main.o
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> new file mode 100644
> index 000000000000..64b2c093bc6e
> --- /dev/null
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright(c) 2020 - 2025 Mucse Corporation. */
> +
> +#ifndef _RNPGBE_H
> +#define _RNPGBE_H
> +
> +enum rnpgbe_boards {
> +       board_n500,
> +       board_n210,
> +       board_n210L,
> +};
> +
> +struct mucse {
> +       struct net_device *netdev;
> +       struct pci_dev *pdev;
> +};
> +
> +/* Device IDs */
> +#define PCI_VENDOR_ID_MUCSE 0x8848
> +#define PCI_DEVICE_ID_N500_QUAD_PORT 0x8308
> +#define PCI_DEVICE_ID_N500_DUAL_PORT 0x8318
> +#define PCI_DEVICE_ID_N210 0x8208
> +#define PCI_DEVICE_ID_N210L 0x820a
> +#endif /* _RNPGBE_H */
> 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..b4a9c5c66af6
> --- /dev/null
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> @@ -0,0 +1,120 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright(c) 2020 - 2025 Mucse Corporation. */
> +
> +#include <linux/types.h>
> +#include <linux/module.h>
> +#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_n210L},
> +       /* 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 on failure
> + **/
> +static int rnpgbe_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> +       int err;
> +
> +       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_dma;
> +       }
> +
> +       err = pci_request_mem_regions(pdev, rnpgbe_driver_name);
> +       if (err) {
> +               dev_err(&pdev->dev,
> +                       "pci_request_selected_regions failed 0x%x\n", err);
> +               goto err_dma;
> +       }
> +
> +       pci_set_master(pdev);
> +       pci_save_state(pdev);
Don't you need to check the return value of this?

Best regards,
Parthiban V
> +
> +       return 0;
> +err_dma:
> +       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);
> +}
> +
> +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@mucse.com>");
> +MODULE_DESCRIPTION("Mucse(R) 1 Gigabit PCI Express Network Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.25.1
> 
> 


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net-next v7 3/5] net: rnpgbe: Add basic mbx ops support
  2025-08-22  2:34 ` [PATCH net-next v7 3/5] net: rnpgbe: Add basic mbx ops support Dong Yibo
@ 2025-08-22  4:41   ` Parthiban.Veerasooran
  2025-08-22  5:25     ` Yibo Dong
  0 siblings, 1 reply; 34+ messages in thread
From: Parthiban.Veerasooran @ 2025-08-22  4:41 UTC (permalink / raw)
  To: dong100, andrew+netdev, davem, edumazet, kuba, pabeni, horms,
	corbet, gur.stavi, maddy, mpe, danishanwar, lee, gongfan1,
	lorenzo, geert+renesas, lukas.bulwahn, alexanderduyck,
	richardcochran, kees, gustavoars
  Cc: netdev, linux-doc, linux-kernel, linux-hardening

On 22/08/25 8:04 am, Dong Yibo wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Initialize basic mbx function.
> 
> Signed-off-by: Dong Yibo <dong100@mucse.com>
> ---
>   drivers/net/ethernet/mucse/rnpgbe/Makefile    |   3 +-
>   drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h    |  17 +
>   .../net/ethernet/mucse/rnpgbe/rnpgbe_chip.c   |   3 +
>   .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c    | 395 ++++++++++++++++++
>   .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h    |  25 ++
>   5 files changed, 442 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
>   create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h
> 
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/Makefile b/drivers/net/ethernet/mucse/rnpgbe/Makefile
> index 42c359f459d9..5fc878ada4b1 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/Makefile
> +++ b/drivers/net/ethernet/mucse/rnpgbe/Makefile
> @@ -6,4 +6,5 @@
> 
>   obj-$(CONFIG_MGBE) += rnpgbe.o
>   rnpgbe-objs := rnpgbe_main.o\
> -              rnpgbe_chip.o
> +              rnpgbe_chip.o\
> +              rnpgbe_mbx.o
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> index 9a86e67d6395..67e28a4667e7 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> @@ -5,6 +5,7 @@
>   #define _RNPGBE_H
> 
>   #include <linux/types.h>
> +#include <linux/mutex.h>
> 
>   extern const struct rnpgbe_info rnpgbe_n500_info;
>   extern const struct rnpgbe_info rnpgbe_n210_info;
> @@ -23,7 +24,23 @@ enum rnpgbe_hw_type {
>          rnpgbe_hw_unknown
>   };
> 
> +struct mucse_mbx_stats {
> +       u32 msgs_tx;
> +       u32 msgs_rx;
> +       u32 acks;
> +       u32 reqs;
> +};
> +
>   struct mucse_mbx_info {
> +       struct mucse_mbx_stats stats;
> +       u32 timeout;
> +       u32 usec_delay;
> +       u16 size;
> +       u16 fw_req;
> +       u16 fw_ack;
> +       /* lock for only one use mbx */
> +       struct mutex lock;
> +       bool irq_enabled;
>          /* fw <--> pf mbx */
>          u32 fw_pf_shm_base;
>          u32 pf2fw_mbox_ctrl;
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
> index 179621ea09f3..f38daef752a3 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
> @@ -1,8 +1,11 @@
>   // SPDX-License-Identifier: GPL-2.0
>   /* Copyright(c) 2020 - 2025 Mucse Corporation. */
> 
> +#include <linux/string.h>
> +
>   #include "rnpgbe.h"
>   #include "rnpgbe_hw.h"
> +#include "rnpgbe_mbx.h"
> 
>   /**
>    * rnpgbe_init_common - Setup common attribute
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
> new file mode 100644
> index 000000000000..e02563b994c2
> --- /dev/null
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
> @@ -0,0 +1,395 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright(c) 2022 - 2025 Mucse Corporation. */
> +
> +#include <linux/pci.h>
> +#include <linux/errno.h>
> +#include <linux/delay.h>
> +#include <linux/iopoll.h>
> +
> +#include "rnpgbe.h"
> +#include "rnpgbe_mbx.h"
> +#include "rnpgbe_hw.h"
> +
> +/**
> + * mbx_data_rd32  - Reads reg with base mbx->fw_pf_shm_base
> + * @mbx: pointer to the MBX structure
> + * @reg: register offset
> + *
> + * @return: register value
> + **/
> +static u32 mbx_data_rd32(struct mucse_mbx_info *mbx, u32 reg)
> +{
> +       struct mucse_hw *hw = container_of(mbx, struct mucse_hw, mbx);
> +
> +       return readl(hw->hw_addr + mbx->fw_pf_shm_base + reg);
> +}
> +
> +/**
> + * mbx_data_wr32  - Writes value to reg with base mbx->fw_pf_shm_base
> + * @mbx: pointer to the MBX structure
> + * @reg: register offset
> + * @value: value to be written
> + *
> + **/
> +static void mbx_data_wr32(struct mucse_mbx_info *mbx, u32 reg, u32 value)
> +{
> +       struct mucse_hw *hw = container_of(mbx, struct mucse_hw, mbx);
> +
> +       writel(value, hw->hw_addr + mbx->fw_pf_shm_base + reg);
> +}
> +
> +/**
> + * mbx_ctrl_rd32  - Reads reg with base mbx->fw2pf_mbox_vec
> + * @mbx: pointer to the MBX structure
> + * @reg: register offset
> + *
> + * @return: register value
> + **/
> +static u32 mbx_ctrl_rd32(struct mucse_mbx_info *mbx, u32 reg)
> +{
> +       struct mucse_hw *hw = container_of(mbx, struct mucse_hw, mbx);
> +
> +       return readl(hw->hw_addr + mbx->fw2pf_mbox_vec + reg);
> +}
> +
> +/**
> + * mbx_ctrl_wr32  - Writes value to reg with base mbx->fw2pf_mbox_vec
> + * @mbx: pointer to the MBX structure
> + * @reg: register offset
> + * @value: value to be written
> + *
> + **/
> +static void mbx_ctrl_wr32(struct mucse_mbx_info *mbx, u32 reg, u32 value)
> +{
> +       struct mucse_hw *hw = container_of(mbx, struct mucse_hw, mbx);
> +
> +       writel(value, hw->hw_addr + mbx->fw2pf_mbox_vec + reg);
> +}
> +
> +/**
> + * mucse_mbx_get_fwreq - Read fw req from reg
> + * @mbx: pointer to the mbx structure
> + *
> + * @return: the req value
> + **/
> +static u16 mucse_mbx_get_fwreq(struct mucse_mbx_info *mbx)
> +{
> +       return mbx_data_rd32(mbx, MBX_FW2PF_COUNTER) & GENMASK_U32(15, 0);
> +}
> +
> +/**
> + * mucse_mbx_get_fwack - Read fw ack from reg
> + * @mbx: pointer to the MBX structure
> + *
> + * @return: the ack value
> + **/
> +static u16 mucse_mbx_get_fwack(struct mucse_mbx_info *mbx)
> +{
> +       return (mbx_data_rd32(mbx, MBX_FW2PF_COUNTER) >> 16);
> +}
> +
> +/**
> + * mucse_mbx_inc_pf_req - Increase req
> + * @hw: pointer to the HW structure
> + *
> + * mucse_mbx_inc_pf_req read pf_req from hw, then write
> + * new value back after increase
> + **/
> +static void mucse_mbx_inc_pf_req(struct mucse_hw *hw)
> +{
> +       struct mucse_mbx_info *mbx = &hw->mbx;
> +       u16 req;
> +       u32 v;
> +
> +       v = mbx_data_rd32(mbx, MBX_PF2FW_COUNTER);
> +       req = (v & GENMASK_U32(15, 0));
> +       req++;
> +       v &= GENMASK_U32(31, 16);
> +       v |= req;
> +       mbx_data_wr32(mbx, MBX_PF2FW_COUNTER, v);
> +       hw->mbx.stats.msgs_tx++;
> +}
> +
> +/**
> + * mucse_mbx_inc_pf_ack - Increase ack
> + * @hw: pointer to the HW structure
> + *
> + * mucse_mbx_inc_pf_ack read pf_ack from hw, then write
> + * new value back after increase
> + **/
> +static void mucse_mbx_inc_pf_ack(struct mucse_hw *hw)
> +{
> +       struct mucse_mbx_info *mbx = &hw->mbx;
> +       u16 ack;
> +       u32 v;
> +
> +       v = mbx_data_rd32(mbx, MBX_PF2FW_COUNTER);
> +       ack = (v >> 16) & GENMASK_U32(15, 0);
> +       ack++;
> +       v &= GENMASK_U32(15, 0);
> +       v |= (ack << 16);
> +       mbx_data_wr32(mbx, MBX_PF2FW_COUNTER, v);
> +       hw->mbx.stats.msgs_rx++;
> +}
> +
> +/**
> + * mucse_check_for_msg_pf - Check to see if the fw has sent mail
> + * @hw: pointer to the HW structure
> + *
> + * @return: 0 if the fw has set the Status bit or else
> + * -EIO
> + **/
> +static int mucse_check_for_msg_pf(struct mucse_hw *hw)
> +{
> +       struct mucse_mbx_info *mbx = &hw->mbx;
> +       u16 hw_req_count = 0;
I don't think you need to assign 0 here as this variable is updated in 
the next line.

Best regards,
Parthiban V
> +
> +       hw_req_count = mucse_mbx_get_fwreq(mbx);
> +       /* chip's register is reset to 0 when rc send reset
> +        * mbx command. This causes 'hw_req_count != hw->mbx.fw_req'
> +        * be TRUE before fw really reply. Driver must wait fw reset
> +        * done reply before using chip, we must check no-zero.
> +        **/
> +       if (hw_req_count != 0 && hw_req_count != hw->mbx.fw_req) {
> +               hw->mbx.stats.reqs++;
> +               return 0;
> +       }
> +
> +       return -EIO;
> +}
> +
> +/**
> + * mucse_poll_for_msg - Wait for message notification
> + * @hw: pointer to the HW structure
> + *
> + * @return: 0 on success, negative on failure
> + **/
> +static int mucse_poll_for_msg(struct mucse_hw *hw)
> +{
> +       struct mucse_mbx_info *mbx = &hw->mbx;
> +       int countdown = mbx->timeout;
> +       int val;
> +
> +       return read_poll_timeout(mucse_check_for_msg_pf,
> +                                val, val == 0, mbx->usec_delay,
> +                                countdown * mbx->usec_delay,
> +                                false, hw);
> +}
> +
> +/**
> + * mucse_check_for_ack_pf - Check to see if the VF has ACKed
> + * @hw: pointer to the HW structure
> + *
> + * @return: 0 if the fw has set the Status bit or else
> + * -EIO
> + **/
> +static int mucse_check_for_ack_pf(struct mucse_hw *hw)
> +{
> +       struct mucse_mbx_info *mbx = &hw->mbx;
> +       u16 hw_fw_ack;
> +
> +       hw_fw_ack = mucse_mbx_get_fwack(mbx);
> +       /* chip's register is reset to 0 when rc send reset
> +        * mbx command. This causes 'hw_fw_ack != hw->mbx.fw_ack'
> +        * be TRUE before fw really reply. Driver must wait fw reset
> +        * done reply before using chip, we must check no-zero.
> +        **/
> +       if (hw_fw_ack != 0 && hw_fw_ack != hw->mbx.fw_ack) {
> +               hw->mbx.stats.acks++;
> +               return 0;
> +       }
> +
> +       return -EIO;
> +}
> +
> +/**
> + * mucse_poll_for_ack - Wait for message acknowledgment
> + * @hw: pointer to the HW structure
> + *
> + * @return: 0 if it successfully received a message acknowledgment
> + **/
> +static int mucse_poll_for_ack(struct mucse_hw *hw)
> +{
> +       struct mucse_mbx_info *mbx = &hw->mbx;
> +       int countdown = mbx->timeout;
> +       int val;
> +
> +       return read_poll_timeout(mucse_check_for_ack_pf,
> +                                val, val == 0, mbx->usec_delay,
> +                                countdown * mbx->usec_delay,
> +                                false, hw);
> +}
> +
> +/**
> + * mucse_obtain_mbx_lock_pf - Obtain mailbox lock
> + * @hw: pointer to the HW structure
> + *
> + * This function maybe used in an irq handler.
> + *
> + * @return: 0 if we obtained the mailbox lock
> + **/
> +static int mucse_obtain_mbx_lock_pf(struct mucse_hw *hw)
> +{
> +       struct mucse_mbx_info *mbx = &hw->mbx;
> +       int try_cnt = 5000;
> +       u32 reg;
> +
> +       reg = PF2FW_MBOX_CTRL(mbx);
> +       while (try_cnt-- > 0) {
> +               mbx_ctrl_wr32(mbx, reg, MBOX_PF_HOLD);
> +               /* force write back before check */
> +               wmb();
> +               if (mbx_ctrl_rd32(mbx, reg) & MBOX_PF_HOLD)
> +                       return 0;
> +               udelay(100);
> +       }
> +       return -EIO;
> +}
> +
> +/**
> + * mucse_read_mbx_pf - Read a message from the mailbox
> + * @hw: pointer to the HW structure
> + * @msg: the message buffer
> + * @size: length of buffer
> + *
> + * This function copies a message from the mailbox buffer to the caller's
> + * memory buffer. The presumption is that the caller knows that there was
> + * a message due to a fw request so no polling for message is needed.
> + *
> + * @return: 0 on success, negative on failure
> + **/
> +static int mucse_read_mbx_pf(struct mucse_hw *hw, u32 *msg, u16 size)
> +{
> +       struct mucse_mbx_info *mbx = &hw->mbx;
> +       int size_inwords = size / 4;
> +       u32 ctrl_reg;
> +       int ret;
> +       int i;
> +
> +       ctrl_reg = PF2FW_MBOX_CTRL(mbx);
> +
> +       ret = mucse_obtain_mbx_lock_pf(hw);
> +       if (ret)
> +               return ret;
> +       for (i = 0; i < size_inwords; i++)
> +               msg[i] = mbx_data_rd32(mbx, MBX_FW_PF_SHM_DATA + 4 * i);
> +       /* Hw need write data_reg at last */
> +       mbx_data_wr32(mbx, MBX_FW_PF_SHM_DATA, 0);
> +       hw->mbx.fw_req = mucse_mbx_get_fwreq(mbx);
> +       mucse_mbx_inc_pf_ack(hw);
> +       mbx_ctrl_wr32(mbx, ctrl_reg, 0);
> +
> +       return 0;
> +}
> +
> +/**
> + * mucse_read_posted_mbx - Wait for message notification and receive message
> + * @hw: pointer to the HW structure
> + * @msg: the message buffer
> + * @size: length of buffer
> + *
> + * @return: 0 if it successfully received a message notification and
> + * copied it into the receive buffer.
> + **/
> +int mucse_read_posted_mbx(struct mucse_hw *hw, u32 *msg, u16 size)
> +{
> +       int ret;
> +
> +       ret = mucse_poll_for_msg(hw);
> +       if (ret)
> +               return ret;
> +
> +       return mucse_read_mbx_pf(hw, msg, size);
> +}
> +
> +/**
> + * mucse_mbx_reset - Reset mbx info, sync info from regs
> + * @hw: pointer to the HW structure
> + *
> + * This function reset all mbx variables to default.
> + **/
> +static void mucse_mbx_reset(struct mucse_hw *hw)
> +{
> +       struct mucse_mbx_info *mbx = &hw->mbx;
> +       u32 v;
> +
> +       v = mbx_data_rd32(mbx, MBX_FW2PF_COUNTER);
> +       hw->mbx.fw_req = v & GENMASK_U32(15, 0);
> +       hw->mbx.fw_ack = (v >> 16) & GENMASK_U32(15, 0);
> +       mbx_ctrl_wr32(mbx, PF2FW_MBOX_CTRL(mbx), 0);
> +       mbx_ctrl_wr32(mbx, FW_PF_MBOX_MASK(mbx), GENMASK_U32(31, 16));
> +}
> +
> +/**
> + * mucse_init_mbx_params_pf - Set initial values for pf mailbox
> + * @hw: pointer to the HW structure
> + *
> + * Initializes the hw->mbx struct to correct values for pf mailbox
> + */
> +void mucse_init_mbx_params_pf(struct mucse_hw *hw)
> +{
> +       struct mucse_mbx_info *mbx = &hw->mbx;
> +
> +       mbx->usec_delay = 100;
> +       mbx->timeout = (4 * USEC_PER_SEC) / mbx->usec_delay;
> +       mbx->stats.msgs_tx = 0;
> +       mbx->stats.msgs_rx = 0;
> +       mbx->stats.reqs = 0;
> +       mbx->stats.acks = 0;
> +       mbx->size = MUCSE_MAILBOX_BYTES;
> +       mutex_init(&mbx->lock);
> +       mucse_mbx_reset(hw);
> +}
> +
> +/**
> + * mucse_write_mbx_pf - Place a message in the mailbox
> + * @hw: pointer to the HW structure
> + * @msg: the message buffer
> + * @size: length of buffer
> + *
> + * This function maybe used in an irq handler.
> + *
> + * @return: 0 if it successfully copied message into the buffer
> + **/
> +int mucse_write_mbx_pf(struct mucse_hw *hw, u32 *msg, u16 size)
> +{
> +       struct mucse_mbx_info *mbx = &hw->mbx;
> +       int size_inwords = size / 4;
> +       u32 ctrl_reg;
> +       int ret;
> +       int i;
> +
> +       ctrl_reg = PF2FW_MBOX_CTRL(mbx);
> +       ret = mucse_obtain_mbx_lock_pf(hw);
> +       if (ret)
> +               return ret;
> +
> +       for (i = 0; i < size_inwords; i++)
> +               mbx_data_wr32(mbx, MBX_FW_PF_SHM_DATA + i * 4, msg[i]);
> +
> +       /* flush msg and acks as we are overwriting the message buffer */
> +       hw->mbx.fw_ack = mucse_mbx_get_fwack(mbx);
> +       mucse_mbx_inc_pf_req(hw);
> +       mbx_ctrl_wr32(mbx, ctrl_reg, MBOX_CTRL_REQ);
> +
> +       return 0;
> +}
> +
> +/**
> + * mucse_write_posted_mbx - Write a message to the mailbox, wait for ack
> + * @hw: pointer to the HW structure
> + * @msg: the message buffer
> + * @size: length of buffer
> + *
> + * @return: 0 if it successfully copied message into the buffer and
> + * received an ack to that message within delay * timeout period
> + **/
> +int mucse_write_posted_mbx(struct mucse_hw *hw, u32 *msg, u16 size)
> +{
> +       int ret;
> +
> +       ret = mucse_write_mbx_pf(hw, msg, size);
> +       if (ret)
> +               return ret;
> +       return mucse_poll_for_ack(hw);
> +}
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h
> new file mode 100644
> index 000000000000..110c1ee025ba
> --- /dev/null
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h
> @@ -0,0 +1,25 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright(c) 2020 - 2025 Mucse Corporation. */
> +
> +#ifndef _RNPGBE_MBX_H
> +#define _RNPGBE_MBX_H
> +
> +#include "rnpgbe.h"
> +
> +#define MUCSE_MAILBOX_BYTES 56
> +#define MBX_FW2PF_COUNTER 0
> +#define MBX_PF2FW_COUNTER 4
> +#define MBX_FW_PF_SHM_DATA 8
> +#define FW2PF_MBOX_VEC 0
> +#define PF2FW_MBOX_CTRL(mbx) ((mbx)->pf2fw_mbox_ctrl)
> +#define FW_PF_MBOX_MASK(mbx) ((mbx)->fw_pf_mbox_mask)
> +#define MBOX_CTRL_REQ BIT(0)
> +#define MBOX_PF_HOLD BIT(3)
> +#define MBOX_IRQ_EN 0
> +#define MBOX_IRQ_DISABLE 1
> +
> +int mucse_write_mbx_pf(struct mucse_hw *hw, u32 *msg, u16 size);
> +int mucse_write_posted_mbx(struct mucse_hw *hw, u32 *msg, u16 size);
> +void mucse_init_mbx_params_pf(struct mucse_hw *hw);
> +int mucse_read_posted_mbx(struct mucse_hw *hw, u32 *msg, u16 size);
> +#endif /* _RNPGBE_MBX_H */
> --
> 2.25.1
> 


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net-next v7 4/5] net: rnpgbe: Add basic mbx_fw support
  2025-08-22  2:34 ` [PATCH net-next v7 4/5] net: rnpgbe: Add basic mbx_fw support Dong Yibo
@ 2025-08-22  4:49   ` Parthiban.Veerasooran
  2025-08-22  5:37     ` Yibo Dong
  2025-08-22 14:43   ` Andrew Lunn
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 34+ messages in thread
From: Parthiban.Veerasooran @ 2025-08-22  4:49 UTC (permalink / raw)
  To: dong100, andrew+netdev, davem, edumazet, kuba, pabeni, horms,
	corbet, gur.stavi, maddy, mpe, danishanwar, lee, gongfan1,
	lorenzo, geert+renesas, lukas.bulwahn, alexanderduyck,
	richardcochran, kees, gustavoars
  Cc: netdev, linux-doc, linux-kernel, linux-hardening

On 22/08/25 8:04 am, Dong Yibo wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Initialize basic mbx_fw ops, such as get_capability, reset phy
> and so on.
> 
> Signed-off-by: Dong Yibo <dong100@mucse.com>
> ---
>   drivers/net/ethernet/mucse/rnpgbe/Makefile    |   3 +-
>   drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h    |   1 +
>   .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c | 333 ++++++++++++++++++
>   .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h | 152 ++++++++
>   4 files changed, 488 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
>   create mode 100644 drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h
> 
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/Makefile b/drivers/net/ethernet/mucse/rnpgbe/Makefile
> index 5fc878ada4b1..de8bcb7772ab 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/Makefile
> +++ b/drivers/net/ethernet/mucse/rnpgbe/Makefile
> @@ -7,4 +7,5 @@
>   obj-$(CONFIG_MGBE) += rnpgbe.o
>   rnpgbe-objs := rnpgbe_main.o\
>                 rnpgbe_chip.o\
> -              rnpgbe_mbx.o
> +              rnpgbe_mbx.o\
> +              rnpgbe_mbx_fw.o
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> index 67e28a4667e7..a32419a34d75 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> @@ -52,6 +52,7 @@ struct mucse_hw {
>          void __iomem *hw_addr;
>          struct pci_dev *pdev;
>          enum rnpgbe_hw_type hw_type;
> +       u8 pfvfnum;
>          struct mucse_mbx_info mbx;
>   };
> 
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
> new file mode 100644
> index 000000000000..84570763cf79
> --- /dev/null
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.c
> @@ -0,0 +1,333 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright(c) 2020 - 2025 Mucse Corporation. */
> +
> +#include <linux/pci.h>
> +#include <linux/if_ether.h>
> +
> +#include "rnpgbe.h"
> +#include "rnpgbe_hw.h"
> +#include "rnpgbe_mbx.h"
> +#include "rnpgbe_mbx_fw.h"
> +
> +/**
> + * mucse_fw_send_cmd_wait - Send cmd req and wait for response
> + * @hw: pointer to the HW structure
> + * @req: pointer to the cmd req structure
> + * @reply: pointer to the fw reply structure
> + *
> + * mucse_fw_send_cmd_wait sends req to pf-fw mailbox and wait
> + * reply from fw.
> + *
> + * @return: 0 on success, negative on failure
> + **/
> +static int mucse_fw_send_cmd_wait(struct mucse_hw *hw,
> +                                 struct mbx_fw_cmd_req *req,
> +                                 struct mbx_fw_cmd_reply *reply)
> +{
> +       int len = le16_to_cpu(req->datalen);
> +       int retry_cnt = 3;
> +       int err;
> +
> +       err = mutex_lock_interruptible(&hw->mbx.lock);
> +       if (err)
> +               return err;
> +       err = mucse_write_posted_mbx(hw, (u32 *)req, len);
> +       if (err)
> +               goto out;
> +       do {
> +               err = mucse_read_posted_mbx(hw, (u32 *)reply,
> +                                           sizeof(*reply));
> +               if (err)
> +                       goto out;
> +               /* mucse_write_posted_mbx return 0 means fw has
> +                * received request, wait for the expect opcode
> +                * reply with 'retry_cnt' times.
> +                */
> +       } while (--retry_cnt >= 0 && reply->opcode != req->opcode);
> +out:
> +       mutex_unlock(&hw->mbx.lock);
> +       if (!err && retry_cnt < 0)
> +               return -ETIMEDOUT;
> +       if (!err && reply->error_code)
> +               return -EIO;
> +       return err;
> +}
> +
> +/**
> + * build_phy_abilities_req - build req with get_phy_ability opcode
> + * @req: pointer to the cmd req structure
> + **/
> +static void build_phy_abilities_req(struct mbx_fw_cmd_req *req)
> +{
> +       req->flags = 0;
> +       req->opcode = cpu_to_le16(GET_PHY_ABILITY);
> +       req->datalen = cpu_to_le16(MBX_REQ_HDR_LEN);
> +       req->reply_lo = 0;
> +       req->reply_hi = 0;
> +}
> +
> +/**
> + * mucse_fw_get_capability - Get hw abilities from fw
> + * @hw: pointer to the HW structure
> + * @abil: pointer to the hw_abilities structure
> + *
> + * mucse_fw_get_capability tries to get hw abilities from
> + * hw.
> + *
> + * @return: 0 on success, negative on failure
> + **/
> +static int mucse_fw_get_capability(struct mucse_hw *hw,
> +                                  struct hw_abilities *abil)
> +{
> +       struct mbx_fw_cmd_reply reply = {};
> +       struct mbx_fw_cmd_req req = {};
> +       int err;
> +
> +       build_phy_abilities_req(&req);
> +       err = mucse_fw_send_cmd_wait(hw, &req, &reply);
> +       if (!err)
> +               memcpy(abil, &reply.hw_abilities, sizeof(*abil));
> +       return err;
> +}
> +
> +/**
> + * mucse_mbx_get_capability - Get hw abilities from fw
> + * @hw: pointer to the HW structure
> + *
> + * mucse_mbx_get_capability tries to get capabities from
> + * hw. Many retrys will do if it is failed.
> + *
> + * @return: 0 on success, negative on failure
> + **/
> +int mucse_mbx_get_capability(struct mucse_hw *hw)
> +{
> +       struct hw_abilities ability = {};
> +       int try_cnt = 3;
> +       int err = -EIO;
Here too you no need to assign -EIO as it is updated in the while.

Best regards,
Parthiban V
> +
> +       while (try_cnt--) {
> +               err = mucse_fw_get_capability(hw, &ability);
> +               if (err)
> +                       continue;
> +               hw->pfvfnum = le16_to_cpu(ability.pfnum) & GENMASK_U16(7, 0);
> +               return 0;
> +       }
> +       return err;
> +}
> +
> +/**
> + * mbx_cookie_zalloc - Alloc a cookie structure
> + * @priv_len: private length for this cookie
> + *
> + * @return: cookie structure on success
> + **/
> +static struct mbx_req_cookie *mbx_cookie_zalloc(int priv_len)
> +{
> +       struct mbx_req_cookie *cookie;
> +
> +       cookie = kzalloc(struct_size(cookie, priv, priv_len), GFP_KERNEL);
> +       if (cookie) {
> +               cookie->timeout_jiffies = 30 * HZ;
> +               cookie->magic = COOKIE_MAGIC;
> +               cookie->priv_len = priv_len;
> +       }
> +       return cookie;
> +}
> +
> +/**
> + * mucse_mbx_fw_post_req - Posts a mbx req to firmware and wait reply
> + * @hw: pointer to the HW structure
> + * @req: pointer to the cmd req structure
> + * @cookie: pointer to the req cookie
> + *
> + * mucse_mbx_fw_post_req posts a mbx req to firmware and wait for the
> + * reply. cookie->wait will be set in irq handler.
> + *
> + * @return: 0 on success, negative on failure
> + **/
> +static int mucse_mbx_fw_post_req(struct mucse_hw *hw,
> +                                struct mbx_fw_cmd_req *req,
> +                                struct mbx_req_cookie *cookie)
> +{
> +       int len = le16_to_cpu(req->datalen);
> +       int err;
> +
> +       cookie->errcode = 0;
> +       cookie->done = 0;
> +       init_waitqueue_head(&cookie->wait);
> +       err = mutex_lock_interruptible(&hw->mbx.lock);
> +       if (err)
> +               return err;
> +       err = mucse_write_mbx_pf(hw, (u32 *)req, len);
> +       if (err)
> +               goto out;
> +       /* if write succeeds, we must wait for firmware response or
> +        * timeout to avoid using the already freed cookie->wait
> +        */
> +       err = wait_event_timeout(cookie->wait,
> +                                cookie->done == 1,
> +                                cookie->timeout_jiffies);
> +
> +       if (!err)
> +               err = -ETIMEDOUT;
> +       else
> +               err = 0;
> +       if (!err && cookie->errcode)
> +               err = cookie->errcode;
> +out:
> +       mutex_unlock(&hw->mbx.lock);
> +       return err;
> +}
> +
> +/**
> + * build_ifinsmod - build req with insmod opcode
> + * @req: pointer to the cmd req structure
> + * @status: true for insmod, false for rmmod
> + **/
> +static void build_ifinsmod(struct mbx_fw_cmd_req *req,
> +                          int status)
> +{
> +       req->flags = 0;
> +       req->opcode = cpu_to_le16(DRIVER_INSMOD);
> +       req->datalen = cpu_to_le16(sizeof(req->ifinsmod) +
> +                                  MBX_REQ_HDR_LEN);
> +       req->cookie = NULL;
> +       req->reply_lo = 0;
> +       req->reply_hi = 0;
> +#define FIXED_VERSION 0xFFFFFFFF
> +       req->ifinsmod.version = cpu_to_le32(FIXED_VERSION);
> +       req->ifinsmod.status = cpu_to_le32(status);
> +}
> +
> +/**
> + * mucse_mbx_ifinsmod - Echo driver insmod status to hw
> + * @hw: pointer to the HW structure
> + * @status: true for insmod, false for rmmod
> + *
> + * @return: 0 on success, negative on failure
> + **/
> +int mucse_mbx_ifinsmod(struct mucse_hw *hw, int status)
> +{
> +       struct mbx_fw_cmd_req req = {};
> +       int len;
> +       int err;
> +
> +       build_ifinsmod(&req, status);
> +       len = le16_to_cpu(req.datalen);
> +       err = mutex_lock_interruptible(&hw->mbx.lock);
> +       if (err)
> +               return err;
> +
> +       if (status) {
> +               err = mucse_write_posted_mbx(hw, (u32 *)&req,
> +                                            len);
> +       } else {
> +               err = mucse_write_mbx_pf(hw, (u32 *)&req,
> +                                        len);
> +       }
> +
> +       mutex_unlock(&hw->mbx.lock);
> +       return err;
> +}
> +
> +/**
> + * build_reset_phy_req - build req with reset_phy opcode
> + * @req: pointer to the cmd req structure
> + * @cookie: pointer of cookie for this cmd
> + **/
> +static void build_reset_phy_req(struct mbx_fw_cmd_req *req,
> +                               void *cookie)
> +{
> +       req->flags = 0;
> +       req->opcode = cpu_to_le16(RESET_PHY);
> +       req->datalen = cpu_to_le16(MBX_REQ_HDR_LEN);
> +       req->reply_lo = 0;
> +       req->reply_hi = 0;
> +       req->cookie = cookie;
> +}
> +
> +/**
> + * mucse_mbx_fw_reset_phy - Posts a mbx req to reset hw
> + * @hw: pointer to the HW structure
> + *
> + * mucse_mbx_fw_reset_phy posts a mbx req to firmware to reset hw.
> + * It uses mucse_fw_send_cmd_wait if no irq, and mucse_mbx_fw_post_req
> + * if other irq is registered.
> + *
> + * @return: 0 on success, negative on failure
> + **/
> +int mucse_mbx_fw_reset_phy(struct mucse_hw *hw)
> +{
> +       struct mbx_fw_cmd_reply reply = {};
> +       struct mbx_fw_cmd_req req = {};
> +       int ret;
> +
> +       if (hw->mbx.irq_enabled) {
> +               struct mbx_req_cookie *cookie = mbx_cookie_zalloc(0);
> +
> +               if (!cookie)
> +                       return -ENOMEM;
> +
> +               build_reset_phy_req(&req, cookie);
> +               ret = mucse_mbx_fw_post_req(hw, &req, cookie);
> +               kfree(cookie);
> +               return ret;
> +       }
> +
> +       build_reset_phy_req(&req, &req);
> +       return mucse_fw_send_cmd_wait(hw, &req, &reply);
> +}
> +
> +/**
> + * build_get_macaddress_req - build req with get_mac opcode
> + * @req: pointer to the cmd req structure
> + * @port_mask: port valid for this cmd
> + * @pfvfnum: pfvfnum for this cmd
> + * @cookie: pointer of cookie for this cmd
> + **/
> +static void build_get_macaddress_req(struct mbx_fw_cmd_req *req,
> +                                    int port_mask, int pfvfnum,
> +                                    void *cookie)
> +{
> +       req->flags = 0;
> +       req->opcode = cpu_to_le16(GET_MAC_ADDRES);
> +       req->datalen = cpu_to_le16(sizeof(req->get_mac_addr) +
> +                                  MBX_REQ_HDR_LEN);
> +       req->cookie = cookie;
> +       req->reply_lo = 0;
> +       req->reply_hi = 0;
> +       req->get_mac_addr.port_mask = cpu_to_le32(port_mask);
> +       req->get_mac_addr.pfvf_num = cpu_to_le32(pfvfnum);
> +}
> +
> +/**
> + * mucse_fw_get_macaddr - Posts a mbx req to request macaddr
> + * @hw: pointer to the HW structure
> + * @pfvfnum: index of pf/vf num
> + * @mac_addr: pointer to store mac_addr
> + * @port: port index
> + *
> + * mucse_fw_get_macaddr posts a mbx req to firmware to get mac_addr.
> + * It uses mucse_fw_send_cmd_wait if no irq, and mucse_mbx_fw_post_req
> + * if other irq is registered.
> + *
> + * @return: 0 on success, negative on failure
> + **/
> +int mucse_fw_get_macaddr(struct mucse_hw *hw, int pfvfnum,
> +                        u8 *mac_addr,
> +                        int port)
> +{
> +       struct mbx_fw_cmd_reply reply = {};
> +       struct mbx_fw_cmd_req req = {};
> +       int err;
> +
> +       build_get_macaddress_req(&req, BIT(port), pfvfnum, &req);
> +       err = mucse_fw_send_cmd_wait(hw, &req, &reply);
> +       if (err)
> +               return err;
> +       if (le32_to_cpu(reply.mac_addr.ports) & BIT(port))
> +               memcpy(mac_addr, reply.mac_addr.addrs[port].mac, ETH_ALEN);
> +       else
> +               return -ENODATA;
> +       return 0;
> +}
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h
> new file mode 100644
> index 000000000000..b73238d0e848
> --- /dev/null
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx_fw.h
> @@ -0,0 +1,152 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright(c) 2020 - 2025 Mucse Corporation. */
> +
> +#ifndef _RNPGBE_MBX_FW_H
> +#define _RNPGBE_MBX_FW_H
> +
> +#include <linux/types.h>
> +#include <linux/errno.h>
> +#include <linux/wait.h>
> +
> +#include "rnpgbe.h"
> +
> +#define MBX_REQ_HDR_LEN 24
> +
> +struct mbx_fw_cmd_reply;
> +typedef void (*cookie_cb)(struct mbx_fw_cmd_reply *reply, void *priv);
> +
> +struct mbx_req_cookie {
> +       int magic;
> +#define COOKIE_MAGIC 0xCE
> +       cookie_cb cb;
> +       int timeout_jiffies;
> +       int errcode;
> +       wait_queue_head_t wait;
> +       int done;
> +       int priv_len;
> +       char priv[] __counted_by(priv_len);
> +};
> +
> +enum MUCSE_FW_CMD {
> +       GET_PHY_ABILITY = 0x0601,
> +       GET_MAC_ADDRES = 0x0602,
> +       RESET_PHY = 0x0603,
> +       DRIVER_INSMOD = 0x0803,
> +};
> +
> +struct hw_abilities {
> +       u8 link_stat;
> +       u8 port_mask;
> +       __le32 speed;
> +       __le16 phy_type;
> +       __le16 nic_mode;
> +       __le16 pfnum;
> +       __le32 fw_version;
> +       __le32 axi_mhz;
> +       union {
> +               u8 port_id[4];
> +               __le32 port_ids;
> +       };
> +       __le32 bd_uid;
> +       __le32 phy_id;
> +       __le32 wol_status;
> +       union {
> +               __le32 ext_ability;
> +               struct {
> +                       u32 valid : 1;
> +                       u32 wol_en : 1;
> +                       u32 pci_preset_runtime_en : 1;
> +                       u32 smbus_en : 1;
> +                       u32 ncsi_en : 1;
> +                       u32 rpu_en : 1;
> +                       u32 v2 : 1;
> +                       u32 pxe_en : 1;
> +                       u32 mctp_en : 1;
> +                       u32 yt8614 : 1;
> +                       u32 pci_ext_reset : 1;
> +                       u32 rpu_availble : 1;
> +                       u32 fw_lldp_ability : 1;
> +                       u32 lldp_enabled : 1;
> +                       u32 only_1g : 1;
> +                       u32 force_down_en: 1;
> +               } e_host;
> +       };
> +} __packed;
> +
> +/* FW stores extended ability information in 'ext_ability' as a 32-bit
> + * little-endian value. To make these flags easily accessible in the
> + * kernel (via named 'bitfields' instead of raw bitmask operations),
> + * we use the union's 'e_host' struct, which provides named bits
> + * (e.g., 'wol_en', 'smbus_en')
> + */
> +static inline void ability_update_host_endian(struct hw_abilities *abi)
> +{
> +       u32 host_val = le32_to_cpu(abi->ext_ability);
> +
> +       abi->e_host = *(typeof(abi->e_host) *)&host_val;
> +}
> +
> +#define FLAGS_DD BIT(0)
> +#define FLAGS_ERR BIT(2)
> +
> +struct mbx_fw_cmd_req {
> +       __le16 flags;
> +       __le16 opcode;
> +       __le16 datalen;
> +       __le16 ret_value;
> +       union {
> +               struct {
> +                       __le32 cookie_lo;
> +                       __le32 cookie_hi;
> +               };
> +
> +               void *cookie;
> +       };
> +       __le32 reply_lo;
> +       __le32 reply_hi;
> +       union {
> +               u8 data[32];
> +               struct {
> +                       __le32 version;
> +                       __le32 status;
> +               } ifinsmod;
> +               struct {
> +                       __le32 port_mask;
> +                       __le32 pfvf_num;
> +               } get_mac_addr;
> +       };
> +} __packed;
> +
> +struct mbx_fw_cmd_reply {
> +       __le16 flags;
> +       __le16 opcode;
> +       __le16 error_code;
> +       __le16 datalen;
> +       union {
> +               struct {
> +                       __le32 cookie_lo;
> +                       __le32 cookie_hi;
> +               };
> +               void *cookie;
> +       };
> +       union {
> +               u8 data[40];
> +               struct mac_addr {
> +                       __le32 ports;
> +                       struct _addr {
> +                               /* for macaddr:01:02:03:04:05:06
> +                                * mac-hi=0x01020304 mac-lo=0x05060000
> +                                */
> +                               u8 mac[8];
> +                       } addrs[4];
> +               } mac_addr;
> +               struct hw_abilities hw_abilities;
> +       };
> +} __packed;
> +
> +int mucse_mbx_get_capability(struct mucse_hw *hw);
> +int mucse_mbx_ifinsmod(struct mucse_hw *hw, int status);
> +int mucse_mbx_fw_reset_phy(struct mucse_hw *hw);
> +int mucse_fw_get_macaddr(struct mucse_hw *hw, int pfvfnum,
> +                        u8 *mac_addr, int port);
> +#endif /* _RNPGBE_MBX_FW_H */
> --
> 2.25.1
> 
> 


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net-next v7 1/5] net: rnpgbe: Add build support for rnpgbe
  2025-08-22  4:32   ` Parthiban.Veerasooran
@ 2025-08-22  5:23     ` Yibo Dong
  0 siblings, 0 replies; 34+ messages in thread
From: Yibo Dong @ 2025-08-22  5:23 UTC (permalink / raw)
  To: Parthiban.Veerasooran
  Cc: andrew+netdev, davem, edumazet, kuba, pabeni, horms, corbet,
	gur.stavi, maddy, mpe, danishanwar, lee, gongfan1, lorenzo,
	geert+renesas, lukas.bulwahn, alexanderduyck, richardcochran,
	kees, gustavoars, netdev, linux-doc, linux-kernel,
	linux-hardening

On Fri, Aug 22, 2025 at 04:32:06AM +0000, Parthiban.Veerasooran@microchip.com wrote:
> > +
> > +/**
> > + * 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 on failure
> > + **/
> > +static int rnpgbe_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> > +{
> > +       int err;
> > +
> > +       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_dma;
> > +       }
> > +
> > +       err = pci_request_mem_regions(pdev, rnpgbe_driver_name);
> > +       if (err) {
> > +               dev_err(&pdev->dev,
> > +                       "pci_request_selected_regions failed 0x%x\n", err);
> > +               goto err_dma;
> > +       }
> > +
> > +       pci_set_master(pdev);
> > +       pci_save_state(pdev);
> Don't you need to check the return value of this?
> 
> Best regards,
> Parthiban V

Ok, I will add the check.

Thanks for your feedback.


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net-next v7 3/5] net: rnpgbe: Add basic mbx ops support
  2025-08-22  4:41   ` Parthiban.Veerasooran
@ 2025-08-22  5:25     ` Yibo Dong
  0 siblings, 0 replies; 34+ messages in thread
From: Yibo Dong @ 2025-08-22  5:25 UTC (permalink / raw)
  To: Parthiban.Veerasooran
  Cc: andrew+netdev, davem, edumazet, kuba, pabeni, horms, corbet,
	gur.stavi, maddy, mpe, danishanwar, lee, gongfan1, lorenzo,
	geert+renesas, lukas.bulwahn, alexanderduyck, richardcochran,
	kees, gustavoars, netdev, linux-doc, linux-kernel,
	linux-hardening

On Fri, Aug 22, 2025 at 04:41:48AM +0000, Parthiban.Veerasooran@microchip.com wrote:
> > +
> > +/**
> > + * mucse_check_for_msg_pf - Check to see if the fw has sent mail
> > + * @hw: pointer to the HW structure
> > + *
> > + * @return: 0 if the fw has set the Status bit or else
> > + * -EIO
> > + **/
> > +static int mucse_check_for_msg_pf(struct mucse_hw *hw)
> > +{
> > +       struct mucse_mbx_info *mbx = &hw->mbx;
> > +       u16 hw_req_count = 0;
> I don't think you need to assign 0 here as this variable is updated in 
> the next line.
> 
> Best regards,
> Parthiban V

Got it, I will update this.

> > +
> > +       hw_req_count = mucse_mbx_get_fwreq(mbx);
> > +       /* chip's register is reset to 0 when rc send reset
> > +        * mbx command. This causes 'hw_req_count != hw->mbx.fw_req'
> > +        * be TRUE before fw really reply. Driver must wait fw reset
> > +        * done reply before using chip, we must check no-zero.
> > +        **/
> > +       if (hw_req_count != 0 && hw_req_count != hw->mbx.fw_req) {
> > +               hw->mbx.stats.reqs++;
> > +               return 0;
> > +       }
> > +
> > +       return -EIO;
> > +}
> > +

Thanks for your feedback.


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net-next v7 4/5] net: rnpgbe: Add basic mbx_fw support
  2025-08-22  4:49   ` Parthiban.Veerasooran
@ 2025-08-22  5:37     ` Yibo Dong
  2025-08-22  6:07       ` Parthiban.Veerasooran
  0 siblings, 1 reply; 34+ messages in thread
From: Yibo Dong @ 2025-08-22  5:37 UTC (permalink / raw)
  To: Parthiban.Veerasooran
  Cc: andrew+netdev, davem, edumazet, kuba, pabeni, horms, corbet,
	gur.stavi, maddy, mpe, danishanwar, lee, gongfan1, lorenzo,
	geert+renesas, lukas.bulwahn, alexanderduyck, richardcochran,
	kees, gustavoars, netdev, linux-doc, linux-kernel,
	linux-hardening

On Fri, Aug 22, 2025 at 04:49:44AM +0000, Parthiban.Veerasooran@microchip.com wrote:
> On 22/08/25 8:04 am, Dong Yibo wrote:
> > +/**
> > + * mucse_mbx_get_capability - Get hw abilities from fw
> > + * @hw: pointer to the HW structure
> > + *
> > + * mucse_mbx_get_capability tries to get capabities from
> > + * hw. Many retrys will do if it is failed.
> > + *
> > + * @return: 0 on success, negative on failure
> > + **/
> > +int mucse_mbx_get_capability(struct mucse_hw *hw)
> > +{
> > +       struct hw_abilities ability = {};
> > +       int try_cnt = 3;
> > +       int err = -EIO;
> Here too you no need to assign -EIO as it is updated in the while.
> 
> Best regards,
> Parthiban V
> > +
> > +       while (try_cnt--) {
> > +               err = mucse_fw_get_capability(hw, &ability);
> > +               if (err)
> > +                       continue;
> > +               hw->pfvfnum = le16_to_cpu(ability.pfnum) & GENMASK_U16(7, 0);
> > +               return 0;
> > +       }
> > +       return err;
> > +}
> > +

err is updated because 'try_cnt = 3'. But to the code logic itself, it should
not leave err uninitialized since no guarantee that codes 'whthin while'
run at least once. Right?

Thanks for your feedback.


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net-next v7 4/5] net: rnpgbe: Add basic mbx_fw support
  2025-08-22  5:37     ` Yibo Dong
@ 2025-08-22  6:07       ` Parthiban.Veerasooran
  2025-08-22  6:51         ` Yibo Dong
  0 siblings, 1 reply; 34+ messages in thread
From: Parthiban.Veerasooran @ 2025-08-22  6:07 UTC (permalink / raw)
  To: dong100
  Cc: andrew+netdev, davem, edumazet, kuba, pabeni, horms, corbet,
	gur.stavi, maddy, mpe, danishanwar, lee, gongfan1, lorenzo,
	geert+renesas, lukas.bulwahn, alexanderduyck, richardcochran,
	kees, gustavoars, netdev, linux-doc, linux-kernel,
	linux-hardening

On 22/08/25 11:07 am, Yibo Dong wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Fri, Aug 22, 2025 at 04:49:44AM +0000, Parthiban.Veerasooran@microchip.com wrote:
>> On 22/08/25 8:04 am, Dong Yibo wrote:
>>> +/**
>>> + * mucse_mbx_get_capability - Get hw abilities from fw
>>> + * @hw: pointer to the HW structure
>>> + *
>>> + * mucse_mbx_get_capability tries to get capabities from
>>> + * hw. Many retrys will do if it is failed.
>>> + *
>>> + * @return: 0 on success, negative on failure
>>> + **/
>>> +int mucse_mbx_get_capability(struct mucse_hw *hw)
>>> +{
>>> +       struct hw_abilities ability = {};
>>> +       int try_cnt = 3;
>>> +       int err = -EIO;
>> Here too you no need to assign -EIO as it is updated in the while.
>>
>> Best regards,
>> Parthiban V
>>> +
>>> +       while (try_cnt--) {
>>> +               err = mucse_fw_get_capability(hw, &ability);
>>> +               if (err)
>>> +                       continue;
>>> +               hw->pfvfnum = le16_to_cpu(ability.pfnum) & GENMASK_U16(7, 0);
>>> +               return 0;
>>> +       }
>>> +       return err;
>>> +}
>>> +
> 
> err is updated because 'try_cnt = 3'. But to the code logic itself, it should
> not leave err uninitialized since no guarantee that codes 'whthin while'
> run at least once. Right?
Yes, but 'try_cnt' is hard coded as 3, so the 'while loop' will always 
execute and err will definitely be updated.

So in this case, the check isn’t needed unless try_cnt is being modified 
externally with unknown values, which doesn’t seem to be happening here.

Best regards,
Parthiban V
> 
> Thanks for your feedback.
> 
> 


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net-next v7 4/5] net: rnpgbe: Add basic mbx_fw support
  2025-08-22  6:07       ` Parthiban.Veerasooran
@ 2025-08-22  6:51         ` Yibo Dong
  2025-08-22  8:05           ` Parthiban.Veerasooran
  2025-08-22 14:33           ` Andrew Lunn
  0 siblings, 2 replies; 34+ messages in thread
From: Yibo Dong @ 2025-08-22  6:51 UTC (permalink / raw)
  To: Parthiban.Veerasooran
  Cc: andrew+netdev, davem, edumazet, kuba, pabeni, horms, corbet,
	gur.stavi, maddy, mpe, danishanwar, lee, gongfan1, lorenzo,
	geert+renesas, lukas.bulwahn, alexanderduyck, richardcochran,
	kees, gustavoars, netdev, linux-doc, linux-kernel,
	linux-hardening

On Fri, Aug 22, 2025 at 06:07:51AM +0000, Parthiban.Veerasooran@microchip.com wrote:
> On 22/08/25 11:07 am, Yibo Dong wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On Fri, Aug 22, 2025 at 04:49:44AM +0000, Parthiban.Veerasooran@microchip.com wrote:
> >> On 22/08/25 8:04 am, Dong Yibo wrote:
> >>> +/**
> >>> + * mucse_mbx_get_capability - Get hw abilities from fw
> >>> + * @hw: pointer to the HW structure
> >>> + *
> >>> + * mucse_mbx_get_capability tries to get capabities from
> >>> + * hw. Many retrys will do if it is failed.
> >>> + *
> >>> + * @return: 0 on success, negative on failure
> >>> + **/
> >>> +int mucse_mbx_get_capability(struct mucse_hw *hw)
> >>> +{
> >>> +       struct hw_abilities ability = {};
> >>> +       int try_cnt = 3;
> >>> +       int err = -EIO;
> >> Here too you no need to assign -EIO as it is updated in the while.
> >>
> >> Best regards,
> >> Parthiban V
> >>> +
> >>> +       while (try_cnt--) {
> >>> +               err = mucse_fw_get_capability(hw, &ability);
> >>> +               if (err)
> >>> +                       continue;
> >>> +               hw->pfvfnum = le16_to_cpu(ability.pfnum) & GENMASK_U16(7, 0);
> >>> +               return 0;
> >>> +       }
> >>> +       return err;
> >>> +}
> >>> +
> > 
> > err is updated because 'try_cnt = 3'. But to the code logic itself, it should
> > not leave err uninitialized since no guarantee that codes 'whthin while'
> > run at least once. Right?
> Yes, but 'try_cnt' is hard coded as 3, so the 'while loop' will always 
> execute and err will definitely be updated.
> 
> So in this case, the check isn’t needed unless try_cnt is being modified 
> externally with unknown values, which doesn’t seem to be happening here.
> 
> Best regards,
> Parthiban V
> > 
> > Thanks for your feedback.
> > 
> > 
> 

Is it fine if I add some comment like this?
.....
/* Initialized as a defensive measure to handle edge cases
 * where try_cnt might be modified
 */
 int err = -EIO;
.....

Additionally, keeping this initialization ensures we’ll no need to consider
its impact every time 'try_cnt' is modified (Although this situation is
almost impossible). 

Thanks for your feedback.


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net-next v7 4/5] net: rnpgbe: Add basic mbx_fw support
  2025-08-22  6:51         ` Yibo Dong
@ 2025-08-22  8:05           ` Parthiban.Veerasooran
  2025-08-22  9:04             ` Yibo Dong
  2025-08-22 14:33           ` Andrew Lunn
  1 sibling, 1 reply; 34+ messages in thread
From: Parthiban.Veerasooran @ 2025-08-22  8:05 UTC (permalink / raw)
  To: dong100
  Cc: andrew+netdev, davem, edumazet, kuba, pabeni, horms, corbet,
	gur.stavi, maddy, mpe, danishanwar, lee, gongfan1, lorenzo,
	geert+renesas, lukas.bulwahn, alexanderduyck, richardcochran,
	kees, gustavoars, netdev, linux-doc, linux-kernel,
	linux-hardening

On 22/08/25 12:21 pm, Yibo Dong wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Fri, Aug 22, 2025 at 06:07:51AM +0000, Parthiban.Veerasooran@microchip.com wrote:
>> On 22/08/25 11:07 am, Yibo Dong wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On Fri, Aug 22, 2025 at 04:49:44AM +0000, Parthiban.Veerasooran@microchip.com wrote:
>>>> On 22/08/25 8:04 am, Dong Yibo wrote:
>>>>> +/**
>>>>> + * mucse_mbx_get_capability - Get hw abilities from fw
>>>>> + * @hw: pointer to the HW structure
>>>>> + *
>>>>> + * mucse_mbx_get_capability tries to get capabities from
>>>>> + * hw. Many retrys will do if it is failed.
>>>>> + *
>>>>> + * @return: 0 on success, negative on failure
>>>>> + **/
>>>>> +int mucse_mbx_get_capability(struct mucse_hw *hw)
>>>>> +{
>>>>> +       struct hw_abilities ability = {};
>>>>> +       int try_cnt = 3;
>>>>> +       int err = -EIO;
>>>> Here too you no need to assign -EIO as it is updated in the while.
>>>>
>>>> Best regards,
>>>> Parthiban V
>>>>> +
>>>>> +       while (try_cnt--) {
>>>>> +               err = mucse_fw_get_capability(hw, &ability);
>>>>> +               if (err)
>>>>> +                       continue;
>>>>> +               hw->pfvfnum = le16_to_cpu(ability.pfnum) & GENMASK_U16(7, 0);
>>>>> +               return 0;
>>>>> +       }
>>>>> +       return err;
>>>>> +}
>>>>> +
>>>
>>> err is updated because 'try_cnt = 3'. But to the code logic itself, it should
>>> not leave err uninitialized since no guarantee that codes 'whthin while'
>>> run at least once. Right?
>> Yes, but 'try_cnt' is hard coded as 3, so the 'while loop' will always
>> execute and err will definitely be updated.
>>
>> So in this case, the check isn’t needed unless try_cnt is being modified
>> externally with unknown values, which doesn’t seem to be happening here.
>>
>> Best regards,
>> Parthiban V
>>>
>>> Thanks for your feedback.
>>>
>>>
>>
> 
> Is it fine if I add some comment like this?
> .....
> /* Initialized as a defensive measure to handle edge cases
>   * where try_cnt might be modified
>   */
>   int err = -EIO;
> .....
> 
> Additionally, keeping this initialization ensures we’ll no need to consider
> its impact every time 'try_cnt' is modified (Although this situation is
> almost impossible).
If you're concerned that 'try_cnt' might be modified, then let's keep 
the existing implementation as is. I also think the comment might not be 
necessary, so feel free to ignore my earlier suggestion.

Best regards,
Parthiban V
> 
> Thanks for your feedback.
> 
> 


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net-next v7 4/5] net: rnpgbe: Add basic mbx_fw support
  2025-08-22  8:05           ` Parthiban.Veerasooran
@ 2025-08-22  9:04             ` Yibo Dong
  0 siblings, 0 replies; 34+ messages in thread
From: Yibo Dong @ 2025-08-22  9:04 UTC (permalink / raw)
  To: Parthiban.Veerasooran
  Cc: andrew+netdev, davem, edumazet, kuba, pabeni, horms, corbet,
	gur.stavi, maddy, mpe, danishanwar, lee, gongfan1, lorenzo,
	geert+renesas, lukas.bulwahn, alexanderduyck, richardcochran,
	kees, gustavoars, netdev, linux-doc, linux-kernel,
	linux-hardening

On Fri, Aug 22, 2025 at 08:05:50AM +0000, Parthiban.Veerasooran@microchip.com wrote:
> On 22/08/25 12:21 pm, Yibo Dong wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On Fri, Aug 22, 2025 at 06:07:51AM +0000, Parthiban.Veerasooran@microchip.com wrote:
> >> On 22/08/25 11:07 am, Yibo Dong wrote:
> >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >>>
> >>> On Fri, Aug 22, 2025 at 04:49:44AM +0000, Parthiban.Veerasooran@microchip.com wrote:
> >>>> On 22/08/25 8:04 am, Dong Yibo wrote:
> >>>>> +/**
> >>>>> + * mucse_mbx_get_capability - Get hw abilities from fw
> >>>>> + * @hw: pointer to the HW structure
> >>>>> + *
> >>>>> + * mucse_mbx_get_capability tries to get capabities from
> >>>>> + * hw. Many retrys will do if it is failed.
> >>>>> + *
> >>>>> + * @return: 0 on success, negative on failure
> >>>>> + **/
> >>>>> +int mucse_mbx_get_capability(struct mucse_hw *hw)
> >>>>> +{
> >>>>> +       struct hw_abilities ability = {};
> >>>>> +       int try_cnt = 3;
> >>>>> +       int err = -EIO;
> >>>> Here too you no need to assign -EIO as it is updated in the while.
> >>>>
> >>>> Best regards,
> >>>> Parthiban V
> >>>>> +
> >>>>> +       while (try_cnt--) {
> >>>>> +               err = mucse_fw_get_capability(hw, &ability);
> >>>>> +               if (err)
> >>>>> +                       continue;
> >>>>> +               hw->pfvfnum = le16_to_cpu(ability.pfnum) & GENMASK_U16(7, 0);
> >>>>> +               return 0;
> >>>>> +       }
> >>>>> +       return err;
> >>>>> +}
> >>>>> +
> >>>
> >>> err is updated because 'try_cnt = 3'. But to the code logic itself, it should
> >>> not leave err uninitialized since no guarantee that codes 'whthin while'
> >>> run at least once. Right?
> >> Yes, but 'try_cnt' is hard coded as 3, so the 'while loop' will always
> >> execute and err will definitely be updated.
> >>
> >> So in this case, the check isn’t needed unless try_cnt is being modified
> >> externally with unknown values, which doesn’t seem to be happening here.
> >>
> >> Best regards,
> >> Parthiban V
> >>>
> >>> Thanks for your feedback.
> >>>
> >>>
> >>
> > 
> > Is it fine if I add some comment like this?
> > .....
> > /* Initialized as a defensive measure to handle edge cases
> >   * where try_cnt might be modified
> >   */
> >   int err = -EIO;
> > .....
> > 
> > Additionally, keeping this initialization ensures we’ll no need to consider
> > its impact every time 'try_cnt' is modified (Although this situation is
> > almost impossible).
> If you're concerned that 'try_cnt' might be modified, then let's keep 
> the existing implementation as is. I also think the comment might not be 
> necessary, so feel free to ignore my earlier suggestion.
> 
> Best regards,
> Parthiban V
> > 
> > Thanks for your feedback.
> > 
> > 
> 

Thank you for your understanding and flexibility. I'll keep the current
implementation with the initialization of err = -EIO as a defensive measure,
as you suggested. I appreciate your willingness to accommodate this consideration.


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net-next v7 4/5] net: rnpgbe: Add basic mbx_fw support
  2025-08-22  6:51         ` Yibo Dong
  2025-08-22  8:05           ` Parthiban.Veerasooran
@ 2025-08-22 14:33           ` Andrew Lunn
  2025-08-23  2:03             ` Yibo Dong
  1 sibling, 1 reply; 34+ messages in thread
From: Andrew Lunn @ 2025-08-22 14:33 UTC (permalink / raw)
  To: Yibo Dong
  Cc: Parthiban.Veerasooran, andrew+netdev, davem, edumazet, kuba,
	pabeni, horms, corbet, gur.stavi, maddy, mpe, danishanwar, lee,
	gongfan1, lorenzo, geert+renesas, lukas.bulwahn, alexanderduyck,
	richardcochran, kees, gustavoars, netdev, linux-doc, linux-kernel,
	linux-hardening

> /* Initialized as a defensive measure to handle edge cases
>  * where try_cnt might be modified
>  */
>  int err = -EIO;

We don't use defensive code in the kernel. Defensive code suggests you
don't actually know what your driver is doing and you are guessing
this might happen. You should convince yourself it is
possible/impossible and write the code as needed.

	Andrew

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net-next v7 4/5] net: rnpgbe: Add basic mbx_fw support
  2025-08-22  2:34 ` [PATCH net-next v7 4/5] net: rnpgbe: Add basic mbx_fw support Dong Yibo
  2025-08-22  4:49   ` Parthiban.Veerasooran
@ 2025-08-22 14:43   ` Andrew Lunn
  2025-08-23  1:58     ` Yibo Dong
  2025-08-23 15:02   ` Vadim Fedorenko
  2025-08-25 16:37   ` Vadim Fedorenko
  3 siblings, 1 reply; 34+ messages in thread
From: Andrew Lunn @ 2025-08-22 14:43 UTC (permalink / raw)
  To: Dong Yibo
  Cc: andrew+netdev, davem, edumazet, kuba, pabeni, horms, corbet,
	gur.stavi, maddy, mpe, danishanwar, lee, gongfan1, lorenzo,
	geert+renesas, Parthiban.Veerasooran, lukas.bulwahn,
	alexanderduyck, richardcochran, kees, gustavoars, netdev,
	linux-doc, linux-kernel, linux-hardening

> +/**
> + * mucse_mbx_get_capability - Get hw abilities from fw
> + * @hw: pointer to the HW structure
> + *
> + * mucse_mbx_get_capability tries to get capabities from
> + * hw. Many retrys will do if it is failed.
> + *
> + * @return: 0 on success, negative on failure
> + **/
> +int mucse_mbx_get_capability(struct mucse_hw *hw)
> +{
> +	struct hw_abilities ability = {};
> +	int try_cnt = 3;
> +	int err = -EIO;
> +
> +	while (try_cnt--) {
> +		err = mucse_fw_get_capability(hw, &ability);
> +		if (err)
> +			continue;
> +		hw->pfvfnum = le16_to_cpu(ability.pfnum) & GENMASK_U16(7, 0);
> +		return 0;
> +	}
> +	return err;
> +}

Please could you add an explanation why it would fail? Is this to do
with getting the driver and firmware in sync? Maybe you should make
this explicit, add a function mucse_mbx_sync() with a comment that
this is used once during probe to synchronise communication with the
firmware. You can then remove this loop here.

I would also differentiate between different error codes. It is
pointless to try again with ENOMEM, EINVAL, etc. These are real errors
which should be reported. However TIMEDOUT might makes sense to
retry.

	Andrew

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net-next v7 4/5] net: rnpgbe: Add basic mbx_fw support
  2025-08-22 14:43   ` Andrew Lunn
@ 2025-08-23  1:58     ` Yibo Dong
  2025-08-23 15:17       ` Andrew Lunn
  0 siblings, 1 reply; 34+ messages in thread
From: Yibo Dong @ 2025-08-23  1:58 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: andrew+netdev, davem, edumazet, kuba, pabeni, horms, corbet,
	gur.stavi, maddy, mpe, danishanwar, lee, gongfan1, lorenzo,
	geert+renesas, Parthiban.Veerasooran, lukas.bulwahn,
	alexanderduyck, richardcochran, kees, gustavoars, netdev,
	linux-doc, linux-kernel, linux-hardening

On Fri, Aug 22, 2025 at 04:43:16PM +0200, Andrew Lunn wrote:
> > +/**
> > + * mucse_mbx_get_capability - Get hw abilities from fw
> > + * @hw: pointer to the HW structure
> > + *
> > + * mucse_mbx_get_capability tries to get capabities from
> > + * hw. Many retrys will do if it is failed.
> > + *
> > + * @return: 0 on success, negative on failure
> > + **/
> > +int mucse_mbx_get_capability(struct mucse_hw *hw)
> > +{
> > +	struct hw_abilities ability = {};
> > +	int try_cnt = 3;
> > +	int err = -EIO;
> > +
> > +	while (try_cnt--) {
> > +		err = mucse_fw_get_capability(hw, &ability);
> > +		if (err)
> > +			continue;
> > +		hw->pfvfnum = le16_to_cpu(ability.pfnum) & GENMASK_U16(7, 0);
> > +		return 0;
> > +	}
> > +	return err;
> > +}
> 
> Please could you add an explanation why it would fail? Is this to do
> with getting the driver and firmware in sync? Maybe you should make
> this explicit, add a function mucse_mbx_sync() with a comment that
> this is used once during probe to synchronise communication with the
> firmware. You can then remove this loop here.

It is just get some fw capability(or info such as fw version).
It is failed maybe:
1. -EIO: return by mucse_obtain_mbx_lock_pf. The function tries to get
pf-fw lock(in chip register, not driver), failed when fw hold the lock.
2. -ETIMEDOUT: return by mucse_poll_for_xx. Failed when timeout.
3. -ETIMEDOUT: return by mucse_fw_send_cmd_wait. Failed when wait
response timeout.
4. -EIO: return by mucse_fw_send_cmd_wait. Failed when error_code in
response.
5. err return by mutex_lock_interruptible.

> 
> I would also differentiate between different error codes. It is
> pointless to try again with ENOMEM, EINVAL, etc. These are real errors
> which should be reported. However TIMEDOUT might makes sense to
> retry.
> 
> 	Andrew
> 

Yes, I didn't differentiate between different error codes. But it cost
~0 to ask firmware again. And error will be reported after 'try_cnt' times
retry to the function caller.
Maybe can simply handle error codes link this?

Thanks for your feedback.






^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net-next v7 4/5] net: rnpgbe: Add basic mbx_fw support
  2025-08-22 14:33           ` Andrew Lunn
@ 2025-08-23  2:03             ` Yibo Dong
  0 siblings, 0 replies; 34+ messages in thread
From: Yibo Dong @ 2025-08-23  2:03 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Parthiban.Veerasooran, andrew+netdev, davem, edumazet, kuba,
	pabeni, horms, corbet, gur.stavi, maddy, mpe, danishanwar, lee,
	gongfan1, lorenzo, geert+renesas, lukas.bulwahn, alexanderduyck,
	richardcochran, kees, gustavoars, netdev, linux-doc, linux-kernel,
	linux-hardening

On Fri, Aug 22, 2025 at 04:33:54PM +0200, Andrew Lunn wrote:
> > /* Initialized as a defensive measure to handle edge cases
> >  * where try_cnt might be modified
> >  */
> >  int err = -EIO;
> 
> We don't use defensive code in the kernel. Defensive code suggests you
> don't actually know what your driver is doing and you are guessing
> this might happen. You should convince yourself it is
> possible/impossible and write the code as needed.
> 
> 	Andrew
> 

Ok, I will change 'while' to 'do...while" and remove '= -EIO'.
That can guarantee 'mucse_fw_get_capability' run at least once to init
err.

Thanks for your feedback.


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net-next v7 4/5] net: rnpgbe: Add basic mbx_fw support
  2025-08-22  2:34 ` [PATCH net-next v7 4/5] net: rnpgbe: Add basic mbx_fw support Dong Yibo
  2025-08-22  4:49   ` Parthiban.Veerasooran
  2025-08-22 14:43   ` Andrew Lunn
@ 2025-08-23 15:02   ` Vadim Fedorenko
  2025-08-24  3:46     ` Yibo Dong
  2025-08-25 16:37   ` Vadim Fedorenko
  3 siblings, 1 reply; 34+ messages in thread
From: Vadim Fedorenko @ 2025-08-23 15:02 UTC (permalink / raw)
  To: Dong Yibo, andrew+netdev, davem, edumazet, kuba, pabeni, horms,
	corbet, gur.stavi, maddy, mpe, danishanwar, lee, gongfan1,
	lorenzo, geert+renesas, Parthiban.Veerasooran, lukas.bulwahn,
	alexanderduyck, richardcochran, kees, gustavoars
  Cc: netdev, linux-doc, linux-kernel, linux-hardening

On 22/08/2025 03:34, Dong Yibo wrote:
> Initialize basic mbx_fw ops, such as get_capability, reset phy
> and so on.
> 
> Signed-off-by: Dong Yibo <dong100@mucse.com>

[...]

> +/**
> + * mucse_mbx_fw_post_req - Posts a mbx req to firmware and wait reply
> + * @hw: pointer to the HW structure
> + * @req: pointer to the cmd req structure
> + * @cookie: pointer to the req cookie
> + *
> + * mucse_mbx_fw_post_req posts a mbx req to firmware and wait for the
> + * reply. cookie->wait will be set in irq handler.
> + *
> + * @return: 0 on success, negative on failure
> + **/
> +static int mucse_mbx_fw_post_req(struct mucse_hw *hw,
> +				 struct mbx_fw_cmd_req *req,
> +				 struct mbx_req_cookie *cookie)
> +{
> +	int len = le16_to_cpu(req->datalen);
> +	int err;
> +
> +	cookie->errcode = 0;
> +	cookie->done = 0;
> +	init_waitqueue_head(&cookie->wait);
> +	err = mutex_lock_interruptible(&hw->mbx.lock);
> +	if (err)
> +		return err;
> +	err = mucse_write_mbx_pf(hw, (u32 *)req, len);
> +	if (err)
> +		goto out;
> +	/* if write succeeds, we must wait for firmware response or
> +	 * timeout to avoid using the already freed cookie->wait
> +	 */
> +	err = wait_event_timeout(cookie->wait,
> +				 cookie->done == 1,
> +				 cookie->timeout_jiffies);
> +
> +	if (!err)
> +		err = -ETIMEDOUT;
> +	else
> +		err = 0;
> +	if (!err && cookie->errcode)
> +		err = cookie->errcode;

can cookie->errcode be non 0 if FW times out?


looks like this can be simplified to

if(!wait_event_timeout())
   err = -ETIMEDOUT
else
   err = cookie->errcode

> +out:
> +	mutex_unlock(&hw->mbx.lock);
> +	return err;
> +}
> +
> +/**
> + * build_ifinsmod - build req with insmod opcode
> + * @req: pointer to the cmd req structure
> + * @status: true for insmod, false for rmmod

naming is misleading here, I believe.. no strong feeling, but
is_insmod might be better

> + **/
> +static void build_ifinsmod(struct mbx_fw_cmd_req *req,
> +			   int status)
> +{
> +	req->flags = 0;
> +	req->opcode = cpu_to_le16(DRIVER_INSMOD);
> +	req->datalen = cpu_to_le16(sizeof(req->ifinsmod) +
> +				   MBX_REQ_HDR_LEN);
> +	req->cookie = NULL;
> +	req->reply_lo = 0;
> +	req->reply_hi = 0;
> +#define FIXED_VERSION 0xFFFFFFFF
> +	req->ifinsmod.version = cpu_to_le32(FIXED_VERSION);
> +	req->ifinsmod.status = cpu_to_le32(status);
> +}
> +
> +/**
> + * mucse_mbx_ifinsmod - Echo driver insmod status to hw
> + * @hw: pointer to the HW structure
> + * @status: true for insmod, false for rmmod

here as well

> + *
> + * @return: 0 on success, negative on failure
> + **/
> +int mucse_mbx_ifinsmod(struct mucse_hw *hw, int status)
> +{
> +	struct mbx_fw_cmd_req req = {};
> +	int len;
> +	int err;
> +
> +	build_ifinsmod(&req, status);
> +	len = le16_to_cpu(req.datalen);
> +	err = mutex_lock_interruptible(&hw->mbx.lock);
> +	if (err)
> +		return err;
> +
> +	if (status) {
> +		err = mucse_write_posted_mbx(hw, (u32 *)&req,
> +					     len);
> +	} else {
> +		err = mucse_write_mbx_pf(hw, (u32 *)&req,
> +					 len);
> +	}
> +
> +	mutex_unlock(&hw->mbx.lock);
> +	return err;
> +}

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net-next v7 4/5] net: rnpgbe: Add basic mbx_fw support
  2025-08-23  1:58     ` Yibo Dong
@ 2025-08-23 15:17       ` Andrew Lunn
  2025-08-24  4:10         ` Yibo Dong
  0 siblings, 1 reply; 34+ messages in thread
From: Andrew Lunn @ 2025-08-23 15:17 UTC (permalink / raw)
  To: Yibo Dong
  Cc: andrew+netdev, davem, edumazet, kuba, pabeni, horms, corbet,
	gur.stavi, maddy, mpe, danishanwar, lee, gongfan1, lorenzo,
	geert+renesas, Parthiban.Veerasooran, lukas.bulwahn,
	alexanderduyck, richardcochran, kees, gustavoars, netdev,
	linux-doc, linux-kernel, linux-hardening

On Sat, Aug 23, 2025 at 09:58:24AM +0800, Yibo Dong wrote:
> On Fri, Aug 22, 2025 at 04:43:16PM +0200, Andrew Lunn wrote:
> > > +/**
> > > + * mucse_mbx_get_capability - Get hw abilities from fw
> > > + * @hw: pointer to the HW structure
> > > + *
> > > + * mucse_mbx_get_capability tries to get capabities from
> > > + * hw. Many retrys will do if it is failed.
> > > + *
> > > + * @return: 0 on success, negative on failure
> > > + **/
> > > +int mucse_mbx_get_capability(struct mucse_hw *hw)
> > > +{
> > > +	struct hw_abilities ability = {};
> > > +	int try_cnt = 3;
> > > +	int err = -EIO;
> > > +
> > > +	while (try_cnt--) {
> > > +		err = mucse_fw_get_capability(hw, &ability);
> > > +		if (err)
> > > +			continue;
> > > +		hw->pfvfnum = le16_to_cpu(ability.pfnum) & GENMASK_U16(7, 0);
> > > +		return 0;
> > > +	}
> > > +	return err;
> > > +}
> > 
> > Please could you add an explanation why it would fail? Is this to do
> > with getting the driver and firmware in sync? Maybe you should make
> > this explicit, add a function mucse_mbx_sync() with a comment that
> > this is used once during probe to synchronise communication with the
> > firmware. You can then remove this loop here.
> 
> It is just get some fw capability(or info such as fw version).
> It is failed maybe:
> 1. -EIO: return by mucse_obtain_mbx_lock_pf. The function tries to get
> pf-fw lock(in chip register, not driver), failed when fw hold the lock.

If it cannot get the lock, isn't that fatal? You cannot do anything
without the lock.

> 2. -ETIMEDOUT: return by mucse_poll_for_xx. Failed when timeout.
> 3. -ETIMEDOUT: return by mucse_fw_send_cmd_wait. Failed when wait
> response timeout.

If its dead, its dead. Why would it suddenly start responding?

> 4. -EIO: return by mucse_fw_send_cmd_wait. Failed when error_code in
> response.

Which should be fatal. No retries necessary.

> 5. err return by mutex_lock_interruptible.

So you want the user to have to ^C three times?

And is mucse_mbx_get_capability() special, or will all interactions
with the firmware have three retries?

	Andrew

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net-next v7 4/5] net: rnpgbe: Add basic mbx_fw support
  2025-08-23 15:02   ` Vadim Fedorenko
@ 2025-08-24  3:46     ` Yibo Dong
  0 siblings, 0 replies; 34+ messages in thread
From: Yibo Dong @ 2025-08-24  3:46 UTC (permalink / raw)
  To: Vadim Fedorenko
  Cc: andrew+netdev, davem, edumazet, kuba, pabeni, horms, corbet,
	gur.stavi, maddy, mpe, danishanwar, lee, gongfan1, lorenzo,
	geert+renesas, Parthiban.Veerasooran, lukas.bulwahn,
	alexanderduyck, richardcochran, kees, gustavoars, netdev,
	linux-doc, linux-kernel, linux-hardening

On Sat, Aug 23, 2025 at 04:02:29PM +0100, Vadim Fedorenko wrote:
> On 22/08/2025 03:34, Dong Yibo wrote:
> > Initialize basic mbx_fw ops, such as get_capability, reset phy
> > and so on.
> > 
> > Signed-off-by: Dong Yibo <dong100@mucse.com>
> 
> [...]
> 
> > +/**
> > + * mucse_mbx_fw_post_req - Posts a mbx req to firmware and wait reply
> > + * @hw: pointer to the HW structure
> > + * @req: pointer to the cmd req structure
> > + * @cookie: pointer to the req cookie
> > + *
> > + * mucse_mbx_fw_post_req posts a mbx req to firmware and wait for the
> > + * reply. cookie->wait will be set in irq handler.
> > + *
> > + * @return: 0 on success, negative on failure
> > + **/
> > +static int mucse_mbx_fw_post_req(struct mucse_hw *hw,
> > +				 struct mbx_fw_cmd_req *req,
> > +				 struct mbx_req_cookie *cookie)
> > +{
> > +	int len = le16_to_cpu(req->datalen);
> > +	int err;
> > +
> > +	cookie->errcode = 0;
> > +	cookie->done = 0;
> > +	init_waitqueue_head(&cookie->wait);
> > +	err = mutex_lock_interruptible(&hw->mbx.lock);
> > +	if (err)
> > +		return err;
> > +	err = mucse_write_mbx_pf(hw, (u32 *)req, len);
> > +	if (err)
> > +		goto out;
> > +	/* if write succeeds, we must wait for firmware response or
> > +	 * timeout to avoid using the already freed cookie->wait
> > +	 */
> > +	err = wait_event_timeout(cookie->wait,
> > +				 cookie->done == 1,
> > +				 cookie->timeout_jiffies);
> > +
> > +	if (!err)
> > +		err = -ETIMEDOUT;
> > +	else
> > +		err = 0;
> > +	if (!err && cookie->errcode)
> > +		err = cookie->errcode;
> 
> can cookie->errcode be non 0 if FW times out?
> 

cookie is alloced by kzalloc, if fw timeout, nochange for it.
So cookie->errcode is 0 if FW times out.

> 
> looks like this can be simplified to
> 
> if(!wait_event_timeout())
>   err = -ETIMEDOUT
> else
>   err = cookie->errcode
> 

Got it, I will update it.

> > +out:
> > +	mutex_unlock(&hw->mbx.lock);
> > +	return err;
> > +}
> > +
> > +/**
> > + * build_ifinsmod - build req with insmod opcode
> > + * @req: pointer to the cmd req structure
> > + * @status: true for insmod, false for rmmod
> 
> naming is misleading here, I believe.. no strong feeling, but
> is_insmod might be better
> 

I see, I will fix it.

> > + **/
> > +static void build_ifinsmod(struct mbx_fw_cmd_req *req,
> > +			   int status)
> > +{
> > +	req->flags = 0;
> > +	req->opcode = cpu_to_le16(DRIVER_INSMOD);
> > +	req->datalen = cpu_to_le16(sizeof(req->ifinsmod) +
> > +				   MBX_REQ_HDR_LEN);
> > +	req->cookie = NULL;
> > +	req->reply_lo = 0;
> > +	req->reply_hi = 0;
> > +#define FIXED_VERSION 0xFFFFFFFF
> > +	req->ifinsmod.version = cpu_to_le32(FIXED_VERSION);
> > +	req->ifinsmod.status = cpu_to_le32(status);
> > +}
> > +
> > +/**
> > + * mucse_mbx_ifinsmod - Echo driver insmod status to hw
> > + * @hw: pointer to the HW structure
> > + * @status: true for insmod, false for rmmod
> 
> here as well
> 

Got it.

> > + *
> > + * @return: 0 on success, negative on failure
> > + **/
> > +int mucse_mbx_ifinsmod(struct mucse_hw *hw, int status)
> > +{
> > +	struct mbx_fw_cmd_req req = {};
> > +	int len;
> > +	int err;
> > +
> > +	build_ifinsmod(&req, status);
> > +	len = le16_to_cpu(req.datalen);
> > +	err = mutex_lock_interruptible(&hw->mbx.lock);
> > +	if (err)
> > +		return err;
> > +
> > +	if (status) {
> > +		err = mucse_write_posted_mbx(hw, (u32 *)&req,
> > +					     len);
> > +	} else {
> > +		err = mucse_write_mbx_pf(hw, (u32 *)&req,
> > +					 len);
> > +	}
> > +
> > +	mutex_unlock(&hw->mbx.lock);
> > +	return err;
> > +}
> 

Thanks for your feedback.


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net-next v7 4/5] net: rnpgbe: Add basic mbx_fw support
  2025-08-23 15:17       ` Andrew Lunn
@ 2025-08-24  4:10         ` Yibo Dong
  2025-08-24 15:15           ` Andrew Lunn
  0 siblings, 1 reply; 34+ messages in thread
From: Yibo Dong @ 2025-08-24  4:10 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: andrew+netdev, davem, edumazet, kuba, pabeni, horms, corbet,
	gur.stavi, maddy, mpe, danishanwar, lee, gongfan1, lorenzo,
	geert+renesas, Parthiban.Veerasooran, lukas.bulwahn,
	alexanderduyck, richardcochran, kees, gustavoars, netdev,
	linux-doc, linux-kernel, linux-hardening

On Sat, Aug 23, 2025 at 05:17:45PM +0200, Andrew Lunn wrote:
> On Sat, Aug 23, 2025 at 09:58:24AM +0800, Yibo Dong wrote:
> > On Fri, Aug 22, 2025 at 04:43:16PM +0200, Andrew Lunn wrote:
> > > > +/**
> > > > + * mucse_mbx_get_capability - Get hw abilities from fw
> > > > + * @hw: pointer to the HW structure
> > > > + *
> > > > + * mucse_mbx_get_capability tries to get capabities from
> > > > + * hw. Many retrys will do if it is failed.
> > > > + *
> > > > + * @return: 0 on success, negative on failure
> > > > + **/
> > > > +int mucse_mbx_get_capability(struct mucse_hw *hw)
> > > > +{
> > > > +	struct hw_abilities ability = {};
> > > > +	int try_cnt = 3;
> > > > +	int err = -EIO;
> > > > +
> > > > +	while (try_cnt--) {
> > > > +		err = mucse_fw_get_capability(hw, &ability);
> > > > +		if (err)
> > > > +			continue;
> > > > +		hw->pfvfnum = le16_to_cpu(ability.pfnum) & GENMASK_U16(7, 0);
> > > > +		return 0;
> > > > +	}
> > > > +	return err;
> > > > +}
> > > 
> > > Please could you add an explanation why it would fail? Is this to do
> > > with getting the driver and firmware in sync? Maybe you should make
> > > this explicit, add a function mucse_mbx_sync() with a comment that
> > > this is used once during probe to synchronise communication with the
> > > firmware. You can then remove this loop here.
> > 
> > It is just get some fw capability(or info such as fw version).
> > It is failed maybe:
> > 1. -EIO: return by mucse_obtain_mbx_lock_pf. The function tries to get
> > pf-fw lock(in chip register, not driver), failed when fw hold the lock.
> 
> If it cannot get the lock, isn't that fatal? You cannot do anything
> without the lock.
> 
> > 2. -ETIMEDOUT: return by mucse_poll_for_xx. Failed when timeout.
> > 3. -ETIMEDOUT: return by mucse_fw_send_cmd_wait. Failed when wait
> > response timeout.
> 
> If its dead, its dead. Why would it suddenly start responding?
> 
> > 4. -EIO: return by mucse_fw_send_cmd_wait. Failed when error_code in
> > response.
> 
> Which should be fatal. No retries necessary.
> 
> > 5. err return by mutex_lock_interruptible.
> 
> So you want the user to have to ^C three times?
> 
> And is mucse_mbx_get_capability() special, or will all interactions
> with the firmware have three retries?

It is the first 'cmd with response' from fw when probe. If it failed,
return err and nothing else todo (no registe netdev ...). So, we design
to give retry for it.
fatal with no retry, maybe like this? 

int mucse_mbx_get_capability(struct mucse_hw *hw)
{
        struct hw_abilities ability = {};
        int try_cnt = 3;
        int err;

        do {
                err = mucse_fw_get_capability(hw, &ability);
                if (err == -ETIMEDOUT)
                        continue;

		break;
        } while(try_cnt--);

	if (!err)
		hw->pfvfnum = le16_to_cpu(ability.pfnum) & GENMASK_U16(7, 0);
        return err;
}

> 
> 	Andrew
> 

Thanks for your feedback.


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net-next v7 4/5] net: rnpgbe: Add basic mbx_fw support
  2025-08-24  4:10         ` Yibo Dong
@ 2025-08-24 15:15           ` Andrew Lunn
  2025-08-25  1:30             ` Yibo Dong
  0 siblings, 1 reply; 34+ messages in thread
From: Andrew Lunn @ 2025-08-24 15:15 UTC (permalink / raw)
  To: Yibo Dong
  Cc: andrew+netdev, davem, edumazet, kuba, pabeni, horms, corbet,
	gur.stavi, maddy, mpe, danishanwar, lee, gongfan1, lorenzo,
	geert+renesas, Parthiban.Veerasooran, lukas.bulwahn,
	alexanderduyck, richardcochran, kees, gustavoars, netdev,
	linux-doc, linux-kernel, linux-hardening

On Sun, Aug 24, 2025 at 12:10:52PM +0800, Yibo Dong wrote:
> On Sat, Aug 23, 2025 at 05:17:45PM +0200, Andrew Lunn wrote:
> > On Sat, Aug 23, 2025 at 09:58:24AM +0800, Yibo Dong wrote:
> > > On Fri, Aug 22, 2025 at 04:43:16PM +0200, Andrew Lunn wrote:
> > > > > +/**
> > > > > + * mucse_mbx_get_capability - Get hw abilities from fw
> > > > > + * @hw: pointer to the HW structure
> > > > > + *
> > > > > + * mucse_mbx_get_capability tries to get capabities from
> > > > > + * hw. Many retrys will do if it is failed.
> > > > > + *
> > > > > + * @return: 0 on success, negative on failure
> > > > > + **/
> > > > > +int mucse_mbx_get_capability(struct mucse_hw *hw)
> > > > > +{
> > > > > +	struct hw_abilities ability = {};
> > > > > +	int try_cnt = 3;
> > > > > +	int err = -EIO;
> > > > > +
> > > > > +	while (try_cnt--) {
> > > > > +		err = mucse_fw_get_capability(hw, &ability);
> > > > > +		if (err)
> > > > > +			continue;
> > > > > +		hw->pfvfnum = le16_to_cpu(ability.pfnum) & GENMASK_U16(7, 0);
> > > > > +		return 0;
> > > > > +	}
> > > > > +	return err;
> > > > > +}
> > > > 
> > > > Please could you add an explanation why it would fail? Is this to do
> > > > with getting the driver and firmware in sync? Maybe you should make
> > > > this explicit, add a function mucse_mbx_sync() with a comment that
> > > > this is used once during probe to synchronise communication with the
> > > > firmware. You can then remove this loop here.
> > > 
> > > It is just get some fw capability(or info such as fw version).
> > > It is failed maybe:
> > > 1. -EIO: return by mucse_obtain_mbx_lock_pf. The function tries to get
> > > pf-fw lock(in chip register, not driver), failed when fw hold the lock.
> > 
> > If it cannot get the lock, isn't that fatal? You cannot do anything
> > without the lock.
> > 
> > > 2. -ETIMEDOUT: return by mucse_poll_for_xx. Failed when timeout.
> > > 3. -ETIMEDOUT: return by mucse_fw_send_cmd_wait. Failed when wait
> > > response timeout.
> > 
> > If its dead, its dead. Why would it suddenly start responding?
> > 
> > > 4. -EIO: return by mucse_fw_send_cmd_wait. Failed when error_code in
> > > response.
> > 
> > Which should be fatal. No retries necessary.
> > 
> > > 5. err return by mutex_lock_interruptible.
> > 
> > So you want the user to have to ^C three times?
> > 
> > And is mucse_mbx_get_capability() special, or will all interactions
> > with the firmware have three retries?
> 

> It is the first 'cmd with response' from fw when probe. If it failed,
> return err and nothing else todo (no registe netdev ...). So, we design
> to give retry for it.
> fatal with no retry, maybe like this? 
 
Quoting myself:

> > > > Is this to do
> > > > with getting the driver and firmware in sync? Maybe you should make
> > > > this explicit, add a function mucse_mbx_sync() with a comment that
> > > > this is used once during probe to synchronise communication with the
> > > > firmware. You can then remove this loop here.

Does the firmware offer a NOP command? Or one to get the firmware
version?  If you are trying to get the driver and firmware in sync, it
make sense to use an operation which is low value and won't be used
anywhere else.

	Andrew

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net-next v7 4/5] net: rnpgbe: Add basic mbx_fw support
  2025-08-24 15:15           ` Andrew Lunn
@ 2025-08-25  1:30             ` Yibo Dong
  0 siblings, 0 replies; 34+ messages in thread
From: Yibo Dong @ 2025-08-25  1:30 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: andrew+netdev, davem, edumazet, kuba, pabeni, horms, corbet,
	gur.stavi, maddy, mpe, danishanwar, lee, gongfan1, lorenzo,
	geert+renesas, Parthiban.Veerasooran, lukas.bulwahn,
	alexanderduyck, richardcochran, kees, gustavoars, netdev,
	linux-doc, linux-kernel, linux-hardening

On Sun, Aug 24, 2025 at 05:15:25PM +0200, Andrew Lunn wrote:
> On Sun, Aug 24, 2025 at 12:10:52PM +0800, Yibo Dong wrote:
> > On Sat, Aug 23, 2025 at 05:17:45PM +0200, Andrew Lunn wrote:
> > > On Sat, Aug 23, 2025 at 09:58:24AM +0800, Yibo Dong wrote:
> > > > On Fri, Aug 22, 2025 at 04:43:16PM +0200, Andrew Lunn wrote:
> > > > > > +/**
> > > > > > + * mucse_mbx_get_capability - Get hw abilities from fw
> > > > > > + * @hw: pointer to the HW structure
> > > > > > + *
> > > > > > + * mucse_mbx_get_capability tries to get capabities from
> > > > > > + * hw. Many retrys will do if it is failed.
> > > > > > + *
> > > > > > + * @return: 0 on success, negative on failure
> > > > > > + **/
> > > > > > +int mucse_mbx_get_capability(struct mucse_hw *hw)
> > > > > > +{
> > > > > > +	struct hw_abilities ability = {};
> > > > > > +	int try_cnt = 3;
> > > > > > +	int err = -EIO;
> > > > > > +
> > > > > > +	while (try_cnt--) {
> > > > > > +		err = mucse_fw_get_capability(hw, &ability);
> > > > > > +		if (err)
> > > > > > +			continue;
> > > > > > +		hw->pfvfnum = le16_to_cpu(ability.pfnum) & GENMASK_U16(7, 0);
> > > > > > +		return 0;
> > > > > > +	}
> > > > > > +	return err;
> > > > > > +}
> > > > > 
> > > > > Please could you add an explanation why it would fail? Is this to do
> > > > > with getting the driver and firmware in sync? Maybe you should make
> > > > > this explicit, add a function mucse_mbx_sync() with a comment that
> > > > > this is used once during probe to synchronise communication with the
> > > > > firmware. You can then remove this loop here.
> > > > 
> > > > It is just get some fw capability(or info such as fw version).
> > > > It is failed maybe:
> > > > 1. -EIO: return by mucse_obtain_mbx_lock_pf. The function tries to get
> > > > pf-fw lock(in chip register, not driver), failed when fw hold the lock.
> > > 
> > > If it cannot get the lock, isn't that fatal? You cannot do anything
> > > without the lock.
> > > 
> > > > 2. -ETIMEDOUT: return by mucse_poll_for_xx. Failed when timeout.
> > > > 3. -ETIMEDOUT: return by mucse_fw_send_cmd_wait. Failed when wait
> > > > response timeout.
> > > 
> > > If its dead, its dead. Why would it suddenly start responding?
> > > 
> > > > 4. -EIO: return by mucse_fw_send_cmd_wait. Failed when error_code in
> > > > response.
> > > 
> > > Which should be fatal. No retries necessary.
> > > 
> > > > 5. err return by mutex_lock_interruptible.
> > > 
> > > So you want the user to have to ^C three times?
> > > 
> > > And is mucse_mbx_get_capability() special, or will all interactions
> > > with the firmware have three retries?
> > 
> 
> > It is the first 'cmd with response' from fw when probe. If it failed,
> > return err and nothing else todo (no registe netdev ...). So, we design
> > to give retry for it.
> > fatal with no retry, maybe like this? 
>  
> Quoting myself:
> 
> > > > > Is this to do
> > > > > with getting the driver and firmware in sync? Maybe you should make
> > > > > this explicit, add a function mucse_mbx_sync() with a comment that
> > > > > this is used once during probe to synchronise communication with the
> > > > > firmware. You can then remove this loop here.

'mucse_mbx_get_capability' is used once during probe in fact, and won't be
used anywhere.

> 
> Does the firmware offer a NOP command? Or one to get the firmware
> version?  If you are trying to get the driver and firmware in sync, it
> make sense to use an operation which is low value and won't be used
> anywhere else.
> 
> 	Andrew
> 

No NOP command.. 'mucse_mbx_get_capability' can get the firmware version
and in fact only used in probe, maybe I should rename it to 'mucse_mbx_sync',
and add comment 'only be used once during probe'?
Or keep the name with that comment?

Thanks for your feedback.


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net-next v7 4/5] net: rnpgbe: Add basic mbx_fw support
  2025-08-22  2:34 ` [PATCH net-next v7 4/5] net: rnpgbe: Add basic mbx_fw support Dong Yibo
                     ` (2 preceding siblings ...)
  2025-08-23 15:02   ` Vadim Fedorenko
@ 2025-08-25 16:37   ` Vadim Fedorenko
  2025-08-26  1:31     ` Yibo Dong
  3 siblings, 1 reply; 34+ messages in thread
From: Vadim Fedorenko @ 2025-08-25 16:37 UTC (permalink / raw)
  To: Dong Yibo, andrew+netdev, davem, edumazet, kuba, pabeni, horms,
	corbet, gur.stavi, maddy, mpe, danishanwar, lee, gongfan1,
	lorenzo, geert+renesas, Parthiban.Veerasooran, lukas.bulwahn,
	alexanderduyck, richardcochran, kees, gustavoars
  Cc: netdev, linux-doc, linux-kernel, linux-hardening

On 22/08/2025 03:34, Dong Yibo wrote:

[...]
> +/**
> + * mucse_mbx_fw_post_req - Posts a mbx req to firmware and wait reply
> + * @hw: pointer to the HW structure
> + * @req: pointer to the cmd req structure
> + * @cookie: pointer to the req cookie
> + *
> + * mucse_mbx_fw_post_req posts a mbx req to firmware and wait for the
> + * reply. cookie->wait will be set in irq handler.
> + *
> + * @return: 0 on success, negative on failure
> + **/
> +static int mucse_mbx_fw_post_req(struct mucse_hw *hw,
> +				 struct mbx_fw_cmd_req *req,
> +				 struct mbx_req_cookie *cookie)
> +{
> +	int len = le16_to_cpu(req->datalen);
> +	int err;
> +
> +	cookie->errcode = 0;
> +	cookie->done = 0;
> +	init_waitqueue_head(&cookie->wait);
> +	err = mutex_lock_interruptible(&hw->mbx.lock);
> +	if (err)
> +		return err;
> +	err = mucse_write_mbx_pf(hw, (u32 *)req, len);
> +	if (err)
> +		goto out;
> +	/* if write succeeds, we must wait for firmware response or
> +	 * timeout to avoid using the already freed cookie->wait
> +	 */
> +	err = wait_event_timeout(cookie->wait,
> +				 cookie->done == 1,
> +				 cookie->timeout_jiffies);

it's unclear to me, what part of the code is managing values of cookie
structure? I didn't get the reason why are you putting the address of
cookie structure into request which is then directly passed to the FW.
Is the FW supposed to change values in cookie?

> +
> +	if (!err)
> +		err = -ETIMEDOUT;
> +	else
> +		err = 0;
> +	if (!err && cookie->errcode)
> +		err = cookie->errcode;
> +out:
> +	mutex_unlock(&hw->mbx.lock);
> +	return err;
> +}

[...]

> +struct mbx_fw_cmd_req {
> +	__le16 flags;
> +	__le16 opcode;
> +	__le16 datalen;
> +	__le16 ret_value;
> +	union {
> +		struct {
> +			__le32 cookie_lo;
> +			__le32 cookie_hi;
> +		};
> +
> +		void *cookie;
> +	};
> +	__le32 reply_lo;
> +	__le32 reply_hi;

what do these 2 fields mean? are you going to provide reply's buffer
address directly to FW?

> +	union {
> +		u8 data[32];
> +		struct {
> +			__le32 version;
> +			__le32 status;
> +		} ifinsmod;
> +		struct {
> +			__le32 port_mask;
> +			__le32 pfvf_num;
> +		} get_mac_addr;
> +	};
> +} __packed;
> +
> +struct mbx_fw_cmd_reply {
> +	__le16 flags;
> +	__le16 opcode;
> +	__le16 error_code;
> +	__le16 datalen;
> +	union {
> +		struct {
> +			__le32 cookie_lo;
> +			__le32 cookie_hi;
> +		};
> +		void *cookie;
> +	};

This part looks like the request, apart from datalen and error_code are
swapped in the header. And it actually means that the FW will put back
the address of provided cookie into reply, right? If yes, then it
doesn't look correct at all...

> +	union {
> +		u8 data[40];
> +		struct mac_addr {
> +			__le32 ports;
> +			struct _addr {
> +				/* for macaddr:01:02:03:04:05:06
> +				 * mac-hi=0x01020304 mac-lo=0x05060000
> +				 */
> +				u8 mac[8];
> +			} addrs[4];
> +		} mac_addr;
> +		struct hw_abilities hw_abilities;
> +	};
> +} __packed;

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net-next v7 4/5] net: rnpgbe: Add basic mbx_fw support
  2025-08-25 16:37   ` Vadim Fedorenko
@ 2025-08-26  1:31     ` Yibo Dong
  2025-08-26 10:14       ` Vadim Fedorenko
  0 siblings, 1 reply; 34+ messages in thread
From: Yibo Dong @ 2025-08-26  1:31 UTC (permalink / raw)
  To: Vadim Fedorenko
  Cc: andrew+netdev, davem, edumazet, kuba, pabeni, horms, corbet,
	gur.stavi, maddy, mpe, danishanwar, lee, gongfan1, lorenzo,
	geert+renesas, Parthiban.Veerasooran, lukas.bulwahn,
	alexanderduyck, richardcochran, kees, gustavoars, netdev,
	linux-doc, linux-kernel, linux-hardening

On Mon, Aug 25, 2025 at 05:37:27PM +0100, Vadim Fedorenko wrote:
> On 22/08/2025 03:34, Dong Yibo wrote:
> 
> [...]
> > +/**
> > + * mucse_mbx_fw_post_req - Posts a mbx req to firmware and wait reply
> > + * @hw: pointer to the HW structure
> > + * @req: pointer to the cmd req structure
> > + * @cookie: pointer to the req cookie
> > + *
> > + * mucse_mbx_fw_post_req posts a mbx req to firmware and wait for the
> > + * reply. cookie->wait will be set in irq handler.
> > + *
> > + * @return: 0 on success, negative on failure
> > + **/
> > +static int mucse_mbx_fw_post_req(struct mucse_hw *hw,
> > +				 struct mbx_fw_cmd_req *req,
> > +				 struct mbx_req_cookie *cookie)
> > +{
> > +	int len = le16_to_cpu(req->datalen);
> > +	int err;
> > +
> > +	cookie->errcode = 0;
> > +	cookie->done = 0;
> > +	init_waitqueue_head(&cookie->wait);
> > +	err = mutex_lock_interruptible(&hw->mbx.lock);
> > +	if (err)
> > +		return err;
> > +	err = mucse_write_mbx_pf(hw, (u32 *)req, len);
> > +	if (err)
> > +		goto out;
> > +	/* if write succeeds, we must wait for firmware response or
> > +	 * timeout to avoid using the already freed cookie->wait
> > +	 */
> > +	err = wait_event_timeout(cookie->wait,
> > +				 cookie->done == 1,
> > +				 cookie->timeout_jiffies);
> 
> it's unclear to me, what part of the code is managing values of cookie
> structure? I didn't get the reason why are you putting the address of
> cookie structure into request which is then directly passed to the FW.
> Is the FW supposed to change values in cookie?
> 

cookie will be used in an irq-handler. like this:
static int rnpgbe_mbx_fw_reply_handler(struct mucse *mucse,
                                       struct mbx_fw_cmd_reply *reply)
{
        struct mbx_req_cookie *cookie;

        cookie = reply->cookie;

        if (cookie->priv_len > 0)
                memcpy(cookie->priv, reply->data, cookie->priv_len);
        cookie->done = 1;
        if (le16_to_cpu(reply->flags) & FLAGS_ERR)
                cookie->errcode = -EIO;
        else
                cookie->errcode = 0;
        wake_up(&cookie->wait);
        return 0;
}
That is why we must wait for firmware response.
But irq is not added in this patch series. Maybe I should move all
cookie relative codes to the patch will add irq?

> > +
> > +	if (!err)
> > +		err = -ETIMEDOUT;
> > +	else
> > +		err = 0;
> > +	if (!err && cookie->errcode)
> > +		err = cookie->errcode;
> > +out:
> > +	mutex_unlock(&hw->mbx.lock);
> > +	return err;
> > +}
> 
> [...]
> 
> > +struct mbx_fw_cmd_req {
> > +	__le16 flags;
> > +	__le16 opcode;
> > +	__le16 datalen;
> > +	__le16 ret_value;
> > +	union {
> > +		struct {
> > +			__le32 cookie_lo;
> > +			__le32 cookie_hi;
> > +		};
> > +
> > +		void *cookie;
> > +	};
> > +	__le32 reply_lo;
> > +	__le32 reply_hi;
> 
> what do these 2 fields mean? are you going to provide reply's buffer
> address directly to FW?
> 

No, this is defined by fw. Some fw can access physical address.
But I don't use it in this driver.

> > +	union {
> > +		u8 data[32];
> > +		struct {
> > +			__le32 version;
> > +			__le32 status;
> > +		} ifinsmod;
> > +		struct {
> > +			__le32 port_mask;
> > +			__le32 pfvf_num;
> > +		} get_mac_addr;
> > +	};
> > +} __packed;
> > +
> > +struct mbx_fw_cmd_reply {
> > +	__le16 flags;
> > +	__le16 opcode;
> > +	__le16 error_code;
> > +	__le16 datalen;
> > +	union {
> > +		struct {
> > +			__le32 cookie_lo;
> > +			__le32 cookie_hi;
> > +		};
> > +		void *cookie;
> > +	};
> 
> This part looks like the request, apart from datalen and error_code are
> swapped in the header. And it actually means that the FW will put back
> the address of provided cookie into reply, right? If yes, then it
> doesn't look correct at all...
> 

It is yes. cookie is used in irq handler as show above.
Sorry, I didn't understand 'the not correct' point?

> > +	union {
> > +		u8 data[40];
> > +		struct mac_addr {
> > +			__le32 ports;
> > +			struct _addr {
> > +				/* for macaddr:01:02:03:04:05:06
> > +				 * mac-hi=0x01020304 mac-lo=0x05060000
> > +				 */
> > +				u8 mac[8];
> > +			} addrs[4];
> > +		} mac_addr;
> > +		struct hw_abilities hw_abilities;
> > +	};
> > +} __packed;
> 

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net-next v7 4/5] net: rnpgbe: Add basic mbx_fw support
  2025-08-26  1:31     ` Yibo Dong
@ 2025-08-26 10:14       ` Vadim Fedorenko
  2025-08-26 11:05         ` Yibo Dong
  0 siblings, 1 reply; 34+ messages in thread
From: Vadim Fedorenko @ 2025-08-26 10:14 UTC (permalink / raw)
  To: Yibo Dong
  Cc: andrew+netdev, davem, edumazet, kuba, pabeni, horms, corbet,
	gur.stavi, maddy, mpe, danishanwar, lee, gongfan1, lorenzo,
	geert+renesas, Parthiban.Veerasooran, lukas.bulwahn,
	alexanderduyck, richardcochran, kees, gustavoars, netdev,
	linux-doc, linux-kernel, linux-hardening

On 26/08/2025 02:31, Yibo Dong wrote:
> On Mon, Aug 25, 2025 at 05:37:27PM +0100, Vadim Fedorenko wrote:
>> On 22/08/2025 03:34, Dong Yibo wrote:
>>
>> [...]
>>> +/**
>>> + * mucse_mbx_fw_post_req - Posts a mbx req to firmware and wait reply
>>> + * @hw: pointer to the HW structure
>>> + * @req: pointer to the cmd req structure
>>> + * @cookie: pointer to the req cookie
>>> + *
>>> + * mucse_mbx_fw_post_req posts a mbx req to firmware and wait for the
>>> + * reply. cookie->wait will be set in irq handler.
>>> + *
>>> + * @return: 0 on success, negative on failure
>>> + **/
>>> +static int mucse_mbx_fw_post_req(struct mucse_hw *hw,
>>> +				 struct mbx_fw_cmd_req *req,
>>> +				 struct mbx_req_cookie *cookie)
>>> +{
>>> +	int len = le16_to_cpu(req->datalen);
>>> +	int err;
>>> +
>>> +	cookie->errcode = 0;
>>> +	cookie->done = 0;
>>> +	init_waitqueue_head(&cookie->wait);
>>> +	err = mutex_lock_interruptible(&hw->mbx.lock);
>>> +	if (err)
>>> +		return err;
>>> +	err = mucse_write_mbx_pf(hw, (u32 *)req, len);
>>> +	if (err)
>>> +		goto out;
>>> +	/* if write succeeds, we must wait for firmware response or
>>> +	 * timeout to avoid using the already freed cookie->wait
>>> +	 */
>>> +	err = wait_event_timeout(cookie->wait,
>>> +				 cookie->done == 1,
>>> +				 cookie->timeout_jiffies);
>>
>> it's unclear to me, what part of the code is managing values of cookie
>> structure? I didn't get the reason why are you putting the address of
>> cookie structure into request which is then directly passed to the FW.
>> Is the FW supposed to change values in cookie?
>>
> 
> cookie will be used in an irq-handler. like this:
> static int rnpgbe_mbx_fw_reply_handler(struct mucse *mucse,
>                                         struct mbx_fw_cmd_reply *reply)
> {
>          struct mbx_req_cookie *cookie;
> 
>          cookie = reply->cookie;
> 
>          if (cookie->priv_len > 0)
>                  memcpy(cookie->priv, reply->data, cookie->priv_len);
>          cookie->done = 1;
>          if (le16_to_cpu(reply->flags) & FLAGS_ERR)
>                  cookie->errcode = -EIO;
>          else
>                  cookie->errcode = 0;
>          wake_up(&cookie->wait);
>          return 0;
> }
> That is why we must wait for firmware response.
> But irq is not added in this patch series. Maybe I should move all
> cookie relative codes to the patch will add irq?

well, yes, in general it's better to introduce the code as a solid
solution. this way it's much easier to review

> 
>>> +
>>> +	if (!err)
>>> +		err = -ETIMEDOUT;
>>> +	else
>>> +		err = 0;
>>> +	if (!err && cookie->errcode)
>>> +		err = cookie->errcode;
>>> +out:
>>> +	mutex_unlock(&hw->mbx.lock);
>>> +	return err;
>>> +}
>>
>> [...]
>>
>>> +struct mbx_fw_cmd_req {
>>> +	__le16 flags;
>>> +	__le16 opcode;
>>> +	__le16 datalen;
>>> +	__le16 ret_value;
>>> +	union {
>>> +		struct {
>>> +			__le32 cookie_lo;
>>> +			__le32 cookie_hi;
>>> +		};
>>> +
>>> +		void *cookie;
>>> +	};
>>> +	__le32 reply_lo;
>>> +	__le32 reply_hi;
>>
>> what do these 2 fields mean? are you going to provide reply's buffer
>> address directly to FW?
>>
> 
> No, this is defined by fw. Some fw can access physical address.
> But I don't use it in this driver.

FW can access physical address without previously configuring IOMMU?
How can that be?

> 
>>> +	union {
>>> +		u8 data[32];
>>> +		struct {
>>> +			__le32 version;
>>> +			__le32 status;
>>> +		} ifinsmod;
>>> +		struct {
>>> +			__le32 port_mask;
>>> +			__le32 pfvf_num;
>>> +		} get_mac_addr;
>>> +	};
>>> +} __packed;
>>> +
>>> +struct mbx_fw_cmd_reply {
>>> +	__le16 flags;
>>> +	__le16 opcode;
>>> +	__le16 error_code;
>>> +	__le16 datalen;
>>> +	union {
>>> +		struct {
>>> +			__le32 cookie_lo;
>>> +			__le32 cookie_hi;
>>> +		};
>>> +		void *cookie;
>>> +	};
>>
>> This part looks like the request, apart from datalen and error_code are
>> swapped in the header. And it actually means that the FW will put back
>> the address of provided cookie into reply, right? If yes, then it
>> doesn't look correct at all...
>>
> 
> It is yes. cookie is used in irq handler as show above.
> Sorry, I didn't understand 'the not correct' point?

The example above showed that the irq handler uses some value received
from the device as a pointer to kernel memory. That's not safe, you
cannot be sure that provided value is valid pointer, and that it points
to previously allocated cookie structure. It is a clear way to corrupt
memory.

> 
>>> +	union {
>>> +		u8 data[40];
>>> +		struct mac_addr {
>>> +			__le32 ports;
>>> +			struct _addr {
>>> +				/* for macaddr:01:02:03:04:05:06
>>> +				 * mac-hi=0x01020304 mac-lo=0x05060000
>>> +				 */
>>> +				u8 mac[8];
>>> +			} addrs[4];
>>> +		} mac_addr;
>>> +		struct hw_abilities hw_abilities;
>>> +	};
>>> +} __packed;
>>


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net-next v7 4/5] net: rnpgbe: Add basic mbx_fw support
  2025-08-26 10:14       ` Vadim Fedorenko
@ 2025-08-26 11:05         ` Yibo Dong
  2025-08-26 12:39           ` Andrew Lunn
  0 siblings, 1 reply; 34+ messages in thread
From: Yibo Dong @ 2025-08-26 11:05 UTC (permalink / raw)
  To: Vadim Fedorenko
  Cc: andrew+netdev, davem, edumazet, kuba, pabeni, horms, corbet,
	gur.stavi, maddy, mpe, danishanwar, lee, gongfan1, lorenzo,
	geert+renesas, Parthiban.Veerasooran, lukas.bulwahn,
	alexanderduyck, richardcochran, kees, gustavoars, netdev,
	linux-doc, linux-kernel, linux-hardening

On Tue, Aug 26, 2025 at 11:14:19AM +0100, Vadim Fedorenko wrote:
> On 26/08/2025 02:31, Yibo Dong wrote:
> > On Mon, Aug 25, 2025 at 05:37:27PM +0100, Vadim Fedorenko wrote:
> > > On 22/08/2025 03:34, Dong Yibo wrote:
> > > 
> > > [...]
> > > > +/**
> > > > + * mucse_mbx_fw_post_req - Posts a mbx req to firmware and wait reply
> > > > + * @hw: pointer to the HW structure
> > > > + * @req: pointer to the cmd req structure
> > > > + * @cookie: pointer to the req cookie
> > > > + *
> > > > + * mucse_mbx_fw_post_req posts a mbx req to firmware and wait for the
> > > > + * reply. cookie->wait will be set in irq handler.
> > > > + *
> > > > + * @return: 0 on success, negative on failure
> > > > + **/
> > > > +static int mucse_mbx_fw_post_req(struct mucse_hw *hw,
> > > > +				 struct mbx_fw_cmd_req *req,
> > > > +				 struct mbx_req_cookie *cookie)
> > > > +{
> > > > +	int len = le16_to_cpu(req->datalen);
> > > > +	int err;
> > > > +
> > > > +	cookie->errcode = 0;
> > > > +	cookie->done = 0;
> > > > +	init_waitqueue_head(&cookie->wait);
> > > > +	err = mutex_lock_interruptible(&hw->mbx.lock);
> > > > +	if (err)
> > > > +		return err;
> > > > +	err = mucse_write_mbx_pf(hw, (u32 *)req, len);
> > > > +	if (err)
> > > > +		goto out;
> > > > +	/* if write succeeds, we must wait for firmware response or
> > > > +	 * timeout to avoid using the already freed cookie->wait
> > > > +	 */
> > > > +	err = wait_event_timeout(cookie->wait,
> > > > +				 cookie->done == 1,
> > > > +				 cookie->timeout_jiffies);
> > > 
> > > it's unclear to me, what part of the code is managing values of cookie
> > > structure? I didn't get the reason why are you putting the address of
> > > cookie structure into request which is then directly passed to the FW.
> > > Is the FW supposed to change values in cookie?
> > > 
> > 
> > cookie will be used in an irq-handler. like this:
> > static int rnpgbe_mbx_fw_reply_handler(struct mucse *mucse,
> >                                         struct mbx_fw_cmd_reply *reply)
> > {
> >          struct mbx_req_cookie *cookie;
> > 
> >          cookie = reply->cookie;
> > 
> >          if (cookie->priv_len > 0)
> >                  memcpy(cookie->priv, reply->data, cookie->priv_len);
> >          cookie->done = 1;
> >          if (le16_to_cpu(reply->flags) & FLAGS_ERR)
> >                  cookie->errcode = -EIO;
> >          else
> >                  cookie->errcode = 0;
> >          wake_up(&cookie->wait);
> >          return 0;
> > }
> > That is why we must wait for firmware response.
> > But irq is not added in this patch series. Maybe I should move all
> > cookie relative codes to the patch will add irq?
> 
> well, yes, in general it's better to introduce the code as a solid
> solution. this way it's much easier to review
> 

Ok, I will remove it in this series and add later.

> > 
> > > > +
> > > > +	if (!err)
> > > > +		err = -ETIMEDOUT;
> > > > +	else
> > > > +		err = 0;
> > > > +	if (!err && cookie->errcode)
> > > > +		err = cookie->errcode;
> > > > +out:
> > > > +	mutex_unlock(&hw->mbx.lock);
> > > > +	return err;
> > > > +}
> > > 
> > > [...]
> > > 
> > > > +struct mbx_fw_cmd_req {
> > > > +	__le16 flags;
> > > > +	__le16 opcode;
> > > > +	__le16 datalen;
> > > > +	__le16 ret_value;
> > > > +	union {
> > > > +		struct {
> > > > +			__le32 cookie_lo;
> > > > +			__le32 cookie_hi;
> > > > +		};
> > > > +
> > > > +		void *cookie;
> > > > +	};
> > > > +	__le32 reply_lo;
> > > > +	__le32 reply_hi;
> > > 
> > > what do these 2 fields mean? are you going to provide reply's buffer
> > > address directly to FW?
> > > 
> > 
> > No, this is defined by fw. Some fw can access physical address.
> > But I don't use it in this driver.
> 
> FW can access physical address without previously configuring IOMMU?
> How can that be?
> 

memory is allocated by dma_alloc_coherent, and get physical address.
Then fw use it.

> > 
> > > > +	union {
> > > > +		u8 data[32];
> > > > +		struct {
> > > > +			__le32 version;
> > > > +			__le32 status;
> > > > +		} ifinsmod;
> > > > +		struct {
> > > > +			__le32 port_mask;
> > > > +			__le32 pfvf_num;
> > > > +		} get_mac_addr;
> > > > +	};
> > > > +} __packed;
> > > > +
> > > > +struct mbx_fw_cmd_reply {
> > > > +	__le16 flags;
> > > > +	__le16 opcode;
> > > > +	__le16 error_code;
> > > > +	__le16 datalen;
> > > > +	union {
> > > > +		struct {
> > > > +			__le32 cookie_lo;
> > > > +			__le32 cookie_hi;
> > > > +		};
> > > > +		void *cookie;
> > > > +	};
> > > 
> > > This part looks like the request, apart from datalen and error_code are
> > > swapped in the header. And it actually means that the FW will put back
> > > the address of provided cookie into reply, right? If yes, then it
> > > doesn't look correct at all...
> > > 
> > 
> > It is yes. cookie is used in irq handler as show above.
> > Sorry, I didn't understand 'the not correct' point?
> 
> The example above showed that the irq handler uses some value received
> from the device as a pointer to kernel memory. That's not safe, you
> cannot be sure that provided value is valid pointer, and that it points
> to previously allocated cookie structure. It is a clear way to corrupt
> memory.
> 

Yes. It is not safe, so I 'must wait_event_timeout before free cookie'....
But is there a safe way to do it?
Maybe:
->allocate cookie
  -> map it to an unique id
    ->set the id to req->cookie
      ->receive response and check id valid? Then access cookie?
Please give me some advice... 

> > 
> > > > +	union {
> > > > +		u8 data[40];
> > > > +		struct mac_addr {
> > > > +			__le32 ports;
> > > > +			struct _addr {
> > > > +				/* for macaddr:01:02:03:04:05:06
> > > > +				 * mac-hi=0x01020304 mac-lo=0x05060000
> > > > +				 */
> > > > +				u8 mac[8];
> > > > +			} addrs[4];
> > > > +		} mac_addr;
> > > > +		struct hw_abilities hw_abilities;
> > > > +	};
> > > > +} __packed;
> > > 
> 
> 

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net-next v7 4/5] net: rnpgbe: Add basic mbx_fw support
  2025-08-26 11:05         ` Yibo Dong
@ 2025-08-26 12:39           ` Andrew Lunn
  2025-08-27  1:42             ` Yibo Dong
  0 siblings, 1 reply; 34+ messages in thread
From: Andrew Lunn @ 2025-08-26 12:39 UTC (permalink / raw)
  To: Yibo Dong
  Cc: Vadim Fedorenko, andrew+netdev, davem, edumazet, kuba, pabeni,
	horms, corbet, gur.stavi, maddy, mpe, danishanwar, lee, gongfan1,
	lorenzo, geert+renesas, Parthiban.Veerasooran, lukas.bulwahn,
	alexanderduyck, richardcochran, kees, gustavoars, netdev,
	linux-doc, linux-kernel, linux-hardening

> Yes. It is not safe, so I 'must wait_event_timeout before free cookie'....
> But is there a safe way to do it?
> Maybe:
> ->allocate cookie
>   -> map it to an unique id
>     ->set the id to req->cookie
>       ->receive response and check id valid? Then access cookie?

This is part of why adding cookies in a separate patch with a good
commit message is important.

Please take a step back. What is the big picture? Why do you need a
cookie? What is it used for? If you describe what your requirements
are, we might be able to suggest a better solution, or point you at a
driver you can copy code from.

	Andrew

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net-next v7 4/5] net: rnpgbe: Add basic mbx_fw support
  2025-08-26 12:39           ` Andrew Lunn
@ 2025-08-27  1:42             ` Yibo Dong
  2025-08-27 19:54               ` Andrew Lunn
  0 siblings, 1 reply; 34+ messages in thread
From: Yibo Dong @ 2025-08-27  1:42 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Vadim Fedorenko, andrew+netdev, davem, edumazet, kuba, pabeni,
	horms, corbet, gur.stavi, maddy, mpe, danishanwar, lee, gongfan1,
	lorenzo, geert+renesas, Parthiban.Veerasooran, lukas.bulwahn,
	alexanderduyck, richardcochran, kees, gustavoars, netdev,
	linux-doc, linux-kernel, linux-hardening

On Tue, Aug 26, 2025 at 02:39:07PM +0200, Andrew Lunn wrote:
> > Yes. It is not safe, so I 'must wait_event_timeout before free cookie'....
> > But is there a safe way to do it?
> > Maybe:
> > ->allocate cookie
> >   -> map it to an unique id
> >     ->set the id to req->cookie
> >       ->receive response and check id valid? Then access cookie?
> 
> This is part of why adding cookies in a separate patch with a good
> commit message is important.
> 
> Please take a step back. What is the big picture? Why do you need a
> cookie? What is it used for? If you describe what your requirements
> are, we might be able to suggest a better solution, or point you at a
> driver you can copy code from.
> 
> 	Andrew
> 

I try to explain the it:

driver-->fw, we has two types request:
1. without response, such as mucse_mbx_ifinsmod
2. with response, such as mucse_fw_get_macaddr

fw --> driver, we has one types request:
1. link status (link speed, duplex, pause status...)

fw tiggers irq when it sends response or request.
In order to handle link status timely, we do an irqhandle like this:

static int rnpgbe_rcv_msg_from_fw(struct mucse *mucse)
{
        u32 msgbuf[MUCSE_FW_MAILBOX_WORDS];
        struct mucse_hw *hw = &mucse->hw;
        struct mbx_fw_cmd_reply *reply;
        int retval;
	/* read mbx data out */
        retval = mucse_read_mbx(hw, msgbuf, MUCSE_FW_MAILBOX_WORDS);
        if (retval)
                return retval;

        reply = (struct mbx_fw_cmd_reply *)msgbuf;
	/* judge request or response */
        if (le16_to_cpu(reply->flags) & FLAGS_DD) {
		/* if it is a response, call wake_up(cookie) */
                return rnpgbe_mbx_fw_reply_handler(mucse,
                                (struct mbx_fw_cmd_reply *)msgbuf);
        } else {
		/* if it is a request, handle link status */
                return rnpgbe_mbx_fw_req_handler(mucse,
                                (struct mbx_fw_cmd_req *)msgbuf);
        }
}

And driver requests with response is bellow 'without' irqhandle:

static int mucse_fw_send_cmd_wait(struct mucse_hw *hw,
				  struct mbx_fw_cmd_req *req,
				  struct mbx_fw_cmd_reply *reply)
{
...
	mucse_write_posted_mbx(hw, (u32 *)req, len);

	...
	/* but as irqhandle be added, mbx data is read out in the
	 * handler, mucse_read_posted_mbx cannot read anything */
	mucse_read_posted_mbx(hw, (u32 *)reply, sizeof(*reply));

}

To solve mucse_read_posted_mbx cannot read data with irq, we add 'cookie'.
After mucse_write_posted_mbx, call wait_event_timeout. wake_up is called
in irqhandle.

Thanks for your feedback.


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net-next v7 4/5] net: rnpgbe: Add basic mbx_fw support
  2025-08-27  1:42             ` Yibo Dong
@ 2025-08-27 19:54               ` Andrew Lunn
  2025-08-28  2:02                 ` Yibo Dong
  0 siblings, 1 reply; 34+ messages in thread
From: Andrew Lunn @ 2025-08-27 19:54 UTC (permalink / raw)
  To: Yibo Dong
  Cc: Vadim Fedorenko, andrew+netdev, davem, edumazet, kuba, pabeni,
	horms, corbet, gur.stavi, maddy, mpe, danishanwar, lee, gongfan1,
	lorenzo, geert+renesas, Parthiban.Veerasooran, lukas.bulwahn,
	alexanderduyck, richardcochran, kees, gustavoars, netdev,
	linux-doc, linux-kernel, linux-hardening

> I try to explain the it:
> 
> driver-->fw, we has two types request:
> 1. without response, such as mucse_mbx_ifinsmod
> 2. with response, such as mucse_fw_get_macaddr
> 
> fw --> driver, we has one types request:
> 1. link status (link speed, duplex, pause status...)

Is the firmware multi threaded? By that, i mean can there be two
request/responses going on at once?

I'm assuming not.

So there appears to be four use cases:

1) Fire and forget, request without response.
2) Request with a response
3) Link state change from the firmware
4) Race condition: Request/response and link state change at the same time.

Again, assuming the firmware is single threaded, there must be a big
mutex around the message box so there can only be one thread doing any
sort of interaction with the firmware.

Since there can only be one thread waiting for the response, the
struct completion can be a member of the message box. The thread
waiting for a response uses wait_for_completion(mbx->completion).

The interrupt handler can look at the type of message it got from the
firmware. If it is a link state, process it, and exit. If it is
anything else, complete(mbx->completion) and exit.

I don't see the need for any sort of cookie.

  Andrew

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH net-next v7 4/5] net: rnpgbe: Add basic mbx_fw support
  2025-08-27 19:54               ` Andrew Lunn
@ 2025-08-28  2:02                 ` Yibo Dong
  0 siblings, 0 replies; 34+ messages in thread
From: Yibo Dong @ 2025-08-28  2:02 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Vadim Fedorenko, andrew+netdev, davem, edumazet, kuba, pabeni,
	horms, corbet, gur.stavi, maddy, mpe, danishanwar, lee, gongfan1,
	lorenzo, geert+renesas, Parthiban.Veerasooran, lukas.bulwahn,
	alexanderduyck, richardcochran, kees, gustavoars, netdev,
	linux-doc, linux-kernel, linux-hardening

On Wed, Aug 27, 2025 at 09:54:58PM +0200, Andrew Lunn wrote:
> > I try to explain the it:
> > 
> > driver-->fw, we has two types request:
> > 1. without response, such as mucse_mbx_ifinsmod
> > 2. with response, such as mucse_fw_get_macaddr
> > 
> > fw --> driver, we has one types request:
> > 1. link status (link speed, duplex, pause status...)
> 
> Is the firmware multi threaded? By that, i mean can there be two
> request/responses going on at once?
> 
> I'm assuming not.

No, fw is single threaded.

> 
> So there appears to be four use cases:
> 
> 1) Fire and forget, request without response.
> 2) Request with a response
> 3) Link state change from the firmware
> 4) Race condition: Request/response and link state change at the same time.
> 
> Again, assuming the firmware is single threaded, there must be a big
> mutex around the message box so there can only be one thread doing any
> sort of interaction with the firmware.
> 
> Since there can only be one thread waiting for the response, the
> struct completion can be a member of the message box. The thread
> waiting for a response uses wait_for_completion(mbx->completion).
> 
> The interrupt handler can look at the type of message it got from the
> firmware. If it is a link state, process it, and exit. If it is
> anything else, complete(mbx->completion) and exit.
> 
> I don't see the need for any sort of cookie.

Got it, I will try in patch which adds irq handler in the future.

> 
>   Andrew
> 

Thanks for your feedback.


^ permalink raw reply	[flat|nested] 34+ messages in thread

end of thread, other threads:[~2025-08-28  2:03 UTC | newest]

Thread overview: 34+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-22  2:34 [PATCH net-next v7 0/5] Add driver for 1Gbe network chips from MUCSE Dong Yibo
2025-08-22  2:34 ` [PATCH net-next v7 1/5] net: rnpgbe: Add build support for rnpgbe Dong Yibo
2025-08-22  4:32   ` Parthiban.Veerasooran
2025-08-22  5:23     ` Yibo Dong
2025-08-22  2:34 ` [PATCH net-next v7 2/5] net: rnpgbe: Add n500/n210 chip support Dong Yibo
2025-08-22  2:34 ` [PATCH net-next v7 3/5] net: rnpgbe: Add basic mbx ops support Dong Yibo
2025-08-22  4:41   ` Parthiban.Veerasooran
2025-08-22  5:25     ` Yibo Dong
2025-08-22  2:34 ` [PATCH net-next v7 4/5] net: rnpgbe: Add basic mbx_fw support Dong Yibo
2025-08-22  4:49   ` Parthiban.Veerasooran
2025-08-22  5:37     ` Yibo Dong
2025-08-22  6:07       ` Parthiban.Veerasooran
2025-08-22  6:51         ` Yibo Dong
2025-08-22  8:05           ` Parthiban.Veerasooran
2025-08-22  9:04             ` Yibo Dong
2025-08-22 14:33           ` Andrew Lunn
2025-08-23  2:03             ` Yibo Dong
2025-08-22 14:43   ` Andrew Lunn
2025-08-23  1:58     ` Yibo Dong
2025-08-23 15:17       ` Andrew Lunn
2025-08-24  4:10         ` Yibo Dong
2025-08-24 15:15           ` Andrew Lunn
2025-08-25  1:30             ` Yibo Dong
2025-08-23 15:02   ` Vadim Fedorenko
2025-08-24  3:46     ` Yibo Dong
2025-08-25 16:37   ` Vadim Fedorenko
2025-08-26  1:31     ` Yibo Dong
2025-08-26 10:14       ` Vadim Fedorenko
2025-08-26 11:05         ` Yibo Dong
2025-08-26 12:39           ` Andrew Lunn
2025-08-27  1:42             ` Yibo Dong
2025-08-27 19:54               ` Andrew Lunn
2025-08-28  2:02                 ` Yibo Dong
2025-08-22  2:34 ` [PATCH net-next v7 5/5] net: rnpgbe: Add register_netdev Dong Yibo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).