Re: [PATCH v4 net-next 13/14] net: dsa: netc: initialize buffer bool table and implement flow-control

Jakub Kicinski
2026-04-03T01:17:56Z

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: dsa: netc: initialize buffer bool table and implement flow-control

This commit adds buffer pool initialization and flow control support for the
NXP netc switch. It maps frame priorities to specific buffer pools and
configures transmission pause thresholds.

> diff --git a/drivers/net/dsa/netc/netc_main.c b/drivers/net/dsa/netc/netc_main.c
> index 3609d83ac363..688dceb486c2 100644
> --- a/drivers/net/dsa/netc/netc_main.c
> +++ b/drivers/net/dsa/netc/netc_main.c
[ ... ]
> @@ -386,6 +388,15 @@ static void netc_port_fixed_config(struct netc_port *np)
> /* Enable L2 and L3 DOS */
> netc_port_rmw(np, NETC_PCR, PCR_L2DOSE | PCR_L3DOSE,
> PCR_L2DOSE | PCR_L3DOSE);
> +
> + /* Set the quanta value of TX PAUSE frame */
> + netc_mac_port_wr(np, NETC_PM_PAUSE_QUANTA(0), pqnt);
> +
> + /* When a quanta timer counts down and reaches this value,
> + * the MAC sends a refresh PAUSE frame with the programmed
> + * full quanta value if a pause condition still exists.
> + */
> + netc_mac_port_wr(np, NETC_PM_PAUSE_TRHESH(0), qth);

This isn't a bug, but there's a typo in the macro name (TRHESH instead of
THRESH).

[ ... ]
> +static u32 netc_get_buffer_pool_num(struct netc_switch *priv)
> +{
> + /* The BPCAPR register is a read only register, the hardware
> + * guarantees that the BPCAPR_NUM_BP field will not be 0.
> + */
> + return netc_base_rd(&priv->regs, NETC_BPCAPR) & BPCAPR_NUM_BP;
> +}
[ ... ]
> +static void netc_ipv_to_buffer_pool_mapping(struct netc_switch *priv)
> +{
> + int bp_per_port = priv->num_bp / priv->info->num_ports;
> + int q = NETC_IPV_NUM / bp_per_port;
> + int r = NETC_IPV_NUM % bp_per_port;

If the hardware register read in netc_get_buffer_pool_num() returns a value
smaller than priv->info->num_ports (for instance, during a bus error or
reset state), bp_per_port will evaluate to 0.

Can this lead to a division by zero panic here?

Should priv->num_bp be validated against the number of ports before
performing this calculation to ensure the driver fails gracefully if the
hardware returns an unexpected value?

[ ... ]
> diff --git a/drivers/net/dsa/netc/netc_switch_hw.h b/drivers/net/dsa/netc/netc_switch_hw.h
> index c6a0c0a8ff8a..1e1c0d279a21 100644
> --- a/drivers/net/dsa/netc/netc_switch_hw.h
> +++ b/drivers/net/dsa/netc/netc_switch_hw.h
[ ... ]
> @@ -145,6 +152,12 @@ enum netc_stg_stage {
> #define NETC_PM_IEVENT(a) (0x1040 + (a) * 0x400)
> #define PM_IEVENT_RX_EMPTY BIT(6)
>
> +#define NETC_PM_PAUSE_QUANTA(a) (0x1054 + (a) * 0x400)
> +#define PAUSE_QUANTA_PQNT GENMASK(15, 0)
> +
> +#define NETC_PM_PAUSE_TRHESH(a) (0x1064 + (a) * 0x400)
> +#define PAUSE_TRHESH_QTH GENMASK(15, 0)

This isn't a bug, but there's a typo in the macro names here as well
(TRHESH instead of THRESH).