From: AI Reviewer <redacted>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
    octeontx2: add TC flow offload path for switch flows

This patch registers a TC flow offload path that translates flower rules
into fl_tuple state, resolves ingress/egress pcifunc via FIB, and forwards
the results to the AF over the PF mailbox.  A few observations follow
inline.

[High]
Is the change from u32 to u16 here intentional?  The next line builds
a value that spans bits 0..31:

	port_id = FIELD_PREP(GENMASK_ULL(31, 16), rep_id) |
		  FIELD_PREP(GENMASK_ULL(15, 0), pcifunc);

Storing that back into a u16 port_id drops the rep_id (bits 16..31)
before the return, so callers that stash the result as u32 (l2_entry,
l3_entry, fl_entry->port_id) always see zero in the upper half.  Would
that not defeat rep_id-based port disambiguation once rep_id != 0?

[ ... ]

+};
+
+static DEFINE_MUTEX(fl_offl_llock);
+static LIST_HEAD(fl_offl_lh);
+static bool fl_offl_work_running;
+
+static struct workqueue_struct *sw_fl_offl_wq;

[Low]
sw_fl_offl_wq is declared twice as a static in the same translation
unit.  Both declarations resolve to the same tentative definition, but
was the duplication intentional?

+int rvu_sw_fl_stats_sync2db(struct rvu *rvu, struct fl_info *fl, int cnt)
+{
+	struct npc_mcam_get_mul_stats_req *req = NULL;
+	struct npc_mcam_get_mul_stats_rsp *rsp = NULL;
+	int tot = 0;
+	u16 i2idx_map[256];
+	int rc = 0;
+	u64 pkts;
+	int idx;
+
+	if (cnt > 256)
+		return -ENOMEM;
+
+	for (int i = 0; i < cnt; i++) {
+		tot++;
+		if (fl[i].uni_di)
+			continue;
+
+		tot++;
+	}

[Critical]
Can this overflow the fixed-size arrays sized by cnt?  The guard only
bounds cnt at 256, but tot can grow up to 2 * cnt = 512 when every
entry has uni_di == 0.  Three arrays are then indexed by tot:

	u16 i2idx_map[256];             /* stack */
	req->entry[256];                /* struct npc_mcam_get_mul_stats_req */
	rsp->stat[256];                 /* struct npc_mcam_get_mul_stats_rsp */

+	req->cnt = tot;
+	idx = 0;
+	for (int i = 0; i < tot; idx++) {
+		i2idx_map[i] = idx;
+		req->entry[i++] = fl[idx].mcam_idx[0];
+		if (fl[idx].uni_di)
+			continue;
+
+		i2idx_map[i] = idx;
+		req->entry[i++] = fl[idx].mcam_idx[1];
+	}

[Critical]
With cnt = 256 and all uni_di = 0, would this loop write i2idx_map[0..511]
and req->entry[0..511], corrupting the kernel stack and the adjacent heap
region?  The subsequent read loop then reads rsp->stat[i++] beyond its
256-slot array too.  rvu_mbox_handler_npc_mcam_mul_stats() rejects
req->cnt > 256 later, but the OOB writes happen before that check.
Should the bound be cnt <= 128 (so tot <= 256), or should tot itself be
capped?

[Critical]
Separately, is cnt validated against the wire format?  struct
swdev2af_notify_req embeds struct fl_info fl[64], and
rvu_mbox_handler_swdev2af_notify() forwards req->cnt to this helper
without checking it.  For any 64 < cnt <= 256 the loops here read
fl[64..cnt-1] out of the mailbox payload into adjacent memory, then
propagate those bytes into req->entry[] and back through the mailbox.
Should the guard be cnt < 0 || cnt > ARRAY_SIZE(fl) rather than
cnt > 256 (cnt is a signed int, so negative values also slip through
today)?

+int rvu_mbox_handler_fl_notify(struct rvu *rvu,
+			       struct fl_notify_req *req,
+			       struct msg_rsp *rsp)
 {
+	struct fl_entry *fl_entry;

[ ... ]

+	mutex_lock(&fl_offl_llock);
+	list_add_tail(&fl_entry->list, &fl_offl_lh);
+
+	if (!fl_offl_work_running) {
+		sw_fl_offl_wq = alloc_workqueue("sw_af_fl_wq", 0, 0);
+		if (sw_fl_offl_wq)
+			fl_offl_work_running = true;
+	}
+	mutex_unlock(&fl_offl_llock);
+	queue_delayed_work(sw_fl_offl_wq, &fl_offl_work, msecs_to_jiffies(10));
+
 	return 0;
 }

[High]
On the very first invocation, if alloc_workqueue() returns NULL (for
example under memory pressure), fl_offl_work_running stays false and
sw_fl_offl_wq stays NULL, and queue_delayed_work(NULL, ...) is then
dispatched unconditionally after the mutex is dropped.  Does that not
NULL-deref inside __queue_delayed_work()?  Also, fl_entry has already
been list_add_tail'd, so if this path returns early there is nothing to
drain the list, and rvu_sw_fl_shutdown() early-returns on
!sw_fl_offl_wq, so the queued entry looks like it would leak.

+void rvu_sw_fl_shutdown(void)
+{
+	struct sw_fl_stats_node *snode, *tmp;
+	struct fl_entry *entry;
+	LIST_HEAD(tlist);
+
+	if (!sw_fl_offl_wq)
+		return;
+
+	cancel_delayed_work_sync(&fl_offl_work);
+	destroy_workqueue(sw_fl_offl_wq);

[ ... ]

+	mutex_lock(&sw_fl_stats_lock);
+	list_for_each_entry_safe(snode, tmp, &sw_fl_stats_lh, list) {
+		list_del_init(&snode->list);
+		kfree(snode);
+	}
+	mutex_unlock(&sw_fl_stats_lock);
+}

[High]
After destroy_workqueue(), sw_fl_offl_wq is not set to NULL and
fl_offl_work_running is not reset.  If a concurrent (or later)
rvu_mbox_handler_fl_notify() runs, it observes fl_offl_work_running ==
true, skips reallocation, and calls queue_delayed_work(sw_fl_offl_wq,
...) on a freed workqueue.  Is that intentional, or should shutdown
clear both under fl_offl_llock so subsequent handlers either reallocate
or bail out?

[Medium]
Also, the early return on !sw_fl_offl_wq skips the cleanup of
sw_fl_stats_lh below.  Nodes on that list are allocated by
rvu_sw_fl_stats_sync2db_one_entry() independently of whether the
offload workqueue ever came up, so on a shutdown where the workqueue
was never allocated they leak.  Would moving the stats-list cleanup
above the early return match the allocation lifetime?

[ ... ]

+static int sw_fl_parse_actions(struct otx2_nic *nic,

[ ... ]

+		case FLOW_ACTION_CT:
+			trace_sw_act_dump(__func__, __LINE__, act->id);
+			err = nf_flow_table_offload_add_cb(act->ct.flow_table,
+							   sw_fl_setup_ft_block_ingress_cb,
+							   nic);
+			if (err != -EEXIST && err) {
+				netdev_err(nic->netdev,
+					   "%s:%d Error to offload flow, err=%d\n",
+					   __func__, __LINE__, err);
+				break;
+			}
+
+			*op |= BIT_ULL(FLOW_ACTION_CT);
+			break;

[Medium]
On a CT offload error that is not -EEXIST, the code only prints and
breaks out of the switch without setting the CT bit and without
returning the error.  Processing continues; if any later action sets
*op (for example FLOW_ACTION_REDIRECT), sw_fl_add() sees a non-zero op
that lacks the CT bit, skips the CT early-return, and offloads the
rule as a plain REDIRECT.  Should the error be returned to the caller
so the block callback surfaces it, rather than silently converting a
CT rule into a non-CT one?

+static int sw_fl_stats(struct otx2_nic *nic, struct flow_cls_offload *f)
+{
+	struct fl_get_stats_req *req;
+	struct fl_get_stats_rsp *rsp;

[ ... ]

+	rc = otx2_sync_mbox_msg(&nic->mbox);
+	if (rc)
+		goto fail;
+
+	rsp = (struct fl_get_stats_rsp *)otx2_mbox_get_rsp
+		(&nic->mbox.mbox, 0, &req->hdr);
+	pkts_diff = rsp->pkts_diff;

[High]
Is an IS_ERR(rsp) check missing here?  otx2_mbox_get_rsp() returns
ERR_PTR(-ENODEV) on failure, and every other caller in this driver
(cn10k.c, cn10k_macsec.c, otx2_common.c, otx2_tc.c, ...) uses
IS_ERR(rsp) before dereferencing.  Without the check, an error return
here is dereferenced as a valid pointer.

+static bool init_done;
+
+int sw_fl_setup_ft_block_ingress_cb(enum tc_setup_type type,
+				    void *type_data, void *cb_priv)
+{
+	struct flow_cls_offload *cls = type_data;
+	struct otx2_nic *nic = cb_priv;
+
+	if (!init_done)
+		return 0;

[ ... ]

+}
+
 int sw_fl_init(void)
 {
+	INIT_WORK(&sw_fl_work, sw_fl_wq_handler);
+	sw_fl_wq = alloc_workqueue("sw_fl_wq", 0, 0);
+	if (!sw_fl_wq)
+		return -ENOMEM;
+
+	init_done = true;
 	return 0;
 }
 
 void sw_fl_deinit(void)
 {

[ ... ]

+	cancel_work_sync(&sw_fl_work);
+	destroy_workqueue(sw_fl_wq);

[ ... ]

 }

[Medium]
init_done and sw_fl_wq are used together as an ad-hoc gate: the
callback reads init_done as a plain load and then paths off that read
call queue_work(sw_fl_wq, ...).  Since the two stores are plain writes
in sw_fl_init(), can a weakly ordered CPU see init_done == true while
still seeing sw_fl_wq as NULL?  A smp_store_release() / smp_load_acquire()
pair, or moving the flag/wq under a real lock, would remove the
ordering hole.

[High]
Also, sw_fl_deinit() destroys sw_fl_wq without clearing init_done or
NULLing sw_fl_wq.  Since the callback is registered via
nf_flow_table_offload_add_cb() and TC block infrastructure, callbacks
that arrive after deinit will still see init_done == true and can
queue_work() on a freed workqueue.  Is there a missing callback
unregistration step here?

[Medium]
The trace system name rvu is already owned by
drivers/net/ethernet/marvell/octeontx2/af/rvu_trace.h, which files the
core AF-PF mailbox protocol events (otx2_msg_alloc / otx2_msg_send /
otx2_msg_process) under /sys/kernel/tracing/events/rvu/.  Adding a
second CREATE_TRACE_POINTS site in nic/switch/sw_trace.c under the
same system name mixes these driver-private debug events with the
pre-existing protocol events in user-visible tracefs.  Would a
distinct name (rvu_sw, octeontx2_sw, ...) avoid the collision?

+TRACE_EVENT(sw_fl_dump,
+	    TP_PROTO(const char *fname, int line, struct fl_tuple *ftuple),

[ ... ]

+	    TP_printk("[%s:%d] %pM %pI4:%u to %pM %pI4:%u eth_type=%#x proto=%u uni=%u in=%#x out=%#x",
+		      __get_str(f), __entry->l, __entry->smac, &__entry->sip, __entry->sport,

[ ... ]

+);
+
+TRACE_EVENT(sw_act_dump,
+	    TP_PROTO(const char *fname, int line, u32 act),

[ ... ]

+	    TP_printk("[%s:%d] %u",
+		      __get_str(fname), __entry->line, __entry->act)
+);

[Low]
These tracepoints capture __func__ and __LINE__ from the call site and
render them in TP_printk.  Since tracepoint output is treated as
stable ABI (see Documentation/trace/tracepoints.rst), any future
refactor in sw_fl.c will silently change the visible trace output.
Since the events are also EXPORT_TRACEPOINT_SYMBOL'd despite only being
used from this driver, would netdev_dbg / dynamic-debug fit the intent
better than TRACE_EVENT here?
-- 
This is an AI-generated review.