Re: [PATCH v3 net-next 4/8] net: dsa: netc: use entry ID instead of pointer to track host flood rule

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 0 · Low: 2 (1)

- [Low] struct netc_port is devm_kzalloc()'d, so the new `u32
  ipft_hf_eid` starts at 0, but the patch only writes the…
- [Low] Removing netc_free_host_flood_rules() also removes the only in-
  tree documentation of a deliberate design decision: 'No need to clear…

Pre-existing issues:
- [Low] netc_port_remove_host_flood() discards the return status of
  ntmp_ipft_delete_entry() (drivers/net/dsa/netc/netc_main.c:1776) and…

--

net: dsa: netc: use entry ID instead of pointer to track host flood rule

This replaces the struct ipft_entry_data pointer in struct netc_port with a
u32 entry ID using NTMP_NULL_ENTRY_ID as the sentinel, frees the
ipft_entry_data allocation inside netc_port_add_host_flood_rule() once the
hardware entry is committed, and drops netc_free_host_flood_rules().

A few questions below.

> diff --git a/drivers/net/dsa/netc/netc_main.c b/drivers/net/dsa/netc/netc_main.c
> index 77077352c1a57..d326a00104e1d 100644
> --- a/drivers/net/dsa/netc/netc_main.c
> +++ b/drivers/net/dsa/netc/netc_main.c
> @@ -286,6 +286,12 @@ static int netc_init_all_ports(struct netc_switch *priv)
>  				dev_err(dev, "Failed to create MDIO bus\n");
>  				return err;
>  			}
> +
> +			/* The ipft_hf_eid is initialized to an invalid entry
> +			 * ID because the host flood rule (IPFT entry) has not
> +			 * been created.
> +			 */
> +			np->ipft_hf_eid = NTMP_NULL_ENTRY_ID;
>  		}
>  	}

[Severity: Low]
Should the sentinel be written for every port rather than only inside the
dsa_port_is_user(dp) branch?

Each netc_port is devm_kzalloc()'d in the earlier loop of
netc_init_all_ports(), so ipft_hf_eid starts at 0, which is a legal
hardware IPFT entry ID rather than "no rule".  The old field was a pointer,
where the zeroed value (NULL) was inherently the safe "no rule" marker.

netc_port_remove_host_flood() only recognizes the 0xffffffff sentinel:

	if (entry_id == NTMP_NULL_ENTRY_ID)
		return;
	...
	ntmp_ipft_delete_entry(&priv->ntmp, entry_id);

so a port that never got the assignment would pass that guard and delete
hardware entry ID 0, which may belong to another port.

I could not find a path that reaches netc_port_remove_host_flood() or
netc_port_bridge_join() with a non-user port today - dsa_port_set_host_flood()
comes only from dsa_user_manage_host_flood(), and the bridge join/leave
callers all operate on user ports - so this looks latent rather than
reachable.  Would moving the assignment into the devm_kzalloc() loop that
sets np->switch_priv and np->iobase make the representation fail-safe for
all ports?

> @@ -938,30 +944,12 @@ static void netc_destroy_all_lists(struct netc_switch *priv)
>  	mutex_destroy(&priv->vft_lock);
>  }
>  
> -static void netc_free_host_flood_rules(struct netc_switch *priv)
> -{
> -	struct dsa_port *dp;
> -
> -	dsa_switch_for_each_user_port(dp, priv->ds) {
> -		struct netc_port *np = priv->ports[dp->index];
> -
> -		/* No need to clear the hardware IPFT entry. Because PCIe
> -		 * FLR will be performed when the switch is re-registered,
> -		 * it will reset hardware state. So only need to free the
> -		 * memory to avoid memory leak.
> -		 */
> -		kfree(np->host_flood);
> -		np->host_flood = NULL;
> -	}
> -}
> -
>  static void netc_teardown(struct dsa_switch *ds)
>  {
>  	struct netc_switch *priv = ds->priv;
>  
>  	disable_delayed_work_sync(&priv->fdbt_ageing_work);
>  	netc_destroy_all_lists(priv);
> -	netc_free_host_flood_rules(priv);
>  	netc_free_ntmp_user(priv);
>  }

[Severity: Low]
Could the deleted rationale be preserved somewhere?  Behaviour is unchanged
(netc_teardown() still leaves the host flood IPFT entries programmed in
hardware), but this hunk removes the only in-tree record of why that is
intentional:

	/* No need to clear the hardware IPFT entry. Because PCIe
	 * FLR will be performed when the switch is re-registered,
	 * it will reset hardware state. ...
	 */

Note that the FLR assumption holds for a PCI re-probe, where
netc_switch_pci_probe() calls pcie_flr(pdev), but a DSA teardown/setup cycle
on the same pci_dev does not perform FLR.  Would it help to keep the comment
in netc_teardown() or next to the ipft_hf_eid field so the assumption can be
re-checked later?

> @@ -1759,37 +1747,36 @@ static int netc_port_add_host_flood_rule(struct netc_port *np,

[ ... ]

> -static void netc_port_remove_host_flood(struct netc_port *np,
> -					struct ipft_entry_data *host_flood)
> +static void netc_port_remove_host_flood(struct netc_port *np, u32 entry_id)
>  {
>  	struct netc_switch *priv = np->switch_priv;
>  	bool disable_host_flood = false;
>  
> -	if (!host_flood)
> +	if (entry_id == NTMP_NULL_ENTRY_ID)
>  		return;
>  
> -	if (np->host_flood == host_flood)
> +	if (np->ipft_hf_eid == entry_id)
>  		disable_host_flood = true;
>  
> -	ntmp_ipft_delete_entry(&priv->ntmp, host_flood->entry_id);
> -	kfree(host_flood);
> +	ntmp_ipft_delete_entry(&priv->ntmp, entry_id);
>  
>  	if (disable_host_flood) {
> -		np->host_flood = NULL;
> +		np->ipft_hf_eid = NTMP_NULL_ENTRY_ID;
>  		np->uc = false;
>  		np->mc = false;
>  		netc_port_wr(np, NETC_PIPFCR, 0);

[Severity: Low]
This is a pre-existing issue and not introduced by this patch, but can the
IPFT entry become unremovable here?

ntmp_ipft_delete_entry() can fail (DMA buffer allocation or command BD
error), and its status is discarded before np->ipft_hf_eid is reset to
NTMP_NULL_ENTRY_ID, so the armed entry (FLTFA=REDIRECT, HR=NETC_HR_HOST_FLOOD)
stays in hardware with no software handle left.

This appears to be addressed by the following patch in the series, which
makes netc_port_remove_host_flood() return int, checks the delete status
before clearing np->ipft_hf_eid/uc/mc, and handles the error at all call
sites.

> @@ -1818,20 +1805,18 @@ static void netc_port_set_host_flood(struct dsa_switch *ds, int port,
>  
>  	/* IPFT does not support in-place updates to the KEYE element,
>  	 * we need to add a new entry and then delete the old one. So
> -	 * save the old entry first.
> +	 * save the old entry ID first.
>  	 */
> -	old_host_flood = np->host_flood;
> -	np->host_flood = NULL;
> +	old_entry_id = np->ipft_hf_eid;
>  
>  	if (netc_port_add_host_flood_rule(np, uc, mc)) {
> -		np->host_flood = old_host_flood;
>  		dev_err(ds->dev, "Failed to add host flood rule on port %d\n",
>  			port);
>  		return;
>  	}
>  
>  	/* Remove the old host flood entry */
> -	netc_port_remove_host_flood(np, old_host_flood);
> +	netc_port_remove_host_flood(np, old_entry_id);
>  }

[Severity: Low]
Same pre-existing concern in this key-update flow, which this patch does not
change: because the new entry is added before the old one is deleted, a
failed delete leaves two live IPFT entries matching the same source port with
PIPFCR still enabled.  The subsequent patch in the series inverts the order to
delete-then-add, which closes that window.

> diff --git a/drivers/net/dsa/netc/netc_switch.h b/drivers/net/dsa/netc/netc_switch.h
> index 305f2a92e2f9a..020e3661b8667 100644
> --- a/drivers/net/dsa/netc/netc_switch.h
> +++ b/drivers/net/dsa/netc/netc_switch.h
> @@ -84,7 +84,11 @@ struct netc_port {
>  	u16 uc:1;
>  	u16 mc:1;
>  	u16 pvid;
> -	struct ipft_entry_data *host_flood;
> +	/* ipft_hf_eid applies only to user ports and should be initialized
> +	 * to NTMP_NULL_ENTRY_ID. Other ports (such as CPU ports) do not
> +	 * require initialization.
> +	 */
> +	u32 ipft_hf_eid;
>  };

[Severity: Low]
This comment records the partial initialization discussed above as an
invariant.  Since 0 is a valid IPFT entry ID, would it be safer to
initialize ipft_hf_eid for all ports and drop the "other ports ... do not
require initialization" wording?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831082335.1184605-1-wei.fang%40oss.nxp.com