[PATCH 1/2] net: dsa: mv88e6xxx: re-order functions

[PATCH 1/2] net: dsa: mv88e6xxx: re-order functions

[Klaus Kudielka]

Move mv88e6xxx_setup() below mv88e6xxx_mdios_register(), so that we are
able to call the latter one from here. Do the same thing for the
inverse functions.

Signed-off-by: Klaus Kudielka <klaus.kudielka@gmail.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 358 +++++++++++++++----------------
 1 file changed, 179 insertions(+), 179 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 0a5d6c7bb1..496015baac 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3672,185 +3672,6 @@ static int mv88e6390_setup_errata(struct mv88e6xxx_chip *chip)
 	return mv88e6xxx_software_reset(chip);
 }
 
-static void mv88e6xxx_teardown(struct dsa_switch *ds)
-{
-	mv88e6xxx_teardown_devlink_params(ds);
-	dsa_devlink_resources_unregister(ds);
-	mv88e6xxx_teardown_devlink_regions_global(ds);
-}
-
-static int mv88e6xxx_setup(struct dsa_switch *ds)
-{
-	struct mv88e6xxx_chip *chip = ds->priv;
-	u8 cmode;
-	int err;
-	int i;
-
-	chip->ds = ds;
-	ds->slave_mii_bus = mv88e6xxx_default_mdio_bus(chip);
-
-	/* Since virtual bridges are mapped in the PVT, the number we support
-	 * depends on the physical switch topology. We need to let DSA figure
-	 * that out and therefore we cannot set this at dsa_register_switch()
-	 * time.
-	 */
-	if (mv88e6xxx_has_pvt(chip))
-		ds->max_num_bridges = MV88E6XXX_MAX_PVT_SWITCHES -
-				      ds->dst->last_switch - 1;
-
-	mv88e6xxx_reg_lock(chip);
-
-	if (chip->info->ops->setup_errata) {
-		err = chip->info->ops->setup_errata(chip);
-		if (err)
-			goto unlock;
-	}
-
-	/* Cache the cmode of each port. */
-	for (i = 0; i < mv88e6xxx_num_ports(chip); i++) {
-		if (chip->info->ops->port_get_cmode) {
-			err = chip->info->ops->port_get_cmode(chip, i, &cmode);
-			if (err)
-				goto unlock;
-
-			chip->ports[i].cmode = cmode;
-		}
-	}
-
-	err = mv88e6xxx_vtu_setup(chip);
-	if (err)
-		goto unlock;
-
-	/* Must be called after mv88e6xxx_vtu_setup (which flushes the
-	 * VTU, thereby also flushing the STU).
-	 */
-	err = mv88e6xxx_stu_setup(chip);
-	if (err)
-		goto unlock;
-
-	/* Setup Switch Port Registers */
-	for (i = 0; i < mv88e6xxx_num_ports(chip); i++) {
-		if (dsa_is_unused_port(ds, i))
-			continue;
-
-		/* Prevent the use of an invalid port. */
-		if (mv88e6xxx_is_invalid_port(chip, i)) {
-			dev_err(chip->dev, "port %d is invalid\n", i);
-			err = -EINVAL;
-			goto unlock;
-		}
-
-		err = mv88e6xxx_setup_port(chip, i);
-		if (err)
-			goto unlock;
-	}
-
-	err = mv88e6xxx_irl_setup(chip);
-	if (err)
-		goto unlock;
-
-	err = mv88e6xxx_mac_setup(chip);
-	if (err)
-		goto unlock;
-
-	err = mv88e6xxx_phy_setup(chip);
-	if (err)
-		goto unlock;
-
-	err = mv88e6xxx_pvt_setup(chip);
-	if (err)
-		goto unlock;
-
-	err = mv88e6xxx_atu_setup(chip);
-	if (err)
-		goto unlock;
-
-	err = mv88e6xxx_broadcast_setup(chip, 0);
-	if (err)
-		goto unlock;
-
-	err = mv88e6xxx_pot_setup(chip);
-	if (err)
-		goto unlock;
-
-	err = mv88e6xxx_rmu_setup(chip);
-	if (err)
-		goto unlock;
-
-	err = mv88e6xxx_rsvd2cpu_setup(chip);
-	if (err)
-		goto unlock;
-
-	err = mv88e6xxx_trunk_setup(chip);
-	if (err)
-		goto unlock;
-
-	err = mv88e6xxx_devmap_setup(chip);
-	if (err)
-		goto unlock;
-
-	err = mv88e6xxx_pri_setup(chip);
-	if (err)
-		goto unlock;
-
-	/* Setup PTP Hardware Clock and timestamping */
-	if (chip->info->ptp_support) {
-		err = mv88e6xxx_ptp_setup(chip);
-		if (err)
-			goto unlock;
-
-		err = mv88e6xxx_hwtstamp_setup(chip);
-		if (err)
-			goto unlock;
-	}
-
-	err = mv88e6xxx_stats_setup(chip);
-	if (err)
-		goto unlock;
-
-unlock:
-	mv88e6xxx_reg_unlock(chip);
-
-	if (err)
-		return err;
-
-	/* Have to be called without holding the register lock, since
-	 * they take the devlink lock, and we later take the locks in
-	 * the reverse order when getting/setting parameters or
-	 * resource occupancy.
-	 */
-	err = mv88e6xxx_setup_devlink_resources(ds);
-	if (err)
-		return err;
-
-	err = mv88e6xxx_setup_devlink_params(ds);
-	if (err)
-		goto out_resources;
-
-	err = mv88e6xxx_setup_devlink_regions_global(ds);
-	if (err)
-		goto out_params;
-
-	return 0;
-
-out_params:
-	mv88e6xxx_teardown_devlink_params(ds);
-out_resources:
-	dsa_devlink_resources_unregister(ds);
-
-	return err;
-}
-
-static int mv88e6xxx_port_setup(struct dsa_switch *ds, int port)
-{
-	return mv88e6xxx_setup_devlink_regions_port(ds, port);
-}
-
-static void mv88e6xxx_port_teardown(struct dsa_switch *ds, int port)
-{
-	mv88e6xxx_teardown_devlink_regions_port(ds, port);
-}
-
 /* prod_id for switch families which do not have a PHY model number */
 static const u16 family_prod_id_table[] = {
 	[MV88E6XXX_FAMILY_6341] = MV88E6XXX_PORT_SWITCH_ID_PROD_6341,
@@ -4054,6 +3875,185 @@ static int mv88e6xxx_mdios_register(struct mv88e6xxx_chip *chip,
 	return 0;
 }
 
+static void mv88e6xxx_teardown(struct dsa_switch *ds)
+{
+	mv88e6xxx_teardown_devlink_params(ds);
+	dsa_devlink_resources_unregister(ds);
+	mv88e6xxx_teardown_devlink_regions_global(ds);
+}
+
+static int mv88e6xxx_setup(struct dsa_switch *ds)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+	u8 cmode;
+	int err;
+	int i;
+
+	chip->ds = ds;
+	ds->slave_mii_bus = mv88e6xxx_default_mdio_bus(chip);
+
+	/* Since virtual bridges are mapped in the PVT, the number we support
+	 * depends on the physical switch topology. We need to let DSA figure
+	 * that out and therefore we cannot set this at dsa_register_switch()
+	 * time.
+	 */
+	if (mv88e6xxx_has_pvt(chip))
+		ds->max_num_bridges = MV88E6XXX_MAX_PVT_SWITCHES -
+				      ds->dst->last_switch - 1;
+
+	mv88e6xxx_reg_lock(chip);
+
+	if (chip->info->ops->setup_errata) {
+		err = chip->info->ops->setup_errata(chip);
+		if (err)
+			goto unlock;
+	}
+
+	/* Cache the cmode of each port. */
+	for (i = 0; i < mv88e6xxx_num_ports(chip); i++) {
+		if (chip->info->ops->port_get_cmode) {
+			err = chip->info->ops->port_get_cmode(chip, i, &cmode);
+			if (err)
+				goto unlock;
+
+			chip->ports[i].cmode = cmode;
+		}
+	}
+
+	err = mv88e6xxx_vtu_setup(chip);
+	if (err)
+		goto unlock;
+
+	/* Must be called after mv88e6xxx_vtu_setup (which flushes the
+	 * VTU, thereby also flushing the STU).
+	 */
+	err = mv88e6xxx_stu_setup(chip);
+	if (err)
+		goto unlock;
+
+	/* Setup Switch Port Registers */
+	for (i = 0; i < mv88e6xxx_num_ports(chip); i++) {
+		if (dsa_is_unused_port(ds, i))
+			continue;
+
+		/* Prevent the use of an invalid port. */
+		if (mv88e6xxx_is_invalid_port(chip, i)) {
+			dev_err(chip->dev, "port %d is invalid\n", i);
+			err = -EINVAL;
+			goto unlock;
+		}
+
+		err = mv88e6xxx_setup_port(chip, i);
+		if (err)
+			goto unlock;
+	}
+
+	err = mv88e6xxx_irl_setup(chip);
+	if (err)
+		goto unlock;
+
+	err = mv88e6xxx_mac_setup(chip);
+	if (err)
+		goto unlock;
+
+	err = mv88e6xxx_phy_setup(chip);
+	if (err)
+		goto unlock;
+
+	err = mv88e6xxx_pvt_setup(chip);
+	if (err)
+		goto unlock;
+
+	err = mv88e6xxx_atu_setup(chip);
+	if (err)
+		goto unlock;
+
+	err = mv88e6xxx_broadcast_setup(chip, 0);
+	if (err)
+		goto unlock;
+
+	err = mv88e6xxx_pot_setup(chip);
+	if (err)
+		goto unlock;
+
+	err = mv88e6xxx_rmu_setup(chip);
+	if (err)
+		goto unlock;
+
+	err = mv88e6xxx_rsvd2cpu_setup(chip);
+	if (err)
+		goto unlock;
+
+	err = mv88e6xxx_trunk_setup(chip);
+	if (err)
+		goto unlock;
+
+	err = mv88e6xxx_devmap_setup(chip);
+	if (err)
+		goto unlock;
+
+	err = mv88e6xxx_pri_setup(chip);
+	if (err)
+		goto unlock;
+
+	/* Setup PTP Hardware Clock and timestamping */
+	if (chip->info->ptp_support) {
+		err = mv88e6xxx_ptp_setup(chip);
+		if (err)
+			goto unlock;
+
+		err = mv88e6xxx_hwtstamp_setup(chip);
+		if (err)
+			goto unlock;
+	}
+
+	err = mv88e6xxx_stats_setup(chip);
+	if (err)
+		goto unlock;
+
+unlock:
+	mv88e6xxx_reg_unlock(chip);
+
+	if (err)
+		return err;
+
+	/* Have to be called without holding the register lock, since
+	 * they take the devlink lock, and we later take the locks in
+	 * the reverse order when getting/setting parameters or
+	 * resource occupancy.
+	 */
+	err = mv88e6xxx_setup_devlink_resources(ds);
+	if (err)
+		return err;
+
+	err = mv88e6xxx_setup_devlink_params(ds);
+	if (err)
+		goto out_resources;
+
+	err = mv88e6xxx_setup_devlink_regions_global(ds);
+	if (err)
+		goto out_params;
+
+	return 0;
+
+out_params:
+	mv88e6xxx_teardown_devlink_params(ds);
+out_resources:
+	dsa_devlink_resources_unregister(ds);
+
+	return err;
+}
+
+static int mv88e6xxx_port_setup(struct dsa_switch *ds, int port)
+{
+	return mv88e6xxx_setup_devlink_regions_port(ds, port);
+}
+
+static void mv88e6xxx_port_teardown(struct dsa_switch *ds, int port)
+{
+	mv88e6xxx_teardown_devlink_regions_port(ds, port);
+}
+
 static int mv88e6xxx_get_eeprom_len(struct dsa_switch *ds)
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
-- 
2.39.2

[PATCH 2/2] net: dsa: mv88e6xxx: move call to mv88e6xxx_mdios_register()

[Klaus Kudielka]

From commit 1a136ca2e089 ("net: mdio: scan bus based on bus capabilities
for C22 and C45") onwards, mdiobus_scan_bus_c45() is being called on buses
with MDIOBUS_NO_CAP. On a Turris Omnia (Armada 385, 88E6176 switch), this
causes a significant increase of boot time, from 1.6 seconds, to 6.3
seconds. The boot time stated here is until start of /init.

Further testing revealed that the C45 scan is indeed expensive (around
2.7 seconds, due to a huge number of bus transactions), and called twice.

It was suggested, to call mv88e6xxx_mdios_register() at the beginning of
mv88e6xxx_setup(), and mv88e6xxx_mdios_unregister() at the end of
mv88e6xxx_teardown(). This is accomplished by this patch.

Testing on the Turris Omnia revealed, that this improves the situation.
Now mdiobus_scan_bus_c45() is called only once, ending up in a boot time
of 4.3 seconds.

Link: https://lore.kernel.org/lkml/449bde236c08d5ab5e54abd73b645d8b29955894.camel@gmail.com/
Suggested-by: Andrew Lunn <andrew@lunn.ch>
Tested-by: Klaus Kudielka <klaus.kudielka@gmail.com>
Signed-off-by: Klaus Kudielka <klaus.kudielka@gmail.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 496015baac..8c0fa4cfcd 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3840,9 +3840,9 @@ static void mv88e6xxx_mdios_unregister(struct mv88e6xxx_chip *chip)
 	}
 }
 
-static int mv88e6xxx_mdios_register(struct mv88e6xxx_chip *chip,
-				    struct device_node *np)
+static int mv88e6xxx_mdios_register(struct mv88e6xxx_chip *chip)
 {
+	struct device_node *np = chip->dev->of_node;
 	struct device_node *child;
 	int err;
 
@@ -3877,9 +3877,12 @@ static int mv88e6xxx_mdios_register(struct mv88e6xxx_chip *chip,
 
 static void mv88e6xxx_teardown(struct dsa_switch *ds)
 {
+	struct mv88e6xxx_chip *chip = ds->priv;
+
 	mv88e6xxx_teardown_devlink_params(ds);
 	dsa_devlink_resources_unregister(ds);
 	mv88e6xxx_teardown_devlink_regions_global(ds);
+	mv88e6xxx_mdios_unregister(chip);
 }
 
 static int mv88e6xxx_setup(struct dsa_switch *ds)
@@ -3889,6 +3892,10 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
 	int err;
 	int i;
 
+	err = mv88e6xxx_mdios_register(chip);
+	if (err)
+		return err;
+
 	chip->ds = ds;
 	ds->slave_mii_bus = mv88e6xxx_default_mdio_bus(chip);
 
@@ -7220,18 +7227,12 @@ static int mv88e6xxx_probe(struct mdio_device *mdiodev)
 	if (err)
 		goto out_g1_atu_prob_irq;
 
-	err = mv88e6xxx_mdios_register(chip, np);
-	if (err)
-		goto out_g1_vtu_prob_irq;
-
 	err = mv88e6xxx_register_switch(chip);
 	if (err)
-		goto out_mdio;
+		goto out_g1_vtu_prob_irq;
 
 	return 0;
 
-out_mdio:
-	mv88e6xxx_mdios_unregister(chip);
 out_g1_vtu_prob_irq:
 	mv88e6xxx_g1_vtu_prob_irq_free(chip);
 out_g1_atu_prob_irq:
@@ -7268,7 +7269,6 @@ static void mv88e6xxx_remove(struct mdio_device *mdiodev)
 
 	mv88e6xxx_phy_destroy(chip);
 	mv88e6xxx_unregister_switch(chip);
-	mv88e6xxx_mdios_unregister(chip);
 
 	mv88e6xxx_g1_vtu_prob_irq_free(chip);
 	mv88e6xxx_g1_atu_prob_irq_free(chip);
-- 
2.39.2

Re: [PATCH 2/2] net: dsa: mv88e6xxx: move call to mv88e6xxx_mdios_register()

[Andrew Lunn]

On Sat, Mar 11, 2023 at 10:41:41AM +0100, Klaus Kudielka wrote:
> >From commit 1a136ca2e089 ("net: mdio: scan bus based on bus capabilities
> for C22 and C45") onwards, mdiobus_scan_bus_c45() is being called on buses
> with MDIOBUS_NO_CAP. On a Turris Omnia (Armada 385, 88E6176 switch), this
> causes a significant increase of boot time, from 1.6 seconds, to 6.3
> seconds. The boot time stated here is until start of /init.
> 
> Further testing revealed that the C45 scan is indeed expensive (around
> 2.7 seconds, due to a huge number of bus transactions), and called twice.
> 
> It was suggested, to call mv88e6xxx_mdios_register() at the beginning of
> mv88e6xxx_setup(), and mv88e6xxx_mdios_unregister() at the end of
> mv88e6xxx_teardown(). This is accomplished by this patch.
> 
> Testing on the Turris Omnia revealed, that this improves the situation.
> Now mdiobus_scan_bus_c45() is called only once, ending up in a boot time
> of 4.3 seconds.

For those who are interested, here is a bit of background on why this
change reduces the number of bus scans.

The MAC driver probes, which i think in this case is mvneta. Part way
through its probe, it registers its MDIO bus. That triggers a scan of
its bus, and the switch is found. The mv88e6xxx driver is then loaded
and its probe function called. towards the end of the mv88e6xxxx probe
function, it registers its MDIO bus. That causes a scan of the
switches MDIO bus. Which is slow. After the scan completes, the
mv88e6xxx probe continues, and registers the switch with DSA core. The
core then parses the DT binding for the switch and looks for the
master ethernet interface. That is the interface which mvneta
provides. But mvneta is still only part way through its probe. It has
not yet registered its interface with the netdev core. So the DSA core
fails to find it and return EPROBE_DEFER. This causes the mv88e6xxx
driver to unwind its probe. The mvneat then gets a chance to finish
its probe and register its netdev. Some timer later, the driver core
runs the probes again for those drivers which returned EPROBE_DEFER,
mv88e6xxx registers its MDIO bus again, another scan is performed, the
switch is registered with the code, and this time the master device is
available, so things continue. The DSA core then calls the drivers
.setup() callback to get the switch into a usable state.

I think what remains in the probe function is cheap, so it can
probable stay there and be done twice. But it might be worth putting
in a few printks to get some time stamps and see if anything is
expensive.

>  static int mv88e6xxx_setup(struct dsa_switch *ds)
> @@ -3889,6 +3892,10 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
>  	int err;
>  	int i;
>  
> +	err = mv88e6xxx_mdios_register(chip);
> +	if (err)
> +		return err;
> +
>  	chip->ds = ds;
>  	ds->slave_mii_bus = mv88e6xxx_default_mdio_bus(chip);

Other calls in mv88e6xxx_setup() can fail, so you need to extend the
cleanup to remove the mdio bus on failure.

	Andrew

Re: [PATCH 2/2] net: dsa: mv88e6xxx: move call to mv88e6xxx_mdios_register()

[Vladimir Oltean]

On Sat, Mar 11, 2023 at 04:19:43PM +0100, Andrew Lunn wrote:
> >  static int mv88e6xxx_setup(struct dsa_switch *ds)
> > @@ -3889,6 +3892,10 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
> >  	int err;
> >  	int i;
> >  
> > +	err = mv88e6xxx_mdios_register(chip);
> > +	if (err)
> > +		return err;
> > +
> >  	chip->ds = ds;
> >  	ds->slave_mii_bus = mv88e6xxx_default_mdio_bus(chip);
> 
> Other calls in mv88e6xxx_setup() can fail, so you need to extend the
> cleanup to remove the mdio bus on failure.

FWIW, here is a snippet of how mv88e6xxx_setup() and mv88e6xxx_teardown()
should look like, with error handling taken into consideration (but I
was lazy and just added forward declarations, something which Klaus
handled better with the movement preparatory patch):
https://lore.kernel.org/lkml/20230210210804.vdyfrog5nq6hrxi5@skbuf/

Re: [PATCH 2/2] net: dsa: mv88e6xxx: move call to mv88e6xxx_mdios_register()

[Vladimir Oltean]

On Sat, Mar 11, 2023 at 10:41:41AM +0100, Klaus Kudielka wrote:
> From commit 1a136ca2e089 ("net: mdio: scan bus based on bus capabilities
> for C22 and C45") onwards, mdiobus_scan_bus_c45() is being called on buses
> with MDIOBUS_NO_CAP. On a Turris Omnia (Armada 385, 88E6176 switch), this
> causes a significant increase of boot time, from 1.6 seconds, to 6.3
> seconds. The boot time stated here is until start of /init.
> 
> Further testing revealed that the C45 scan is indeed expensive (around
> 2.7 seconds, due to a huge number of bus transactions), and called twice.
> 
> It was suggested, to call mv88e6xxx_mdios_register() at the beginning of
> mv88e6xxx_setup(), and mv88e6xxx_mdios_unregister() at the end of
> mv88e6xxx_teardown(). This is accomplished by this patch.
> 
> Testing on the Turris Omnia revealed, that this improves the situation.
> Now mdiobus_scan_bus_c45() is called only once, ending up in a boot time
> of 4.3 seconds.
> 
> Link: https://lore.kernel.org/lkml/449bde236c08d5ab5e54abd73b645d8b29955894.camel@gmail.com/
> Suggested-by: Andrew Lunn <andrew@lunn.ch>
> Tested-by: Klaus Kudielka <klaus.kudielka@gmail.com>
> Signed-off-by: Klaus Kudielka <klaus.kudielka@gmail.com>
> ---

No objection to the change. However you might want to bundle it up with
another patch for the phy_mask restriction, and resend the series using
Andrew's indications.

Re: [PATCH 1/2] net: dsa: mv88e6xxx: re-order functions

[Andrew Lunn]

On Sat, Mar 11, 2023 at 10:41:40AM +0100, Klaus Kudielka wrote:
> Move mv88e6xxx_setup() below mv88e6xxx_mdios_register(), so that we are
> able to call the latter one from here. Do the same thing for the
> inverse functions.
> 
> Signed-off-by: Klaus Kudielka <klaus.kudielka@gmail.com>

Hi Klaus

If this your first patchset for netdev? There are a few process issues
you missed. Please take a look at:

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html

This patchset if for net-next, so the subject should indicate that.
It is also normal to include a patch 0/X which explains the big
picture. Part of the commit message you have in patch 2/2 would then
appear in 0/2.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew