| 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 | expand |
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
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.
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);