Re: [RFC PATCH net-next 07/11] net: dsa: microchip: lan937x: update switch register

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:

quoted

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.