Re: [PATCH v33 net-next 4/8] 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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] nbl_dvn_init() and nbl_uvn_init() in nbl_hw_leonis.c turn the
  relaxed-ordering (RO) enables in two chip-wide registers on or off:…

--

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

This adds the Leonis chip-wide init_module() and deinit_module() hooks
in the hw and resource layers. They run only on the control PF. They
program the DP datapath registers (DPED, UPED, SHAPING, DSCH, USTORE,
DSTORE, DVN, UVN, UQM, PADPT) and drive the driver_status handshake
with firmware.

> 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 75d6982ae1e83..2b55f2c2518b4 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

[ ... ]

> @@ -150,6 +175,719 @@ static void nbl_hw_get_fw_eth_map(struct nbl_hw_mgt *hw_mgt, u32 *eth_map)

[ ... ]

> +static void nbl_dvn_init(struct nbl_hw_mgt *hw_mgt, u8 speed)
> +{
> +	u32 timeout = 0;
> +	u32 ro_flag = 0;
> +

[ ... ]

> +	spin_lock(&hw_mgt->reg_lock);
> +	nbl_hw_rd_regs(hw_mgt, NBL_DVN_DIF_REQ_RD_RO_FLAG, &ro_flag,
> +		       sizeof(ro_flag));
> +	if (pcie_relaxed_ordering_enabled(hw_mgt->common->pdev)) {
> +		ro_flag |=
> +			FIELD_PREP(NBL_DVN_DIF_REQ_RD_RO_FLAG_DESC_RO_EN_MASK,
> +				   1) |
> +			FIELD_PREP(NBL_DVN_DIF_REQ_RD_RO_FLAG_DATA_RO_EN_MASK,
> +				   1) |
> +			FIELD_PREP(NBL_DVN_DIF_REQ_RD_RO_FLAG_AVRING_RO_EN_MASK,
> +				   1);
> +	} else {

[ ... ]

> +	}
> +	nbl_hw_wr_regs(hw_mgt, NBL_DVN_DIF_REQ_RD_RO_FLAG, &ro_flag,
> +		       sizeof(ro_flag));
> +	spin_unlock(&hw_mgt->reg_lock);

[ ... ]

> +static void nbl_uvn_init(struct nbl_hw_mgt *hw_mgt)
> +{

[ ... ]

> +	ro_enabled = pcie_relaxed_ordering_enabled(hw_mgt->common->pdev);
> +
> +	nbl_hw_rd_regs(hw_mgt, NBL_UVN_DIF_REQ_RO_FLAG, &flag, sizeof(flag));
> +	if (ro_enabled) {
> +		flag |= FIELD_PREP(NBL_UVN_DIF_REQ_RO_FLAG_AVAIL_RD_MASK, 1) |
> +		FIELD_PREP(NBL_UVN_DIF_REQ_RO_FLAG_DESC_RD_MASK, 1) |
> +		FIELD_PREP(NBL_UVN_DIF_REQ_RO_FLAG_PKT_WR_MASK, 1);
> +		flag &= ~FIELD_PREP(NBL_UVN_DIF_REQ_RO_FLAG_DESC_WR_MASK, 1);
> +	} else {

[ ... ]

> +	}
> +	nbl_hw_wr_regs(hw_mgt, NBL_UVN_DIF_REQ_RO_FLAG, &flag, sizeof(flag));

[Severity: Medium]
Both of these RO flag registers seem to be single chip-wide registers
in the DP block, not per-function ones:

#define NBL_DVN_DIF_REQ_RD_RO_FLAG		(NBL_DP_DVN_BASE + 0x0000045C)
#define NBL_UVN_DIF_REQ_RO_FLAG			(NBL_DP_UVN_BASE + 0x00000250)

nbl_res_chip_init_module() in nbl_chip.c runs this only on the
control PF:

	if (!common->has_ctrl)
		return -EINVAL;

That means nbl_dvn_init() and nbl_uvn_init() check
PCI_EXP_DEVCTL_RELAX_EN only on the control PF's pdev, and that one bit
sets relaxed ordering for every function's queues on the chip.

In PCIe, Enable Relaxed Ordering is a per-function permission. Suppose
a sibling PF has RO cleared in its own DEVCTL, for example by a guest
that owns it through vfio-pci, by setpci, or by a driver applying an
erratum. If the control PF has RO enabled, would the DVN/UVN engines
still issue RO-tagged descriptor and avail-ring reads, and RO packet
writes, for that sibling's queues?

The value is also read only once, here. deinit_module() leaves it
programmed, so later DEVCTL changes on any function are never picked
up.

Does the hardware also gate the RO attribute on each requester
function's own DEVCTL? If not, could this use the AND of RO enable
across all functions on the chip, or program RO per function if the
hardware supports that? If the hardware does gate it per function,
could a comment here say so?

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