Re: [PATCH v13 net-next 03/11] net/nebula-matrix: add chip related definitions

[Paolo Abeni <pabeni@redhat.com>]

On 4/28/26 1:48 PM, illusion.wang wrote:
> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis.h b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis.h
> index 77c67b67ba31..8831394ed11b 100644
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis.h
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis.h
> @@ -11,4 +11,473 @@
>  #include "../../nbl_include/nbl_include.h"
>  #include "../nbl_hw_reg.h"
>  
> +#define NBL_DRIVER_STATUS_REG			0x1300444
> +#define NBL_DRIVER_STATUS_BIT			16
> +
> +#pragma pack(1)

The kernel style for packed layouts is the __packed attribute; #pragma
pack is a non-portable compiler directive.

> +
> +/*  ----------  REG BASE ADDR  ----------  */
> +/* Interface modules base addr */
> +#define NBL_INTF_HOST_PCOMPLETER_BASE		0x00f08000
> +#define NBL_INTF_HOST_PADPT_BASE		0x00f4c000
> +#define NBL_INTF_HOST_MAILBOX_BASE		0x00fb0000
> +#define NBL_INTF_HOST_PCIE_BASE			0X01504000
> +/* DP modules base addr */
> +#define NBL_DP_USTORE_BASE			0x00104000
> +#define NBL_DP_UQM_BASE				0x00114000
> +#define NBL_DP_UPED_BASE			0x0015c000
> +#define NBL_DP_UVN_BASE				0x00244000
> +#define NBL_DP_DSCH_BASE			0x00404000
> +#define NBL_DP_SHAPING_BASE			0x00504000
> +#define NBL_DP_DVN_BASE				0x00514000
> +#define NBL_DP_DSTORE_BASE			0x00704000
> +#define NBL_DP_DQM_BASE				0x00714000
> +#define NBL_DP_DPED_BASE			0x0075c000
> +#define NBL_DP_DDMUX_BASE			0x00984000
> +/*  --------  MAILBOX BAR2 -----  */
> +#define NBL_MAILBOX_NOTIFY_ADDR			0x00000000
> +#define NBL_MAILBOX_BAR_REG			0x00000000
> +#define NBL_MAILBOX_QINFO_CFG_RX_TABLE_ADDR	0x10
> +#define NBL_MAILBOX_QINFO_CFG_TX_TABLE_ADDR	0x20
> +#define NBL_MAILBOX_QINFO_CFG_DBG_TABLE_ADDR	0x30
> +
> +/*  --------  MAILBOX  --------  */
> +
> +/* mailbox BAR qinfo_cfg_table */
> +struct nbl_mailbox_qinfo_cfg_table {
> +	u32 queue_base_addr_l;
> +	u32 queue_base_addr_h;
> +	u32 queue_size_bwind:4;
> +	u32 rsv1:28;
> +	u32 queue_rst:1;
> +	u32 queue_en:1;
> +	u32 dif_err:1;
> +	u32 ptr_err:1;
> +	u32 rsv2:28;

Sashiko says:

Can these bitfield register layouts work correctly on big-endian
hosts?
The C standard leaves allocation of bitfields within a storage unit
implementation-defined, and with GCC the order flips between LE and
BE targets (LSB-first on little-endian, MSB-first on big-endian).
Because nbl_hw_wr32() ultimately uses writel(), which only does
byte-level LE conversion, the hardware-visible bit positions produced
by these structs will differ between LE and BE builds.
The same question applies to every other bitfield struct added in
this header, e.g. nbl_mailbox_qinfo_map_table, nbl_host_msix_info,
ped_hw_edit_profile, dstore_disc_bp_th, nbl_shaping_net, ustore_pkt_len,
uvn_queue_err_mask, board_cfg_dw3, etc.
Would the explicit shift/mask helpers in <linux/bitfield.h>
(FIELD_PREP/FIELD_GET with GENMASK) be a better match here?

[...]
> +void nbl_write_all_regs(struct nbl_hw_mgt *hw_mgt)
> +{
> +	struct nbl_common_info *common = hw_mgt->common;
> +	u8 eth_mode = common->eth_mode;
> +	const u32 *nbl_sec046_data;
> +	const u32 *nbl_sec071_data;
> +	u32 i;
> +
> +	switch (eth_mode) {
> +	case 1:
> +		nbl_sec046_data = nbl_sec046_1p_data;
> +		nbl_sec071_data = nbl_sec071_1p_data;
> +		break;
> +	case 2:
> +		nbl_sec046_data = nbl_sec046_2p_data;
> +		nbl_sec071_data = nbl_sec071_2p_data;
> +		break;
> +	case 4:
> +		nbl_sec046_data = nbl_sec046_4p_data;
> +		nbl_sec071_data = nbl_sec071_4p_data;
> +		break;
> +	default:
> +		nbl_sec046_data = nbl_sec046_2p_data;
> +		nbl_sec071_data = nbl_sec071_2p_data;
> +	}
> +
> +	nbl_flush_writes(hw_mgt);
> +	for (i = 0; i < NBL_SEC006_SIZE; i++) {
> +		if ((i + 1) % NBL_SEC_BLOCK_SIZE == 0)
> +			nbl_hw_rd32(hw_mgt, NBL_HW_DUMMY_REG);
> +
> +		nbl_hw_wr32(hw_mgt, NBL_SEC006_REGI(i), nbl_sec006_data[i]);
> +	}
> +
> +	nbl_flush_writes(hw_mgt);
> +	for (i = 0; i < NBL_SEC007_SIZE; i++)
> +		nbl_hw_wr32(hw_mgt, NBL_SEC007_REGI(i), nbl_sec007_data[i]);
> +
> +	nbl_flush_writes(hw_mgt);
> +	for (i = 0; i < NBL_SEC008_SIZE; i++) {
> +		if ((i + 1) % NBL_SEC_BLOCK_SIZE == 0)
> +			nbl_hw_rd32(hw_mgt, NBL_HW_DUMMY_REG);
> +
> +		nbl_hw_wr32(hw_mgt, NBL_SEC008_REGI(i), nbl_sec008_data[i]);
> +	}
> +
> +	nbl_flush_writes(hw_mgt);
> +	for (i = 0; i < NBL_SEC009_SIZE; i++) {
> +		if ((i + 1) % NBL_SEC_BLOCK_SIZE == 0)
> +			nbl_hw_rd32(hw_mgt, NBL_HW_DUMMY_REG);
> +
> +		nbl_hw_wr32(hw_mgt, NBL_SEC009_REGI(i), nbl_sec009_data[i]);
> +	}
> +
> +	nbl_flush_writes(hw_mgt);
> +	for (i = 0; i < NBL_SEC010_SIZE; i++)
> +		nbl_hw_wr32(hw_mgt, NBL_SEC010_REGI(i), nbl_sec010_data[i]);
> +
> +	nbl_flush_writes(hw_mgt);
> +	for (i = 0; i < NBL_SEC011_SIZE; i++) {
> +		if ((i + 1) % NBL_SEC_BLOCK_SIZE == 0)
> +			nbl_hw_rd32(hw_mgt, NBL_HW_DUMMY_REG);
> +
> +		nbl_hw_wr32(hw_mgt, NBL_SEC011_REGI(i), nbl_sec011_data[i]);
> +	}
> +
> +	nbl_flush_writes(hw_mgt);
> +	for (i = 0; i < NBL_SEC012_SIZE; i++)
> +		nbl_hw_wr32(hw_mgt, NBL_SEC012_REGI(i), nbl_sec012_data[i]);
> +
> +	nbl_flush_writes(hw_mgt);
> +	for (i = 0; i < NBL_SEC013_SIZE; i++)
> +		nbl_hw_wr32(hw_mgt, NBL_SEC013_REGI(i), nbl_sec013_data[i]);
> +
> +	nbl_flush_writes(hw_mgt);
> +	for (i = 0; i < NBL_SEC014_SIZE; i++)
> +		nbl_hw_wr32(hw_mgt, NBL_SEC014_REGI(i), nbl_sec014_data[i]);
> +
> +	nbl_flush_writes(hw_mgt);
> +	for (i = 0; i < NBL_SEC022_SIZE; i++)
> +		nbl_hw_wr32(hw_mgt, NBL_SEC022_REGI(i), nbl_sec022_data[i]);
> +
> +	nbl_flush_writes(hw_mgt);
> +	for (i = 0; i < NBL_SEC023_SIZE; i++)
> +		nbl_hw_wr32(hw_mgt, NBL_SEC023_REGI(i), nbl_sec023_data[i]);
> +
> +	nbl_flush_writes(hw_mgt);
> +	for (i = 0; i < NBL_SEC024_SIZE; i++) {
> +		if ((i + 1) % NBL_SEC_BLOCK_SIZE == 0)
> +			nbl_hw_rd32(hw_mgt, NBL_HW_DUMMY_REG);
> +
> +		nbl_hw_wr32(hw_mgt, NBL_SEC024_REGI(i), nbl_sec024_data[i]);
> +	}
> +
> +	nbl_flush_writes(hw_mgt);
> +	for (i = 0; i < NBL_SEC025_SIZE; i++) {
> +		if ((i + 1) % NBL_SEC_BLOCK_SIZE == 0)
> +			nbl_hw_rd32(hw_mgt, NBL_HW_DUMMY_REG);
> +
> +		nbl_hw_wr32(hw_mgt, NBL_SEC025_REGI(i), nbl_sec025_data[i]);
> +	}
> +
> +	nbl_flush_writes(hw_mgt);
> +	for (i = 0; i < NBL_SEC026_SIZE; i++)
> +		nbl_hw_wr32(hw_mgt, NBL_SEC026_REGI(i), nbl_sec026_data[i]);
> +
> +	nbl_flush_writes(hw_mgt);
> +	for (i = 0; i < NBL_SEC027_SIZE; i++) {
> +		if ((i + 1) % NBL_SEC_BLOCK_SIZE == 0)
> +			nbl_hw_rd32(hw_mgt, NBL_HW_DUMMY_REG);
> +
> +		nbl_hw_wr32(hw_mgt, NBL_SEC027_REGI(i), nbl_sec027_data[i]);

Sashiko says:

Could this loop read past the end of the nbl_sec009_data array?
The macro NBL_SEC009_SIZE is defined as 2048, but the nbl_sec009_data array
contains significantly fewer elements (around 754). This appears to cause
sequential out-of-bounds reads into the .rodata section, writing unrelated
memory to the device registers.
Similar size mismatches exist for nbl_sec025_data (262 elements vs size
1024)
and nbl_sec022_data (506 elements vs size 256).
Would it be safer to use ARRAY_SIZE() to bound these iterations?

/P