| Message ID | 20200702095439.1355119-1-codrin.ciubotariu@microchip.com |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] net: dsa: microchip: split adjust_link() in phylink_mac_link_{up|down}() | expand |
On Thu, Jul 02, 2020 at 12:54:39PM +0300, Codrin Ciubotariu wrote: > The DSA subsystem moved to phylink and adjust_link() became deprecated in > the process. This patch removes adjust_link from the KSZ DSA switches and > adds phylink_mac_link_up() and phylink_mac_link_down(). > > Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> For the code _transformation_ that the patch does: Reviewed-by: Russell King <rmk+kernel@armlinux.org.uk> as it is equivalent. However, for a deeper review of what is going on here, I've a question: $ grep live_ports * ksz8795.c: dev->live_ports = dev->host_mask; ksz8795.c: dev->live_ports |= BIT(port); ksz_common.h: u16 live_ports; ksz_common.c: dev->live_ports &= ~(1 << port); ksz_common.c: dev->live_ports |= (1 << port) & dev->on_ports; ksz_common.c: dev->live_ports &= ~(1 << port); ksz9477.c: dev->live_ports = dev->host_mask; ksz9477.c: dev->live_ports |= (1 << port); These are the only references to dev->live_ports, so the purpose of dev->live_ports seems unclear; it seems it is only updated but never read. Can it be removed, along with the locking in your new functions? Thanks.
On 02.07.2020 13:19, Russell King - ARM Linux admin wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Thu, Jul 02, 2020 at 12:54:39PM +0300, Codrin Ciubotariu wrote: >> The DSA subsystem moved to phylink and adjust_link() became deprecated in >> the process. This patch removes adjust_link from the KSZ DSA switches and >> adds phylink_mac_link_up() and phylink_mac_link_down(). >> >> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> > > For the code _transformation_ that the patch does: > > Reviewed-by: Russell King <rmk+kernel@armlinux.org.uk> > > as it is equivalent. However, for a deeper review of what is going > on here, I've a question: > > $ grep live_ports * > ksz8795.c: dev->live_ports = dev->host_mask; > ksz8795.c: dev->live_ports |= BIT(port); > ksz_common.h: u16 live_ports; > ksz_common.c: dev->live_ports &= ~(1 << port); > ksz_common.c: dev->live_ports |= (1 << port) & dev->on_ports; > ksz_common.c: dev->live_ports &= ~(1 << port); > ksz9477.c: dev->live_ports = dev->host_mask; > ksz9477.c: dev->live_ports |= (1 << port); > > These are the only references to dev->live_ports, so the purpose of > dev->live_ports seems unclear; it seems it is only updated but never > read. Can it be removed, along with the locking in your new functions? Sure, I'll make a patch to clean things up. I will resend this patch in a series to mark the dependency. Thanks and best regards, Codrin > > Thanks. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! >
diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c index 47d65b77caf7..862306a9db2c 100644 --- a/drivers/net/dsa/microchip/ksz8795.c +++ b/drivers/net/dsa/microchip/ksz8795.c @@ -1111,7 +1111,8 @@ static const struct dsa_switch_ops ksz8795_switch_ops = { .setup = ksz8795_setup, .phy_read = ksz_phy_read16, .phy_write = ksz_phy_write16, - .adjust_link = ksz_adjust_link, + .phylink_mac_link_down = ksz_mac_link_down, + .phylink_mac_link_up = ksz_mac_link_up, .port_enable = ksz_enable_port, .port_disable = ksz_disable_port, .get_strings = ksz8795_get_strings, diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c index 9a51b8a4de5d..9e4bdd950194 100644 --- a/drivers/net/dsa/microchip/ksz9477.c +++ b/drivers/net/dsa/microchip/ksz9477.c @@ -1399,7 +1399,8 @@ static const struct dsa_switch_ops ksz9477_switch_ops = { .setup = ksz9477_setup, .phy_read = ksz9477_phy_read16, .phy_write = ksz9477_phy_write16, - .adjust_link = ksz_adjust_link, + .phylink_mac_link_down = ksz_mac_link_down, + .phylink_mac_link_up = ksz_mac_link_up, .port_enable = ksz_enable_port, .port_disable = ksz_disable_port, .get_strings = ksz9477_get_strings, diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c index fd1d6676ae4f..55ceaf00ece1 100644 --- a/drivers/net/dsa/microchip/ksz_common.c +++ b/drivers/net/dsa/microchip/ksz_common.c @@ -135,26 +135,34 @@ 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) +void ksz_mac_link_down(struct dsa_switch *ds, int port, unsigned int mode, + phy_interface_t interface) { struct ksz_device *dev = ds->priv; struct ksz_port *p = &dev->ports[port]; /* Read all MIB counters when the link is going down. */ - if (!phydev->link) { - p->read = true; - schedule_delayed_work(&dev->mib_read, 0); - } + p->read = true; + schedule_delayed_work(&dev->mib_read, 0); + + mutex_lock(&dev->dev_mutex); + dev->live_ports &= ~(1 << port); + mutex_unlock(&dev->dev_mutex); +} +EXPORT_SYMBOL_GPL(ksz_mac_link_down); + +void ksz_mac_link_up(struct dsa_switch *ds, int port, unsigned int mode, + phy_interface_t interface, struct phy_device *phydev, + int speed, int duplex, bool tx_pause, bool rx_pause) +{ + struct ksz_device *dev = ds->priv; + + /* Remember which port is connected and active. */ mutex_lock(&dev->dev_mutex); - if (!phydev->link) - dev->live_ports &= ~(1 << port); - else - /* Remember which port is connected and active. */ - dev->live_ports |= (1 << port) & dev->on_ports; + dev->live_ports |= (1 << port) & dev->on_ports; mutex_unlock(&dev->dev_mutex); } -EXPORT_SYMBOL_GPL(ksz_adjust_link); +EXPORT_SYMBOL_GPL(ksz_mac_link_up); int ksz_sset_count(struct dsa_switch *ds, int port, int sset) { diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h index f2c9bb68fd33..c0224dd0cf8a 100644 --- a/drivers/net/dsa/microchip/ksz_common.h +++ b/drivers/net/dsa/microchip/ksz_common.h @@ -159,8 +159,11 @@ void ksz_init_mib_timer(struct ksz_device *dev); 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); +void ksz_mac_link_down(struct dsa_switch *ds, int port, unsigned int mode, + phy_interface_t interface); +void ksz_mac_link_up(struct dsa_switch *ds, int port, unsigned int mode, + phy_interface_t interface, struct phy_device *phydev, + int speed, int duplex, bool tx_pause, bool rx_pause); 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,
The DSA subsystem moved to phylink and adjust_link() became deprecated in the process. This patch removes adjust_link from the KSZ DSA switches and adds phylink_mac_link_up() and phylink_mac_link_down(). Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com> --- drivers/net/dsa/microchip/ksz8795.c | 3 ++- drivers/net/dsa/microchip/ksz9477.c | 3 ++- drivers/net/dsa/microchip/ksz_common.c | 32 ++++++++++++++++---------- drivers/net/dsa/microchip/ksz_common.h | 7 ++++-- 4 files changed, 29 insertions(+), 16 deletions(-)