[PATCH net-next] net: phy: improve genphy_read_status

[PATCH net-next] net: phy: improve genphy_read_status

[Heiner Kallweit]

This patch improves few aspects of genphy_read_status():

- Don't initialize lpagb, it's not needed.

- Move initializing phydev->speed et al before the if clause.

- In auto-neg case, skip populating lp_advertising if we
  don't have a link. This avoids quite some unnecessary
  MDIO reads in case of phylib polling mode.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy_device.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 0c4cc9b54..67fc581e3 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1743,19 +1743,21 @@ EXPORT_SYMBOL(genphy_update_link);
  */
 int genphy_read_status(struct phy_device *phydev)
 {
-	int adv;
-	int err;
-	int lpa;
-	int lpagb = 0;
+	int adv, lpa, lpagb, err;
 
 	/* Update the link, but return if there was an error */
 	err = genphy_update_link(phydev);
 	if (err)
 		return err;
 
+	phydev->speed = SPEED_UNKNOWN;
+	phydev->duplex = DUPLEX_UNKNOWN;
+	phydev->pause = 0;
+	phydev->asym_pause = 0;
+
 	linkmode_zero(phydev->lp_advertising);
 
-	if (AUTONEG_ENABLE == phydev->autoneg) {
+	if (phydev->autoneg == AUTONEG_ENABLE && phydev->link) {
 		if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
 				      phydev->supported) ||
 		    linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
@@ -1785,14 +1787,8 @@ int genphy_read_status(struct phy_device *phydev)
 			return lpa;
 
 		mii_lpa_mod_linkmode_lpa_t(phydev->lp_advertising, lpa);
-
-		phydev->speed = SPEED_UNKNOWN;
-		phydev->duplex = DUPLEX_UNKNOWN;
-		phydev->pause = 0;
-		phydev->asym_pause = 0;
-
 		phy_resolve_aneg_linkmode(phydev);
-	} else {
+	} else if (phydev->autoneg == AUTONEG_DISABLE) {
 		int bmcr = phy_read(phydev, MII_BMCR);
 
 		if (bmcr < 0)
@@ -1809,9 +1805,6 @@ int genphy_read_status(struct phy_device *phydev)
 			phydev->speed = SPEED_100;
 		else
 			phydev->speed = SPEED_10;
-
-		phydev->pause = 0;
-		phydev->asym_pause = 0;
 	}
 
 	return 0;
-- 
2.21.0

Re: [PATCH net-next] net: phy: improve genphy_read_status

[Andrew Lunn]

> -	if (AUTONEG_ENABLE == phydev->autoneg) {
> +	if (phydev->autoneg == AUTONEG_ENABLE && phydev->link) {

Hi Heiner

I don't necessary agree with placing the constant first in the
comparison, but it is best practice not to change it when making
additions. It makes it a little bit harder to see what the actual
change was.

       Andrew

Re: [PATCH net-next] net: phy: improve genphy_read_status

[Andrew Lunn]

> - In auto-neg case, skip populating lp_advertising if we
>   don't have a link. This avoids quite some unnecessary
>   MDIO reads in case of phylib polling mode.

Hi Heiner

Could it be that we don't have a link because what the partner is
advertising does not intersect with what we are advertising?

Hence knowing what it is adverting is actually useful in this case?

      Andrew

Re: [PATCH net-next] net: phy: improve genphy_read_status

[Heiner Kallweit]

On 31.03.2019 16:52, Andrew Lunn wrote:
>> -	if (AUTONEG_ENABLE == phydev->autoneg) {
>> +	if (phydev->autoneg == AUTONEG_ENABLE && phydev->link) {
> 
> Hi Heiner
> 
> I don't necessary agree with placing the constant first in the
> comparison, but it is best practice not to change it when making
> additions. It makes it a little bit harder to see what the actual
> change was.
> 
I understand the point. However a patch to only change the order
of the operands most likely would also be rejected as not being
worth it. As a consequence we would have to live with it forever.
So I think it needs some "if we touch the code anyway" situation.
Or what would be the preferred way to change something like that
that is in general ok and can be just a little bit improved?

>        Andrew
> 
Heiner

Re: [PATCH net-next] net: phy: improve genphy_read_status

[Heiner Kallweit]

On 31.03.2019 16:58, Andrew Lunn wrote:
>> - In auto-neg case, skip populating lp_advertising if we
>>   don't have a link. This avoids quite some unnecessary
>>   MDIO reads in case of phylib polling mode.
> 
> Hi Heiner
> 
> Could it be that we don't have a link because what the partner is
> advertising does not intersect with what we are advertising?
> 
Interesting point. Yes, I think I missed that scenario.

> Hence knowing what it is adverting is actually useful in this case?
> 
>       Andrew
> 
Heiner

Re: [PATCH net-next] net: phy: improve genphy_read_status

[Andrew Lunn]

On Sun, Mar 31, 2019 at 04:59:13PM +0200, Heiner Kallweit wrote:
> On 31.03.2019 16:52, Andrew Lunn wrote:
> >> -	if (AUTONEG_ENABLE == phydev->autoneg) {
> >> +	if (phydev->autoneg == AUTONEG_ENABLE && phydev->link) {
> > 
> > Hi Heiner
> > 
> > I don't necessary agree with placing the constant first in the
> > comparison, but it is best practice not to change it when making
> > additions. It makes it a little bit harder to see what the actual
> > change was.
> > 
> I understand the point. However a patch to only change the order
> of the operands most likely would also be rejected as not being
> worth it.

As i said, i don't necessarily agree with the ordering, but i don't
object to it. There are reasonable arguments which it is better. So i
would just leave it alone.

       Andrew

Re: [PATCH net-next] net: phy: improve genphy_read_status

[David Miller]

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Sat, 30 Mar 2019 10:22:45 +0100

> This patch improves few aspects of genphy_read_status():
> 
> - Don't initialize lpagb, it's not needed.
> 
> - Move initializing phydev->speed et al before the if clause.
> 
> - In auto-neg case, skip populating lp_advertising if we
>   don't have a link. This avoids quite some unnecessary
>   MDIO reads in case of phylib polling mode.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

I decided that I'm not going to push back on this patch purely on the
issue of not touching the conditional ordering.

Applied, thanks Heiner.

Re: [PATCH net-next] net: phy: improve genphy_read_status

[Heiner Kallweit]

On 31.03.2019 17:05, Heiner Kallweit wrote:
> On 31.03.2019 16:58, Andrew Lunn wrote:
>>> - In auto-neg case, skip populating lp_advertising if we
>>>   don't have a link. This avoids quite some unnecessary
>>>   MDIO reads in case of phylib polling mode.
>>
>> Hi Heiner
>>
>> Could it be that we don't have a link because what the partner is
>> advertising does not intersect with what we are advertising?
>>
> Interesting point. Yes, I think I missed that scenario.
> 
Just saw that Dave applied this patch. So I will send a fix to deal
with this scenario properly.

>> Hence knowing what it is adverting is actually useful in this case?
>>
>>       Andrew
>>
> Heiner
>