| Message ID | 36da82de-6dad-939e-7604-d3c0b93f54c8@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] net: phy: improve genphy_read_status | expand |
> - 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
> - 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
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
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
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
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.
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 >
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;
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(-)