Re: [net-next,v4] net: dsa: mv88e6xxx: Add partial support for TCAM entries

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: dsa: mv88e6xxx: Add partial support for TCAM entries

This patch adds partial Ternary Content Addressable Memory (TCAM)
support for the mv88e6390 and mv88e6393 family of switches. The
implementation introduces matching on IP addresses/protocol with the
ability to trap matched frames to the CPU.

> diff --git a/drivers/net/dsa/mv88e6xxx/tcam.c b/drivers/net/dsa/mv88e6xxx/tcam.c
> new file mode 100644
> index 000000000000..f56cc7e0c00f
> --- /dev/null
> +++ b/drivers/net/dsa/mv88e6xxx/tcam.c

[ ... ]

> +/* insert tcam entry in ordered list and move existing entries if necessary */
> +static int mv88e6xxx_tcam_insert_entry(struct mv88e6xxx_chip *chip,
> +				       struct mv88e6xxx_tcam_entry *entry)
> +{
> +	struct mv88e6xxx_tcam_entry *elem;
> +	struct list_head *hpos;
> +	int err;
> +
> +	list_for_each_prev(hpos, &chip->tcam.entries) {
> +		u8 move_idx;
> +
> +		elem = list_entry(hpos, struct mv88e6xxx_tcam_entry, list);
> +		if (entry->prio >= elem->prio)
> +			break;
> +
> +		move_idx = elem->hw_idx + 1;
> +
> +		err = mv88e6xxx_tcam_flush_entry(chip, move_idx);
> +		if (err)
> +			return err;
> +
> +		err = chip->info->ops->tcam_ops->entry_add(chip, elem,
> +							   move_idx);
> +		if (err)
> +			return err;
> +
> +		elem->hw_idx = move_idx;
> +	}

If entry_add() fails after some entries have already been moved to new
hardware positions with their hw_idx updated, doesn't the function return
without rolling back those partial moves?

The software list would then have some entries pointing to their new
positions while the gap for the new entry is only partially created.
This could leave the TCAM in an inconsistent state for subsequent
operations.

Is there error handling to ensure the TCAM state remains consistent when
insertion partially succeeds?

[ ... ]

> diff --git a/drivers/net/dsa/mv88e6xxx/tcflower.c b/drivers/net/dsa/mv88e6xxx/tcflower.c
> new file mode 100644
> index 000000000000..05b289463834
> --- /dev/null
> +++ b/drivers/net/dsa/mv88e6xxx/tcflower.c
> @@ -0,0 +1,164 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Marvell 88E6xxx Switch flower support
> + *
> + * Copyright (c) 2026 Luminex Network Intelligence
> + */
> +
> +#include "chip.h"
> +#include "tcflower.h"
> +#include "tcam.h"
> +
> +#define MV88E6XXX_ETHTYPE_OFFSET 16
> +#define MV88E6XXX_IP_PROTO_OFFSET 27
> +#define MV88E6XXX_IPV4_SRC_OFFSET 30
> +#define MV88E6XXX_IPV4_DST_OFFSET 34
> +
> +static int mv88e6xx_flower_parse_key(struct mv88e6xxx_chip *chip,
> +				     struct netlink_ext_ack *extack,
> +				     struct flow_cls_offload *cls,
> +				     struct mv88e6xxx_tcam_key *key)
> +{
> +	struct flow_rule *rule = flow_cls_offload_flow_rule(cls);
> +	struct flow_dissector *dissector = rule->match.dissector;
> +	u16 addr_type = 0;
> +
> +	if (dissector->used_keys &
> +	    ~(BIT_ULL(FLOW_DISSECTOR_KEY_BASIC) |
> +	      BIT_ULL(FLOW_DISSECTOR_KEY_CONTROL) |
> +	      BIT_ULL(FLOW_DISSECTOR_KEY_IPV4_ADDRS) |
> +	      BIT_ULL(FLOW_DISSECTOR_KEY_IPV6_ADDRS))) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Unsupported keys used");
> +		return -EOPNOTSUPP;
> +	}

FLOW_DISSECTOR_KEY_IPV6_ADDRS is included in the allowed keys bitmask,
but there is no code below to parse IPv6 addresses into the TCAM key.

If a user creates a tc flower rule with IPv6 addresses, the rule passes
this validation check but the addresses are never programmed into the
TCAM. This would cause the hardware entry to match broader traffic than
intended.

Looking at the code further down:

> +	if (addr_type == FLOW_DISSECTOR_KEY_IPV4_ADDRS) {
> +		struct flow_match_ipv4_addrs match;
> +
> +		flow_rule_match_ipv4_addrs(cls->rule, &match);
> +		mv88e6xxx_tcam_match_set(key, MV88E6XXX_IPV4_SRC_OFFSET,
> +					 match.key->src,
> +					 match.mask->src);
> +		mv88e6xxx_tcam_match_set(key, MV88E6XXX_IPV4_DST_OFFSET,
> +					 match.key->dst,
> +					 match.mask->dst);
> +	} else {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Unsupported address type");
> +		return -EOPNOTSUPP;
> +	}

The check at line 68-72 rejects non-IPv4 address types, but the IPv6 key
is still in the allowed keys bitmask at line 30. This creates a confusing
situation where IPv6 is both 'allowed' and 'unsupported'.

Should FLOW_DISSECTOR_KEY_IPV6_ADDRS be removed from the allowed keys
until IPv6 parsing is implemented?
-- 
pw-bot: cr