Re: [PATCH v27 net-next 06/10] net/nebula-matrix: add chip-wide hardware init/deinit implementation

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] No hardware unwind path exists for the chip-wide state armed
  by nbl_hw_init_module() (nbl_hw_leonis.c:681), neither on init failure…

--

net/nebula-matrix: add chip-wide hardware init/deinit implementation

This adds the Leonis chip-wide init/deinit implementation, programming the
datapath pipeline (packet engines, scheduler, buffer stores, queue manager,
port shaping and flow control) plus the driver_status flag read by firmware.
The new hw_ops.init_module/deinit_module and the matching resource_ops entry
points are registered here, with the invocation path landing in a later
patch.

> 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 4c2e577610230..9f1054067e14c 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
> @@ -115,6 +115,585 @@ static void nbl_hw_get_fw_eth_map(struct nbl_hw_mgt *hw_mgt, u32 *eth_map)

[ ... ]

> +static int nbl_shaping_eth_init(struct nbl_hw_mgt *hw_mgt, u8 eth_id, u8 speed)
> +{
> +	struct nbl_shaping_dvn_dport_u dvn_dport = { 0 };
> +	struct nbl_shaping_dport_u dport = { 0 };
> +	u32 rate, half_rate;
> +	u32 depth;
> +	u64 low_val, high_val;
> +
> +	switch (speed) {
> +	case NBL_FW_PORT_SPEED_100G:
> +		rate = 100000;
> +		break;

[ ... ]

> +	default:
> +		dev_err(hw_mgt->common->dev,
> +			"Unsupported port speed %u for eth%u\n", speed, eth_id);
> +		return -EINVAL;
> +	}

[ ... ]

> +	/* 3. commit: set valid last */
> +	low_val = FIELD_PREP(NBL_DPORT_VALID_MASK, 1);
> +	dport.data[0] |= lower_32_bits(low_val);
> +
> +	low_val = FIELD_PREP(NBL_DPORT_VALID_MASK, 1);
> +	dvn_dport.data[0] |= lower_32_bits(low_val);
> +
> +	nbl_hw_wr_regs(hw_mgt, NBL_SHAPING_DPORT_REG(eth_id), dport.data,
> +		       sizeof(dport));
> +	nbl_hw_wr_regs(hw_mgt, NBL_SHAPING_DVN_DPORT_REG(eth_id),
> +		       dvn_dport.data, sizeof(dvn_dport));
> +	spin_unlock(&hw_mgt->reg_lock);
> +	return 0;
> +}
> +
> +static int nbl_shaping_init(struct nbl_hw_mgt *hw_mgt, u8 speed)
> +{
> +#define NBL_SHAPING_FLUSH_INTERVAL 128
> +	struct nbl_shaping_net_u net_shaping = { 0 };
> +	u32 eth_bitmap = 0;
> +	u32 psha_en = 0;
> +	int ret;
> +	int i;
> +
> +	nbl_hw_get_fw_eth_map(hw_mgt, &eth_bitmap);
> +	for (i = 0; i < NBL_MAX_ETHERNET; i++) {
> +		if (!(eth_bitmap & BIT(i)))
> +			continue;
> +		ret = nbl_shaping_eth_init(hw_mgt, i, speed);
> +		if (ret)
> +			return ret;
> +	}
> +	psha_en = eth_bitmap & GENMASK(3, 0);
> +	psha_en = FIELD_PREP(NBL_DSCH_PSHA_EN_MASK, psha_en);
> +	nbl_hw_wr_regs_lock(hw_mgt, NBL_DSCH_PSHA_EN_ADDR, &psha_en,
> +			    sizeof(psha_en));

[ ... ]

> +static int nbl_ustore_init(struct nbl_hw_mgt *hw_mgt, u8 eth_num)
> +{

[ ... ]

> +	if (WARN_ON(eth_num != 1 && eth_num != 2 && eth_num != 4))
> +		return -EINVAL;

[ ... ]

> +		fc_th |= FIELD_PREP(NBL_DSTORE_D_DPORT_FC_FC_EN_MASK, 1);
> +		nbl_hw_wr_regs(hw_mgt, NBL_DSTORE_D_DPORT_FC_TH_REG(i), &fc_th,
> +			       sizeof(fc_th));
> +		spin_unlock(&hw_mgt->reg_lock);
> +	}
> +}

[ ... ]

> +static int nbl_dp_init(struct nbl_hw_mgt *hw_mgt, u8 speed, u8 eth_num)
> +{
> +	int ret;
> +
> +	nbl_dped_init(hw_mgt);
> +	nbl_uped_init(hw_mgt);
> +	ret = nbl_shaping_init(hw_mgt, speed);
> +	if (ret)
> +		return ret;
> +	nbl_dsch_qid_max_init(hw_mgt);
> +	ret = nbl_ustore_init(hw_mgt, eth_num);
> +	if (ret)
> +		return ret;
> +	nbl_dstore_init(hw_mgt, speed);
> +	nbl_dvn_init(hw_mgt, speed);
> +	nbl_uvn_init(hw_mgt);
> +	nbl_uqm_init(hw_mgt);
> +	return 0;
> +}

[ ... ]

> +/*
> + * This design is intentional. Setting driver status to false is the
> + * official teardown mechanism: it notifies firmware to perform full
> + * cleanup of all per-PF hardware state, including qinfo registers.
> + * An inverse helper would duplicate work that the firmware already
> + * does, and would add error-path complexity for no benefit. We keep
> + * the deinit path minimal and rely on firmware cleanup for correctness,
> + * including abnormal hardware reset scenarios.
> + */
> +static void nbl_hw_deinit_module(struct nbl_hw_mgt *hw_mgt)
> +{
> +	nbl_hw_set_driver_status(hw_mgt, false);
> +	/* ensure registers written */
> +	nbl_flush_writes(hw_mgt);

[ ... ]

> +static int nbl_hw_init_module(struct nbl_hw_mgt *hw_mgt, u8 eth_speed,
> +			      u8 eth_num)
> +{
> +	int ret;
> +
> +	ret = nbl_dp_init(hw_mgt, eth_speed, eth_num);
> +	if (ret)
> +		return ret;
> +	nbl_intf_init(hw_mgt);
> +	nbl_hw_set_driver_status(hw_mgt, true);

[Severity: Medium]

nbl_hw_init_module() can return an error after a good part of the chip has
already been programmed, and nothing puts those registers back. There are
two failure points inside nbl_dp_init():

nbl_shaping_init() -> nbl_shaping_eth_init() returns -EINVAL from the
switch default for any speed outside 10/25/50/100G. By then the ports
earlier in the eth_bitmap loop have already been committed with VALID=1,
and NBL_DSCH_PSHA_EN plus the NBL_SHAPING_NET_REG() clearing loop are
skipped.

nbl_ustore_init() returns -EINVAL from
WARN_ON(eth_num != 1 && eth_num != 2 && eth_num != 4) after DPED, UPED,
shaping and DSCH were already programmed.

The speed comes from firmware, read by nbl_hw_get_board_info() out of
NBL_FW_BOARD_DW3, so a value outside enum nbl_fw_port_speed reaches
nbl_shaping_eth_init() without any earlier check.

In both cases the error is propagated before
nbl_hw_set_driver_status(hw_mgt, true), so firmware is never told a driver
attached and the firmware-side cleanup that deinit relies on is not
triggered either.

Should nbl_dp_init() unwind the sub-modules it has already programmed, or
should eth_speed and eth_num be validated before the first register write?

On the teardown side, the comment above nbl_hw_deinit_module() says
setting driver status to false makes firmware "perform full cleanup of all
per-PF hardware state, including qinfo registers", but the state this
patch programs is chip-wide rather than per-PF:

	shaping DPORT/DVN_DPORT entries with VALID=1
	NBL_DSCH_PSHA_EN
	USTORE per-port drop threshold EN
	DSTORE DISC_BP_TH EN and per-port FC_EN
	DVN/UVN relaxed-ordering flags
	UVN prefetch init and queue err mask
	UQM que_type

Most of those are programmed read-modify-write, for example:

	nbl_hw_rd_regs(hw_mgt, NBL_DSTORE_DISC_BP_TH, &bp_th, sizeof(bp_th));
	bp_th |= FIELD_PREP(NBL_DSTORE_DISC_BP_TH_EN_MASK, 1);
	nbl_hw_wr_regs(hw_mgt, NBL_DSTORE_DISC_BP_TH, &bp_th, sizeof(bp_th));

so after an unbind or a failed control PF probe, do these enables stay
armed and get carried into the next attach, or into another consumer such
as vfio-pci? Also, the relaxed-ordering bits are derived from the PCIe
DEVCTL value observed at init time only.

Could the commit message or the comment state which of these chip-wide
registers firmware actually reverts, so the claim can be checked against
the register list above?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260907123848.30256-1-illusion.wang%40nebula-matrix.com