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
