| Message ID | E1rWbNI-002cCz-4x@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3465df5533af94af8e3d3a524329e21a7698618c |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: eee network driver cleanups | expand |
On Sun, Feb 04, 2024 at 12:13:28PM +0000, Russell King (Oracle) wrote: > b53_get_mac_eee() sets both eee_enabled and eee_active, and then > returns zero. > > dsa_slave_get_eee(), which calls this function, will then continue to > call phylink_ethtool_get_eee(), which will return -EOPNOTSUPP if there > is no PHY present, otherwise calling phy_ethtool_get_eee() which in > turn will call genphy_c45_ethtool_get_eee(). Nitpick: If you need to resend, the function name changed to dsa_user_get_eee(). > > genphy_c45_ethtool_get_eee() will overwrite eee_enabled and eee_active > with its own interpretation from the PHYs settings and negotiation > result. > > Thus, when there is no PHY, dsa_slave_get_eee() will fail with Here too. > -EOPNOTSUPP, meaning eee_enabled and eee_active will not be returned to > userspace. When there is a PHY, eee_enabled and eee_active will be > overwritten by phylib, making the setting of these members in > b53_get_mac_eee() entirely unnecessary. > > Remove this code, thus simplifying b53_get_mac_eee(). > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > --- > drivers/net/dsa/b53/b53_common.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c > index adc93abf4551..9e4c9bd6abcc 100644 > --- a/drivers/net/dsa/b53/b53_common.c > +++ b/drivers/net/dsa/b53/b53_common.c > @@ -2227,16 +2227,10 @@ EXPORT_SYMBOL(b53_eee_init); > int b53_get_mac_eee(struct dsa_switch *ds, int port, struct ethtool_keee *e) > { > struct b53_device *dev = ds->priv; > - struct ethtool_keee *p = &dev->ports[port].eee; > - u16 reg; > > if (is5325(dev) || is5365(dev)) > return -EOPNOTSUPP; > > - b53_read16(dev, B53_EEE_PAGE, B53_EEE_LPI_INDICATE, ®); > - e->eee_enabled = p->eee_enabled; > - e->eee_active = !!(reg & BIT(port)); > - I know next to nothing about EEE and especially the implementation on Broadcom switches. But is the information brought by B53_EEE_LPI_INDICATE completely redundant? Is it actually in the system's best interest to ignore it? > return 0; > } > EXPORT_SYMBOL(b53_get_mac_eee); > -- > 2.30.2 >
On Tue, Feb 06, 2024 at 01:20:24PM +0200, Vladimir Oltean wrote: > On Sun, Feb 04, 2024 at 12:13:28PM +0000, Russell King (Oracle) wrote: > > b53_get_mac_eee() sets both eee_enabled and eee_active, and then > > returns zero. > > > > dsa_slave_get_eee(), which calls this function, will then continue to > > call phylink_ethtool_get_eee(), which will return -EOPNOTSUPP if there > > is no PHY present, otherwise calling phy_ethtool_get_eee() which in > > turn will call genphy_c45_ethtool_get_eee(). > > Nitpick: If you need to resend, the function name changed to > dsa_user_get_eee(). Thanks. > > @@ -2227,16 +2227,10 @@ EXPORT_SYMBOL(b53_eee_init); > > int b53_get_mac_eee(struct dsa_switch *ds, int port, struct ethtool_keee *e) > > { > > struct b53_device *dev = ds->priv; > > - struct ethtool_keee *p = &dev->ports[port].eee; > > - u16 reg; > > > > if (is5325(dev) || is5365(dev)) > > return -EOPNOTSUPP; > > > > - b53_read16(dev, B53_EEE_PAGE, B53_EEE_LPI_INDICATE, ®); > > - e->eee_enabled = p->eee_enabled; > > - e->eee_active = !!(reg & BIT(port)); > > - > > I know next to nothing about EEE and especially the implementation on > Broadcom switches. But is the information brought by B53_EEE_LPI_INDICATE > completely redundant? Is it actually in the system's best interest to > ignore it? That's a review comment that should have been made when the original change to phylib was done, because it's already ignored in kernels today since the commit changing phylib that I've referenced in this series - since e->eee_enabled and e->eee_active will be overwritten by phylib. If we need B53_EEE_LPI_INDICATE to do something, then we need to have a discussion about it, and decide how that fits in with the EEE interface, and how to work around phylib's implementation.
On Tue, Feb 06, 2024 at 01:12:33PM +0000, Russell King (Oracle) wrote: > > I know next to nothing about EEE and especially the implementation on > > Broadcom switches. But is the information brought by B53_EEE_LPI_INDICATE > > completely redundant? Is it actually in the system's best interest to > > ignore it? > > That's a review comment that should have been made when the original > change to phylib was done, because it's already ignored in kernels > today since the commit changing phylib that I've referenced in this > series - since e->eee_enabled and e->eee_active will be overwritten by > phylib. That's fair, but commit d1420bb99515 ("net: phy: improve generic EEE ethtool functions") is dated November 2018, and my involvement with the kernel started in March 2019. So it would have been a bit difficult for me to make this observation back then. > If we need B53_EEE_LPI_INDICATE to do something, then we need to have > a discussion about it, and decide how that fits in with the EEE > interface, and how to work around phylib's implementation. Hopefully Florian or Doug can quickly clarify whether this is the case or not.
On 2/6/2024 5:29 AM, Vladimir Oltean wrote: > On Tue, Feb 06, 2024 at 01:12:33PM +0000, Russell King (Oracle) wrote: >>> I know next to nothing about EEE and especially the implementation on >>> Broadcom switches. But is the information brought by B53_EEE_LPI_INDICATE >>> completely redundant? Is it actually in the system's best interest to >>> ignore it? >> >> That's a review comment that should have been made when the original >> change to phylib was done, because it's already ignored in kernels >> today since the commit changing phylib that I've referenced in this >> series - since e->eee_enabled and e->eee_active will be overwritten by >> phylib. > > That's fair, but commit d1420bb99515 ("net: phy: improve generic EEE > ethtool functions") is dated November 2018, and my involvement with the > kernel started in March 2019. So it would have been a bit difficult for > me to make this observation back then. > >> If we need B53_EEE_LPI_INDICATE to do something, then we need to have >> a discussion about it, and decide how that fits in with the EEE >> interface, and how to work around phylib's implementation. > > Hopefully Florian or Doug can quickly clarify whether this is the case > or not. Russell's replacement is actually a better one because it will return a stable state. B53_EEE_LPI_INDICATE would indicate when the switch port's built-in PHY asserts the LPI signal to its MAC, which could be transient AFAICT.
On Tue, Feb 06, 2024 at 08:25:17PM -0800, Florian Fainelli wrote: > > > On 2/6/2024 5:29 AM, Vladimir Oltean wrote: > > On Tue, Feb 06, 2024 at 01:12:33PM +0000, Russell King (Oracle) wrote: > > > > I know next to nothing about EEE and especially the implementation on > > > > Broadcom switches. But is the information brought by B53_EEE_LPI_INDICATE > > > > completely redundant? Is it actually in the system's best interest to > > > > ignore it? > > > > > > That's a review comment that should have been made when the original > > > change to phylib was done, because it's already ignored in kernels > > > today since the commit changing phylib that I've referenced in this > > > series - since e->eee_enabled and e->eee_active will be overwritten by > > > phylib. > > > > That's fair, but commit d1420bb99515 ("net: phy: improve generic EEE > > ethtool functions") is dated November 2018, and my involvement with the > > kernel started in March 2019. So it would have been a bit difficult for > > me to make this observation back then. > > > > > If we need B53_EEE_LPI_INDICATE to do something, then we need to have > > > a discussion about it, and decide how that fits in with the EEE > > > interface, and how to work around phylib's implementation. > > > > Hopefully Florian or Doug can quickly clarify whether this is the case > > or not. > > Russell's replacement is actually a better one because it will return a > stable state. B53_EEE_LPI_INDICATE would indicate when the switch port's > built-in PHY asserts the LPI signal to its MAC, which could be transient > AFAICT. > -- > Florian Thanks, Florian.
On Sun, Feb 04, 2024 at 12:13:28PM +0000, Russell King (Oracle) wrote: > b53_get_mac_eee() sets both eee_enabled and eee_active, and then > returns zero. > > dsa_slave_get_eee(), which calls this function, will then continue to > call phylink_ethtool_get_eee(), which will return -EOPNOTSUPP if there > is no PHY present, otherwise calling phy_ethtool_get_eee() which in > turn will call genphy_c45_ethtool_get_eee(). > > genphy_c45_ethtool_get_eee() will overwrite eee_enabled and eee_active > with its own interpretation from the PHYs settings and negotiation > result. > > Thus, when there is no PHY, dsa_slave_get_eee() will fail with > -EOPNOTSUPP, meaning eee_enabled and eee_active will not be returned to > userspace. When there is a PHY, eee_enabled and eee_active will be > overwritten by phylib, making the setting of these members in > b53_get_mac_eee() entirely unnecessary. > > Remove this code, thus simplifying b53_get_mac_eee(). > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > --- Reviewed-by: Vladimir Oltean <olteanv@gmail.com> I see the series was put in "Changes Requested", possibly due to my clarification question. Let's see if I can change that. --- pw-bot: under-review
diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c index adc93abf4551..9e4c9bd6abcc 100644 --- a/drivers/net/dsa/b53/b53_common.c +++ b/drivers/net/dsa/b53/b53_common.c @@ -2227,16 +2227,10 @@ EXPORT_SYMBOL(b53_eee_init); int b53_get_mac_eee(struct dsa_switch *ds, int port, struct ethtool_keee *e) { struct b53_device *dev = ds->priv; - struct ethtool_keee *p = &dev->ports[port].eee; - u16 reg; if (is5325(dev) || is5365(dev)) return -EOPNOTSUPP; - b53_read16(dev, B53_EEE_PAGE, B53_EEE_LPI_INDICATE, ®); - e->eee_enabled = p->eee_enabled; - e->eee_active = !!(reg & BIT(port)); - return 0; } EXPORT_SYMBOL(b53_get_mac_eee);