diff mbox series

[net-next,v2,6/6] net: dsa: b53: remove eee_enabled/eee_active in b53_get_mac_eee()

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

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1048 this patch: 1048
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 0 of 0 maintainers
netdev/build_clang success Errors and warnings before: 1065 this patch: 1065
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1065 this patch: 1065
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 16 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-02-04--21-00 (tests: 721)

Commit Message

Russell King (Oracle) Feb. 4, 2024, 12:13 p.m. UTC
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>
---
 drivers/net/dsa/b53/b53_common.c | 6 ------
 1 file changed, 6 deletions(-)

Comments

Vladimir Oltean Feb. 6, 2024, 11:20 a.m. UTC | #1
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, &reg);
> -	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
>
Russell King (Oracle) Feb. 6, 2024, 1:12 p.m. UTC | #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, &reg);
> > -	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.
Vladimir Oltean Feb. 6, 2024, 1:29 p.m. UTC | #3
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.
Florian Fainelli Feb. 7, 2024, 4:25 a.m. UTC | #4
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.
Vladimir Oltean Feb. 7, 2024, 1:52 p.m. UTC | #5
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.
Vladimir Oltean Feb. 7, 2024, 1:55 p.m. UTC | #6
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 mbox series

Patch

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, &reg);
-	e->eee_enabled = p->eee_enabled;
-	e->eee_active = !!(reg & BIT(port));
-
 	return 0;
 }
 EXPORT_SYMBOL(b53_get_mac_eee);