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

[Jakub Kicinski]

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).