Re: [v4,net-next,08/11] net/nebula-matrix: add vsi resource implementation

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email says "This is an automated AI code reviewer, please treat what
it says with a grain of salt."

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net/nebula-matrix: add vsi resource implementation

This patch adds vsi (virtual station interface) resource implementation
to the nebula-matrix driver, including initialization for various
chip modules such as dped, uped, dsch, ustore, dstore, dvn, uvn, and uqm.

> diff --git a/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis.c b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis.c
> index 837a4591849d..a34dad884b47 100644
> --- a/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis.c
> +++ b/drivers/net/ethernet/nebula-matrix/nbl/nbl_hw/nbl_hw_leonis/nbl_hw_leonis.c

[ ... ]

> +static int nbl_ustore_init(struct nbl_hw_mgt *hw_mgt, u8 eth_num)
> +{
> +	struct nbl_ustore_port_drop_th drop_th;
> +	struct ustore_pkt_len pkt_len;
> +	int i;
> +
> +	nbl_hw_rd_regs(hw_mgt, NBL_USTORE_PKT_LEN_ADDR, (u8 *)&pkt_len,
> +		       sizeof(pkt_len));
> +	/* min arp packet length 42 (14 + 28) */
> +	pkt_len.min = 42;
> +	nbl_hw_wr_regs(hw_mgt, NBL_USTORE_PKT_LEN_ADDR, (u8 *)&pkt_len,
> +		       sizeof(pkt_len));
> +
> +	drop_th.en = 1;
> +	if (eth_num == 1)
> +		drop_th.disc_th = NBL_USTORE_SIGNLE_ETH_DROP_TH;
> +	else if (eth_num == 2)
> +		drop_th.disc_th = NBL_USTORE_DUAL_ETH_DROP_TH;
> +	else
> +		drop_th.disc_th = NBL_USTORE_QUAD_ETH_DROP_TH;
> +
> +	for (i = 0; i < 4; i++)
> +		nbl_hw_wr_regs(hw_mgt, NBL_USTORE_PORT_DROP_TH_REG_ARR(i),
> +			       (u8 *)&drop_th, sizeof(drop_th));

Is the drop_th structure fully initialized before writing to hardware?
The structure declaration doesn't initialize its fields, and only 'en'
and 'disc_th' fields are set before the entire structure is written to
hardware registers. Looking at the structure definition in
nbl_hw_leonis.h:775, nbl_ustore_port_drop_th has a 19-bit reserved
field 'rsv':

struct nbl_ustore_port_drop_th {
	u32 disc_th : 12;
	u32 rsv : 19;
	u32 en : 1;
} __packed;

The uninitialized 'rsv' field would contain undefined stack values when
written to hardware.

Other similar structures in this patch use zero-initialization (e.g.,
nbl_shaping_dport, nbl_shaping_dvn_dport, dsch_vn_quanta). Would it be
safer to initialize drop_th with '= { 0 }' to ensure all fields,
including reserved bits, are zeroed?

[ ... ]

Andrew Lunn raised a concern in v3 about the NBL_CORE naming in the
Kconfig (not modified in this commit):

https://lore.kernel.org/netdev/88c7e349-f941-4216-89ca-70a49e82b378@lunn.ch/

"_CORE is a bit unusual for a driver. CORE is generally a library which
is shared by a number of drivers. It does not have a tristate string,
it is just selected by the drivers which need it."

The author acknowledged this feedback but indicated it is still under
discussion. Will this naming issue be addressed in a follow-up patch?