[PATCH net-next] net: phy: improve pause mode reporting in phy_print_status

[PATCH net-next] net: phy: improve pause mode reporting in phy_print_status

[Heiner Kallweit]

So far we report symmetric pause only, and we don't consider the local
pause capabilities. Let's properly consider local and remote
capabilities, and report also asymmetric pause.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/phy/phy.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 1a146c5c5..e88854292 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -60,6 +60,32 @@ static void phy_link_down(struct phy_device *phydev, bool do_carrier)
 	phy_led_trigger_change_speed(phydev);
 }
 
+static const char *phy_pause_str(struct phy_device *phydev)
+{
+	bool local_pause, local_asym_pause;
+
+	if (phydev->autoneg == AUTONEG_DISABLE)
+		goto no_pause;
+
+	local_pause = linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
+					phydev->advertising);
+	local_asym_pause = linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
+					     phydev->advertising);
+
+	if (local_pause && phydev->pause)
+		return "rx/tx";
+
+	if (local_asym_pause && phydev->asym_pause) {
+		if (local_pause)
+			return "rx";
+		if (phydev->pause)
+			return "tx";
+	}
+
+no_pause:
+	return "off";
+}
+
 /**
  * phy_print_status - Convenience function to print out the current phy status
  * @phydev: the phy_device struct
@@ -71,7 +97,7 @@ void phy_print_status(struct phy_device *phydev)
 			"Link is Up - %s/%s - flow control %s\n",
 			phy_speed_to_str(phydev->speed),
 			phy_duplex_to_str(phydev->duplex),
-			phydev->pause ? "rx/tx" : "off");
+			phy_pause_str(phydev));
 	} else	{
 		netdev_info(phydev->attached_dev, "Link is Down\n");
 	}
-- 
2.21.0

Re: [PATCH net-next] net: phy: improve pause mode reporting in phy_print_status

[Florian Fainelli]

On 5/5/2019 10:03 AM, Heiner Kallweit wrote:
> So far we report symmetric pause only, and we don't consider the local
> pause capabilities. Let's properly consider local and remote
> capabilities, and report also asymmetric pause.

I would go one step further which is to print what is the link state of
RX/TX pause, so something like:

- local RX/TX pause advertisement
- link partnr RX/TX pause advertisement
- RX/TX being enabled for the link (auto-negotiated or manual)

this sort of duplicates what ethtool offers already but arguably so does
printing the link state so this would not be that big of a stretch.

I would make the print be something like:

Link is Up - 1Gb/Full - local pause: rx/tx, lpa pause: rx/tx, link
pause: auto-negotiated
Link is Up - 1Gb/Full - local pause: rx/tx, lpa pause: rx/tx, link
pause: forced off
Link is Up - 1Gb/Full - local pause: rx/tx, lpa pause: rx/tx, link
pause: forced on


Thanks!

> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/net/phy/phy.c | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 1a146c5c5..e88854292 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -60,6 +60,32 @@ static void phy_link_down(struct phy_device *phydev, bool do_carrier)
>  	phy_led_trigger_change_speed(phydev);
>  }
>  
> +static const char *phy_pause_str(struct phy_device *phydev)
> +{
> +	bool local_pause, local_asym_pause;
> +
> +	if (phydev->autoneg == AUTONEG_DISABLE)
> +		goto no_pause;
> +
> +	local_pause = linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> +					phydev->advertising);
> +	local_asym_pause = linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
> +					     phydev->advertising);
> +
> +	if (local_pause && phydev->pause)
> +		return "rx/tx";
> +
> +	if (local_asym_pause && phydev->asym_pause) {
> +		if (local_pause)
> +			return "rx";
> +		if (phydev->pause)
> +			return "tx";
> +	}
> +
> +no_pause:
> +	return "off";
> +}
> +
>  /**
>   * phy_print_status - Convenience function to print out the current phy status
>   * @phydev: the phy_device struct
> @@ -71,7 +97,7 @@ void phy_print_status(struct phy_device *phydev)
>  			"Link is Up - %s/%s - flow control %s\n",
>  			phy_speed_to_str(phydev->speed),
>  			phy_duplex_to_str(phydev->duplex),
> -			phydev->pause ? "rx/tx" : "off");
> +			phy_pause_str(phydev));
>  	} else	{
>  		netdev_info(phydev->attached_dev, "Link is Down\n");
>  	}
> 

-- 
Florian

Re: [PATCH net-next] net: phy: improve pause mode reporting in phy_print_status

[Heiner Kallweit]

On 05.05.2019 19:10, Florian Fainelli wrote:
> 
> 
> On 5/5/2019 10:03 AM, Heiner Kallweit wrote:
>> So far we report symmetric pause only, and we don't consider the local
>> pause capabilities. Let's properly consider local and remote
>> capabilities, and report also asymmetric pause.
> 
> I would go one step further which is to print what is the link state of
> RX/TX pause, so something like:
> 
> - local RX/TX pause advertisement
> - link partnr RX/TX pause advertisement
> - RX/TX being enabled for the link (auto-negotiated or manual)
> 
> this sort of duplicates what ethtool offers already but arguably so does
> printing the link state so this would not be that big of a stretch.
> 
> I would make the print be something like:
> 
> Link is Up - 1Gb/Full - local pause: rx/tx, lpa pause: rx/tx, link
> pause: auto-negotiated
> Link is Up - 1Gb/Full - local pause: rx/tx, lpa pause: rx/tx, link
> pause: forced off
> Link is Up - 1Gb/Full - local pause: rx/tx, lpa pause: rx/tx, link
> pause: forced on
> 
For speed and duplex we don't print the capabilities of both sides
but the negotiation result. Therefore I think it's more plausible
to do the same for pause.
IMO the intention of phy_print_status() is to print what is
effectively used. If a user is interested in the detailed capabilities
of both sides he can use ethtool, as mentioned by you.

In fixed mode we currently report pause "off" always.

Maybe, before we go further, one question for my understanding:
If the link partner doesn't support pause, who tells the MAC how that
it must not send pause frames? Is the network driver supposed to
do this in the adjust_link callback?

In the Realtek network chip datasheet I found a vague comment that
the MAC checks the aneg result of the internal PHY to decide
whether send pause frames or not.

> 
> Thanks!
> 
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/net/phy/phy.c | 28 +++++++++++++++++++++++++++-
>>  1 file changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>> index 1a146c5c5..e88854292 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -60,6 +60,32 @@ static void phy_link_down(struct phy_device *phydev, bool do_carrier)
>>  	phy_led_trigger_change_speed(phydev);
>>  }
>>  
>> +static const char *phy_pause_str(struct phy_device *phydev)
>> +{
>> +	bool local_pause, local_asym_pause;
>> +
>> +	if (phydev->autoneg == AUTONEG_DISABLE)
>> +		goto no_pause;
>> +
>> +	local_pause = linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
>> +					phydev->advertising);
>> +	local_asym_pause = linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
>> +					     phydev->advertising);
>> +
>> +	if (local_pause && phydev->pause)
>> +		return "rx/tx";
>> +
>> +	if (local_asym_pause && phydev->asym_pause) {
>> +		if (local_pause)
>> +			return "rx";
>> +		if (phydev->pause)
>> +			return "tx";
>> +	}
>> +
>> +no_pause:
>> +	return "off";
>> +}
>> +
>>  /**
>>   * phy_print_status - Convenience function to print out the current phy status
>>   * @phydev: the phy_device struct
>> @@ -71,7 +97,7 @@ void phy_print_status(struct phy_device *phydev)
>>  			"Link is Up - %s/%s - flow control %s\n",
>>  			phy_speed_to_str(phydev->speed),
>>  			phy_duplex_to_str(phydev->duplex),
>> -			phydev->pause ? "rx/tx" : "off");
>> +			phy_pause_str(phydev));
>>  	} else	{
>>  		netdev_info(phydev->attached_dev, "Link is Down\n");
>>  	}
>>
> 

Re: [PATCH net-next] net: phy: improve pause mode reporting in phy_print_status

[Florian Fainelli]

On 5/5/2019 10:31 AM, Heiner Kallweit wrote:
> On 05.05.2019 19:10, Florian Fainelli wrote:
>>
>>
>> On 5/5/2019 10:03 AM, Heiner Kallweit wrote:
>>> So far we report symmetric pause only, and we don't consider the local
>>> pause capabilities. Let's properly consider local and remote
>>> capabilities, and report also asymmetric pause.
>>
>> I would go one step further which is to print what is the link state of
>> RX/TX pause, so something like:
>>
>> - local RX/TX pause advertisement
>> - link partnr RX/TX pause advertisement
>> - RX/TX being enabled for the link (auto-negotiated or manual)
>>
>> this sort of duplicates what ethtool offers already but arguably so does
>> printing the link state so this would not be that big of a stretch.
>>
>> I would make the print be something like:
>>
>> Link is Up - 1Gb/Full - local pause: rx/tx, lpa pause: rx/tx, link
>> pause: auto-negotiated
>> Link is Up - 1Gb/Full - local pause: rx/tx, lpa pause: rx/tx, link
>> pause: forced off
>> Link is Up - 1Gb/Full - local pause: rx/tx, lpa pause: rx/tx, link
>> pause: forced on
>>
> For speed and duplex we don't print the capabilities of both sides
> but the negotiation result. Therefore I think it's more plausible
> to do the same for pause.

Pause is different though, if the link speed does not match, there is no
link, if the duplex do not match you may establish a link but there will
be a duplex mismatch which will cause all sorts of issues. Pause is not
an essential link parameter and is more of an optimization.

> IMO the intention of phy_print_status() is to print what is
> effectively used. If a user is interested in the detailed capabilities
> of both sides he can use ethtool, as mentioned by you.
> 
> In fixed mode we currently report pause "off" always.
> 
> Maybe, before we go further, one question for my understanding:
> If the link partner doesn't support pause, who tells the MAC how that
> it must not send pause frames? Is the network driver supposed to
> do this in the adjust_link callback?

If the link partner does not support pause, they are not advertised by
the link partner and you can read that from the LPA and the resolution
of the local pause and link partner pause settings should come back as
"not possible" (there may be caveats with symmetric vs. asymmetric pause
support).

PHYLINK is a good example of how pause should be reported towards the MAC.

> 
> In the Realtek network chip datasheet I found a vague comment that
> the MAC checks the aneg result of the internal PHY to decide
> whether send pause frames or not.

That would mean that the MAC behaves in a mode where it defaults to
pause frame being auto-negotiated, which is something that some Ethernet
MAC drivers default to as well. As long as you can disable pause when
the user requests it, that should be fine.

> 
>>
>> Thanks!
>>
>>>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>> ---
>>>  drivers/net/phy/phy.c | 28 +++++++++++++++++++++++++++-
>>>  1 file changed, 27 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>>> index 1a146c5c5..e88854292 100644
>>> --- a/drivers/net/phy/phy.c
>>> +++ b/drivers/net/phy/phy.c
>>> @@ -60,6 +60,32 @@ static void phy_link_down(struct phy_device *phydev, bool do_carrier)
>>>  	phy_led_trigger_change_speed(phydev);
>>>  }
>>>  
>>> +static const char *phy_pause_str(struct phy_device *phydev)
>>> +{
>>> +	bool local_pause, local_asym_pause;
>>> +
>>> +	if (phydev->autoneg == AUTONEG_DISABLE)
>>> +		goto no_pause;
>>> +
>>> +	local_pause = linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
>>> +					phydev->advertising);
>>> +	local_asym_pause = linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
>>> +					     phydev->advertising);
>>> +
>>> +	if (local_pause && phydev->pause)
>>> +		return "rx/tx";
>>> +
>>> +	if (local_asym_pause && phydev->asym_pause) {
>>> +		if (local_pause)
>>> +			return "rx";
>>> +		if (phydev->pause)
>>> +			return "tx";
>>> +	}
>>> +
>>> +no_pause:
>>> +	return "off";
>>> +}
>>> +
>>>  /**
>>>   * phy_print_status - Convenience function to print out the current phy status
>>>   * @phydev: the phy_device struct
>>> @@ -71,7 +97,7 @@ void phy_print_status(struct phy_device *phydev)
>>>  			"Link is Up - %s/%s - flow control %s\n",
>>>  			phy_speed_to_str(phydev->speed),
>>>  			phy_duplex_to_str(phydev->duplex),
>>> -			phydev->pause ? "rx/tx" : "off");
>>> +			phy_pause_str(phydev));
>>>  	} else	{
>>>  		netdev_info(phydev->attached_dev, "Link is Down\n");
>>>  	}
>>>
>>
> 

-- 
Florian

Re: [PATCH net-next] net: phy: improve pause mode reporting in phy_print_status

[Heiner Kallweit]

On 05.05.2019 20:46, Florian Fainelli wrote:
> 
> 
> On 5/5/2019 10:31 AM, Heiner Kallweit wrote:
>> On 05.05.2019 19:10, Florian Fainelli wrote:
>>>
>>>
>>> On 5/5/2019 10:03 AM, Heiner Kallweit wrote:
>>>> So far we report symmetric pause only, and we don't consider the local
>>>> pause capabilities. Let's properly consider local and remote
>>>> capabilities, and report also asymmetric pause.
>>>
>>> I would go one step further which is to print what is the link state of
>>> RX/TX pause, so something like:
>>>
>>> - local RX/TX pause advertisement
>>> - link partnr RX/TX pause advertisement
>>> - RX/TX being enabled for the link (auto-negotiated or manual)
>>>
>>> this sort of duplicates what ethtool offers already but arguably so does
>>> printing the link state so this would not be that big of a stretch.
>>>
>>> I would make the print be something like:
>>>
>>> Link is Up - 1Gb/Full - local pause: rx/tx, lpa pause: rx/tx, link
>>> pause: auto-negotiated
>>> Link is Up - 1Gb/Full - local pause: rx/tx, lpa pause: rx/tx, link
>>> pause: forced off
>>> Link is Up - 1Gb/Full - local pause: rx/tx, lpa pause: rx/tx, link
>>> pause: forced on
>>>
>> For speed and duplex we don't print the capabilities of both sides
>> but the negotiation result. Therefore I think it's more plausible
>> to do the same for pause.
> 
> Pause is different though, if the link speed does not match, there is no
> link, if the duplex do not match you may establish a link but there will
> be a duplex mismatch which will cause all sorts of issues. Pause is not
> an essential link parameter and is more of an optimization.
> 
Right, still I think this is too much and only partially relevant
information for the user. And if e.g. the remote side doesn't support
pause, then it's irrelevant what we support. I think the user is
(if at all) interested in the information which pause mode is effectively
used.

>> IMO the intention of phy_print_status() is to print what is
>> effectively used. If a user is interested in the detailed capabilities
>> of both sides he can use ethtool, as mentioned by you.
>>
>> In fixed mode we currently report pause "off" always.
>>
>> Maybe, before we go further, one question for my understanding:
>> If the link partner doesn't support pause, who tells the MAC how that
>> it must not send pause frames? Is the network driver supposed to
>> do this in the adjust_link callback?
> 
> If the link partner does not support pause, they are not advertised by
> the link partner and you can read that from the LPA and the resolution
> of the local pause and link partner pause settings should come back as
> "not possible" (there may be caveats with symmetric vs. asymmetric pause
> support).
> 
> PHYLINK is a good example of how pause should be reported towards the MAC.
> 
Thanks. So I think the usual MAC driver would have to check pause support
in the handler passed as argument to phy_connect_direct().

>>
>> In the Realtek network chip datasheet I found a vague comment that
>> the MAC checks the aneg result of the internal PHY to decide
>> whether send pause frames or not.
> 
> That would mean that the MAC behaves in a mode where it defaults to
> pause frame being auto-negotiated, which is something that some Ethernet
> MAC drivers default to as well. As long as you can disable pause when
> the user requests it, that should be fine.
> 
At least for the Realtek chips there is no documented way to disable pause.
If the remote side doesn't support pause, what happens if a pause frame is
sent? Is it just ignored or can we expect some sort of issue?

>>
>>>
>>> Thanks!
>>>
>>>>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>> ---
>>>>  drivers/net/phy/phy.c | 28 +++++++++++++++++++++++++++-
>>>>  1 file changed, 27 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>>>> index 1a146c5c5..e88854292 100644
>>>> --- a/drivers/net/phy/phy.c
>>>> +++ b/drivers/net/phy/phy.c
>>>> @@ -60,6 +60,32 @@ static void phy_link_down(struct phy_device *phydev, bool do_carrier)
>>>>  	phy_led_trigger_change_speed(phydev);
>>>>  }
>>>>  
>>>> +static const char *phy_pause_str(struct phy_device *phydev)
>>>> +{
>>>> +	bool local_pause, local_asym_pause;
>>>> +
>>>> +	if (phydev->autoneg == AUTONEG_DISABLE)
>>>> +		goto no_pause;
>>>> +
>>>> +	local_pause = linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
>>>> +					phydev->advertising);
>>>> +	local_asym_pause = linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
>>>> +					     phydev->advertising);
>>>> +
>>>> +	if (local_pause && phydev->pause)
>>>> +		return "rx/tx";
>>>> +
>>>> +	if (local_asym_pause && phydev->asym_pause) {
>>>> +		if (local_pause)
>>>> +			return "rx";
>>>> +		if (phydev->pause)
>>>> +			return "tx";
>>>> +	}
>>>> +
>>>> +no_pause:
>>>> +	return "off";
>>>> +}
>>>> +
>>>>  /**
>>>>   * phy_print_status - Convenience function to print out the current phy status
>>>>   * @phydev: the phy_device struct
>>>> @@ -71,7 +97,7 @@ void phy_print_status(struct phy_device *phydev)
>>>>  			"Link is Up - %s/%s - flow control %s\n",
>>>>  			phy_speed_to_str(phydev->speed),
>>>>  			phy_duplex_to_str(phydev->duplex),
>>>> -			phydev->pause ? "rx/tx" : "off");
>>>> +			phy_pause_str(phydev));
>>>>  	} else	{
>>>>  		netdev_info(phydev->attached_dev, "Link is Down\n");
>>>>  	}
>>>>
>>>
>>
> 

Re: [PATCH net-next] net: phy: improve pause mode reporting in phy_print_status

[Florian Fainelli]

On 5/5/2019 12:06 PM, Heiner Kallweit wrote:
> On 05.05.2019 20:46, Florian Fainelli wrote:
>>
>>
>> On 5/5/2019 10:31 AM, Heiner Kallweit wrote:
>>> On 05.05.2019 19:10, Florian Fainelli wrote:
>>>>
>>>>
>>>> On 5/5/2019 10:03 AM, Heiner Kallweit wrote:
>>>>> So far we report symmetric pause only, and we don't consider the local
>>>>> pause capabilities. Let's properly consider local and remote
>>>>> capabilities, and report also asymmetric pause.
>>>>
>>>> I would go one step further which is to print what is the link state of
>>>> RX/TX pause, so something like:
>>>>
>>>> - local RX/TX pause advertisement
>>>> - link partnr RX/TX pause advertisement
>>>> - RX/TX being enabled for the link (auto-negotiated or manual)
>>>>
>>>> this sort of duplicates what ethtool offers already but arguably so does
>>>> printing the link state so this would not be that big of a stretch.
>>>>
>>>> I would make the print be something like:
>>>>
>>>> Link is Up - 1Gb/Full - local pause: rx/tx, lpa pause: rx/tx, link
>>>> pause: auto-negotiated
>>>> Link is Up - 1Gb/Full - local pause: rx/tx, lpa pause: rx/tx, link
>>>> pause: forced off
>>>> Link is Up - 1Gb/Full - local pause: rx/tx, lpa pause: rx/tx, link
>>>> pause: forced on
>>>>
>>> For speed and duplex we don't print the capabilities of both sides
>>> but the negotiation result. Therefore I think it's more plausible
>>> to do the same for pause.
>>
>> Pause is different though, if the link speed does not match, there is no
>> link, if the duplex do not match you may establish a link but there will
>> be a duplex mismatch which will cause all sorts of issues. Pause is not
>> an essential link parameter and is more of an optimization.
>>
> Right, still I think this is too much and only partially relevant
> information for the user. And if e.g. the remote side doesn't support
> pause, then it's irrelevant what we support. I think the user is
> (if at all) interested in the information which pause mode is effectively
> used.

My point was really that I would rather see the resolved pause status,
which takes into account the link partner advertised, locally advertised
pause settings and local policy (enabled/disabled/auto-negotiated),
rather than the current/locally advertised pause settings which are just
one view of the link. Your patch is fine in that it properly decouples
the symetric/assymetric nature of the settings though.

> 
>>> IMO the intention of phy_print_status() is to print what is
>>> effectively used. If a user is interested in the detailed capabilities
>>> of both sides he can use ethtool, as mentioned by you.
>>>
>>> In fixed mode we currently report pause "off" always.
>>>
>>> Maybe, before we go further, one question for my understanding:
>>> If the link partner doesn't support pause, who tells the MAC how that
>>> it must not send pause frames? Is the network driver supposed to
>>> do this in the adjust_link callback?
>>
>> If the link partner does not support pause, they are not advertised by
>> the link partner and you can read that from the LPA and the resolution
>> of the local pause and link partner pause settings should come back as
>> "not possible" (there may be caveats with symmetric vs. asymmetric pause
>> support).
>>
>> PHYLINK is a good example of how pause should be reported towards the MAC.
>>
> Thanks. So I think the usual MAC driver would have to check pause support
> in the handler passed as argument to phy_connect_direct().

Given that pause can be changed from ethtool -A, would not that just be
a partial view of pause at the time the MAC and PHY get bound together?

> 
>>>
>>> In the Realtek network chip datasheet I found a vague comment that
>>> the MAC checks the aneg result of the internal PHY to decide
>>> whether send pause frames or not.
>>
>> That would mean that the MAC behaves in a mode where it defaults to
>> pause frame being auto-negotiated, which is something that some Ethernet
>> MAC drivers default to as well. As long as you can disable pause when
>> the user requests it, that should be fine.
>>
> At least for the Realtek chips there is no documented way to disable pause.
> If the remote side doesn't support pause, what happens if a pause frame is
> sent? Is it just ignored or can we expect some sort of issue?

Your mileage may vary of course, but if the remote side either does not
support pause or has receive pause frame support disabled, then these
frames should be ignored.
-- 
Florian

Re: [PATCH net-next] net: phy: improve pause mode reporting in phy_print_status

[Heiner Kallweit]

On 05.05.2019 21:12, Florian Fainelli wrote:
> 
> 
> On 5/5/2019 12:06 PM, Heiner Kallweit wrote:
>> On 05.05.2019 20:46, Florian Fainelli wrote:
>>>
>>>
>>> On 5/5/2019 10:31 AM, Heiner Kallweit wrote:
>>>> On 05.05.2019 19:10, Florian Fainelli wrote:
>>>>>
>>>>>
>>>>> On 5/5/2019 10:03 AM, Heiner Kallweit wrote:
>>>>>> So far we report symmetric pause only, and we don't consider the local
>>>>>> pause capabilities. Let's properly consider local and remote
>>>>>> capabilities, and report also asymmetric pause.
>>>>>
>>>>> I would go one step further which is to print what is the link state of
>>>>> RX/TX pause, so something like:
>>>>>
>>>>> - local RX/TX pause advertisement
>>>>> - link partnr RX/TX pause advertisement
>>>>> - RX/TX being enabled for the link (auto-negotiated or manual)
>>>>>
>>>>> this sort of duplicates what ethtool offers already but arguably so does
>>>>> printing the link state so this would not be that big of a stretch.
>>>>>
>>>>> I would make the print be something like:
>>>>>
>>>>> Link is Up - 1Gb/Full - local pause: rx/tx, lpa pause: rx/tx, link
>>>>> pause: auto-negotiated
>>>>> Link is Up - 1Gb/Full - local pause: rx/tx, lpa pause: rx/tx, link
>>>>> pause: forced off
>>>>> Link is Up - 1Gb/Full - local pause: rx/tx, lpa pause: rx/tx, link
>>>>> pause: forced on
>>>>>
>>>> For speed and duplex we don't print the capabilities of both sides
>>>> but the negotiation result. Therefore I think it's more plausible
>>>> to do the same for pause.
>>>
>>> Pause is different though, if the link speed does not match, there is no
>>> link, if the duplex do not match you may establish a link but there will
>>> be a duplex mismatch which will cause all sorts of issues. Pause is not
>>> an essential link parameter and is more of an optimization.
>>>
>> Right, still I think this is too much and only partially relevant
>> information for the user. And if e.g. the remote side doesn't support
>> pause, then it's irrelevant what we support. I think the user is
>> (if at all) interested in the information which pause mode is effectively
>> used.
> 
> My point was really that I would rather see the resolved pause status,
> which takes into account the link partner advertised, locally advertised
> pause settings and local policy (enabled/disabled/auto-negotiated),
> rather than the current/locally advertised pause settings which are just
> one view of the link. Your patch is fine in that it properly decouples
> the symetric/assymetric nature of the settings though.
> 
Not that we misunderstand each other:
phydev->pause and phydev->asym_pause represent the link partner
capabilities, see phy_resolve_aneg_linkmode(). Therefore my patch should
exactly do what you describe: resolve the pause status based on what
local and remote side advertise.

>>
>>>> IMO the intention of phy_print_status() is to print what is
>>>> effectively used. If a user is interested in the detailed capabilities
>>>> of both sides he can use ethtool, as mentioned by you.
>>>>
>>>> In fixed mode we currently report pause "off" always.
>>>>
>>>> Maybe, before we go further, one question for my understanding:
>>>> If the link partner doesn't support pause, who tells the MAC how that
>>>> it must not send pause frames? Is the network driver supposed to
>>>> do this in the adjust_link callback?
>>>
>>> If the link partner does not support pause, they are not advertised by
>>> the link partner and you can read that from the LPA and the resolution
>>> of the local pause and link partner pause settings should come back as
>>> "not possible" (there may be caveats with symmetric vs. asymmetric pause
>>> support).
>>>
>>> PHYLINK is a good example of how pause should be reported towards the MAC.
>>>
>> Thanks. So I think the usual MAC driver would have to check pause support
>> in the handler passed as argument to phy_connect_direct().
> 
> Given that pause can be changed from ethtool -A, would not that just be
> a partial view of pause at the time the MAC and PHY get bound together?
> 
>>
>>>>
>>>> In the Realtek network chip datasheet I found a vague comment that
>>>> the MAC checks the aneg result of the internal PHY to decide
>>>> whether send pause frames or not.
>>>
>>> That would mean that the MAC behaves in a mode where it defaults to
>>> pause frame being auto-negotiated, which is something that some Ethernet
>>> MAC drivers default to as well. As long as you can disable pause when
>>> the user requests it, that should be fine.
>>>
>> At least for the Realtek chips there is no documented way to disable pause.
>> If the remote side doesn't support pause, what happens if a pause frame is
>> sent? Is it just ignored or can we expect some sort of issue?
> 
> Your mileage may vary of course, but if the remote side either does not
> support pause or has receive pause frame support disabled, then these
> frames should be ignored.
> 

Re: [PATCH net-next] net: phy: improve pause mode reporting in phy_print_status

[David Miller]

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Sun, 5 May 2019 19:03:51 +0200

> So far we report symmetric pause only, and we don't consider the local
> pause capabilities. Let's properly consider local and remote
> capabilities, and report also asymmetric pause.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

It looks like Florian and Heiner are sort-of almost in agreement
here so I'll apply this and we can make whatever minor tweaks
are still necessary.

Thanks.