| Message ID | 1393257178-25052-1-git-send-email-cristian.bercaru@freescale.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
Hi Christian, 2014-02-24 7:52 GMT-08:00 <cristian.bercaru@freescale.com>: > From: Cristian Bercaru <cristian.bercaru@freescale.com> > > Masking the link partner's capabilities with local capabilities can be > misleading in autonegotiation scenarios such as PAUSE frame > autonegotiation. > This patch calculates the join between the local capabilities and the > link parner capabilities, when it calculates the speed and duplex > settings, but does not mask any of the link partner capabilities > when it calculates PAUSE frame settings. > > Signed-off-by: Cristian Bercaru <cristian.bercaru@freescale.com> > --- > v2 caches (lpa & adv) and (lpa & adv << 2) in intermediate variables. Looks much better: Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > > drivers/net/phy/phy_device.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index a70b604..4b021a3 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -933,6 +933,8 @@ int genphy_read_status(struct phy_device *phydev) > int err; > int lpa; > int lpagb = 0; > + int common_adv; > + int common_adv_gb; > > /* Update the link, but return if there was an error */ > err = genphy_update_link(phydev); > @@ -954,7 +956,7 @@ int genphy_read_status(struct phy_device *phydev) > > phydev->lp_advertising = > mii_stat1000_to_ethtool_lpa_t(lpagb); > - lpagb &= adv << 2; > + common_adv_gb = lpagb & adv << 2; > } > > lpa = phy_read(phydev, MII_LPA); > @@ -967,25 +969,25 @@ int genphy_read_status(struct phy_device *phydev) > if (adv < 0) > return adv; > > - lpa &= adv; > + common_adv = lpa & adv; > > phydev->speed = SPEED_10; > phydev->duplex = DUPLEX_HALF; > phydev->pause = 0; > phydev->asym_pause = 0; > > - if (lpagb & (LPA_1000FULL | LPA_1000HALF)) { > + if (common_adv_gb & (LPA_1000FULL | LPA_1000HALF)) { > phydev->speed = SPEED_1000; > > - if (lpagb & LPA_1000FULL) > + if (common_adv_gb & LPA_1000FULL) > phydev->duplex = DUPLEX_FULL; > - } else if (lpa & (LPA_100FULL | LPA_100HALF)) { > + } else if (common_adv & (LPA_100FULL | LPA_100HALF)) { > phydev->speed = SPEED_100; > > - if (lpa & LPA_100FULL) > + if (common_adv & LPA_100FULL) > phydev->duplex = DUPLEX_FULL; > } else > - if (lpa & LPA_10FULL) > + if (common_adv & LPA_10FULL) > phydev->duplex = DUPLEX_FULL; > > if (phydev->duplex == DUPLEX_FULL) { > -- > 1.7.11.7 > > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2014-02-24 at 17:52 +0200, cristian.bercaru@freescale.com wrote: > From: Cristian Bercaru <cristian.bercaru@freescale.com> > > Masking the link partner's capabilities with local capabilities can be > misleading in autonegotiation scenarios such as PAUSE frame > autonegotiation. > This patch calculates the join between the local capabilities and the > link parner capabilities, when it calculates the speed and duplex > settings, but does not mask any of the link partner capabilities > when it calculates PAUSE frame settings. > > Signed-off-by: Cristian Bercaru <cristian.bercaru@freescale.com> > --- > v2 caches (lpa & adv) and (lpa & adv << 2) in intermediate variables. > > drivers/net/phy/phy_device.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index a70b604..4b021a3 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -933,6 +933,8 @@ int genphy_read_status(struct phy_device *phydev) > int err; > int lpa; > int lpagb = 0; > + int common_adv; > + int common_adv_gb; I think common_adv_gb needs to be initialised to 0... > > /* Update the link, but return if there was an error */ > err = genphy_update_link(phydev); > @@ -954,7 +956,7 @@ int genphy_read_status(struct phy_device *phydev) > > phydev->lp_advertising = > mii_stat1000_to_ethtool_lpa_t(lpagb); > - lpagb &= adv << 2; > + common_adv_gb = lpagb & adv << 2; > } [...] ...as this assignment is conditional. Ben.
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index a70b604..4b021a3 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -933,6 +933,8 @@ int genphy_read_status(struct phy_device *phydev) int err; int lpa; int lpagb = 0; + int common_adv; + int common_adv_gb; /* Update the link, but return if there was an error */ err = genphy_update_link(phydev); @@ -954,7 +956,7 @@ int genphy_read_status(struct phy_device *phydev) phydev->lp_advertising = mii_stat1000_to_ethtool_lpa_t(lpagb); - lpagb &= adv << 2; + common_adv_gb = lpagb & adv << 2; } lpa = phy_read(phydev, MII_LPA); @@ -967,25 +969,25 @@ int genphy_read_status(struct phy_device *phydev) if (adv < 0) return adv; - lpa &= adv; + common_adv = lpa & adv; phydev->speed = SPEED_10; phydev->duplex = DUPLEX_HALF; phydev->pause = 0; phydev->asym_pause = 0; - if (lpagb & (LPA_1000FULL | LPA_1000HALF)) { + if (common_adv_gb & (LPA_1000FULL | LPA_1000HALF)) { phydev->speed = SPEED_1000; - if (lpagb & LPA_1000FULL) + if (common_adv_gb & LPA_1000FULL) phydev->duplex = DUPLEX_FULL; - } else if (lpa & (LPA_100FULL | LPA_100HALF)) { + } else if (common_adv & (LPA_100FULL | LPA_100HALF)) { phydev->speed = SPEED_100; - if (lpa & LPA_100FULL) + if (common_adv & LPA_100FULL) phydev->duplex = DUPLEX_FULL; } else - if (lpa & LPA_10FULL) + if (common_adv & LPA_10FULL) phydev->duplex = DUPLEX_FULL; if (phydev->duplex == DUPLEX_FULL) {