[PATCH net-next] net: phy: improve link partner capability detection

[PATCH net-next] net: phy: improve link partner capability detection

[Heiner Kallweit]

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(-)

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;
-- 
2.21.0

Re: [PATCH net-next] net: phy: improve link partner capability detection

[Andrew Lunn]

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

Re: [PATCH net-next] net: phy: improve link partner capability detection

[Heiner Kallweit]

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

Re: [PATCH net-next] net: phy: improve link partner capability detection

[Andrew Lunn]

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

Re: [PATCH net-next] net: phy: improve link partner capability detection

[Heiner Kallweit]

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

Re: [PATCH net-next] net: phy: improve link partner capability detection

[Andrew Lunn]

> 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

Re: [PATCH net-next] net: phy: improve link partner capability detection

[Heiner Kallweit]

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

Re: [PATCH net-next] net: phy: improve link partner capability detection

[Florian Fainelli]

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?
-- 
Florian

Re: [PATCH net-next] net: phy: improve link partner capability detection

[Heiner Kallweit]

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.

Re: [PATCH net-next] net: phy: improve link partner capability detection

[Heiner Kallweit]

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.

Re: [PATCH net-next] net: phy: improve link partner capability detection

[Florian Fainelli]

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).
-- 
Florian

Re: [PATCH net-next] net: phy: improve link partner capability detection

[Andrew Lunn]

> +	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

Re: [PATCH net-next] net: phy: improve link partner capability detection

[Heiner Kallweit]

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

Re: [PATCH net-next] net: phy: improve link partner capability detection

[Heiner Kallweit]

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
> 

Re: [PATCH net-next] net: phy: improve link partner capability detection

[Florian Fainelli]

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...
-- 
Florian

Re: [PATCH net-next] net: phy: improve link partner capability detection

[Heiner Kallweit]

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 ..

Re: [PATCH net-next] net: phy: improve link partner capability detection

[Florian Fainelli]

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.
-- 
Florian

Re: [PATCH net-next] net: phy: improve link partner capability detection

[Florian Fainelli]

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>
-- 
Florian

Re: [PATCH net-next] net: phy: improve link partner capability detection

[David Miller]

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.