Re: [PATCH net-next 13/15] net: dsa: microchip: bypass dev_ops for port_setup()

[Bastien Curutchet]

Hi,

On 5/15/26 3:15 AM, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net: dsa: microchip: bypass dev_ops for port_setup()
>
> This commit removes an indirection layer for KSZ switches by connecting
> the port_setup operations directly to dsa_switch_ops, bypassing the
> ksz_dev_ops callback mechanism.
>
> > diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
> > index 7385aa4e788a1..2f63a3a7d6ceb 100644
> > --- a/drivers/net/dsa/microchip/ksz9477.c
> > +++ b/drivers/net/dsa/microchip/ksz9477.c
> [ ... ]
> > @@ -1309,6 +1309,23 @@ static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
> >   	ksz_pwrite8(dev, port, regs[REG_PORT_PME_CTRL], 0);
> >   }
> >   
> > +int ksz9477_dsa_port_setup(struct dsa_switch *ds, int port)
> > +{
> > +	struct ksz_device *dev = ds->priv;
> > +	int ret;
> > +
> > +	if (!dsa_is_user_port(ds, port))
> > +		return 0;
> > +
> > +	ksz9477_port_setup(dev, port, false);
> > +
> > +	ret = ksz9477_set_default_prio_queue_mapping(dev, port);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return ksz_dcb_init_port(dev, port);
> > +};
>
> This isn't a bug, but there's a stray semicolon at the end of the function definition.

Good catch.

> [ ... ]
> > diff --git a/drivers/net/dsa/microchip/lan937x_main.c b/drivers/net/dsa/microchip/lan937x_main.c
> > index ecb072cd99819..aff0c2735c307 100644
> > --- a/drivers/net/dsa/microchip/lan937x_main.c
> > +++ b/drivers/net/dsa/microchip/lan937x_main.c
> [ ... ]
> > @@ -823,7 +822,7 @@ const struct dsa_switch_ops lan937x_switch_ops = {
> >   	.phy_read		= ksz_phy_read16,
> >   	.phy_write		= ksz_phy_write16,
> >   	.phylink_get_caps	= lan937x_phylink_get_caps,
> > -	.port_setup		= ksz_port_setup,
> > +	.port_setup		= ksz9477_dsa_port_setup,
>
> Is it intentional to use ksz9477_dsa_port_setup() for LAN937x switches here?
>
> Previously, the shared ksz_port_setup() dynamically dispatched the
> initialization via dev->dev_ops->port_setup(), which correctly resolved to
> lan937x_port_setup() for LAN937x devices.
>
> Since the new ksz9477_dsa_port_setup() function explicitly hardcodes a call
> to ksz9477_port_setup(), won't this cause LAN937x ports to skip their
> specific initialization logic and instead receive incorrect register
> configurations meant for KSZ9477 hardware?

True, I'll fix this in next iteration


Best regards,
Bastien