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

[Jakub Kicinski]

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) for
the mv88e6390 and mv88e6393 family of switches, allowing the switch to
match the first 48 or 96 bytes of a frame and take actions on matched
frames. This introduces a subset of TCAM functionality: matching on IP
addresses/protocol and trapping to the cpu.

> diff --git a/drivers/net/dsa/mv88e6xxx/Makefile b/drivers/net/dsa/mv88e6xxx/Makefile
> index dd961081d631..b0b08c6f159c 100644
> --- a/drivers/net/dsa/mv88e6xxx/Makefile
> +++ b/drivers/net/dsa/mv88e6xxx/Makefile
> @@ -21,6 +21,8 @@ mv88e6xxx-objs += serdes.o
>  mv88e6xxx-objs += smi.o
>  mv88e6xxx-objs += switchdev.o
>  mv88e6xxx-objs += trace.o
> +mv88e6xxx-objs += tcflower.o
> +mv88e6xxx-objs += tcam.o

[ ... ]

> diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
> index 66b1b7277281..38616a2f8de3 100644
> --- a/drivers/net/dsa/mv88e6xxx/port.c
> +++ b/drivers/net/dsa/mv88e6xxx/port.c
> @@ -1380,7 +1380,32 @@ int mv88e6xxx_port_disable_learn_limit(struct mv88e6xxx_chip *chip, int port)
>
>  int mv88e6xxx_port_disable_pri_override(struct mv88e6xxx_chip *chip, int port)
>  {
> -	return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_PRI_OVERRIDE, 0);
> +	u16 reg;
> +	int err;
> +
> +	err = mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_PRI_OVERRIDE,
> +				  &reg);
> +	if (err)
> +		return err;
> +
> +	reg &= MV88E6XXX_PORT_PRI_OVERRIDE_TCAM_MODE_MASK;
> +	return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_PRI_OVERRIDE,
> +				    reg);
> +}
> +
> +int mv88e6xxx_port_enable_tcam(struct mv88e6xxx_chip *chip, int port)
> +{
> +	u16 reg;
> +	int err;
> +
> +	err = mv88e6xxx_port_read(chip, port, MV88E6XXX_PORT_PRI_OVERRIDE,
> +				  &reg);
> +	if (err)
> +		return err;
> +
> +	reg |= MV88E6XXX_PORT_PRI_OVERRIDE_TCAM_MODE_96_BYTE;
                ^^^

Would ORing the TCAM mode work correctly if the firmware has already
configured the port to 48-byte mode? The TCAM mode field (bits 1:0) in
the Priority Override register is a 2-bit enumeration where 0=disabled,
1=48-byte, 2=96-byte, and 3=reserved.

If bit 0 is already set (48-byte mode with value 0x0001), ORing with
0x0002 produces 0x0003 (reserved mode) instead of the intended 0x0002
(96-byte mode). The standard pattern for setting an enumerated register
field is clear-then-set:

    reg &= ~MV88E6XXX_PORT_PRI_OVERRIDE_TCAM_MODE_MASK;
    reg |= MV88E6XXX_PORT_PRI_OVERRIDE_TCAM_MODE_96_BYTE;

In normal operation (hardware reset value 0x0000), the current OR
produces the correct result. The issue only occurs if firmware or a
bootloader has pre-configured 48-byte TCAM mode before the Linux driver
initializes.

> +	return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_PRI_OVERRIDE,
> +				    reg);
> +}

[ ... ]

> diff --git a/drivers/net/dsa/mv88e6xxx/tcflower.c b/drivers/net/dsa/mv88e6xxx/tcflower.c
> new file mode 100644
> index 000000000000..c28282ead486
> --- /dev/null
> +++ b/drivers/net/dsa/mv88e6xxx/tcflower.c
> @@ -0,0 +1,157 @@
> +// 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,
                    ^^^

This isn't a bug, but the function name uses prefix mv88e6xx (two x's)
instead of mv88e6xxx (three x's). Every other function in the mv88e6xxx
driver uses the mv88e6xxx_ prefix, including the caller
mv88e6xxx_cls_flower_add in the same file.

> +				     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))) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Unsupported keys used");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_CONTROL)) {
> +		struct flow_match_control match;
> +
> +		flow_rule_match_control(rule, &match);
> +		addr_type = match.key->addr_type;
> +
> +		if (flow_rule_has_control_flags(match.mask->flags,
> +						cls->common.extack))
> +			return -EOPNOTSUPP;
> +	}
> +	if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_BASIC)) {
> +		struct flow_match_basic match;
> +
> +		flow_rule_match_basic(rule, &match);
> +		mv88e6xxx_tcam_match_set(key, MV88E6XXX_ETHTYPE_OFFSET,
> +					 match.key->n_proto,
> +					 match.mask->n_proto);
> +		mv88e6xxx_tcam_match_set(key, MV88E6XXX_IP_PROTO_OFFSET,
> +					 match.key->ip_proto,
> +					 match.mask->ip_proto);
> +	}
> +
> +	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);
> +	}
> +
> +	return 0;
> +}

[ ... ]
-- 
pw-bot: cr