Re: [PATCH v2 03/15] net: rnpgbe: Add basic mbx ops support

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

On 21/07/2025 12:32, Dong Yibo wrote:
> Initialize basic mbx function.
> 
> Signed-off-by: Dong Yibo <dong100@mucse.com>
> ---
>   drivers/net/ethernet/mucse/rnpgbe/Makefile    |   5 +-
>   drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h    |  46 ++
>   .../net/ethernet/mucse/rnpgbe/rnpgbe_chip.c   |   5 +-
>   drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h |   2 +
>   .../net/ethernet/mucse/rnpgbe/rnpgbe_main.c   |   1 +
>   .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c    | 623 ++++++++++++++++++
>   .../net/ethernet/mucse/rnpgbe/rnpgbe_mbx.h    |  48 ++
>   7 files changed, 727 insertions(+), 3 deletions(-)
>   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..41177103b50c 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/Makefile
> +++ b/drivers/net/ethernet/mucse/rnpgbe/Makefile
> @@ -5,5 +5,6 @@
>   #
>   
>   obj-$(CONFIG_MGBE) += rnpgbe.o
> -rnpgbe-objs := rnpgbe_main.o\
> -	       rnpgbe_chip.o
> +rnpgbe-objs := rnpgbe_main.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 2ae836fc8951..46e2bb2fe71e 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe.h
> @@ -63,9 +63,51 @@ struct mucse_mac_info {
>   	int clk_csr;
>   };
>   
> +struct mucse_hw;
> +
> +enum MBX_ID {
> +	MBX_VF0 = 0,
> +	MBX_VF1,
> +	MBX_VF2,
> +	MBX_VF3,
> +	MBX_VF4,
> +	MBX_VF5,
> +	MBX_VF6,
> +	MBX_VF7,
> +	MBX_CM3CPU,
> +	MBX_FW = MBX_CM3CPU,
> +	MBX_VFCNT
> +};
> +
> +struct mucse_mbx_operations {
> +	void (*init_params)(struct mucse_hw *hw);
> +	int (*read)(struct mucse_hw *hw, u32 *msg,
> +		    u16 size, enum MBX_ID id);
> +	int (*write)(struct mucse_hw *hw, u32 *msg,
> +		     u16 size, enum MBX_ID id);
> +	int (*read_posted)(struct mucse_hw *hw, u32 *msg,
> +			   u16 size, enum MBX_ID id);
> +	int (*write_posted)(struct mucse_hw *hw, u32 *msg,
> +			    u16 size, enum MBX_ID id);
> +	int (*check_for_msg)(struct mucse_hw *hw, enum MBX_ID id);
> +	int (*check_for_ack)(struct mucse_hw *hw, enum MBX_ID id);
> +	void (*configure)(struct mucse_hw *hw, int num_vec,
> +			  bool enable);
> +};
> +
> +struct mucse_mbx_stats {
> +	u32 msgs_tx;
> +	u32 msgs_rx;
> +	u32 acks;
> +	u32 reqs;
> +	u32 rsts;
> +};
> +
>   #define MAX_VF_NUM (8)
>   
>   struct mucse_mbx_info {
> +	struct mucse_mbx_operations ops;
> +	struct mucse_mbx_stats stats;
>   	u32 timeout;
>   	u32 usec_delay;
>   	u32 v2p_mailbox;
> @@ -99,6 +141,8 @@ struct mucse_mbx_info {
>   	int share_size;
>   };
>   
> +#include "rnpgbe_mbx.h"
> +
>   struct mucse_hw {
>   	void *back;
>   	u8 pfvfnum;
> @@ -110,6 +154,8 @@ struct mucse_hw {
>   	u16 vendor_id;
>   	u16 subsystem_device_id;
>   	u16 subsystem_vendor_id;
> +	int max_vfs;
> +	int max_vfs_noari;
>   	enum rnpgbe_hw_type hw_type;
>   	struct mucse_dma_info dma;
>   	struct mucse_eth_info eth;
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
> index 38c094965db9..b0e5fda632f3 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_chip.c
> @@ -6,6 +6,7 @@
>   
>   #include "rnpgbe.h"
>   #include "rnpgbe_hw.h"
> +#include "rnpgbe_mbx.h"
>   
>   /**
>    * rnpgbe_get_invariants_n500 - setup for hw info
> @@ -67,7 +68,7 @@ static void rnpgbe_get_invariants_n500(struct mucse_hw *hw)
>   	mbx->fw_pf_mbox_mask = 0x2e200;
>   	mbx->fw_vf_share_ram = 0x2b000;
>   	mbx->share_size = 512;
> -
> +	memcpy(&hw->mbx.ops, &mucse_mbx_ops_generic, sizeof(hw->mbx.ops));

that's bad pattern. it's better to have a constant set of callbacks per
device type and assign const pointer to it. It will make further debugs
much easier.

>   	/* setup net feature here */
>   	hw->feature_flags |= M_NET_FEATURE_SG |
>   			     M_NET_FEATURE_TX_CHECKSUM |
> @@ -83,6 +84,7 @@ static void rnpgbe_get_invariants_n500(struct mucse_hw *hw)
>   			     M_NET_FEATURE_STAG_OFFLOAD;
>   	/* start the default ahz, update later */
>   	hw->usecstocount = 125;
> +	hw->max_vfs = 7;
>   }
>   
>   /**
> @@ -117,6 +119,7 @@ static void rnpgbe_get_invariants_n210(struct mucse_hw *hw)
>   	/* update hw feature */
>   	hw->feature_flags |= M_HW_FEATURE_EEE;
>   	hw->usecstocount = 62;
> +	hw->max_vfs_noari = 7;
>   }
>   
>   const struct rnpgbe_info rnpgbe_n500_info = {
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
> index 2c7372a5e88d..ff7bd9b21550 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_hw.h
> @@ -14,6 +14,8 @@
>   #define RNPGBE_RING_BASE (0x1000)
>   #define RNPGBE_MAC_BASE (0x20000)
>   #define RNPGBE_ETH_BASE (0x10000)
> +
> +#define RNPGBE_DMA_DUMY (0x000c)
>   /* chip resourse */
>   #define RNPGBE_MAX_QUEUES (8)
>   /* multicast control table */
> diff --git a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> index 08f773199e9b..1e8360cae560 100644
> --- a/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_main.c
> @@ -114,6 +114,7 @@ static int rnpgbe_add_adapter(struct pci_dev *pdev,
>   	hw->hw_addr = hw_addr;
>   	hw->dma.dma_version = dma_version;
>   	ii->get_invariants(hw);
> +	hw->mbx.ops.init_params(hw);
>   
>   	return 0;
>   
> 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..56ace3057fea
> --- /dev/null
> +++ b/drivers/net/ethernet/mucse/rnpgbe/rnpgbe_mbx.c
> @@ -0,0 +1,623 @@
> +// 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"
> +
> +/**
> + * mucse_read_mbx - Reads a message from the mailbox
> + * @hw: Pointer to the HW structure
> + * @msg: The message buffer
> + * @size: Length of buffer
> + * @mbx_id: Id of vf/fw to read
> + *
> + * @return: 0 on success, negative on failure
> + **/
> +int mucse_read_mbx(struct mucse_hw *hw, u32 *msg, u16 size,
> +		   enum MBX_ID mbx_id)
> +{
> +	struct mucse_mbx_info *mbx = &hw->mbx;
> +
> +	/* limit read to size of mailbox */
> +	if (size > mbx->size)
> +		size = mbx->size;
> +
> +	if (!mbx->ops.read)
> +		return -EIO;

is it even possible? you control the set of callbacks, and these
operations must be setup to have HW working. avoid defensive programming
here and in other places you use callbacks.

> +
> +	return mbx->ops.read(hw, msg, size, mbx_id);
> +}
> +

[...]