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.
quoted hunk ↗ jump to hunk
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?
quoted hunk ↗ jump to hunk
@@ -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?
quoted hunk ↗ jump to hunk
@@ -1759,37 +1747,36 @@ static int netc_port_add_host_flood_rule(struct netc_port *np,
[ ... ]
quoted hunk ↗ jump to hunk
-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.
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
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