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

[Jens Emil Schulz Ostergaard <jensemil.schulzostergaard@microchip.com>]

This is a reply to:
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260805-dsa_lan9645x_switch_driver_base-v11-0-007ebc983a0a%40microchip.com?part=7

> --- Patch [7]: [PATCH net-next v11 7/9] net: dsa: lan9645x: add mac table integration ---
> commit c7af50889a34728dc6c99946b25b62bfb26fc31e
> Author: Jens Emil Schulz Østergaard <jensemil.schulzostergaard@microchip.com>
>
> net: dsa: lan9645x: add mac table integration
>
> This adds a new lan9645x_mac.c unit that drives the ANA MAC table through
> the ANA_MACACCESS/ANA_MACHDATA/ANA_MACLDATA registers, and wires it up to
> the DSA .port_fdb_add, .port_fdb_del, .port_fdb_dump and .port_fast_age
> callbacks. A new mact_lock mutex serializes all MAC table register access,
> including the aging period updates.
>
> > 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..9d5282754b7a0
> > --- /dev/null
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_mac.c
> > @@ -0,0 +1,293 @@
>
> [ ... ]
>
> > +int lan9645x_mact_flush(struct lan9645x *lan9645x, int port)
> > +{
> > +	int err;
> > +
> > +	mutex_lock(&lan9645x->mact_lock);
> > +	/* MAC table entries with dst index matching port are aged on scan. */
> > +	lan_wr(ANA_ANAGEFIL_PID_EN_SET(1) |
> > +	       ANA_ANAGEFIL_PID_VAL_SET(port),
> > +	       lan9645x, ANA_ANAGEFIL);
> > +
> > +	/* Flushing requires two scans. First sets AGE_FLAG=1, second removes
> > +	 * entries with AGE_FLAG=1.
> > +	 */
> > +	lan_wr(ANA_MACACCESS_MAC_TABLE_CMD_SET(CMD_AGE),
> > +	       lan9645x,
> > +	       ANA_MACACCESS);
> > +
> > +	err = lan9645x_mac_wait_for_completion(lan9645x, NULL);
> > +	if (err)
> > +		goto mact_unlock;
> > +
> > +	lan_wr(ANA_MACACCESS_MAC_TABLE_CMD_SET(CMD_AGE),
> > +	       lan9645x,
> > +	       ANA_MACACCESS);
> > +
> > +	err = lan9645x_mac_wait_for_completion(lan9645x, NULL);
> > +
> > +mact_unlock:
> > +	lan_wr(0, lan9645x, ANA_ANAGEFIL);
> > +	mutex_unlock(&lan9645x->mact_lock);
> > +	return err;
> > +}
>
> [ ... ]
>
> > +		/* Only dynamic entries are surfaced through the user port dump.
> > +		 * ENTRYTYPE_LOCKED entries are already reported by the bridge
> > +		 * master's ndo_fdb_dump as NTF_MASTER, so we avoid duplicating
> > +		 * them as NTF_SELF.
> > +		 * Entries toward the host (NTF_SELF) have DEST_IDX == the CPU
> > +		 * port module and are filtered out by the DEST_IDX check.
> > +		 */
> > +		if (ANA_MACACCESS_DEST_IDX_GET(maca) == port &&
> > +		    type == ENTRYTYPE_NORMAL) {
>
> Discovered by: opus-5 · Missed by: sashiko-gemini
> Is the comment in lan9645x_mact_dsa_dump() accurate about what DEST_IDX
> holds for host entries?
>
> lan9645x_fdb_add() programs the destination as PGID_CPU:
>
> 	dest = dsa_port_is_cpu(dp) ? PGID_CPU : port;
>
> 	return lan9645x_mact_learn(lan9645x, dest, addr, vid, ENTRYTYPE_LOCKED);
>
> and lan9645x_main.h defines PGID_CPU as (PGID_AGGR - 6), i.e. 58. The CPU
> port module itself is denoted by lan9645x->num_phys_ports elsewhere in the
> driver, for instance ANA_VLAN_CFG(num_phys_ports) in lan9645x_vlan_init()
> and BIT(num_phys_ports) in the PGID_BC/PGID_CPU masks written by
> lan9645x_port_cpu_init():
>
> 	lan_wr(BIT(lan9645x->num_phys_ports), lan9645x, ANA_PGID(PGID_CPU));
>
> So the value stored in DEST_IDX for host entries is the reserved PGID index
> 58, not the CPU port module index. Nothing misbehaves today, since both 58
> and num_phys_ports fail the comparison against a user port index, but would
> it be clearer to say the entries carry DEST_IDX == PGID_CPU? A later change
> that adds an explicit host-entry test against num_phys_ports based on this
> comment would not match any entry.

Yes, I will update this comment.

>
> > diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> > index 3281726284dc2..af158d4461aee 100644
> > --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
>
> [ ... ]
>
> > @@ -626,6 +636,75 @@ static int lan9645x_port_vlan_del(struct dsa_switch *ds, int port,
> >  	return lan9645x_vlan_port_del_vlan(p, vlan->vid);
> >  }
> >  
> > +static void lan9645x_port_fast_age(struct dsa_switch *ds, int port)
> > +{
> > +	lan9645x_mact_flush(ds->priv, port);
> > +}
>
> Discovered by: opus-5 · Missed by: sashiko-gemini
> Should lan9645x_port_fast_age() log the dropped error here?
>
> lan9645x_mact_flush() returns the result of both
> lan9645x_mac_wait_for_completion() calls, which yield -ETIMEDOUT when
> ANA_MACACCESS.MAC_TABLE_CMD does not return to CMD_IDLE within the poll
> timeout. Since the DSA .port_fast_age op returns void, this callback is the
> only place the failure can be observed, and the value is discarded.
>
> On a failed flush the ANA_ANAGEFIL filter is cleared and CMD_AGE is not
> retried, so the port's dynamic entries stay in the MAC table after an STP
> topology change or bridge leave, with no kernel message. If aging was
> disabled with a zero ageing time in lan9645x_set_ageing_time()
> (ANA_AUTOAGE_AGE_PERIOD == 0), the stale entries are not aged out later
> either.
>
> felix_port_fast_age() handles the same situation for the related IP by
> reporting it:
>
> 	err = ocelot_mact_flush(ocelot, port);
> 	if (err)
> 		dev_err(ds->dev, "Flushing MAC table on port %d returned %pe\n",
> 			port, ERR_PTR(err));
>

Sure, I will add an error log here.

> > +
> > +static int lan9645x_fdb_dump(struct dsa_switch *ds, int port,
> > +			     dsa_fdb_dump_cb_t *cb, void *data)
> > +{
> > +	return lan9645x_mact_dsa_dump(ds->priv, port, cb, data);
> > +}
>
> [ ... ]