Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 12 Dec 2022 00:46:08 +0200
From:   Vladimir Oltean <olteanv@gmail.com>
To:     Jerry Ray <jerry.ray@microchip.com>
Cc:     Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, jbe@pengutronix.de,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux@armlinux.org.uk
Subject: Re: [PATCH net-next v5 5/6] dsa: lan9303: Determine CPU port based
 on dsa_switch ptr
Message-ID: <20221211224608.rdlcuqg4d37f7z66@skbuf>
References: <20221209224713.19980-1-jerry.ray@microchip.com>
 <20221209224713.19980-6-jerry.ray@microchip.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221209224713.19980-6-jerry.ray@microchip.com>
Precedence: bulk

On Fri, Dec 09, 2022 at 04:47:12PM -0600, Jerry Ray wrote:
> In preparing to move the adjust_link logic into the phylink_mac_link_up
> api, change the macro used to check for the cpu port.

No justification given for why this change is made.

As a counter argument, I could point out that DSA can support configurations
where the CPU port is one of the 100BASE-TX PHY ports, and the MII port
can be used as a regular user port where a PHY has been connected. Some
people are already doing this, for example connecting a Beaglebone Black
(can also be Raspberry Pi or what have you) over SPI to a VSC7512 switch
evaluation board.

The LAN9303 documentation makes it rather clear to me that such a
configuration would be possible, because the Switch Engine Ingress Port
Type Register allows any of the 3 switch ports to expect DSA tags. It's
lan9303_setup_tagging() the one who hardcodes the driver configuration
to be that where port 0 is the only acceptable CPU port (as well as the
early check from lan9303_setup()).

DSA's understanding of a CPU port is only that - a port which carries
DSA-tagged traffic, and is not exposed as a net_device to user space.
Nothing about MII vs internal PHY ports - that is a separate classification,
and a dsa_is_cpu_port() test is simply not something relevant from
phylink's perspective, to put it simply.

What we see in other device drivers for phylink handling is that there
is driver-level knowledge as to which ports have internal PHYs and which
have xMII pins. See priv->info->supports_mii[] in sja1105, dev->info->supports_mii[]
in the ksz drivers, mv88e6xxx_phy_is_internal() in mv88e6xxx,
felix->info->port_modes[] in the ocelot drivers, etc etc. Hardcoding
port number 0 is also better from my perspective than looking for the
CPU port. That's because the switch IP was built with the idea in mind
that port 0 is MII.

I would very much appreciate if this driver did not make any assumptions
that the internal PHY ports cannot carry DSA-tagged traffic. This
assumption was true when the driver was introduced, but it changed with
commit 0e27921816ad ("net: dsa: Use PHYLINK for the CPU/DSA ports").
Coincidentally, that is also the commit which started prompting the
lan9303 driver for an update, via the dmesg warnings you are seeing.

It looks like there is potentially more code to unlock than this simple
API change, which is something you could look at.

> 
> Signed-off-by: Jerry Ray <jerry.ray@microchip.com>
> ---
>  drivers/net/dsa/lan9303-core.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
> index 694249aa1f19..1d22e4b74308 100644
> --- a/drivers/net/dsa/lan9303-core.c
> +++ b/drivers/net/dsa/lan9303-core.c
> @@ -1064,7 +1064,11 @@ static void lan9303_adjust_link(struct dsa_switch *ds, int port,
>  	struct lan9303 *chip = ds->priv;
>  	int ctl;
>  
> -	if (!phy_is_pseudo_fixed_link(phydev))
> +	/* On this device, we are only interested in doing something here if
> +	 * this is the CPU port. All other ports are 10/100 phys using MDIO
> +	 * to control there link settings.
> +	 */
> +	if (!dsa_is_cpu_port(ds, port))
>  		return;
>  
>  	ctl = lan9303_phy_read(ds, port, MII_BMCR);
> -- 
> 2.17.1
>