Re: [RFC PATCH net-next 2/2] icssg: Add ethtool RX flow classification support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 13 potential issue(s) to consider:
- [Critical] net: ti: icssg: heap overflow and NULL deref in icssg_rxnfc_rule_get_all()
- [High] net: ti: icssg: icssg_rxnfc_classifier_update() clobbers HSR classifiers
- [High] icssg: uninitialized rxnfc lock and list on SR1.0 via icssg_ethtool_ops
- [High] icssg: RXNFC rules wiped on open/restart and deleted across suspend
- [High] icssg: icssg_rxnfc_classifier_update() clobbers HSR/PRP classifiers
- [High] icssg: premature page_pool_destroy() and FDQ race in prueth_destroy_rxq()
- [High] icssg: napi_disable() hang on prueth_create_rxq() error and FDQ overflow
- [Medium] net: ti: icssg: missing location validation in icssg_rxnfc_rule_add()
- [Medium] net: ti: icssg: unvalidated ether_spec masks and queue 0 in validate_rule()
- [Medium] net: ti: icssg: unreachable ETHTOOL_GRXRINGS in emac_get_rxnfc()
- [Medium] icssg: memory leak of rxnfc rules in prueth_netdev_exit()
- [Low] net: ti: icssg: sparse endianness mismatch in icssg_ft3_program_filter()
- [Low] icssg: undocumented UDMA RX completion drain in prueth_destroy_rxq()
--

commit 21ba5b8fca0b052692d50b8d741061eff437183c
Author: Meghana Malladi <m-malladi@ti.com>

icssg: Add ethtool RX flow classification support

This commit adds ethtool get_rxnfc and set_rxnfc support to the ICSSG
driver to steer packets to RX flows using FT3 hardware filters and RX
classifier OR tables, and drains the UDMA RX completion ring during RX
queue teardown.

>     icssg: Add ethtool RX flow classification support
>     
>     Enable dynamic RX flow classification via ethtool set_rxnfc/get_rxnfc.
>     Users can install/remove filter rules to steer packets to specific RX flows
>     based on EtherType patterns via FT3 hardware filters.
>     
>     - Add RXNFC rule tracking structures and lifecycle management
>     - Implement ethtool get_rxnfc/set_rxnfc callbacks
>     - FT3 slot allocation and hardware programming for pattern matching
>     - RX classifier OR table updates based on active rules
>     - Max 15 rules per EMAC, using FT3 slots 0-13,15 (slot 14 reserved)
>     
>     Signed-off-by: Meghana Malladi <m-malladi@ti.com>

[Severity: Low]
This isn't a bug, but prueth_destroy_rxq() in
drivers/net/ethernet/ti/icssg/icssg_prueth.c also adds a loop to drain the
UDMA RX completion ring before calling k3_udma_glue_reset_rx_chn() so that
page_pool_destroy() does not stall waiting for inflight pages. Should that
change be documented in the commit message or split into a separate patch?

> diff --git a/drivers/net/ethernet/ti/icssg/icssg_classifier.c b/drivers/net/ethernet/ti/icssg/icssg_classifier.c
> index 87a8577093dbb..f11539611df75 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_classifier.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_classifier.c
[ ... ]
> +/* Program FT3 filter slot with ethertype pattern */
> +static void icssg_ft3_program_filter(struct regmap *miig_rt, int slice,
> +				     int slot, struct icssg_rxnfc_rule *rule)
> +{
> +	const struct ethtool_rx_flow_spec *fs = &rule->flow_spec;
> +	u16 ether_type;
> +
> +	/* Extract EtherType from flow spec (network byte order) */
> +	ether_type = fs->h_u.ether_spec.h_proto;

[Severity: Low]
Does assigning fs->h_u.ether_spec.h_proto (which is __be16) directly to u16
ether_type trigger a sparse endianness warning when building
drivers/net/ethernet/ti/icssg/icssg_classifier.c with C=1, while the
(__force u32) cast further down in icssg_ft3_program_filter() is applied to
what is already a plain u16?

> +
> +	/* Deactivate filter before programming */
> +	regmap_write(miig_rt, FT3_N_REG(slice, slot, FT3_CFG), 0);
[ ... ]
> +static void icssg_rxnfc_classifier_update(struct prueth_emac *emac)
> +{
> +	struct prueth *prueth = emac->prueth;
> +	struct regmap *miig_rt = prueth->miig_rt;
> +	int slice = prueth_emac_slice(emac);
> +	u32 or_table[ICSSG_NUM_CLASSIFIERS] = { };
> +	struct icssg_rxnfc_rule *rule;
> +	int n;
> +
> +	if (slice < 0)
> +		return;
> +
> +	/* Build per-classifier OR table: classifier index = target queue index */
> +	list_for_each_entry(rule, &emac->rxnfc.rules, list) {
> +		int cls = rule->queue_index;
> +
> +		if (cls < ICSSG_NUM_CLASSIFIERS)
> +			or_table[cls] |= RX_CLASS_FT_FT3_MATCH(rule->ft3_slot);
> +	}
> +
> +	/* Write OR tables for user-rule classifiers; skip classifier 0 (default flow).
> +	 * Use RX_CLASS_N_REG with RX_CLASS_OR_EN — NOT RX_CLASS_OR_REG which uses
> +	 * rx_class_or_base (= rx_class_base + 4), causing an off-by-4 that lands on
> +	 * AND_EN of classifier n+1 instead of OR_EN of classifier n.
> +	 */
> +	for (n = 1; n < ICSSG_NUM_CLASSIFIERS; n++) {
> +		regmap_write(miig_rt,
> +			     RX_CLASS_N_REG(slice, n, RX_CLASS_OR_EN),
> +			     or_table[n]);
> +	}
> +}

[Severity: High]
Won't looping up to ICSSG_NUM_CLASSIFIERS (16) in
icssg_rxnfc_classifier_update() overwrite the HSR/PRP classifiers with 0?

In icssg_ft3_hsr_configurations(), HSR/PRP offload configures FT3_PTP_SLOT
(slot 14) and programs CLASSIFIER_PTP_DUP (classifier 10) and
CLASSIFIER_HSR_TAG (classifier 11):

drivers/net/ethernet/ti/icssg/icssg_classifier.c:icssg_ft3_hsr_configurations() {
	...
	regmap_write(miig_rt, RX_CLASS_OR_REG(slice, CLASSIFIER_PTP_DUP, RX_CLASS_AND_EN),
		     RX_CLASS_OR_DUP_PTP);
	...
	if (prueth->hsr_prp_version != PRP_V1) {
		...
		regmap_write(miig_rt, RX_CLASS_OR_REG(slice, CLASSIFIER_HSR_TAG, RX_CLASS_AND_EN),
			     RX_CLASS_OR_HSR_TAG);
	...
}

Although FT3 slot 14 is reserved via ICSSG_FT3_RESERVED_SLOTS, user RX
queues are bounded by emac->rx_flow_num <= ICSSG_MAX_RFLOWS (8, classifiers
0..7), so or_table[8..15] are always 0. When a rule is added or deleted via
emac_set_rxnfc() -> icssg_rxnfc_rule_add() / icssg_rxnfc_rule_del() ->
icssg_rxnfc_classifier_update(), writing or_table[n] for n = 1..15 zeroes
RX_CLASS_OR_EN on classifiers 10 and 11.

In addition, when prueth_emac_common_start() runs during emac_ndo_open() or
prueth_emac_restart(), it calls icssg_class_default() ->
icssg_class_disable(), which zeroes RX_CLASS_OR_EN across all classifiers
without calling icssg_rxnfc_classifier_update() to restore active rules in
emac->rxnfc.rules.

> +
> +/* Validate rule parameters */
> +static int validate_rule(struct prueth_emac *emac,
> +			 struct ethtool_rx_flow_spec *fs)
> +{
> +	/* Only ETHER_FLOW supported for Phase 1 */
> +	if (fs->flow_type != ETHER_FLOW)
> +		return -EOPNOTSUPP;
> +
> +	/* Check RX queue is valid */
> +	if (fs->ring_cookie >= emac->rx_flow_num)
> +		return -EINVAL;
> +
> +	return 0;
> +}

[Severity: Medium]
What happens in validate_rule() when userspace passes an ETHER_FLOW rule
with MAC address masks or a partial EtherType mask in fs->m_u.ether_spec,
flow_type extension flags (FLOW_EXT or FLOW_MAC_EXT), or fs->ring_cookie ==
0?

In validate_rule(), fs->m_u.ether_spec.h_dest, fs->m_u.ether_spec.h_source,
and fs->m_u.ether_spec.h_proto are not checked before
icssg_ft3_program_filter() programs FT3_T with fs->h_u.ether_spec.h_proto
and FT3_T_MASK with FT3_ETHERTYPE_MASK, so MAC filter requests are silently
accepted and programmed as EtherType matches (matching 0x0000 if h_proto was
not set).

Also, validate_rule() allows fs->ring_cookie == 0 (which is the only valid
queue under the default emac->rx_flow_num = 1 in prueth_netdev_init()). A
rule with ring_cookie == 0 allocates an FT3 slot and sets or_table[0] in
icssg_rxnfc_classifier_update(), but the register write loop starts at
n = 1 and never writes or_table[0] to hardware.

[ ... ]
> +/* Cleanup RX flow classification */
> +void icssg_rxnfc_rule_cleanup(struct prueth_emac *emac)
> +{
> +	struct prueth *prueth = emac->prueth;
> +	struct regmap *miig_rt = prueth->miig_rt;
> +	int slice = prueth_emac_slice(emac);
> +	struct icssg_rxnfc_rule *rule, *tmp;
> +	int n;
> +
> +	if (slice < 0)
> +		return;
> +
> +	mutex_lock(&emac->rxnfc.rxnfc_lock);
> +
> +	/* Disable all active FT3 filters */
> +	list_for_each_entry_safe(rule, tmp, &emac->rxnfc.rules, list) {
> +		icssg_ft3_disable_filter(miig_rt, slice, rule->ft3_slot);
> +		free_ft3_slot(emac, rule->ft3_slot);
> +		list_del(&rule->list);
> +		kfree(rule);
> +	}
> +
> +	emac->rxnfc.rxnfc_rule_count = 0;
> +	emac->rxnfc.ft3_slots_used = ICSSG_FT3_RESERVED_SLOTS;
> +
> +	/* Clear all user-rule classifier OR tables (classifier 0 = default, skip) */
> +	for (n = 1; n < ICSSG_NUM_CLASSIFIERS; n++)
> +		regmap_write(miig_rt,
> +			     RX_CLASS_N_REG(slice, n, RX_CLASS_OR_EN), 0);
> +
> +	mutex_unlock(&emac->rxnfc.rxnfc_lock);
> +}
> +EXPORT_SYMBOL_GPL(icssg_rxnfc_rule_cleanup);

[Severity: High]
Similar to icssg_rxnfc_classifier_update(), doesn't the loop in
icssg_rxnfc_rule_cleanup() clear RX_CLASS_N_REG(slice, n, RX_CLASS_OR_EN)
for n = 1..15 (ICSSG_NUM_CLASSIFIERS - 1) instead of stopping at
ICSSG_MAX_RFLOWS - 1 (7)?

Because offs[slice].rx_class_or_base is offs[slice].rx_class_base + 4
(RX_CLASS_OR_EN), RX_CLASS_N_REG(slice, n, RX_CLASS_OR_EN) is the same
register as RX_CLASS_OR_REG(slice, n, RX_CLASS_AND_EN) programmed by
icssg_ft3_hsr_configurations() for CLASSIFIER_PTP_DUP (10) and
CLASSIFIER_HSR_TAG (11), so calling icssg_rxnfc_rule_cleanup() clobbers both
HSR/PRP classifiers.

> +
> +/* Add a new RX flow classification rule */
> +int icssg_rxnfc_rule_add(struct prueth_emac *emac, struct ethtool_rxnfc *info)
> +{
[ ... ]
> +	/* Check for duplicate */
> +	if (rule_exists(emac, fs)) {
> +		ret = -EEXIST;
> +		goto unlock;
> +	}
> +
> +	/* Allocate FT3 slot */
> +	ft3_slot = allocate_ft3_slot(emac);
> +	if (ft3_slot < 0) {
> +		ret = -ENOSPC;
> +		goto unlock;
> +	}
> +
> +	/* Auto-assign location from FT3 slot if RX_CLS_LOC_ANY requested */
> +	if (fs->location == RX_CLS_LOC_ANY)
> +		fs->location = ft3_slot;
> +
> +	/* Allocate rule structure */
> +	rule = kzalloc(sizeof(*rule), GFP_KERNEL);
> +	if (!rule) {
> +		ret = -ENOMEM;
> +		free_ft3_slot(emac, ft3_slot);
> +		goto unlock;
> +	}
> +
> +	/* Program FT3 hardware */
> +	rule->flow_spec = *fs;
> +	rule->ft3_slot = ft3_slot;
> +	rule->queue_index = fs->ring_cookie;
> +	rule->location = fs->location;

[Severity: Medium]
How are explicit or special fs->location values validated in
icssg_rxnfc_rule_add()?

In emac_get_rxnfc(), ETHTOOL_GRXCLSRLCNT sets info->data to
ICSSG_RXNFC_MAX_RULES | RX_CLS_LOC_SPECIAL (advertising 15 locations,
0..14, plus special location specifiers). However:

1. Neither validate_rule() nor icssg_rxnfc_rule_add() validates fs->location
   when fs->location != RX_CLS_LOC_ANY, so RX_CLS_LOC_FIRST,
   RX_CLS_LOC_LAST, reserved slot 14, or values >= ICSSG_RXNFC_MAX_RULES are
   stored directly in rule->location.
2. When fs->location == RX_CLS_LOC_ANY, allocate_ft3_slot() skips reserved
   slot 14 and returns ft3_slot = 15 for the 15th rule, assigning
   fs->location = 15 (>= ICSSG_RXNFC_MAX_RULES).
3. Because allocate_ft3_slot() picks the lowest free FT3 slot independently
   of an explicit fs->location and rule_exists() only checks flow_type and
   h_proto, adding a rule with explicit location 1 (getting ft3_slot = 0,
   rule->location = 1) followed by a rule with RX_CLS_LOC_ANY (getting
   ft3_slot = 1, rule->location = 1) or another rule with location 1 creates
   duplicate rule->location entries in emac->rxnfc.rules. Since
   icssg_rxnfc_rule_get() and icssg_rxnfc_rule_del() stop at the first match
    from list_add(), the older rule is shadowed and also hidden from
   ethtool_get_max_rxnfc_channel() when reducing channel count.

[ ... ]
> +/* Get all RX flow classification rules */
> +int icssg_rxnfc_rule_get_all(struct prueth_emac *emac, u32 *rule_locs,
> +			     u32 *rule_cnt)
> +{
> +	struct icssg_rxnfc_rule *rule;
> +	u32 count = 0;
> +
> +	mutex_lock(&emac->rxnfc.rxnfc_lock);
> +
> +	list_for_each_entry(rule, &emac->rxnfc.rules, list) {
> +		rule_locs[count++] = rule->location;
> +	}
> +
> +	*rule_cnt = count;
> +
> +	mutex_unlock(&emac->rxnfc.rxnfc_lock);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(icssg_rxnfc_rule_get_all);

[Severity: Critical]
Can icssg_rxnfc_rule_get_all() dereference a NULL rule_locs pointer or
overflow the rule_locs[] heap buffer when called from emac_get_rxnfc() for
ETHTOOL_GRXCLSRLALL?

In net/ethtool/ioctl.c:ethtool_get_rxnfc(), when info.cmd ==
ETHTOOL_GRXCLSRLALL, the kernel allocates rule_buf based on the
user-supplied info.rule_cnt (or leaves rule_buf as NULL if info.rule_cnt ==
0):

net/ethtool/ioctl.c:ethtool_get_rxnfc() {
	...
	if (info.cmd == ETHTOOL_GRXCLSRLALL) {
		if (info.rule_cnt > 0) {
			if (info.rule_cnt <= KMALLOC_MAX_SIZE / sizeof(u32))
				rule_buf = kcalloc(info.rule_cnt, sizeof(u32),
						   GFP_USER);
			if (!rule_buf)
				return -ENOMEM;
		}
	}
	...
	ret = ops->get_rxnfc(dev, &info, rule_buf);
}

In emac_get_rxnfc(), the ETHTOOL_GRXCLSRLALL case ignores the caller's
info->rule_cnt capacity, passes a local rule_cnt = 0 to
icssg_rxnfc_rule_get_all(), and does not set info->data to the rule table
size (ICSSG_RXNFC_MAX_RULES). Inside icssg_rxnfc_rule_get_all(), the loop
writes rule_locs[count++] = rule->location for every active rule without
checking if rule_locs is NULL or if count has reached the caller's
info->rule_cnt capacity (where it should return -EMSGSIZE).

If rules are installed and userspace passes info.rule_cnt == 0,
rule_locs[count++] dereferences NULL. If userspace passes 0 < info.rule_cnt
< emac->rxnfc.rxnfc_rule_count, rule_locs[count++] writes past the end of
the kcalloc() buffer and updates info->rule_cnt to the larger count, causing
ethtool_rxnfc_copy_to_user() to read out-of-bounds kernel heap memory back
to userspace.

> diff --git a/drivers/net/ethernet/ti/icssg/icssg_ethtool.c b/drivers/net/ethernet/ti/icssg/icssg_ethtool.c
> index 5c2936ecd595c..9f0a1116e530c 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_ethtool.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_ethtool.c
> @@ -299,6 +299,52 @@ static int emac_set_per_queue_coalesce(struct net_device *ndev, u32 queue,
>  	return 0;
>  }
>  
> +static int emac_get_rxnfc(struct net_device *ndev, struct ethtool_rxnfc *info,
> +			  u32 *rule_locs)
> +{
> +	struct prueth_emac *emac = netdev_priv(ndev);
> +	int ret = 0;
> +
> +	switch (info->cmd) {
> +	case ETHTOOL_GRXRINGS:
> +		info->data = emac->rx_flow_num;
> +		break;

[Severity: Medium]
Is the ETHTOOL_GRXRINGS case in emac_get_rxnfc() reachable?

In net/ethtool/ioctl.c:ethtool_get_rxrings(), ETHTOOL_GRXRINGS is handled by
calling ethtool_get_rx_ring_count(), which checks ops->get_rx_ring_count in
struct ethtool_ops and returns -EOPNOTSUPP when it is NULL, rather than
calling ops->get_rxnfc(). Should icssg_ethtool_ops implement
.get_rx_ring_count instead?

[ ... ]
>  const struct ethtool_ops icssg_ethtool_ops = {
[ ... ]
>  	.nway_reset = emac_nway_reset,
>  	.get_rmon_stats = emac_get_rmon_stats,
> +	.get_rxnfc = emac_get_rxnfc,
> +	.set_rxnfc = emac_set_rxnfc,
>  };
>  EXPORT_SYMBOL_GPL(icssg_ethtool_ops);

[Severity: High]
What happens when emac_get_rxnfc() or emac_set_rxnfc() (or
ethtool_set_channels() -> ethtool_check_max_channel() ->
ethtool_get_max_rxnfc_channel()) is called on an SR1.0 device?

In drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c:prueth_netdev_init(),
SR1.0 net_devices also use icssg_ethtool_ops:

drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c:prueth_netdev_init() {
	...
	ndev->netdev_ops = &emac_netdev_ops;
	ndev->ethtool_ops = &icssg_ethtool_ops;
	...
}

However, icssg_rxnfc_rule_init(emac) is only called in
drivers/net/ethernet/ti/icssg/icssg_prueth.c:prueth_netdev_init() and not in
icssg_prueth_sr1.c, nor do emac_get_rxnfc() / emac_set_rxnfc() guard against
emac->is_sr1. On SR1.0 devices, emac->rxnfc.rxnfc_lock is uninitialized and
emac->rxnfc.rules has NULL next/prev pointers, so any RXNFC call will lock
an uninitialized mutex and dereference a NULL list_head. (Note also that
drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c:emac_ndo_open() and related
RX channel calls in this series still use the old struct prueth_rx_chn
fields and helper signatures, breaking the build when
CONFIG_TI_ICSSG_PRUETH_SR1 is enabled.)

> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> index 8338402597b21..f234198d4cc42 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> @@ -874,10 +874,29 @@ static void prueth_destroy_rxq(struct prueth_emac *emac, int qid)
>  {
>  	struct prueth_rx_chn *rx_chn = &emac->rx_chns;
>  	struct prueth_rx_flow *flow = &rx_chn->flows[qid];
> +	dma_addr_t desc_dma;
>  
> -	/* reset napi and hrtimer, and free all pending buffers */
>  	napi_disable(&flow->napi_rx);
>  	hrtimer_cancel(&flow->rx_hrtimer);
> +
> +	/* Drain the UDMA completion ring for this flow before resetting
> +	 * the free ring.  k3_udma_glue_reset_rx_chn() only drains the
> +	 * free ring (descriptors pushed as empty receive buffers).  Any
> +	 * frames that the hardware already DMA-completed but NAPI hadn't
> +	 * popped yet remain in the completion ring with their pages still
> +	 * counted as "inflight" in the page pool.  Draining them here
> +	 * ensures page_pool_destroy() finds zero inflight pages and
> +	 * completes immediately instead of stalling for 60+ seconds.
> +	 *
> +	 * The caller must have stopped new frames from arriving (e.g. via
> +	 * icssg_set_port_state(ICSSG_EMAC_PORT_DISABLE)) before calling
> +	 * prueth_destroy_rxq() so this loop terminates.
> +	 */
> +	while (!k3_udma_glue_pop_rx_chn(rx_chn->rx_chn, qid, &desc_dma)) {
> +		if (!cppi5_desc_is_tdcm(desc_dma))
> +			prueth_rx_cleanup(rx_chn, desc_dma);
> +	}
> +
>  	k3_udma_glue_reset_rx_chn(rx_chn->rx_chn, flow->id, rx_chn,
>  				  prueth_rx_cleanup);
>  

[Severity: High]
When multiple RX flows are active (emac->rx_flow_num > 1), doesn't calling
prueth_destroy_rxq(emac, i) sequentially in prueth_destroy_rxqs() destroy
flows[0].pg_pool before higher flows' completion rings are drained?

In prueth_init_rx_chns(), all RX flows share flow 0's Free Descriptor Queue
(ringrxfdq), so descriptors pushed by flow 0 (with swdata->flow_id == 0 and
pages from flows[0].pg_pool) can be popped by UDMA when packets are steered
by FT3 rules to flow qid > 0 and placed into flows[qid]'s completion ring.

When prueth_destroy_rxqs() runs:

1. On qid == 0, prueth_destroy_rxq(emac, 0) disables only flows[0].napi_rx
   while flows[1..rx_flow_num-1].napi_rx are still enabled and can push
   requeued descriptors into the shared ringrxfdq concurrently with
   k3_udma_glue_reset_rx_chn(rx_chn->rx_chn, 0, ...).
2. prueth_destroy_rxq(emac, 0) drains only flow 0's completion ring and
   immediately calls page_pool_destroy(flows[0].pg_pool) and sets
   flows[0].pg_pool = NULL while descriptors with swdata->flow_id == 0 are
   still sitting in flows[1..rx_flow_num-1]'s completion rings.
3. When prueth_destroy_rxq(emac, qid) subsequently drains flows[qid] for
   qid >= 1 and calls prueth_rx_cleanup():

drivers/net/ethernet/ti/icssg/icssg_common.c:prueth_rx_cleanup() {
	...
	} else if (swdata->type == PRUETH_SWDATA_PAGE) {
		pool = rx_chn->flows[swdata->flow_id].pg_pool;
		if (pool) {
			page = swdata->data.page;
			page_pool_recycle_direct(pool, page);
		}
	}
}

   rx_chn->flows[0].pg_pool is already NULL, so page_pool_recycle_direct()
   is skipped and the inflight pages from flows[0].pg_pool are never
   recycled, causing page_pool_destroy() to stall and leaking the pages.

[Severity: High]
Can prueth_create_rxq() also hang in napi_disable() when its error path
calls prueth_destroy_rxq()?

In prueth_create_rxq(), if prueth_prepare_rx_flow(), xdp_rxq_info_reg(), or
xdp_rxq_info_reg_mem_model() fails, execution jumps to err before
napi_enable(&flow->napi_rx) is called:

drivers/net/ethernet/ti/icssg/icssg_prueth.c:prueth_create_rxq() {
	...
	ret = prueth_prepare_rx_flow(emac, id);
	if (ret)
		goto err;
	...
	napi_enable(&flow->napi_rx);
	return 0;

err:
	prueth_destroy_rxq(emac, id);
	return ret;
}

Because netif_napi_add() sets NAPIF_STATE_SCHED and napi_enable() was not
called to clear it, calling napi_disable(&flow->napi_rx) at the start of
prueth_destroy_rxq() loops forever waiting for NAPIF_STATE_SCHED to clear.
Furthermore, in prueth_init_rx_chns(), the shared ringrxfdq is sized to
PRUETH_MAX_RX_DESC (512) while each flow pushes 512 descriptors in
prueth_prepare_rx_flow(), so opening an interface with emac->rx_flow_num > 1
deterministically overflows ringrxfdq on flow 1 and hits this hang.

> @@ -1160,6 +1179,9 @@ static int emac_ndo_stop(struct net_device *ndev)
>  	prueth_cleanup_rx_chns(emac);
>  	prueth_cleanup_tx_chns(emac);
>  
> +	/* Cleanup RX flow classification rules */
> +	icssg_rxnfc_rule_cleanup(emac);
> +
>  	prueth->emacs_initialized--;
>  
>  	return 0;

[Severity: High]
Does calling icssg_rxnfc_rule_cleanup(emac) in emac_ndo_stop() cause rules
to be lost across system suspend/resume, while rules added before
emac_ndo_open() or before prueth_emac_restart() lose their hardware
classifier programming?

Specifically:

1. Changing RX channel count in emac_set_channels() requires the interface
   to be down (!netif_running(emac->ndev)), and emac_set_rxnfc() allows
   adding rules while down. When emac_ndo_open() or prueth_emac_restart()
    subsequently calls prueth_emac_common_start() -> icssg_class_default() ->
   icssg_class_disable(), all RX_CLASS_OR_EN registers on both slices are
   reset to 0 without re-applying emac->rxnfc.rules via
   icssg_rxnfc_classifier_update().
2. During suspend, prueth_suspend() calls ndev->netdev_ops->ndo_stop(ndev)
   -> emac_ndo_stop() -> icssg_rxnfc_rule_cleanup(emac), which frees and
   deletes all user-configured RXNFC rules so they are gone upon
   prueth_resume().

> @@ -1676,6 +1698,9 @@ static int prueth_netdev_init(struct prueth *prueth,
>  	emac->ndev->pcpu_stat_type = NETDEV_PCPU_STAT_TSTATS;
>  	INIT_WORK(&emac->rx_mode_work, emac_ndo_set_rx_mode_work);
>  
> +	/* Initialize RX flow classification */
> +	icssg_rxnfc_rule_init(emac);
> +
>  	INIT_DELAYED_WORK(&emac->stats_work, icssg_stats_work_handler);
>  
>  	ret = pruss_request_mem_region(prueth->pruss,

[Severity: Medium]
Can struct icssg_rxnfc_rule entries in emac->rxnfc.rules be leaked when the
driver is removed in prueth_netdev_exit()?

Because icssg_rxnfc_rule_add() allocates struct icssg_rxnfc_rule via
kzalloc() even when the interface is down, and icssg_rxnfc_rule_cleanup() is
only called in emac_ndo_stop() (and not in
drivers/net/ethernet/ti/icssg/icssg_common.c:prueth_netdev_exit()), any
rules added while the interface is down are never freed if the device is
unregistered and freed without being opened and stopped first.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103134501.4123194-1-m-malladi@ti.com?part=2