Date:   Sat, 4 Feb 2023 01:37:17 +0200
From:   Vladimir Oltean <olteanv@gmail.com>
To:     Andrew Lunn <andrew@lunn.ch>
Cc:     Rakesh Sankaranarayanan <rakesh.sankaranarayanan@microchip.com>,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        f.fainelli@gmail.com, davem@davemloft.net, edumazet@google.com,
        kuba@kernel.org, pabeni@redhat.com, woojung.huh@microchip.com,
        UNGLinuxDriver@microchip.com, linux@armlinux.org.uk
Subject: Re: [RFC PATCH net-next 07/11] net: dsa: microchip: lan937x: update
 switch register
Message-ID: <20230203233717.m3s6xm4fynqmkx7h@skbuf>
References: <20230202125930.271740-1-rakesh.sankaranarayanan@microchip.com>
 <20230202125930.271740-8-rakesh.sankaranarayanan@microchip.com>
 <Y9vZdMQgqhaGIcdf@lunn.ch>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y9vZdMQgqhaGIcdf@lunn.ch>
Precedence: bulk

On Thu, Feb 02, 2023 at 04:40:36PM +0100, Andrew Lunn wrote:
> On Thu, Feb 02, 2023 at 06:29:26PM +0530, Rakesh Sankaranarayanan wrote:
> > Second switch in cascaded connection doesn't have port with macb
> > interface. dsa_switch_register returns error if macb interface is
> > not up. Due to this reason, second switch in cascaded connection will
> > not report error during dsa_switch_register and mib thread work will be
> > invoked even if actual switch register is not done. This will lead to
> > kernel warning and it can be avoided by checking device tree setup
> > status. This will return true only after actual switch register is done.
> 
> What i think you need to do is move the code into ksz_setup().
> 
> With a D in DSA setup, dsa_switch_register() adds the switch to the
> list of switches, and then a check is performed to see if all switches
> in the cluster have been registered. If not, it just returns. If all
> switches have been registered, it then iterates over all the switches
> can calls dsa_switch_ops.setup().
> 
> By moving the start of the MIB counter into setup(), it will only be
> started once all the switches are present, and it means you don't need
> to look at DSA core internal state.

+1

Also there's a bug in its own right in ksz_mib_read_work(), here:

			if (!netif_carrier_ok(dp->slave))
				mib->cnt_ptr = dev->info->reg_mib_cnt;

The code accesses dp->slave, so naturally it kicks the bucket for
cascade ports.

It doesn't crash with CPU ports because dp->slave is in a union with
dp->master, which is also a struct net_device * with its own carrier:

struct dsa_port {
	/* A CPU port is physically connected to a master device.
	 * A user port exposed to userspace has a slave device.
	 */
	union {
		struct net_device *master;
		struct net_device *slave;
	};

This needs to be fixed, since accessing the DSA master through a
dp->slave pointer is dangerous and unintended.

Easiest thing to do would be to only check link state if (dsa_port_is_user(dp)).
For other ports always read all MIB counters.