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

[Cedric Jehasse]

>> +/* 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?

Here's the reply i gave to the ai-review on the v3 version of the patch:
I don't think this is an issue. For entries that are successfully moved the
hw_idx points to the new index. And for entries that haven't successfully
moved it's still pointing to the old index. Hence the hw_idx field for every
list entry should still point to the correct TCAM index. There will be a gap
in the TCAM entries in the hardware, but that's not an issue.
If moving an existing entry returns an error. The rollback moving of entries
is as likely to return an error, and potentially leave the TCAM in an
inconsistent state.

>
>[ ... ]
>
>> 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?

At first FLOW_DISSECTOR_KEY_IPV6_ADDRS wasn't in the allowed keys bitmask, but
creating ipv4 filter entries failed with -EOPNOTSUPP.
Eg. when using the following tc command, the FLOW_DISSECTOR_KEY_IPV6_ADDRS bit
is set in dissector->used_keys:
tc filter add dev p1 ingress protocol ip flower skip_sw dst_ip 224.0.1.100 \
    action trap
    
To make ipv4 filter entries work i had to add FLOW_DISSECTOR_KEY_IPV6_ADDRS to
the allowed keys bitmask and check the addr_type instead.

thanks,
Cedric