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

Simon Horman
2026-03-24T16:43:09Z

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: dsa: netc: initialize buffer bool table and implement flow-control

The buffer pool is a quantity of memory available for buffering a group
of flows (e.g. frames having the same priority, frames received from the
same port), while waiting to be transmitted on a port. The buffer pool
tracks internal memory consumption with upper bound limits and optionally
a non-shared portion when associated with a shared buffer pool. Currently
the shared buffer pool is not supported, it will be added in the future.

For i.MX94, the switch has 4 ports and 8 buffer pools, so each port is
allocated two buffer pools. For frames with priorities of 0 to 3, they
will be mapped to the first buffer pool; For frames with priorities of
4 to 7, they will be mapped to the second buffer pool. Each buffer pool
has a flow control on threshold and a flow control off threshold. By
setting these threshold, add the flow control support to each port.

> diff --git a/drivers/net/dsa/netc/netc_main.c b/drivers/net/dsa/netc/netc_main.c
> index 4b331971dd8c..2a7ad39f8bed 100644
> --- a/drivers/net/dsa/netc/netc_main.c
> +++ b/drivers/net/dsa/netc/netc_main.c

[ ... ]

> @@ -617,6 +628,80 @@ static int netc_add_standalone_fdb_bcast_entry(struct netc_switch *priv)
> bcast, NETC_STANDALONE_PVID);
> }
>
> +static u32 netc_get_buffer_pool_num(struct netc_switch *priv)
> +{
> + return netc_base_rd(&priv->regs, NETC_BPCAPR) & BPCAPR_NUM_BP;
> +}
> +
> +static void netc_port_set_pbpmcr(struct netc_port *np, u64 mapping)
> +{
> + u32 pbpmcr0 = lower_32_bits(mapping);
> + u32 pbpmcr1 = upper_32_bits(mapping);
> +
> + netc_port_wr(np, NETC_PBPMCR0, pbpmcr0);
> + netc_port_wr(np, NETC_PBPMCR1, pbpmcr1);
> +}
> +
> +static void netc_ipv_to_buffer_pool_mapping(struct netc_switch *priv)
> +{
> + int num_port_bp = priv->num_bp / priv->info->num_ports;
> + int q = NETC_IPV_NUM / num_port_bp;
> ^^^^

Can this cause a division by zero? If netc_get_buffer_pool_num() reads
the hardware register NETC_BPCAPR and returns 0, then num_port_bp would be
0 and this division would crash the kernel.

The call chain is:

netc_setup()
netc_switch_bpt_default_config()
priv->num_bp = netc_get_buffer_pool_num()
netc_ipv_to_buffer_pool_mapping()
num_port_bp = priv->num_bp / priv->info->num_ports;
q = NETC_IPV_NUM / num_port_bp; <- division by zero

Should netc_switch_bpt_default_config() validate that priv->num_bp is
non-zero before proceeding?

> + int r = NETC_IPV_NUM % num_port_bp;
> + int num = q + r;
> +
> + /* IPV-to–buffer-pool mapping per port:

[ ... ]

> +static int netc_switch_bpt_default_config(struct netc_switch *priv)
> +{
> + priv->num_bp = netc_get_buffer_pool_num(priv);
> + priv->bpt_list = devm_kcalloc(priv->dev, priv->num_bp,
> + sizeof(struct bpt_cfge_data),
> + GFP_KERNEL);
> + if (!priv->bpt_list)
> + return -ENOMEM;
> +
> + /* Initialize the maximum threshold of each buffer pool entry */
> + for (int i = 0; i < priv->num_bp; i++) {
> + struct bpt_cfge_data *cfge = &priv->bpt_list[i];
> + int err;
> +
> + cfge->max_thresh = cpu_to_le16(NETC_BP_THRESH);
> + err = ntmp_bpt_update_entry(&priv->ntmp, i, cfge);
> + if (err)
> + return err;
> + }
> +
> + netc_ipv_to_buffer_pool_mapping(priv);
> +
> + return 0;
> +}
> +

[ ... ]

> @@ -1224,6 +1313,40 @@ static void netc_port_set_hd_flow_control(struct netc_port *np, bool en)
> en ? PM_CMD_CFG_HD_FCEN : 0);
> }
>
> +static void netc_port_set_tx_pause(struct netc_port *np, bool tx_pause)
> +{
> + struct netc_switch *priv = np->switch_priv;
> + int port = np->dp->index;
> + int i, j, num_bp;
> +
> + num_bp = priv->num_bp / priv->info->num_ports;
> + for (i = 0, j = port * num_bp; i < num_bp; i++, j++) {
> + struct bpt_cfge_data *cfge;
> +
> + cfge = &priv->bpt_list[j];
> + if (tx_pause) {
> + cfge->fc_on_thresh = cpu_to_le16(NETC_FC_THRESH_ON);
> + cfge->fc_off_thresh = cpu_to_le16(NETC_FC_THRESH_OFF);
> + cfge->fccfg_sbpen = FIELD_PREP(BPT_FC_CFG,
> + BPT_FC_CFG_EN_BPFC);
> + cfge->fc_ports = cpu_to_le32(BIT(port));
> + } else {
> + cfge->fc_on_thresh = cpu_to_le16(0);
> + cfge->fc_off_thresh = cpu_to_le16(0);
> + cfge->fccfg_sbpen = 0;
> + cfge->fc_ports = cpu_to_le32(0);
> + }
> +
> + ntmp_bpt_update_entry(&priv->ntmp, j, cfge);
> ^^^^

Should the return value of ntmp_bpt_update_entry() be checked here?

Simon Horman noted in v1 review that other NTMP table operations in the
driver propagate errors from the update functions. While this was fixed
in netc_switch_bpt_default_config() in v2, the same issue remains here.

If the hardware update fails (for example, DMA allocation failure or
hardware timeout), the driver's bpt_list will have flow control values that
do not match the actual hardware state, potentially causing incorrect flow
control behavior.

https://lore.kernel.org/netdev/20260318145452.1898637-2-horms@kernel.org/

> + }
> +}
> +

[ ... ]