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

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.

[ ... ]

+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