Messages in this thread | | | Date | Sat, 4 Feb 2023 01:37:17 +0200 | From | Vladimir Oltean <> | Subject | 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: > > 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.
| |