diff mbox series

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

Message ID 20230202125930.271740-8-rakesh.sankaranarayanan@microchip.com
State New
Headers
Series net: dsa: microchip: lan937x: add switch cascade support |

Commit Message

Rakesh Sankaranarayanan Feb. 2, 2023, 12:59 p.m. UTC 
  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.

Signed-off-by: Rakesh Sankaranarayanan <rakesh.sankaranarayanan@microchip.com>
---
 drivers/net/dsa/microchip/ksz_common.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Andrew Lunn Feb. 2, 2023, 3:40 p.m. UTC | #1 
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.

	Andrew
Rakesh Sankaranarayanan Feb. 3, 2023, 10:48 a.m. UTC | #2 
Hi Andrew,

Thanks for the comment, I will change and test the code as you
explained and update the patch in next revision.

Thanks,
Rakesh S.

On Thu, 2023-02-02 at 16:40 +0100, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> 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.
> 
>         Andrew
Vladimir Oltean Feb. 3, 2023, 11:37 p.m. UTC | #3 
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.
diff mbox series

Patch

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 2160a3e61a5a..0df71156a540 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -3213,6 +3213,7 @@  int ksz_switch_register(struct ksz_device *dev)
 {
 	const struct ksz_chip_data *info;
 	struct device_node *port, *ports;
+	struct dsa_switch_tree *dst;
 	phy_interface_t interface;
 	unsigned int port_num;
 	int ret;
@@ -3330,6 +3331,15 @@  int ksz_switch_register(struct ksz_device *dev)
 		return ret;
 	}
 
+	/* Do not proceed further if device tree setup is not done.
+	 * dsa_register_switch() will not report error in case of
+	 * cascaded switch. This will lead to scheduling mib read
+	 * work and kernel warning.
+	 */
+	dst = dev->ds->dst;
+	if (!dst->setup)
+		return 0;
+
 	/* Read MIB counters every 30 seconds to avoid overflow. */
 	dev->mib_read_interval = msecs_to_jiffies(5000);