Re: [PATCH net-next v11 4/9] net: dsa: lan9645x: add basic dsa driver for LAN9645X

On Thu, 2026-08-06 at 14:38 +0000, [email protected] 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:
> - [High] Standalone ports drop all normal data plane traffic destined for the CPU because the CPU port module is missing from the source port forwarding masks (PGID_SRC) and the flooding masks (PGID_UC, PGID_MC).
> - [Medium] Pause frame start watermark (PAUSE_START) exceeds the port's maximum allowed memory (ATOP) for jumbo frames, breaking flow control and causing silent tail drops.
> --
> 
> --- Patch [4]: [PATCH net-next v11 4/9] net: dsa: lan9645x: add basic dsa driver for LAN9645X ---
> commit dd8bddd9c4ac1f2dbff1feda2560552c682fa913
> Author: Jens Emil Schulz Østergaard <[email protected]>
> Date:   Wed Aug 5 14:34:52 2026 +0000
> 
>     net: dsa: lan9645x: add basic dsa driver for LAN9645X
> 
>     Add the LAN9645X basic DSA driver with initialization, parent regmap
>     requests, port module initialization for NPI, CPU ports and front ports,
>     and phylink integration for MAC side configuration.
> 
> > --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> [ ... ]
> > +static int lan9645x_setup(struct dsa_switch *ds)
> > +{
> [ ... ]
> > +     /* Disable bridging by default */
> > +     for (int p = 0; p < lan9645x->num_phys_ports; p++) {
> > +             lan_wr(0, lan9645x, ANA_PGID(PGID_SRC + p));
> > +
> > +             /* Do not forward BPDU frames to the front ports and copy them
> > +              * to CPU
> > +              */
> > +             lan_wr(ANA_CPU_FWD_BPDU_CFG_BPDU_REDIR_ENA,
> > +                    lan9645x, ANA_CPU_FWD_BPDU_CFG(p));
> > +     }
> [ ... ]
> > +     /* Multicast to all front ports */
> > +     lan_wr(front_ports, lan9645x, ANA_PGID(PGID_MC));
> > +
> > +     /* IP multicast to all front ports */
> > +     lan_wr(front_ports, lan9645x, ANA_PGID(PGID_MCIPV4));
> > +     lan_wr(front_ports, lan9645x, ANA_PGID(PGID_MCIPV6));
> > +
> > +     /* Unicast to all front ports */
> > +     lan_wr(front_ports, lan9645x, ANA_PGID(PGID_UC));
> 
> [Severity: High]
> Does this configuration drop normal data plane traffic intended for the CPU on
> standalone ports? Setting the source forwarding mask (PGID_SRC) to 0 for all
> user ports seems to block them from forwarding frames to any destination,
> including the NPI or CPU port module.

False positive. The CPU port module is ignored in PGID_SRC. Setting this to 0
for standalone ports is exactly what we want. They must not forward to
their siblings. Copy or redirect to the CPU is not inhibited by this.

> 
> Furthermore, deriving front_ports from dsa_user_ports(ds) excludes the CPU
> port from the flooding masks. Should the CPU port be included in the source
> forwarding and flooding masks to ensure host traffic is not blackholed?

False positive. I have split the patches to make review easier.
The callbacks

port_fdb_add
port_mdb_add
port_set_host_flood

are added in later patches in the same series. These handle unknown and
known traffic toward the CPU. For standalone ports flooding of unknown
traffic to the cpu can be configured with promiscuous mode and
all_multi. So nothing is blackholed.


> 
> > --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_port.c
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_port.c
> [ ... ]
> > +void lan9645x_port_set_tail_drop_wm(struct lan9645x *lan9645x)
> > +{
> > +     int shared_per_port;
> > +     struct dsa_port *dp;
> > +
> > +     /* Configure tail dropping watermark */
> > +     shared_per_port =
> > +             lan9645x->shared_queue_sz / (lan9645x->num_phys_ports + 1);
> > +
> > +     /* The total memory size is divided by number of front ports plus CPU
> > +      * port.
> > +      */
> > +     dsa_switch_for_each_available_port(dp, lan9645x->ds)
> > +             lan_wr(lan9645x_wm_enc(shared_per_port), lan9645x,
> > +                    SYS_ATOP(dp->index));
> [ ... ]
> > +int lan9645x_port_set_maxlen(struct lan9645x *lan9645x, int port, size_t sdu)
> > +{
> > +     struct lan9645x_port *p = lan9645x_to_port(lan9645x, port);
> > +     int maxlen = sdu + ETH_HLEN + ETH_FCS_LEN;
> [ ... ]
> > +     /* Set Pause WM hysteresis */
> > +     lan_rmw(SYS_PAUSE_CFG_PAUSE_STOP_SET(lan9645x_wm_enc(4 * maxlen)) |
> > +             SYS_PAUSE_CFG_PAUSE_START_SET(lan9645x_wm_enc(6 * maxlen)),
> > +             SYS_PAUSE_CFG_PAUSE_START |
> > +             SYS_PAUSE_CFG_PAUSE_STOP,
> > +             lan9645x,
> > +             SYS_PAUSE_CFG(p->chip_port));
> > +
> > +     return 0;
> > +}
> 
> [Severity: Medium]
> Will jumbo frames hit the tail drop watermark before triggering flow control?
> 
> The tail drop watermark (SYS_ATOP) in lan9645x_port_set_tail_drop_wm() appears
> to be statically set to around 16KB. However, PAUSE_START dynamically scales
> with maxlen (6 * maxlen) in lan9645x_port_set_maxlen().
> 
> For MTUs larger than roughly 2.7KB, PAUSE_START will exceed SYS_ATOP. Could
> this cause the port to silently drop frames because it hits the tail drop
> limit before ever reaching the threshold to emit pause frames?
> 

The failure mode does not occur, even though the arithmetic is right.
However, this is due to another problem. The SYS_ATOP writes are inert
now because the taildrop watermarks only apply when cut-through is
enabled. I will remove the function lan9645x_port_set_tail_drop_wm


> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260805-dsa_lan9645x_switch_driver_base-v11-0-007ebc983a0a@microchip.com?part=4