Re: [PATCH net-next 2/8] eth: mpnic: add register init for the device

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · 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).

- [High] pci: missing pci_resource_len() check in mpnic_probe() allows
  OOB MMIO access
- [Medium] The 1 MiB Tx buffer is split between MAC and BMC differently
  in TXB and in TQS.

--

eth: mpnic: add register init for the device

This commit adds one-time CSR initialization to mpnic_probe() through a
new mpnic_dev_init(). It programs the TXB, RXB, TQS, descriptor SRAM,
AXI and relaxed ordering settings, starts the on-chip SRAM
initializations and polls until they complete.

> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_init.c b/drivers/net/ethernet/meta/mpnic/mpnic_init.c
> new file mode 100644
> index 0000000000000..f8ebb19766731
> --- /dev/null
> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_init.c
> @@ -0,0 +1,553 @@

[ ... ]

> +/* A total of 1 MiB worth of Tx credits is available, in units of 128 B.
> + * The BMC gets a guaranteed share of them whether or not the host is
> + * routing anything its way, everything else goes to MAC TC0.
> + */
> +#define MPNIC_TXB_BMC_PVT_CRDT_INIT_VAL		800
> +#define MPNIC_TXB_P0_MAC_PVT_CRDT_INIT_VAL	\
> +	(SZ_1M / 128 - 2 * MPNIC_TXB_BMC_PVT_CRDT_INIT_VAL)

[ ... ]

> +/* The 1 MiB Tx buffer is partitioned between the MAC and the BMC in units
> + * of 1 KiB. The BMC portion is fixed at 100 KB.
> + */
> +#define MPNIC_TQS_GLBL_TXB_CRDT_BMC	100
> +#define MPNIC_TQS_GLBL_TXB_CRDT_MAC	(SZ_1M / SZ_1K - \
> +					 MPNIC_TQS_GLBL_TXB_CRDT_BMC)

[ ... ]

> +static void mpnic_txb_init(struct mpnic_dev *mpd)
> +{
> +	int i;
> +
> +	mpnic_wr64(mpd, MPNIC_TXB_BMC, MPNIC_TXB_BMC_PVT_CRDT_INIT_VAL);
> +
> +	/* Zero the private credits of every traffic class, then hand the
> +	 * unreserved ones to MAC TC0.
> +	 */
> +	for (i = 0; i < MPNIC_TXB_P0_CNT; i++)
> +		mpnic_wr64(mpd, MPNIC_TXB_P0(i), 0);
> +	mpnic_wr64(mpd, MPNIC_TXB_P0(MPNIC_TXB_TC_IDX_MAC_0),
> +		   MPNIC_TXB_P0_MAC_PVT_CRDT_INIT_VAL);
> +	mpnic_wr64(mpd, MPNIC_TXB_P0(MPNIC_TXB_TC_IDX_BMC),
> +		   MPNIC_TXB_BMC_PVT_CRDT_INIT_VAL);

[ ... ]

> +static void mpnic_tqs_init(struct mpnic_dev *mpd)
> +{

[ ... ]

> +	/* Only port 0 is used. A single traffic class is in use as well, so
> +	 * give all of the Tx buffer credits to TC0.
> +	 */
> +	mpnic_wr64(mpd, MPNIC_TQS_GLBL_P0_0,
> +		   FIELD_PREP(MPNIC_TQS_GLBL_P0_0_TXB_MAX_CRDTS_0,
> +			      MPNIC_TQS_GLBL_TXB_CRDT_MAC));
> +	mpnic_wr64(mpd, MPNIC_TQS_GLBL_P0_1, 0);
> +	mpnic_wr64(mpd, MPNIC_TQS_GLBL_BMC,
> +		   FIELD_PREP(MPNIC_TQS_GLBL_BMC_TXB_MAX_CRDTS,
> +			      MPNIC_TQS_GLBL_TXB_CRDT_BMC));

[Severity: Medium]
Do TXB and TQS split the 1 MiB Tx buffer the same way?

On the TXB side, mpnic_txb_init() writes MPNIC_TXB_BMC_PVT_CRDT_INIT_VAL
(800 x 128 B = 100 KiB) to both MPNIC_TXB_BMC and
MPNIC_TXB_P0(MPNIC_TXB_TC_IDX_BMC). MAC TC0 then gets:

  SZ_1M / 128 - 2 * 800 = 6592 credits = 824 KiB

On the TQS side, mpnic_tqs_init() programs
MPNIC_TQS_GLBL_P0_0_TXB_MAX_CRDTS_0 with:

  SZ_1M / SZ_1K - 100 = 924 (1 KiB units) = 924 KiB

That lets the scheduler use 100 KiB more for TC0 than TXB reserved as
TC0 private credit.

Suppose TXB_MAX_CRDTS limits how much data the scheduler can have
outstanding in the TC0 TXB pool. Could a Tx burst that fills TC0 then
overrun the pool once the Tx datapath is running?

If the BMC only needs a single 100 KiB share, as the TQS comment says,
does the "2 *" in MPNIC_TXB_P0_MAC_PVT_CRDT_INIT_VAL leave 100 KiB of Tx
buffer unused?

The TXB comment says "everything else goes to MAC TC0", but the code
subtracts the BMC share twice, so the two don't match either.

[ ... ]

> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_pci.c b/drivers/net/ethernet/meta/mpnic/mpnic_pci.c
> --- a/drivers/net/ethernet/meta/mpnic/mpnic_pci.c
> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_pci.c
> @@ -108,6 +112,10 @@ static int mpnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	pci_set_master(pdev);
>  	pci_save_state(pdev);
>  
> +	err = mpnic_dev_init(mpd);
> +	if (err)
> +		goto err_free_mpd;
> +
>  	return 0;
>  
>  err_free_mpd:
>

[Severity: High]
mpnic_dev_init() reaches much further into BAR 0 than anything the
driver touched before. Before this patch, the furthest access was
MPNIC_BDQ_SPARE in mpnic_rd64(). Now the TXB, RXB, RPC, ROF, TOF and
PEMO_WRAPPER blocks are all programmed.

The highest one is MPNIC_OB_ATTR_RDE_P in mpnic_ro_init(). It is
0x9a001a in u32 units, so the writeq() lands at byte offset 0x2680068.
BAR 0 therefore has to be at least ~38.5 MiB for this sequence to stay
inside the mapping.

mpnic_probe() maps BAR 0 with pcim_iomap_region(), which accepts
whatever length the device advertised. Nothing checks
pci_resource_len(pdev, 0) against the register file the driver is
about to use. mpnic_wr64() and mpnic_rd64() only test that uc_addr0
is non-NULL, then add the register index unconditionally.

Suppose a device presents a BAR 0 smaller than that. It could be a
misconfigured board, or something spoofing this ID behind
Thunderbolt/USB4. Won't the first out-of-range writeq() in
mpnic_dev_init() fault on an unmapped address during probe, or
scribble over whatever happens to be mapped after the BAR?

Could mpnic_probe() reject the device with -ENODEV before calling
mpnic_dev_init() when pci_resource_len(pdev, 0) is too small? Does the
hardware have a documented BAR 0 size that could serve as the bound?
That seems better than deriving one from the highest CSR currently
defined in mpnic_csr.h, which will keep moving as more blocks are
added.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922-linux-mpnic-v1-0-236844f53072%40gmail.com