| Message ID | ba751e0e-7fa1-d687-24ac-303c5f54c08f@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] net: phy: improve link partner capability detection | expand |
On Fri, Apr 05, 2019 at 09:23:13PM +0200, Heiner Kallweit wrote: > genphy_read_status() so far checks phydev->supported, not the actual > PHY capabilities. This can make a difference if the supported speeds > have been limited by of_set_phy_supported() or phy_set_max_speed(). > > It seems that this issue only affects the link partner advertisements > as displayed by ethtool. Also this patch wouldn't apply to older > kernels because linkmode bitmaps have been introduced recently. > Therefore net-next. Hi Heiner Are you saying, if the local PHY/MAC does not support 1000Base-T, we should not check if the peer is advertising 1000Base-T? That seems wrong. We should report everything it offers. The fact we cannot make use of 1000Base-T should not matter, and resolving of the auto-get should do the right thing when presented with modes it cannot use. What do we do in the c45 case? The peer can do 2.5G, 5G and 10G, but the local can only do 2.5G? Do we just report the peers 2.5G and skip the others? Or do we report them all? Andrew
On 05.04.2019 21:48, Andrew Lunn wrote: > On Fri, Apr 05, 2019 at 09:23:13PM +0200, Heiner Kallweit wrote: >> genphy_read_status() so far checks phydev->supported, not the actual >> PHY capabilities. This can make a difference if the supported speeds >> have been limited by of_set_phy_supported() or phy_set_max_speed(). >> >> It seems that this issue only affects the link partner advertisements >> as displayed by ethtool. Also this patch wouldn't apply to older >> kernels because linkmode bitmaps have been introduced recently. >> Therefore net-next. > > Hi Heiner > Hi Andrew, > Are you saying, if the local PHY/MAC does not support 1000Base-T, we > should not check if the peer is advertising 1000Base-T? That seems > wrong. We should report everything it offers. The fact we cannot make > use of 1000Base-T should not matter, and resolving of the auto-get > should do the right thing when presented with modes it cannot use. > Link partner gigabit advertisement is reported in register MII_STAT1000. If we have a 100MBit PHY w/o this register, then we will never know whether the link partner supports gigabit. I would have to check the 802.3 specs, but it could even be that the gigabit autoneg is a separate step in the autoneg process. If this separate step isn't supported by a 100MBit PHY, then this PHY per se has no chance to detect whether link partner is gigabit-capable. > What do we do in the c45 case? The peer can do 2.5G, 5G and > 10G, but the local can only do 2.5G? Do we just report the peers 2.5G > and skip the others? Or do we report them all? > We report everything the chip reports. But similar to what I wrote about C22 and gigabit, what the chip reports may depend on which autoneg steps it does. Again I would have to check the standard whether e.g. NBase-T (2.5Gbps, 5Gbps) uses a separate autoneg step. If we don't advertise at least of these modes, then the PHY may decide to skip this autoneg step, and in the end we won't know whether link partner supports 2.5Gbps/5Gbps. > Andrew > Heiner
On Fri, Apr 05, 2019 at 10:04:22PM +0200, Heiner Kallweit wrote: > On 05.04.2019 21:48, Andrew Lunn wrote: > > On Fri, Apr 05, 2019 at 09:23:13PM +0200, Heiner Kallweit wrote: > >> genphy_read_status() so far checks phydev->supported, not the actual > >> PHY capabilities. This can make a difference if the supported speeds > >> have been limited by of_set_phy_supported() or phy_set_max_speed(). > >> > >> It seems that this issue only affects the link partner advertisements > >> as displayed by ethtool. Also this patch wouldn't apply to older > >> kernels because linkmode bitmaps have been introduced recently. > >> Therefore net-next. > > > > Hi Heiner > > > Hi Andrew, > > > Are you saying, if the local PHY/MAC does not support 1000Base-T, we > > should not check if the peer is advertising 1000Base-T? That seems > > wrong. We should report everything it offers. The fact we cannot make > > use of 1000Base-T should not matter, and resolving of the auto-get > > should do the right thing when presented with modes it cannot use. > > > Link partner gigabit advertisement is reported in register MII_STAT1000. > If we have a 100MBit PHY w/o this register, then we will never know > whether the link partner supports gigabit. Hi Heiner There seems to be two uses cases: A Fast MAC connected to a Fast PHY. In that case, MII_STAT1000 probably does not exist, so we should not read it. Hopefully .features says BASIC, or if we ask the PHY what is it, it reports it is a Fast PHY. A Fast MAC connected to a Giga PHY. The MAC driver will of used phy_set_max_speed() to indicate its limits. In that case, MII_STAT1000 does exist and we should report what the peer is advertising. Andrew
On 05.04.2019 22:27, Andrew Lunn wrote: > On Fri, Apr 05, 2019 at 10:04:22PM +0200, Heiner Kallweit wrote: >> On 05.04.2019 21:48, Andrew Lunn wrote: >>> On Fri, Apr 05, 2019 at 09:23:13PM +0200, Heiner Kallweit wrote: >>>> genphy_read_status() so far checks phydev->supported, not the actual >>>> PHY capabilities. This can make a difference if the supported speeds >>>> have been limited by of_set_phy_supported() or phy_set_max_speed(). >>>> >>>> It seems that this issue only affects the link partner advertisements >>>> as displayed by ethtool. Also this patch wouldn't apply to older >>>> kernels because linkmode bitmaps have been introduced recently. >>>> Therefore net-next. >>> >>> Hi Heiner >>> >> Hi Andrew, >> >>> Are you saying, if the local PHY/MAC does not support 1000Base-T, we >>> should not check if the peer is advertising 1000Base-T? That seems >>> wrong. We should report everything it offers. The fact we cannot make >>> use of 1000Base-T should not matter, and resolving of the auto-get >>> should do the right thing when presented with modes it cannot use. >>> >> Link partner gigabit advertisement is reported in register MII_STAT1000. >> If we have a 100MBit PHY w/o this register, then we will never know >> whether the link partner supports gigabit. > > Hi Heiner > > There seems to be two uses cases: > > A Fast MAC connected to a Fast PHY. In that case, MII_STAT1000 > probably does not exist, so we should not read it. Hopefully .features > says BASIC, or if we ask the PHY what is it, it reports it is a Fast > PHY. > Right. BMSR_ESTATEN should not be set on a Fast PHY. Handling of this case didn't change. > A Fast MAC connected to a Giga PHY. The MAC driver will of used > phy_set_max_speed() to indicate its limits. In that case, MII_STAT1000 > does exist and we should report what the peer is advertising. > That's what we're doing now with this patch. > Andrew > Heiner
> Right. BMSR_ESTATEN should not be set on a Fast PHY. Handling of this > case didn't change. > > > A Fast MAC connected to a Giga PHY. The MAC driver will of used > > phy_set_max_speed() to indicate its limits. In that case, MII_STAT1000 > > does exist and we should report what the peer is advertising. > > > That's what we're doing now with this patch. Hi Heiner What i don't get is why we need to do anything based on the MAC. All we need to do is look at BMSR_ESTATEN, and from that decided if we should look at MII_STAT1000 or not. When reporting what the peer can do, we should not care what the local MAC can do. Andrew
On 05.04.2019 22:43, Andrew Lunn wrote: >> Right. BMSR_ESTATEN should not be set on a Fast PHY. Handling of this >> case didn't change. >> >>> A Fast MAC connected to a Giga PHY. The MAC driver will of used >>> phy_set_max_speed() to indicate its limits. In that case, MII_STAT1000 >>> does exist and we should report what the peer is advertising. >>> >> That's what we're doing now with this patch. > > Hi Heiner > > What i don't get is why we need to do anything based on the MAC. All > we need to do is look at BMSR_ESTATEN, and from that decided if we > should look at MII_STAT1000 or not. When reporting what the peer can > do, we should not care what the local MAC can do. > Do we have a misunderstanding? What you describe is exactly what we're doing now. BMSR_ESTATEN is read by genphy_read_abilities(). I just don't want to read BMSR whenever genphy_read_status() is called. > Andrew > Heiner
On 4/5/19 1:51 PM, Heiner Kallweit wrote: > On 05.04.2019 22:43, Andrew Lunn wrote: >>> Right. BMSR_ESTATEN should not be set on a Fast PHY. Handling of this >>> case didn't change. >>> >>>> A Fast MAC connected to a Giga PHY. The MAC driver will of used >>>> phy_set_max_speed() to indicate its limits. In that case, MII_STAT1000 >>>> does exist and we should report what the peer is advertising. >>>> >>> That's what we're doing now with this patch. >> >> Hi Heiner >> >> What i don't get is why we need to do anything based on the MAC. All >> we need to do is look at BMSR_ESTATEN, and from that decided if we >> should look at MII_STAT1000 or not. When reporting what the peer can >> do, we should not care what the local MAC can do. >> > Do we have a misunderstanding? What you describe is exactly what we're > doing now. BMSR_ESTATEN is read by genphy_read_abilities(). > I just don't want to read BMSR whenever genphy_read_status() is called. You have to read the BMSR to determine the link status anyway, did you mean: have to check BMSR_ESTATEN whenever genphy_read_status() is called?
On 05.04.2019 23:11, Florian Fainelli wrote: > On 4/5/19 1:51 PM, Heiner Kallweit wrote: >> On 05.04.2019 22:43, Andrew Lunn wrote: >>>> Right. BMSR_ESTATEN should not be set on a Fast PHY. Handling of this >>>> case didn't change. >>>> >>>>> A Fast MAC connected to a Giga PHY. The MAC driver will of used >>>>> phy_set_max_speed() to indicate its limits. In that case, MII_STAT1000 >>>>> does exist and we should report what the peer is advertising. >>>>> >>>> That's what we're doing now with this patch. >>> >>> Hi Heiner >>> >>> What i don't get is why we need to do anything based on the MAC. All >>> we need to do is look at BMSR_ESTATEN, and from that decided if we >>> should look at MII_STAT1000 or not. When reporting what the peer can >>> do, we should not care what the local MAC can do. >>> >> Do we have a misunderstanding? What you describe is exactly what we're >> doing now. BMSR_ESTATEN is read by genphy_read_abilities(). >> I just don't want to read BMSR whenever genphy_read_status() is called. > > You have to read the BMSR to determine the link status anyway, did you > mean: have to check BMSR_ESTATEN whenever genphy_read_status() is called? > I would have to add extra logic to propagate BMSR_ESTATEN from genphy_update_link() to genphy_read_status(). BMSR_ESTATEN has a fixed value and therefore it's sufficient to read it initially.
On 05.04.2019 23:16, Heiner Kallweit wrote: > On 05.04.2019 23:11, Florian Fainelli wrote: >> On 4/5/19 1:51 PM, Heiner Kallweit wrote: >>> On 05.04.2019 22:43, Andrew Lunn wrote: >>>>> Right. BMSR_ESTATEN should not be set on a Fast PHY. Handling of this >>>>> case didn't change. >>>>> >>>>>> A Fast MAC connected to a Giga PHY. The MAC driver will of used >>>>>> phy_set_max_speed() to indicate its limits. In that case, MII_STAT1000 >>>>>> does exist and we should report what the peer is advertising. >>>>>> >>>>> That's what we're doing now with this patch. >>>> >>>> Hi Heiner >>>> >>>> What i don't get is why we need to do anything based on the MAC. All >>>> we need to do is look at BMSR_ESTATEN, and from that decided if we >>>> should look at MII_STAT1000 or not. When reporting what the peer can >>>> do, we should not care what the local MAC can do. >>>> >>> Do we have a misunderstanding? What you describe is exactly what we're >>> doing now. BMSR_ESTATEN is read by genphy_read_abilities(). >>> I just don't want to read BMSR whenever genphy_read_status() is called. >> >> You have to read the BMSR to determine the link status anyway, did you >> mean: have to check BMSR_ESTATEN whenever genphy_read_status() is called? >> > I would have to add extra logic to propagate BMSR_ESTATEN from > genphy_update_link() to genphy_read_status(). BMSR_ESTATEN has a fixed > value and therefore it's sufficient to read it initially. > In addition: BMSR_ESTATEN being set isn't a guarantee yet that the PHY supports gigabit. Still in MII_ESTATUS the gigabit bits couldn't be set. A Fast PHY could use some IP which is gigabit-prepared. I don't know if such a PHY exists, but it would be totally compliant with C22.
> + if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, > + phydev->supported)) > + phydev->is_gigabit_capable = 1; > + if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, > + phydev->supported)) > + phydev->is_gigabit_capable = 1; > + What i'm trying to get at is, why do we need this bit of the patch? Why do we need this flag? The hardware should tell us if it can do gigabit. Andrew
On 4/5/19 2:20 PM, Heiner Kallweit wrote: > On 05.04.2019 23:16, Heiner Kallweit wrote: >> On 05.04.2019 23:11, Florian Fainelli wrote: >>> On 4/5/19 1:51 PM, Heiner Kallweit wrote: >>>> On 05.04.2019 22:43, Andrew Lunn wrote: >>>>>> Right. BMSR_ESTATEN should not be set on a Fast PHY. Handling of this >>>>>> case didn't change. >>>>>> >>>>>>> A Fast MAC connected to a Giga PHY. The MAC driver will of used >>>>>>> phy_set_max_speed() to indicate its limits. In that case, MII_STAT1000 >>>>>>> does exist and we should report what the peer is advertising. >>>>>>> >>>>>> That's what we're doing now with this patch. >>>>> >>>>> Hi Heiner >>>>> >>>>> What i don't get is why we need to do anything based on the MAC. All >>>>> we need to do is look at BMSR_ESTATEN, and from that decided if we >>>>> should look at MII_STAT1000 or not. When reporting what the peer can >>>>> do, we should not care what the local MAC can do. >>>>> >>>> Do we have a misunderstanding? What you describe is exactly what we're >>>> doing now. BMSR_ESTATEN is read by genphy_read_abilities(). >>>> I just don't want to read BMSR whenever genphy_read_status() is called. >>> >>> You have to read the BMSR to determine the link status anyway, did you >>> mean: have to check BMSR_ESTATEN whenever genphy_read_status() is called? >>> >> I would have to add extra logic to propagate BMSR_ESTATEN from >> genphy_update_link() to genphy_read_status(). BMSR_ESTATEN has a fixed >> value and therefore it's sufficient to read it initially. >> > In addition: BMSR_ESTATEN being set isn't a guarantee yet that the PHY > supports gigabit. Still in MII_ESTATUS the gigabit bits couldn't be set. > A Fast PHY could use some IP which is gigabit-prepared. > I don't know if such a PHY exists, but it would be totally compliant > with C22. > There are Gigabit PHYs that are being limited in software to 10/100 because the Ethernet MAC is not capable, yet it's easier/cheaper to populate a gigabit capable PHY (Simmon's use case is one of them I believe, but I have come across that too).
On 05.04.2019 23:27, Andrew Lunn wrote: >> + if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, >> + phydev->supported)) >> + phydev->is_gigabit_capable = 1; >> + if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, >> + phydev->supported)) >> + phydev->is_gigabit_capable = 1; >> + > > What i'm trying to get at is, why do we need this bit of the patch? > Why do we need this flag? The hardware should tell us if it can do > gigabit. > The code to query BMSR_ESTATEN and MII_ESTATUS is in genphy_read_status. However we also have to cover the case that this function isn't used. Therefore I query phydev->supported before the speed could be limited. (relying on the PHY driver not lying about gigabit capability) This part of the patch is directly before of_set_phy_supported(). I just see that we can re-use is_gigabit_capable also in genphy_config_advert. Of course I can read in every place the hardware for gigabit support. But IMO this creates unnecessary code duplication. > Andrew > Heiner
On 05.04.2019 23:38, Heiner Kallweit wrote: > On 05.04.2019 23:27, Andrew Lunn wrote: >>> + if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, >>> + phydev->supported)) >>> + phydev->is_gigabit_capable = 1; >>> + if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, >>> + phydev->supported)) >>> + phydev->is_gigabit_capable = 1; >>> + >> >> What i'm trying to get at is, why do we need this bit of the patch? >> Why do we need this flag? The hardware should tell us if it can do >> gigabit. >> > The code to query BMSR_ESTATEN and MII_ESTATUS is in genphy_read_status. > However we also have to cover the case that this function isn't used. > Therefore I query phydev->supported before the speed could be limited. > (relying on the PHY driver not lying about gigabit capability) > This part of the patch is directly before of_set_phy_supported(). > > I just see that we can re-use is_gigabit_capable also in > genphy_config_advert. > > Of course I can read in every place the hardware for gigabit support. > But IMO this creates unnecessary code duplication. > I just see that we can reuse is_gigabit_capable also in genphy_config_advert(). And when checking occurrences of BMSR_ESTATEN there seems to be more work waiting: In swphy BMSR_ESTATEN is set, but MII_ESTATUS isn't configured. It just works by chance becausing reading this register returns the default 0xffff. >> Andrew >> > Heiner >
On 4/5/19 2:52 PM, Heiner Kallweit wrote: > On 05.04.2019 23:38, Heiner Kallweit wrote: >> On 05.04.2019 23:27, Andrew Lunn wrote: >>>> + if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, >>>> + phydev->supported)) >>>> + phydev->is_gigabit_capable = 1; >>>> + if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, >>>> + phydev->supported)) >>>> + phydev->is_gigabit_capable = 1; >>>> + >>> >>> What i'm trying to get at is, why do we need this bit of the patch? >>> Why do we need this flag? The hardware should tell us if it can do >>> gigabit. >>> >> The code to query BMSR_ESTATEN and MII_ESTATUS is in genphy_read_status. >> However we also have to cover the case that this function isn't used. >> Therefore I query phydev->supported before the speed could be limited. >> (relying on the PHY driver not lying about gigabit capability) >> This part of the patch is directly before of_set_phy_supported(). >> >> I just see that we can re-use is_gigabit_capable also in >> genphy_config_advert. >> >> Of course I can read in every place the hardware for gigabit support. >> But IMO this creates unnecessary code duplication. >> > I just see that we can reuse is_gigabit_capable also in > genphy_config_advert(). > > And when checking occurrences of BMSR_ESTATEN there seems to be more > work waiting: In swphy BMSR_ESTATEN is set, but MII_ESTATUS isn't > configured. It just works by chance becausing reading this register > returns the default 0xffff. It is not clear to me what we are trying to optimize for here, given everything is pretty much a slow path anyway. I am concerned though about mis-behaving PHYs where caching of BMSR_ESTATEN could result in incorrect behaviors, I don't have any data to back that claim, but reading the registers should always be the safest way to determine what HW is capable of (famous last words). Not feeling strongly about either direction though...
On 05.04.2019 23:54, Florian Fainelli wrote: > On 4/5/19 2:52 PM, Heiner Kallweit wrote: >> On 05.04.2019 23:38, Heiner Kallweit wrote: >>> On 05.04.2019 23:27, Andrew Lunn wrote: >>>>> + if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, >>>>> + phydev->supported)) >>>>> + phydev->is_gigabit_capable = 1; >>>>> + if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, >>>>> + phydev->supported)) >>>>> + phydev->is_gigabit_capable = 1; >>>>> + >>>> >>>> What i'm trying to get at is, why do we need this bit of the patch? >>>> Why do we need this flag? The hardware should tell us if it can do >>>> gigabit. >>>> >>> The code to query BMSR_ESTATEN and MII_ESTATUS is in genphy_read_status. >>> However we also have to cover the case that this function isn't used. >>> Therefore I query phydev->supported before the speed could be limited. >>> (relying on the PHY driver not lying about gigabit capability) >>> This part of the patch is directly before of_set_phy_supported(). >>> >>> I just see that we can re-use is_gigabit_capable also in >>> genphy_config_advert. >>> >>> Of course I can read in every place the hardware for gigabit support. >>> But IMO this creates unnecessary code duplication. >>> >> I just see that we can reuse is_gigabit_capable also in >> genphy_config_advert(). >> >> And when checking occurrences of BMSR_ESTATEN there seems to be more >> work waiting: In swphy BMSR_ESTATEN is set, but MII_ESTATUS isn't >> configured. It just works by chance becausing reading this register >> returns the default 0xffff. > > It is not clear to me what we are trying to optimize for here, given > everything is pretty much a slow path anyway. I am concerned though > about mis-behaving PHYs where caching of BMSR_ESTATEN could result in > incorrect behaviors, I don't have any data to back that claim, but > reading the registers should always be the safest way to determine what > HW is capable of (famous last words). Not feeling strongly about either > direction though... > At this point I have to quote Einstein: "If you can't explain it simply, you don't understand it well enough." Means I'll spend few more thoughts on it ..
On 4/6/2019 9:25 AM, Heiner Kallweit wrote: > On 05.04.2019 23:54, Florian Fainelli wrote: >> On 4/5/19 2:52 PM, Heiner Kallweit wrote: >>> On 05.04.2019 23:38, Heiner Kallweit wrote: >>>> On 05.04.2019 23:27, Andrew Lunn wrote: >>>>>> + if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, >>>>>> + phydev->supported)) >>>>>> + phydev->is_gigabit_capable = 1; >>>>>> + if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, >>>>>> + phydev->supported)) >>>>>> + phydev->is_gigabit_capable = 1; >>>>>> + >>>>> >>>>> What i'm trying to get at is, why do we need this bit of the patch? >>>>> Why do we need this flag? The hardware should tell us if it can do >>>>> gigabit. >>>>> >>>> The code to query BMSR_ESTATEN and MII_ESTATUS is in genphy_read_status. >>>> However we also have to cover the case that this function isn't used. >>>> Therefore I query phydev->supported before the speed could be limited. >>>> (relying on the PHY driver not lying about gigabit capability) >>>> This part of the patch is directly before of_set_phy_supported(). >>>> >>>> I just see that we can re-use is_gigabit_capable also in >>>> genphy_config_advert. >>>> >>>> Of course I can read in every place the hardware for gigabit support. >>>> But IMO this creates unnecessary code duplication. >>>> >>> I just see that we can reuse is_gigabit_capable also in >>> genphy_config_advert(). >>> >>> And when checking occurrences of BMSR_ESTATEN there seems to be more >>> work waiting: In swphy BMSR_ESTATEN is set, but MII_ESTATUS isn't >>> configured. It just works by chance becausing reading this register >>> returns the default 0xffff. >> >> It is not clear to me what we are trying to optimize for here, given >> everything is pretty much a slow path anyway. I am concerned though >> about mis-behaving PHYs where caching of BMSR_ESTATEN could result in >> incorrect behaviors, I don't have any data to back that claim, but >> reading the registers should always be the safest way to determine what >> HW is capable of (famous last words). Not feeling strongly about either >> direction though... >> > > At this point I have to quote Einstein: > "If you can't explain it simply, you don't understand it well enough." > Means I'll spend few more thoughts on it .. I have re-read the patch more carefully now and understand what it is fixing, sorry for the pointless discussion.
On 4/5/2019 12:23 PM, Heiner Kallweit wrote: > genphy_read_status() so far checks phydev->supported, not the actual > PHY capabilities. This can make a difference if the supported speeds > have been limited by of_set_phy_supported() or phy_set_max_speed(). > > It seems that this issue only affects the link partner advertisements > as displayed by ethtool. Also this patch wouldn't apply to older > kernels because linkmode bitmaps have been introduced recently. > Therefore net-next. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
From: Heiner Kallweit <hkallweit1@gmail.com> Date: Fri, 5 Apr 2019 21:23:13 +0200 > genphy_read_status() so far checks phydev->supported, not the actual > PHY capabilities. This can make a difference if the supported speeds > have been limited by of_set_phy_supported() or phy_set_max_speed(). > > It seems that this issue only affects the link partner advertisements > as displayed by ethtool. Also this patch wouldn't apply to older > kernels because linkmode bitmaps have been introduced recently. > Therefore net-next. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> Applied, thanks Heiner.
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index f7a6d0ffb..dab3ecbb4 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -1756,10 +1756,7 @@ int genphy_read_status(struct phy_device *phydev) linkmode_zero(phydev->lp_advertising); if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) { - if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, - phydev->supported) || - linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, - phydev->supported)) { + if (phydev->is_gigabit_capable) { lpagb = phy_read(phydev, MII_STAT1000); if (lpagb < 0) return lpagb; @@ -2154,6 +2151,13 @@ static int phy_probe(struct device *dev) if (err) goto out; + if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, + phydev->supported)) + phydev->is_gigabit_capable = 1; + if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, + phydev->supported)) + phydev->is_gigabit_capable = 1; + of_set_phy_supported(phydev); linkmode_copy(phydev->advertising, phydev->supported); diff --git a/include/linux/phy.h b/include/linux/phy.h index ab7439b3d..0f9552b17 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -345,6 +345,7 @@ struct phy_c45_device_ids { * is_c45: Set to true if this phy uses clause 45 addressing. * is_internal: Set to true if this phy is internal to a MAC. * is_pseudo_fixed_link: Set to true if this phy is an Ethernet switch, etc. + * is_gigabit_capable: Set to true if PHY supports 1000Mbps * has_fixups: Set to true if this phy has fixups/quirks. * suspended: Set to true if this phy has been suspended successfully. * sysfs_links: Internal boolean tracking sysfs symbolic links setup/removal. @@ -382,6 +383,7 @@ struct phy_device { unsigned is_c45:1; unsigned is_internal:1; unsigned is_pseudo_fixed_link:1; + unsigned is_gigabit_capable:1; unsigned has_fixups:1; unsigned suspended:1; unsigned sysfs_links:1;
genphy_read_status() so far checks phydev->supported, not the actual PHY capabilities. This can make a difference if the supported speeds have been limited by of_set_phy_supported() or phy_set_max_speed(). It seems that this issue only affects the link partner advertisements as displayed by ethtool. Also this patch wouldn't apply to older kernels because linkmode bitmaps have been introduced recently. Therefore net-next. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/phy/phy_device.c | 12 ++++++++---- include/linux/phy.h | 2 ++ 2 files changed, 10 insertions(+), 4 deletions(-)