Re: [PATCH net-next v13 6/9] net: dsa: lan9645x: add mac table integration

[Jens Emil Schulz Ostergaard]

On Fri, 2026-10-02 at 21:14 +0000, netdev-bot+sashiko@xxxxxxxxxx wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 1
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] For a non-zero VID, lan9645x_fdb_add() and lan9645x_fdb_del()
>   map DSA_DB_PORT and DSA_DB_BRIDGE host entries to the same hardware…
> - [Low] Each new MAC table sequence writes the key, the filter and the
>   command through lan_wr().
> 
> --
> 
> net: dsa: lan9645x: add mac table integration
> 
> This commit adds MAC table support to the lan9645x DSA driver. It adds
> learn, forget, flush and GET_NEXT based dump helpers keyed on (vid, mac).
> These are wired into the port_fast_age, port_fdb_dump, port_fdb_add and
> port_fdb_del DSA callbacks.
> 
> > diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_mac.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_mac.c
> > new file mode 100644
> > index 0000000000000..50ce1378b9dae
> > --- /dev/null
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_mac.c
> > @@ -0,0 +1,268 @@
> 
> [ ... ]
> 
> > +static void lan9645x_mac_select(struct lan9645x *lan9645x,
> > +                             const unsigned char mac[ETH_ALEN],
> > +                             unsigned int vid)
> > +{
> > +     u64 maddr = ether_addr_to_u64(mac);
> > +
> > +     lockdep_assert_held(&lan9645x->mact_lock);
> > +
> > +     lan_wr(ANA_MACHDATA_VID_SET(vid) |
> > +            ANA_MACHDATA_MACHDATA_SET(upper_32_bits(maddr)),
> > +            lan9645x,
> > +            ANA_MACHDATA);
> > +
> > +     lan_wr(lower_32_bits(maddr),
> > +            lan9645x,
> > +            ANA_MACLDATA);
> > +}
> 
> [ ... ]
> 
> > +static int __lan9645x_mact_learn(struct lan9645x *lan9645x, int port,
> > +                              const unsigned char mac[ETH_ALEN],
> > +                              unsigned int vid,
> > +                              enum macaccess_entry_type type,
> > +                              bool cpu_copy)
> > +{
> > +     lockdep_assert_held(&lan9645x->mact_lock);
> > +
> > +     lan9645x_mac_select(lan9645x, mac, vid);
> > +
> > +     lan_wr(ANA_MACACCESS_VALID_SET(1) |
> > +            ANA_MACACCESS_DEST_IDX_SET(port) |
> > +            ANA_MACACCESS_MAC_CPU_COPY_SET(cpu_copy) |
> > +            ANA_MACACCESS_ENTRYTYPE_SET(type) |
> > +            ANA_MACACCESS_MAC_TABLE_CMD_SET(CMD_LEARN),
> > +            lan9645x, ANA_MACACCESS);
> > +
> > +     return lan9645x_mac_wait_for_completion(lan9645x, NULL);
> > +}
> 
> [Severity: Low]
> Can a failed register write here be reported to DSA as a successful learn?
> 
> lan_wr() returns void and only calls WARN_ON_ONCE() when regmap_write()
> fails. Suppose the ANA_MACACCESS command write is lost but the reads that
> follow still work. Then lan9645x_mac_wait_for_completion() sees CMD_IDLE
> immediately and returns 0:
> 
>         err = lan9645x_rd_poll_timeout(lan9645x, ANA_MACACCESS, val,
>                                        ANA_MACACCESS_MAC_TABLE_CMD_GET(val) ==
>                                        CMD_IDLE);
> 
> In that case lan9645x_fdb_add() returns success and the bridge marks the
> entry as offloaded, but the table was never changed. The same applies to
> CMD_FORGET in __lan9645x_mact_forget().
> 
> What happens if only the ANA_MACHDATA or ANA_MACLDATA write in
> lan9645x_mac_select() fails? Would CMD_LEARN or CMD_FORGET then act on a
> stale key? That could be the previous operation's key, or whatever the
> last GET_NEXT walk in lan9645x_mact_dsa_dump() left in the registers.
> 
> lan9645x_mact_flush() has the same problem with its ANA_ANAGEFIL write:
> 
>         lan_wr(ANA_ANAGEFIL_PID_EN_SET(1) |
>                ANA_ANAGEFIL_PID_VAL_SET(port),
>                lan9645x, ANA_ANAGEFIL);
> 
> If that write fails, both CMD_AGE scans would run without the port
> filter. Would that flush dynamic entries from every port instead of only
> the requested one?
> 
> This only happens if the parent MFD bus can fail a write while later
> reads succeed. The parent driver is not in the tree, so that could not be
> confirmed. The ocelot-family drivers follow the same lan_wr() convention.
> 
> [ ... ]
> 
> > diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> > index 6818b91f4bed2..5cb4f5fe13740 100644
> > --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> 
> [ ... ]
> 
> > @@ -360,6 +364,81 @@ static int lan9645x_port_vlan_del(struct dsa_switch *ds, int port,
> 
> [ ... ]
> 
> > +static int lan9645x_fdb_add(struct dsa_switch *ds, int port,
> > +                         const unsigned char *addr, u16 vid,
> > +                         struct dsa_db db)
> > +{
> 
> [ ... ]
> 
> > +     if (dsa_port_is_cpu(dp) && !br &&
> > +         dsa_fdb_present_in_other_db(ds, port, addr, vid, db))
> > +             return 0;
> > +
> > +     if (!vid)
> > +             vid = lan9645x_vlan_unaware_pvid(!!br);
> > +
> > +     dest = dsa_port_is_cpu(dp) ? PGID_CPU : port;
> > +
> > +     return lan9645x_mact_learn(lan9645x, dest, addr, vid, ENTRYTYPE_LOCKED,
> > +                                false);
> > +}
> > +
> > +static int lan9645x_fdb_del(struct dsa_switch *ds, int port,
> > +                         const unsigned char *addr, u16 vid,
> > +                         struct dsa_db db)
> > +{
> > +     struct net_device *br = lan9645x_db2bridge(db);
> > +     struct dsa_port *dp = dsa_to_port(ds, port);
> > +     struct lan9645x *lan9645x = ds->priv;
> > +
> > +     if (IS_ERR(br))
> > +             return PTR_ERR(br);
> > +
> > +     if (dsa_port_is_cpu(dp) && !br &&
> > +         dsa_fdb_present_in_other_db(ds, port, addr, vid, db))
> > +             return 0;
> > +
> > +     if (!vid)
> > +             vid = lan9645x_vlan_unaware_pvid(!!br);
> > +
> > +     return lan9645x_mact_forget(lan9645x, addr, vid, ENTRYTYPE_LOCKED);
> > +}
> 
> [Severity: Medium]
> Could this forget a CPU port entry that another database still needs?
> 
> For a non-zero vid, lan9645x_fdb_add() and lan9645x_fdb_del() map two
> host entries with the same (vid, mac) to one hardware key pointing at
> PGID_CPU: the DSA_DB_PORT one and the DSA_DB_BRIDGE one. Only vid 0 is
> split, into HOST_PVID and UNAWARE_PVID by lan9645x_vlan_unaware_pvid().
> 
> DSA refcounts the two entries separately on the CPU port, because
> dsa_mac_addr_find() includes the db in its key. The shared-key guard only
> matches entries of the same db type:
> 
> net/dsa/dsa.c:dsa_fdb_present_in_other_db() {
>     ...
>         if (a->db.type == db.type && !dsa_db_equal(&a->db, &db))
>                 return true;
>     ...
> }
> 
> So when a DSA_DB_PORT entry is deleted, the guard never sees a
> DSA_DB_BRIDGE entry with the same key. For DSA_DB_BRIDGE deletions, the
> !br check skips the guard entirely.
> 
> One way to hit this:
> 
> - swp1 is in a VLAN-aware bridge.
> - swp1.100 is a VLAN upper whose MAC M differs from swp1's.
> - dsa_user_vlan_rx_add_vid()->dsa_user_schedule_standalone_work(DSA_UC_ADD)
>   installs a DSA_DB_PORT host entry (M, 100).
> - M is also a bridge host address in VID 100, which adds a DSA_DB_BRIDGE
>   host entry (M, 100). This happens, for example, with br0 using vid 100
>   self and the same MAC, or with a local entry of another port in VID 100.
> 
> Removing either entry, for example by taking swp1.100 down or deleting
> the bridge VLAN, goes through:
> 
> dsa_switch_host_fdb_del()
>   dsa_port_do_fdb_del()
>     lan9645x_fdb_del()
>       dsa_fdb_present_in_other_db()   /* false, db types differ */
>       lan9645x_mact_forget()          /* CMD_FORGET on (M, 100) */
> 
> Wouldn't the hardware then lose the entry for the host address that is
> still wanted?
> 
> __lan9645x_port_set_host_flood() leaves the CPU out of PGID_UC when only
> bridged ports request host flooding. Unicast to M in VID 100 could then
> be dropped instead of reaching the host. The felix driver uses the same
> pattern.
> 

Correct, a DSA_DB_PORT and a DSA_DB_BRIDGE host entry with the same
address and a non-zero VID map to one MAC table entry, and
dsa_fdb_present_in_other_db() only considers databases of the same type.
v14 replaces the check with one that walks the CPU port address list and
compares the MAC table key each entry maps to, so the entry is only
learned or forgotten when no other database uses that key.


> [ ... ]
> 
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929-dsa_lan9645x_switch_driver_base-v13-0-827c2d3617f2%40microchip.com