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, 25 Jul 2022 02:38:07 +0300
From:   Vladimir Oltean <olteanv@gmail.com>
To:     Marcin Wojtas <mw@semihalf.com>
Cc:     linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        andrew@lunn.ch, vivien.didelot@gmail.com, f.fainelli@gmail.com,
        davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, linux@armlinux.org.uk, jaz@semihalf.com,
        tn@semihalf.com
Subject: Re: [net-next: PATCH] net: dsa: mv88e6xxx: fix speed setting for
 CPU/DSA ports
Message-ID: <20220724233807.bthah6ctjadl35by@skbuf>
References: <20220714010021.1786616-1-mw@semihalf.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220714010021.1786616-1-mw@semihalf.com>
Precedence: bulk

Hi Marcin,

On Thu, Jul 14, 2022 at 03:00:21AM +0200, Marcin Wojtas wrote:
> Commit 3c783b83bd0f ("net: dsa: mv88e6xxx: get rid of SPEED_MAX setting")
> stopped relying on SPEED_MAX constant and hardcoded speed settings
> for the switch ports and rely on phylink configuration.
> 
> It turned out, however, that when the relevant code is called,
> the mac_capabilites of CPU/DSA port remain unset.
> mv88e6xxx_setup_port() is called via mv88e6xxx_setup() in
> dsa_tree_setup_switches(), which precedes setting the caps in
> phylink_get_caps down in the chain of dsa_tree_setup_ports().
> 
> As a result the mac_capabilites are 0 and the default speed for CPU/DSA
> port is 10M at the start. To fix that execute phylink_get_caps() callback
> which fills port's mac_capabilities before they are processed.
> 
> Fixes: 3c783b83bd0f ("net: dsa: mv88e6xxx: get rid of SPEED_MAX setting")
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  drivers/net/dsa/mv88e6xxx/chip.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 37b649501500..9fab76f256bb 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -3293,7 +3293,12 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port)
>  	 * port and all DSA ports to their maximum bandwidth and full duplex.
>  	 */
>  	if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port)) {
> -		unsigned long caps = dp->pl_config.mac_capabilities;
> +		unsigned long caps;
> +
> +		if (ds->ops->phylink_get_caps)
> +			ds->ops->phylink_get_caps(ds, port, &dp->pl_config);
> +
> +		caps = dp->pl_config.mac_capabilities;

We'll need this bug fixed in net-next one way or another.
If you resend this patch, please change the following:

(1) it's silly to (a) check for the presence of ds->ops->phylink_get_caps and
    (b) do an indirect function call when you know that the implementation is
    mv88e6xxx_get_caps(). So just call that.

(2) please don't touch &dp->pl_config, just create an on-stack
    struct phylink_config pl_config, and let DSA do its thing with
    &dp->pl_config whenever the timing of dsa_port_phylink_create() is.

>  
>  		if (chip->info->ops->port_max_speed_mode)
>  			mode = chip->info->ops->port_max_speed_mode(port);
> -- 
> 2.29.0
>