[v3,net-next,3/4] net: dsa: microchip: get port link status
diff mbox series

Message ID 1550786597-591-4-git-send-email-Tristram.Ha@microchip.com
State Superseded
Delegated to: David Miller
Headers show
Series
  • net: dsa: microchip: add MIB counters support
Related show

Commit Message

Tristram.Ha@microchip.com Feb. 21, 2019, 10:03 p.m. UTC
From: Tristram Ha <Tristram.Ha@microchip.com>

Get port link status to know whether to read MIB counters when the link
is going down.  Add port_cleanup function to read MIB counters the last
time as when the port is disabled the PHY is also powered down.

Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
---
 drivers/net/dsa/microchip/ksz9477.c    |  2 ++
 drivers/net/dsa/microchip/ksz_common.c | 39 ++++++++++++++++++++++++++++++++--
 drivers/net/dsa/microchip/ksz_common.h |  3 +++
 drivers/net/dsa/microchip/ksz_priv.h   |  1 +
 4 files changed, 43 insertions(+), 2 deletions(-)

Comments

Andrew Lunn Feb. 21, 2019, 10:20 p.m. UTC | #1
On Thu, Feb 21, 2019 at 02:03:16PM -0800, Tristram.Ha@microchip.com wrote:
> From: Tristram Ha <Tristram.Ha@microchip.com>
> 
> Get port link status to know whether to read MIB counters when the link
> is going down.  Add port_cleanup function to read MIB counters the last
> time as when the port is disabled the PHY is also powered down.
> 
> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>

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

    Andrew
Florian Fainelli Feb. 21, 2019, 10:55 p.m. UTC | #2
On 2/21/19 2:03 PM, Tristram.Ha@microchip.com wrote:
> From: Tristram Ha <Tristram.Ha@microchip.com>
> 
> Get port link status to know whether to read MIB counters when the link
> is going down.  Add port_cleanup function to read MIB counters the last
> time as when the port is disabled the PHY is also powered down.
> 
> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
> ---

Some comments here and there again, not against a merge, but would be
nice to address in the future.

[snip]

> +void ksz_port_cleanup(struct ksz_device *dev, int port)
> +{
> +	/* Read all MIB counters when the link is going down. */
> +	if (dev->live_ports & (1 << port)) {
> +		struct ksz_port *p = &dev->ports[port];
> +
> +		p->read = true;
> +		schedule_work(&dev->mib_read);
> +	}
> +
> +	/* Common code for port cleanup. */
> +	dev->on_ports &= ~(1 << port);

It seems to be that dev->on_ports is a shorthand for a port is enabled
whether its link is up or down, which is equivalent to using:
!dsa_port_is_unused(ds, port)) unless I am mistaken about the intention
of the code.

In general you seem to be very prone to adding a lot of these
bookkeeping variables which just creates more opportunities for bugs to
creep in.

> +	dev->live_ports &= ~(1 << port);
> +}
> +EXPORT_SYMBOL_GPL(ksz_port_cleanup);
> +
>  void ksz_update_port_member(struct ksz_device *dev, int port)
>  {
>  	struct ksz_port *p;
> @@ -156,6 +172,26 @@ int ksz_phy_write16(struct dsa_switch *ds, int addr, int reg, u16 val)
>  }
>  EXPORT_SYMBOL_GPL(ksz_phy_write16);
>  
> +void ksz_adjust_link(struct dsa_switch *ds, int port,
> +		     struct phy_device *phydev)
> +{
> +	struct ksz_device *dev = ds->priv;
> +	struct ksz_port *p = &dev->ports[port];
> +
> +	if (!phydev->link) {
> +		/* Read all MIB counters when the link is going down. */
> +		if (dev->live_ports & (1 << port)) {
> +			p->read = true;
> +			schedule_work(&dev->mib_read);
> +		}
> +		dev->live_ports &= ~(1 << port);
> +	} else {
> +		/* Remember which port is connected and active. */
> +		dev->live_ports |= (1 << port) & dev->on_ports;
> +	}

How about something simpler than this:

	if (phydev->link != p->old_link) {
		p->read = !!phydev->link;
		schedule_work(&dev->mib_read);
	}

This is also super racy because you schedule the workqueue without any
locking against the state machine which runs with the PHY device's mutex
held, so there is the possibility of the following happening:

Thread 0			Thread 1

ksz_adjust_link()
	phydev->link = 0
	schedule_work()
				ksz_adjust_link()
				p->read = true
	mib_read_workqueue()

and so we don't read counters anymore. It is probably fine since this is
clearly entirely opportunistic and in order to minimize register bandwidth.

Patch
diff mbox series

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index e2d74c7..6ad28e2 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -1191,6 +1191,7 @@  static int ksz9477_setup(struct dsa_switch *ds)
 	.setup			= ksz9477_setup,
 	.phy_read		= ksz9477_phy_read16,
 	.phy_write		= ksz9477_phy_write16,
+	.adjust_link		= ksz_adjust_link,
 	.port_enable		= ksz_enable_port,
 	.port_disable		= ksz_disable_port,
 	.get_strings		= ksz9477_get_strings,
@@ -1340,6 +1341,7 @@  static void ksz9477_switch_exit(struct ksz_device *dev)
 	.cfg_port_member = ksz9477_cfg_port_member,
 	.flush_dyn_mac_table = ksz9477_flush_dyn_mac_table,
 	.phy_setup = ksz9477_phy_setup,
+	.port_cleanup = ksz_port_cleanup,
 	.port_setup = ksz9477_port_setup,
 	.r_mib_cnt = ksz9477_r_mib_cnt,
 	.r_mib_pkt = ksz9477_r_mib_pkt,
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index ff32ace..dd66056 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -20,6 +20,22 @@ 
 
 #include "ksz_priv.h"
 
+void ksz_port_cleanup(struct ksz_device *dev, int port)
+{
+	/* Read all MIB counters when the link is going down. */
+	if (dev->live_ports & (1 << port)) {
+		struct ksz_port *p = &dev->ports[port];
+
+		p->read = true;
+		schedule_work(&dev->mib_read);
+	}
+
+	/* Common code for port cleanup. */
+	dev->on_ports &= ~(1 << port);
+	dev->live_ports &= ~(1 << port);
+}
+EXPORT_SYMBOL_GPL(ksz_port_cleanup);
+
 void ksz_update_port_member(struct ksz_device *dev, int port)
 {
 	struct ksz_port *p;
@@ -156,6 +172,26 @@  int ksz_phy_write16(struct dsa_switch *ds, int addr, int reg, u16 val)
 }
 EXPORT_SYMBOL_GPL(ksz_phy_write16);
 
+void ksz_adjust_link(struct dsa_switch *ds, int port,
+		     struct phy_device *phydev)
+{
+	struct ksz_device *dev = ds->priv;
+	struct ksz_port *p = &dev->ports[port];
+
+	if (!phydev->link) {
+		/* Read all MIB counters when the link is going down. */
+		if (dev->live_ports & (1 << port)) {
+			p->read = true;
+			schedule_work(&dev->mib_read);
+		}
+		dev->live_ports &= ~(1 << port);
+	} else {
+		/* Remember which port is connected and active. */
+		dev->live_ports |= (1 << port) & dev->on_ports;
+	}
+}
+EXPORT_SYMBOL_GPL(ksz_adjust_link);
+
 int ksz_sset_count(struct dsa_switch *ds, int port, int sset)
 {
 	struct ksz_device *dev = ds->priv;
@@ -367,8 +403,7 @@  void ksz_disable_port(struct dsa_switch *ds, int port, struct phy_device *phy)
 {
 	struct ksz_device *dev = ds->priv;
 
-	dev->on_ports &= ~(1 << port);
-	dev->live_ports &= ~(1 << port);
+	dev->dev_ops->port_cleanup(dev, port);
 
 	/* port_stp_state_set() will be called after to disable the port so
 	 * there is no need to do anything.
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 0b0ed3d..ebe6f8e 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -7,6 +7,7 @@ 
 #ifndef __KSZ_COMMON_H
 #define __KSZ_COMMON_H
 
+void ksz_port_cleanup(struct ksz_device *dev, int port);
 void ksz_update_port_member(struct ksz_device *dev, int port);
 void ksz_init_mib_timer(struct ksz_device *dev);
 
@@ -14,6 +15,8 @@ 
 
 int ksz_phy_read16(struct dsa_switch *ds, int addr, int reg);
 int ksz_phy_write16(struct dsa_switch *ds, int addr, int reg, u16 val);
+void ksz_adjust_link(struct dsa_switch *ds, int port,
+		     struct phy_device *phydev);
 int ksz_sset_count(struct dsa_switch *ds, int port, int sset);
 void ksz_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *buf);
 int ksz_port_bridge_join(struct dsa_switch *ds, int port,
diff --git a/drivers/net/dsa/microchip/ksz_priv.h b/drivers/net/dsa/microchip/ksz_priv.h
index 1d2d98f..a1d84c1 100644
--- a/drivers/net/dsa/microchip/ksz_priv.h
+++ b/drivers/net/dsa/microchip/ksz_priv.h
@@ -137,6 +137,7 @@  struct ksz_dev_ops {
 	void (*flush_dyn_mac_table)(struct ksz_device *dev, int port);
 	void (*phy_setup)(struct ksz_device *dev, int port,
 			  struct phy_device *phy);
+	void (*port_cleanup)(struct ksz_device *dev, int port);
 	void (*port_setup)(struct ksz_device *dev, int port, bool cpu_port);
 	void (*r_phy)(struct ksz_device *dev, u16 phy, u16 reg, u16 *val);
 	void (*w_phy)(struct ksz_device *dev, u16 phy, u16 reg, u16 val);