[PATCH net-next 0/4] Extend phylib implementation of pause support

[PATCH net-next 0/4] Extend phylib implementation of pause support

[Doug Berger]

This commit set extends the implementation introduced by
commit 5652b46e4e80 ("Merge branch 'Pause-updates-for-phylib-and-phylink'")
to support the less problematic behavior alluded to by Russell King's
comment in commit f904f15ea9b5 ("net: phylink: allow ethtool -A to
change flow control advertisement").

+	/*
+	 * See the comments for linkmode_set_pause(), wrt the deficiencies
+	 * with the current implementation.  A solution to this issue would
+	 * be:
+	 * ethtool  Local device
+	 *  rx  tx  Pause AsymDir
+	 *  0   0   0     0
+	 *  1   0   1     1
+	 *  0   1   0     1
+	 *  1   1   1     1
+	 * and then use the ethtool rx/tx enablement status to mask the
+	 * rx/tx pause resolution.
+	 */

Specifically, the linkmode_set_pause() function is extended to support
both the existing Pause/AsymPause mapping and the mapping specified by
the IEEE standard (and Russell). A phy_set_pause() function is added to
the phylib that can make use of this extension based on the value of
the pause autoneg parameter. The bcmgenet driver adds support for the
ethtool pauseparam ops based on these phylib services and uses "the
ethtool rx/tx enablement status to mask the rx/tx pause resolution".

The first commit in this set addresses a small deficiency in the 
phy_validate_pause() function.

The second extends linkmode_set_pause() with an autoneg parameter to
allow selection of the desired mapping for advertisement.

The third introduces the phy_set_pause() function based on the existing
phy_set_asym_pause() implementation. One aberration here is the direct
manipulation of the phy state machine to allow a new link up event to
notify the MAC that the pause parameters may have changed. This is a
convenience to simplify the MAC driver by allowing one implementation
of the pause configuration logic to be located in its adjust_link
callback. Otherwise, the MAC driver would need to handle configuring
the pause parameters for an already active PHY link which would likely
require additional synchronization logic to protect the logic from
asynchronous changes in the PHY state.

The logic in phy_set_asym_pause() that looks for a change in
advertising is not particularly helpful here since now a change from
tx=1 rx=1 to tx=0 rx=1 no longer changes the advertising if autoneg is
enabled so phy_start_aneg() would not be called. I took the alternate
approach of unconditionally calling phy_start_aneg() since it
accommodates both manual and autoneg configured links. The "aberrant"
logic allows manually configured and autonegotiated links that don't
change their advertised parameters to receive an adjust_link call to
act on pause parameters that have no effect on the PHY layer.

It seemed excessive to bring the PHY down and back up when nogotiation
is not necessary, but that could be an alternative approach. I am
certainly open to any suggestions on how to improve that portion of
the code if it is controversial and a consensus can be reached.

The last commit is a reference implementation of pause support by the
bcmgenet driver based on my preferences for the functionality. It is my
desire that other network drivers prefer this behavior and the changes
to the phylib will make it easier for them to support.

Many thanks to Russell King and Andrew Lunn for their efforts to clean
up and centralize support for pause and to document its shortcommings.

Doug Berger (4):
  net: ethernet: validate pause autoneg setting
  net: add autoneg parameter to linkmode_set_pause
  net: ethernet: introduce phy_set_pause
  net: bcmgenet: add support for ethtool flow control

 drivers/net/ethernet/broadcom/genet/bcmgenet.c |  54 +++++++++++++
 drivers/net/ethernet/broadcom/genet/bcmgenet.h |   4 +
 drivers/net/ethernet/broadcom/genet/bcmmii.c   |  38 +++++++--
 drivers/net/phy/linkmode.c                     | 104 +++++++++++++++++++------
 drivers/net/phy/phy_device.c                   |  37 ++++++++-
 drivers/net/phy/phylink.c                      |   6 +-
 include/linux/linkmode.h                       |   3 +-
 include/linux/phy.h                            |   1 +
 8 files changed, 212 insertions(+), 35 deletions(-)

-- 
2.7.4

[PATCH net-next 1/4] net: ethernet: validate pause autoneg setting

[Doug Berger]

A comment in uapi/linux/ethtool.h states "Drivers should reject a
non-zero setting of @autoneg when autoneogotiation is disabled (or
not supported) for the link".

That check should be added to phy_validate_pause() to consolidate
the code where possible.

Fixes: 22b7d29926b5 ("net: ethernet: Add helper to determine if pause configuration is supported")
Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/net/phy/phy_device.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index c3a107cf578e..5a9618ba232e 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2624,6 +2624,9 @@ EXPORT_SYMBOL(phy_set_asym_pause);
 bool phy_validate_pause(struct phy_device *phydev,
 			struct ethtool_pauseparam *pp)
 {
+	if (pp->autoneg && !phydev->autoneg)
+		return false;
+
 	if (!linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
 			       phydev->supported) && pp->rx_pause)
 		return false;
-- 
2.7.4

Re: [PATCH net-next 1/4] net: ethernet: validate pause autoneg setting

[Andrew Lunn]

On Mon, May 11, 2020 at 05:24:07PM -0700, Doug Berger wrote:
> A comment in uapi/linux/ethtool.h states "Drivers should reject a
> non-zero setting of @autoneg when autoneogotiation is disabled (or
> not supported) for the link".
> 
> That check should be added to phy_validate_pause() to consolidate
> the code where possible.
> 
> Fixes: 22b7d29926b5 ("net: ethernet: Add helper to determine if pause configuration is supported")

Hi Doug

If this is a real fix, please submit this to net, not net-next.

   Andrew

Re: [PATCH net-next 1/4] net: ethernet: validate pause autoneg setting

[Doug Berger]

On 5/11/2020 5:47 PM, Andrew Lunn wrote:
> On Mon, May 11, 2020 at 05:24:07PM -0700, Doug Berger wrote:
>> A comment in uapi/linux/ethtool.h states "Drivers should reject a
>> non-zero setting of @autoneg when autoneogotiation is disabled (or
>> not supported) for the link".
>>
>> That check should be added to phy_validate_pause() to consolidate
>> the code where possible.
>>
>> Fixes: 22b7d29926b5 ("net: ethernet: Add helper to determine if pause configuration is supported")
> 
> Hi Doug
> 
> If this is a real fix, please submit this to net, not net-next.
> 
>    Andrew
> 
This was intended as a fix, but I thought it would be better to keep it
as part of this set for context and since net-next is currently open.

The context is trying to improve the phylib support for offloading
ethtool pause configuration and this is something that could be checked
in a single location rather than by individual drivers.

I included it here to get feedback about its appropriateness as a common
behavior. I should have been more explicit about that.

Personally, I'm actually not that fond of this change since it can
easily be a source of confusion with the ethtool interface because the
link autonegotiation and the pause autonegotiation are controlled by
different commands.

Since the ethtool -A command performs a read/modify/write of pause
parameters, you can get strange results like these:
# ethtool -s eth0 speed 100 duplex full autoneg off
# ethtool -A eth0 tx off
Cannot set device pause parameters: Invalid argument
#
Because, the get read pause autoneg as enabled and only the tx_pause
member of the structure was updated.

The network driver could attempt to change one in response to the other,
but it might be difficult to reach consensus and it adds more
"worthless" code to the network driver in opposition to the intent.

I would like to get more feedback about the approach of the patch set as
a whole before I resubmit, and would be happy to drop this commit at
that time.

But there is still a question of how the comment "Drivers should reject
a non-zero setting of @autoneg when autoneogotiation is disabled (or not
supported) for the link" in ethtool.h should be interpreted.

Should that comment be removed?

Thanks for taking the time to look at this,
    Doug

Re: [PATCH net-next 1/4] net: ethernet: validate pause autoneg setting

[Andrew Lunn]

On Tue, May 12, 2020 at 11:31:39AM -0700, Doug Berger wrote:
> On 5/11/2020 5:47 PM, Andrew Lunn wrote:
> > On Mon, May 11, 2020 at 05:24:07PM -0700, Doug Berger wrote:
> >> A comment in uapi/linux/ethtool.h states "Drivers should reject a
> >> non-zero setting of @autoneg when autoneogotiation is disabled (or
> >> not supported) for the link".
> >>
> >> That check should be added to phy_validate_pause() to consolidate
> >> the code where possible.
> >>
> >> Fixes: 22b7d29926b5 ("net: ethernet: Add helper to determine if pause configuration is supported")
> > 
> > Hi Doug
> > 
> > If this is a real fix, please submit this to net, not net-next.
> > 
> >    Andrew
> > 
> This was intended as a fix, but I thought it would be better to keep it
> as part of this set for context and since net-next is currently open.

My real question is, do you think this should be back ported in
stable?  If so, it should be against net. If this is only intended for
new kernels, don't add a Fixes: tag.

> Personally, I'm actually not that fond of this change since it can
> easily be a source of confusion with the ethtool interface because the
> link autonegotiation and the pause autonegotiation are controlled by
> different commands.
> 
> Since the ethtool -A command performs a read/modify/write of pause
> parameters, you can get strange results like these:
> # ethtool -s eth0 speed 100 duplex full autoneg off
> # ethtool -A eth0 tx off
> Cannot set device pause parameters: Invalid argument
> #
> Because, the get read pause autoneg as enabled and only the tx_pause
> member of the structure was updated.

We can at least improve the error message when using netlink
ethtool. Using extack, we can pass back a string, saying why this
configuration is invalid, that link autoneg is off.

	Andrew

Re: [PATCH net-next 1/4] net: ethernet: validate pause autoneg setting

[Russell King - ARM Linux admin]

On Tue, May 12, 2020 at 11:31:39AM -0700, Doug Berger wrote:
> This was intended as a fix, but I thought it would be better to keep it
> as part of this set for context and since net-next is currently open.
> 
> The context is trying to improve the phylib support for offloading
> ethtool pause configuration and this is something that could be checked
> in a single location rather than by individual drivers.
> 
> I included it here to get feedback about its appropriateness as a common
> behavior. I should have been more explicit about that.
> 
> Personally, I'm actually not that fond of this change since it can
> easily be a source of confusion with the ethtool interface because the
> link autonegotiation and the pause autonegotiation are controlled by
> different commands.
> 
> Since the ethtool -A command performs a read/modify/write of pause
> parameters, you can get strange results like these:
> # ethtool -s eth0 speed 100 duplex full autoneg off
> # ethtool -A eth0 tx off
> Cannot set device pause parameters: Invalid argument
> #
> Because, the get read pause autoneg as enabled and only the tx_pause
> member of the structure was updated.

This looks like the same argument I've been having with Heiner over
the EEE interface, except there's a difference here.

# ethtool -A eth0 autoneg on
# ethtool -s eth0 autoneg off speed 100 duplex full

After those two commands, what is the state of pause mode?  The answer
is, it's disabled.

# ethtool -A eth0 autoneg off rx on tx on

is perfectly acceptable, as we are forcing pause modes at the local
end of the link.

# ethtool -A eth0 autoneg on

Now, the question is whether that should be allowed or not - but this
is merely restoring the "pause" settings that were in effect prior
to the previous command.  It does not enable pause negotiation,
because autoneg as a whole is disabled, but it _allows_ pause
negotiation to occur when autoneg is enabled at some point in the
future.

Also, allowing "ethtool -A eth0 autoneg on" when "ethtool -s eth0
autoneg off" means you can configure the negotiation parameters
_before_ triggering a negotiation cycle on the link.  In other words,
it would avoid:

# ethtool -s eth0 autoneg on
# # Link renegotiates
# ethtool -A eth0 autoneg on
# # Link renegotiates a second time

and it also means that if stuff has already been scripted to avoid
this, nothing breaks.

If we start rejecting ethtool -A because autoneg is disabled, then
things get difficult to configure - we would need ethtool documentation
to state that autoneg must be enabled before configuration of pause
and EEE can be done.  IMHO, that hurts usability, and adds confusion.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH net-next 1/4] net: ethernet: validate pause autoneg setting

[Doug Berger]

On 5/12/2020 11:55 AM, Russell King - ARM Linux admin wrote:
> On Tue, May 12, 2020 at 11:31:39AM -0700, Doug Berger wrote:
>> This was intended as a fix, but I thought it would be better to keep it
>> as part of this set for context and since net-next is currently open.
>>
>> The context is trying to improve the phylib support for offloading
>> ethtool pause configuration and this is something that could be checked
>> in a single location rather than by individual drivers.
>>
>> I included it here to get feedback about its appropriateness as a common
>> behavior. I should have been more explicit about that.
>>
>> Personally, I'm actually not that fond of this change since it can
>> easily be a source of confusion with the ethtool interface because the
>> link autonegotiation and the pause autonegotiation are controlled by
>> different commands.
>>
>> Since the ethtool -A command performs a read/modify/write of pause
>> parameters, you can get strange results like these:
>> # ethtool -s eth0 speed 100 duplex full autoneg off
>> # ethtool -A eth0 tx off
>> Cannot set device pause parameters: Invalid argument
>> #
>> Because, the get read pause autoneg as enabled and only the tx_pause
>> member of the structure was updated.
> 
> This looks like the same argument I've been having with Heiner over
> the EEE interface, except there's a difference here.
> 
> # ethtool -A eth0 autoneg on
> # ethtool -s eth0 autoneg off speed 100 duplex full
> 
> After those two commands, what is the state of pause mode?  The answer
> is, it's disabled.
> 
> # ethtool -A eth0 autoneg off rx on tx on
> 
> is perfectly acceptable, as we are forcing pause modes at the local
> end of the link.
> 
> # ethtool -A eth0 autoneg on
> 
> Now, the question is whether that should be allowed or not - but this
> is merely restoring the "pause" settings that were in effect prior
> to the previous command.  It does not enable pause negotiation,
> because autoneg as a whole is disabled, but it _allows_ pause
> negotiation to occur when autoneg is enabled at some point in the
> future.
> 
> Also, allowing "ethtool -A eth0 autoneg on" when "ethtool -s eth0
> autoneg off" means you can configure the negotiation parameters
> _before_ triggering a negotiation cycle on the link.  In other words,
> it would avoid:
> 
> # ethtool -s eth0 autoneg on
> # # Link renegotiates
> # ethtool -A eth0 autoneg on
> # # Link renegotiates a second time
> 
> and it also means that if stuff has already been scripted to avoid
> this, nothing breaks.
> 
> If we start rejecting ethtool -A because autoneg is disabled, then
> things get difficult to configure - we would need ethtool documentation
> to state that autoneg must be enabled before configuration of pause
> and EEE can be done.  IMHO, that hurts usability, and adds confusion.
> 
Thanks for your input and I agree with what you have said here. I will
remove this commit from the set when I resubmit and I assume that, like
Michal, you would like to see the comment in ethtool.h revised.

I think the crux of the matter is that the meaning of the autoneg pause
parameter is not well specified, and that is fundamentally what I am
trying to clarify in a common implementation that might help unify a
consistent behavior across network drivers.

My interpretation is that the link autonegotiation and the pause
autonegotiation can be meaningfully set independently from each other
and that the interplay between the two has easily overlooked subtleties.

My opinion (which is at least in part drawn from my interpretation of
your opinion) is as follows with regard to pause behaviors:

The link autonegotiation parameter concerns itself with whether the
Pause capabilities are advertised as part of autonegotiation of link
parameters.

The pause autonegotiation parameter concerns itself with whether the
local node is willing to accept the advertised capabilities of its peer
as input into its pause configuration.

The Tx_Pause and Rx_Pause parameters indicate in which directions pause
frames should be supported.

If the pause autonegotiation is off, the MAC is allowed to act
exclusively according to the Tx_Pause and Rx_Pause parameters. If
Tx_Pause is on the MAC should send pause control frames whenever it
needs to assert back pressure to ease the load on its receiver. If
Tx_Pause is off the MAC should not transmit any pause control frames. If
Rx_Pause is on the MAC should delay its transmissions in response to any
pause control frames it receives. If Rx_Pause is off received pause
control frames should be ignored. If link autonegotiation is on the
Tx_Pause and Rx_Pause values should be advertised in the PHY Pause and
AsymPause bits for informational purposes according to the following
mapping:
    tx rx  Pause AsymPause
    0  0   0     0
    0  1   1     1
    1  0   0     1
    1  1   1     0

If the pause autonegotiation is on, and the link autonegotiation is also
on then the Tx_Pause and Rx_Pause values should be advertised in the PHY
Pause and AsymPause bits according to the IEEE 802.3 spec according to
the following mapping:
    tx rx  Pause AsymPause
    0  0   0     0
    0  1   1     1
    1  0   0     1
    1  1   1     1
If link autonegotiation succeeds the peer's advertised Pause and
AsymPause bits should be used in combination with the local Pause and
Pause Asym bits to determine in which directions pause frames are
supported. However, regardless of the negotiated result, if the Tx_Pause
is off no pause frames should be sent and if the Rx_Pause is off
received pause frames should be ignored. If Tx_Pause is on and the
negotiated result allows pause frames to be sent then pause frames may
be sent by the local node to apply back pressure to reduce the load on
its receive path. If Rx_Pause is on and the negotiated result allows
pause frames to be received then the local node should delay its
transmission in response to received pause frames. In this way the local
settings can only override the negotiated settings to disable the use of
pause frames.

If the pause autonegotiation is on, and the link autonegotiation is off
then the values of the peer's Pause and AsymPause bits are forced to 0
(because they can't be exchanged without link autonegotiation) which
always produces the negotiated result of pause frame use being disabled
in both directions. Since the local Tx_Pause and Rx_Pause parameters can
only override the negotiation when they are off, pause frames should not
be sent or received.

This is the behavior I have attempted to implement by this patch set for
the bcmgenet driver, but I see now that I made an error in this last
case since I made the negotiation also dependent on the link
autonegotiation being enabled. I will correct that in a re-submission.

I would appreciate if you can confirm that you agree that this is a good
general behavior for all network devices before I resubmit, or please
help me understand what could be done better.

Many thanks,
    Doug

Re: [PATCH net-next 1/4] net: ethernet: validate pause autoneg setting

[Russell King - ARM Linux admin]

On Tue, May 12, 2020 at 08:48:22PM -0700, Doug Berger wrote:
> On 5/12/2020 11:55 AM, Russell King - ARM Linux admin wrote:
> > On Tue, May 12, 2020 at 11:31:39AM -0700, Doug Berger wrote:
> >> This was intended as a fix, but I thought it would be better to keep it
> >> as part of this set for context and since net-next is currently open.
> >>
> >> The context is trying to improve the phylib support for offloading
> >> ethtool pause configuration and this is something that could be checked
> >> in a single location rather than by individual drivers.
> >>
> >> I included it here to get feedback about its appropriateness as a common
> >> behavior. I should have been more explicit about that.
> >>
> >> Personally, I'm actually not that fond of this change since it can
> >> easily be a source of confusion with the ethtool interface because the
> >> link autonegotiation and the pause autonegotiation are controlled by
> >> different commands.
> >>
> >> Since the ethtool -A command performs a read/modify/write of pause
> >> parameters, you can get strange results like these:
> >> # ethtool -s eth0 speed 100 duplex full autoneg off
> >> # ethtool -A eth0 tx off
> >> Cannot set device pause parameters: Invalid argument
> >> #
> >> Because, the get read pause autoneg as enabled and only the tx_pause
> >> member of the structure was updated.
> > 
> > This looks like the same argument I've been having with Heiner over
> > the EEE interface, except there's a difference here.
> > 
> > # ethtool -A eth0 autoneg on
> > # ethtool -s eth0 autoneg off speed 100 duplex full
> > 
> > After those two commands, what is the state of pause mode?  The answer
> > is, it's disabled.
> > 
> > # ethtool -A eth0 autoneg off rx on tx on
> > 
> > is perfectly acceptable, as we are forcing pause modes at the local
> > end of the link.
> > 
> > # ethtool -A eth0 autoneg on
> > 
> > Now, the question is whether that should be allowed or not - but this
> > is merely restoring the "pause" settings that were in effect prior
> > to the previous command.  It does not enable pause negotiation,
> > because autoneg as a whole is disabled, but it _allows_ pause
> > negotiation to occur when autoneg is enabled at some point in the
> > future.
> > 
> > Also, allowing "ethtool -A eth0 autoneg on" when "ethtool -s eth0
> > autoneg off" means you can configure the negotiation parameters
> > _before_ triggering a negotiation cycle on the link.  In other words,
> > it would avoid:
> > 
> > # ethtool -s eth0 autoneg on
> > # # Link renegotiates
> > # ethtool -A eth0 autoneg on
> > # # Link renegotiates a second time
> > 
> > and it also means that if stuff has already been scripted to avoid
> > this, nothing breaks.
> > 
> > If we start rejecting ethtool -A because autoneg is disabled, then
> > things get difficult to configure - we would need ethtool documentation
> > to state that autoneg must be enabled before configuration of pause
> > and EEE can be done.  IMHO, that hurts usability, and adds confusion.
> > 
> Thanks for your input and I agree with what you have said here. I will
> remove this commit from the set when I resubmit and I assume that, like
> Michal, you would like to see the comment in ethtool.h revised.
> 
> I think the crux of the matter is that the meaning of the autoneg pause
> parameter is not well specified, and that is fundamentally what I am
> trying to clarify in a common implementation that might help unify a
> consistent behavior across network drivers.
> 
> My interpretation is that the link autonegotiation and the pause
> autonegotiation can be meaningfully set independently from each other
> and that the interplay between the two has easily overlooked subtleties.
> 
> My opinion (which is at least in part drawn from my interpretation of
> your opinion) is as follows with regard to pause behaviors:
> 
> The link autonegotiation parameter concerns itself with whether the
> Pause capabilities are advertised as part of autonegotiation of link
> parameters.
> 
> The pause autonegotiation parameter concerns itself with whether the
> local node is willing to accept the advertised capabilities of its peer
> as input into its pause configuration.
> 
> The Tx_Pause and Rx_Pause parameters indicate in which directions pause
> frames should be supported.

This is where the ethtool interface breaks down - they are unable
to sanely define which should be supported, as what you end up with
could be wildly different from what you thought.  See the
documentation against linkmode_set_pause() where I detail the issues
in this API.

For example, if you specify Tx_Pause = 0, Rx_Pause = 1, you can end
up with the pause negotiating transmit and receive pause.

If you specify Tx_Pause = 1, Rx_Pause = 1, and the far end supports
only AsymPause, then you end up with pause disabled, despite the
link actually being able to support receive pause at the local end.
Whereas if you specified Tx_Pause = 0, Rx_Pause=1 in this scenario,
you would get receive pause.  That's very counter intuitive.

> If the pause autonegotiation is off, the MAC is allowed to act
> exclusively according to the Tx_Pause and Rx_Pause parameters. If
> Tx_Pause is on the MAC should send pause control frames whenever it
> needs to assert back pressure to ease the load on its receiver. If
> Tx_Pause is off the MAC should not transmit any pause control frames. If
> Rx_Pause is on the MAC should delay its transmissions in response to any
> pause control frames it receives. If Rx_Pause is off received pause
> control frames should be ignored. If link autonegotiation is on the
> Tx_Pause and Rx_Pause values should be advertised in the PHY Pause and
> AsymPause bits for informational purposes according to the following
> mapping:
>     tx rx  Pause AsymPause
>     0  0   0     0
>     0  1   1     1
>     1  0   0     1
>     1  1   1     0

That is what is presently implemented by the helpers, and leads to
the above counter intuitive behaviour.

> If the pause autonegotiation is on, and the link autonegotiation is also
> on then the Tx_Pause and Rx_Pause values should be advertised in the PHY
> Pause and AsymPause bits according to the IEEE 802.3 spec according to
> the following mapping:
>     tx rx  Pause AsymPause
>     0  0   0     0
>     0  1   1     1
>     1  0   0     1
>     1  1   1     1

That would be an API change - and note that in the case of 'tx=0
rx=1' and the result of negotiation being used, you can still end
up with transmit and receive pause being enabled.

Basically, trying to define the pause advertisment in terms of
desired TX and RX pause enablement is *very* problematical - they
really do not mean anything as we can see if we work through the
various settings and results.

You're much better using the raw advertisment mask to set the
pause and asym pause bits manually.

> If link autonegotiation succeeds the peer's advertised Pause and
> AsymPause bits should be used in combination with the local Pause and
> Pause Asym bits to determine in which directions pause frames are
> supported. However, regardless of the negotiated result, if the Tx_Pause
> is off no pause frames should be sent and if the Rx_Pause is off
> received pause frames should be ignored. If Tx_Pause is on and the
> negotiated result allows pause frames to be sent then pause frames may
> be sent by the local node to apply back pressure to reduce the load on
> its receive path. If Rx_Pause is on and the negotiated result allows
> pause frames to be received then the local node should delay its
> transmission in response to received pause frames. In this way the local
> settings can only override the negotiated settings to disable the use of
> pause frames.
> 
> If the pause autonegotiation is on, and the link autonegotiation is off
> then the values of the peer's Pause and AsymPause bits are forced to 0
> (because they can't be exchanged without link autonegotiation) which
> always produces the negotiated result of pause frame use being disabled
> in both directions. Since the local Tx_Pause and Rx_Pause parameters can
> only override the negotiation when they are off, pause frames should not
> be sent or received.
> 
> This is the behavior I have attempted to implement by this patch set for
> the bcmgenet driver, but I see now that I made an error in this last
> case since I made the negotiation also dependent on the link
> autonegotiation being enabled. I will correct that in a re-submission.
> 
> I would appreciate if you can confirm that you agree that this is a good
> general behavior for all network devices before I resubmit, or please
> help me understand what could be done better.

It's gratifying that someone else has run into the same issue I did a
while back, has put thought into it, and come up with a similar idea
that I did.  You'll find your idea already spelt out in the comments
in phylink_ethtool_set_pauseparam().

However, as I say, it's an API change.

I've long considered the ethtool APIs to be very deficient in its
pause handling in many ways.  Another example is:

        Supported pause frame use: Symmetric Receive-only

which leads to the obvious observation: the link can negotiate that
this end should transmit only, but the terminology used here does
not seem to permit it (there's no "Transmit-only" indicated.) In
reality, one shold read "Asymmetric" for "Receive-only" in this
output, because that is exactly what the bit that controls that
indication is.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH net-next 1/4] net: ethernet: validate pause autoneg setting

[Russell King - ARM Linux admin]

On Wed, May 13, 2020 at 06:34:05AM +0100, Russell King - ARM Linux admin wrote:
> On Tue, May 12, 2020 at 08:48:22PM -0700, Doug Berger wrote:
> > On 5/12/2020 11:55 AM, Russell King - ARM Linux admin wrote:
> > > On Tue, May 12, 2020 at 11:31:39AM -0700, Doug Berger wrote:
> > >> This was intended as a fix, but I thought it would be better to keep it
> > >> as part of this set for context and since net-next is currently open.
> > >>
> > >> The context is trying to improve the phylib support for offloading
> > >> ethtool pause configuration and this is something that could be checked
> > >> in a single location rather than by individual drivers.
> > >>
> > >> I included it here to get feedback about its appropriateness as a common
> > >> behavior. I should have been more explicit about that.
> > >>
> > >> Personally, I'm actually not that fond of this change since it can
> > >> easily be a source of confusion with the ethtool interface because the
> > >> link autonegotiation and the pause autonegotiation are controlled by
> > >> different commands.
> > >>
> > >> Since the ethtool -A command performs a read/modify/write of pause
> > >> parameters, you can get strange results like these:
> > >> # ethtool -s eth0 speed 100 duplex full autoneg off
> > >> # ethtool -A eth0 tx off
> > >> Cannot set device pause parameters: Invalid argument
> > >> #
> > >> Because, the get read pause autoneg as enabled and only the tx_pause
> > >> member of the structure was updated.
> > > 
> > > This looks like the same argument I've been having with Heiner over
> > > the EEE interface, except there's a difference here.
> > > 
> > > # ethtool -A eth0 autoneg on
> > > # ethtool -s eth0 autoneg off speed 100 duplex full
> > > 
> > > After those two commands, what is the state of pause mode?  The answer
> > > is, it's disabled.
> > > 
> > > # ethtool -A eth0 autoneg off rx on tx on
> > > 
> > > is perfectly acceptable, as we are forcing pause modes at the local
> > > end of the link.
> > > 
> > > # ethtool -A eth0 autoneg on
> > > 
> > > Now, the question is whether that should be allowed or not - but this
> > > is merely restoring the "pause" settings that were in effect prior
> > > to the previous command.  It does not enable pause negotiation,
> > > because autoneg as a whole is disabled, but it _allows_ pause
> > > negotiation to occur when autoneg is enabled at some point in the
> > > future.
> > > 
> > > Also, allowing "ethtool -A eth0 autoneg on" when "ethtool -s eth0
> > > autoneg off" means you can configure the negotiation parameters
> > > _before_ triggering a negotiation cycle on the link.  In other words,
> > > it would avoid:
> > > 
> > > # ethtool -s eth0 autoneg on
> > > # # Link renegotiates
> > > # ethtool -A eth0 autoneg on
> > > # # Link renegotiates a second time
> > > 
> > > and it also means that if stuff has already been scripted to avoid
> > > this, nothing breaks.
> > > 
> > > If we start rejecting ethtool -A because autoneg is disabled, then
> > > things get difficult to configure - we would need ethtool documentation
> > > to state that autoneg must be enabled before configuration of pause
> > > and EEE can be done.  IMHO, that hurts usability, and adds confusion.
> > > 
> > Thanks for your input and I agree with what you have said here. I will
> > remove this commit from the set when I resubmit and I assume that, like
> > Michal, you would like to see the comment in ethtool.h revised.
> > 
> > I think the crux of the matter is that the meaning of the autoneg pause
> > parameter is not well specified, and that is fundamentally what I am
> > trying to clarify in a common implementation that might help unify a
> > consistent behavior across network drivers.
> > 
> > My interpretation is that the link autonegotiation and the pause
> > autonegotiation can be meaningfully set independently from each other
> > and that the interplay between the two has easily overlooked subtleties.
> > 
> > My opinion (which is at least in part drawn from my interpretation of
> > your opinion) is as follows with regard to pause behaviors:
> > 
> > The link autonegotiation parameter concerns itself with whether the
> > Pause capabilities are advertised as part of autonegotiation of link
> > parameters.
> > 
> > The pause autonegotiation parameter concerns itself with whether the
> > local node is willing to accept the advertised capabilities of its peer
> > as input into its pause configuration.
> > 
> > The Tx_Pause and Rx_Pause parameters indicate in which directions pause
> > frames should be supported.
> 
> This is where the ethtool interface breaks down - they are unable
> to sanely define which should be supported, as what you end up with
> could be wildly different from what you thought.  See the
> documentation against linkmode_set_pause() where I detail the issues
> in this API.
> 
> For example, if you specify Tx_Pause = 0, Rx_Pause = 1, you can end
> up with the pause negotiating transmit and receive pause.
> 
> If you specify Tx_Pause = 1, Rx_Pause = 1, and the far end supports
> only AsymPause, then you end up with pause disabled, despite the
> link actually being able to support receive pause at the local end.
> Whereas if you specified Tx_Pause = 0, Rx_Pause=1 in this scenario,
> you would get receive pause.  That's very counter intuitive.
> 
> > If the pause autonegotiation is off, the MAC is allowed to act
> > exclusively according to the Tx_Pause and Rx_Pause parameters. If
> > Tx_Pause is on the MAC should send pause control frames whenever it
> > needs to assert back pressure to ease the load on its receiver. If
> > Tx_Pause is off the MAC should not transmit any pause control frames. If
> > Rx_Pause is on the MAC should delay its transmissions in response to any
> > pause control frames it receives. If Rx_Pause is off received pause
> > control frames should be ignored. If link autonegotiation is on the
> > Tx_Pause and Rx_Pause values should be advertised in the PHY Pause and
> > AsymPause bits for informational purposes according to the following
> > mapping:
> >     tx rx  Pause AsymPause
> >     0  0   0     0
> >     0  1   1     1
> >     1  0   0     1
> >     1  1   1     0
> 
> That is what is presently implemented by the helpers, and leads to
> the above counter intuitive behaviour.
> 
> > If the pause autonegotiation is on, and the link autonegotiation is also
> > on then the Tx_Pause and Rx_Pause values should be advertised in the PHY
> > Pause and AsymPause bits according to the IEEE 802.3 spec according to
> > the following mapping:
> >     tx rx  Pause AsymPause
> >     0  0   0     0
> >     0  1   1     1
> >     1  0   0     1
> >     1  1   1     1
> 
> That would be an API change - and note that in the case of 'tx=0
> rx=1' and the result of negotiation being used, you can still end
> up with transmit and receive pause being enabled.
> 
> Basically, trying to define the pause advertisment in terms of
> desired TX and RX pause enablement is *very* problematical - they
> really do not mean anything as we can see if we work through the
> various settings and results.
> 
> You're much better using the raw advertisment mask to set the
> pause and asym pause bits manually.
> 
> > If link autonegotiation succeeds the peer's advertised Pause and
> > AsymPause bits should be used in combination with the local Pause and
> > Pause Asym bits to determine in which directions pause frames are
> > supported. However, regardless of the negotiated result, if the Tx_Pause
> > is off no pause frames should be sent and if the Rx_Pause is off
> > received pause frames should be ignored. If Tx_Pause is on and the
> > negotiated result allows pause frames to be sent then pause frames may
> > be sent by the local node to apply back pressure to reduce the load on
> > its receive path. If Rx_Pause is on and the negotiated result allows
> > pause frames to be received then the local node should delay its
> > transmission in response to received pause frames. In this way the local
> > settings can only override the negotiated settings to disable the use of
> > pause frames.
> > 
> > If the pause autonegotiation is on, and the link autonegotiation is off
> > then the values of the peer's Pause and AsymPause bits are forced to 0
> > (because they can't be exchanged without link autonegotiation) which
> > always produces the negotiated result of pause frame use being disabled
> > in both directions. Since the local Tx_Pause and Rx_Pause parameters can
> > only override the negotiation when they are off, pause frames should not
> > be sent or received.
> > 
> > This is the behavior I have attempted to implement by this patch set for
> > the bcmgenet driver, but I see now that I made an error in this last
> > case since I made the negotiation also dependent on the link
> > autonegotiation being enabled. I will correct that in a re-submission.
> > 
> > I would appreciate if you can confirm that you agree that this is a good
> > general behavior for all network devices before I resubmit, or please
> > help me understand what could be done better.
> 
> It's gratifying that someone else has run into the same issue I did a
> while back, has put thought into it, and come up with a similar idea
> that I did.  You'll find your idea already spelt out in the comments
> in phylink_ethtool_set_pauseparam().
> 
> However, as I say, it's an API change.
> 
> I've long considered the ethtool APIs to be very deficient in its
> pause handling in many ways.  Another example is:
> 
>         Supported pause frame use: Symmetric Receive-only
> 
> which leads to the obvious observation: the link can negotiate that
> this end should transmit only, but the terminology used here does
> not seem to permit it (there's no "Transmit-only" indicated.) In
> reality, one shold read "Asymmetric" for "Receive-only" in this
> output, because that is exactly what the bit that controls that
> indication is.

One thing I didn't mention is - although I identified the same problem
w.r.t interpretation of pause tx/rx to advertisement, I regard that
it's more important for a user interface to behave consistently.
phylib implements generic code that converts the set_pauseparam method
parameters to the PHY advertisement using the "very-odd" behaviour.

Since phylink uses phylib, phylink has to follow phylib, otherwise we
end up with the API having different behaviours depending on which SFP
modules are plugged in, or whether the network driver is connected to
a PHY or SFP.

So, I think consistency of implementation is more important than fixing
this; the current behaviour has been established for many years now.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH net-next 1/4] net: ethernet: validate pause autoneg setting

[Andrew Lunn]

> So, I think consistency of implementation is more important than fixing
> this; the current behaviour has been established for many years now.

Hi Russell, Doug

With netlink ethtool we have the possibility of adding a new API to
control this. And we can leave the IOCTL API alone, and the current
ethtool commands. We can add a new command to ethtool which uses the new API.

Question is, do we want to do this? Would we be introducing yet more
confusion, rather than making the situation better?

	Andrew

Re: [PATCH net-next 1/4] net: ethernet: validate pause autoneg setting

[Michal Kubecek]

On Wed, May 13, 2020 at 03:49:25PM +0200, Andrew Lunn wrote:
> > So, I think consistency of implementation is more important than fixing
> > this; the current behaviour has been established for many years now.
> 
> With netlink ethtool we have the possibility of adding a new API to
> control this. And we can leave the IOCTL API alone, and the current
> ethtool commands. We can add a new command to ethtool which uses the new API.
> 
> Question is, do we want to do this? Would we be introducing yet more
> confusion, rather than making the situation better?

For the record, netlink interface for pause parameters which is based on
existing ioctl and ethtool_ops is in mainline but not in v5.6. If there
is a consensus that it should be rethought, it might still be possible
to drop these two request types and come with a better API later (i.e.
in 5.8 or 5.9 cycle).

Michal

Re: [PATCH net-next 1/4] net: ethernet: validate pause autoneg setting

[Russell King - ARM Linux admin]

On Wed, May 13, 2020 at 03:49:25PM +0200, Andrew Lunn wrote:
> Hi Russell, Doug
> 
> With netlink ethtool we have the possibility of adding a new API to
> control this. And we can leave the IOCTL API alone, and the current
> ethtool commands. We can add a new command to ethtool which uses the new API.
> 
> Question is, do we want to do this? Would we be introducing yet more
> confusion, rather than making the situation better?

The conclusion I came to was that I would document the deficiencies
and do no more; I think people are used to its current quirky
behaviour.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH net-next 1/4] net: ethernet: validate pause autoneg setting

[Doug Berger]

On 5/13/2020 6:49 AM, Andrew Lunn wrote:
>> So, I think consistency of implementation is more important than fixing
>> this; the current behaviour has been established for many years now.
> 
> Hi Russell, Doug
> 
> With netlink ethtool we have the possibility of adding a new API to
> control this. And we can leave the IOCTL API alone, and the current
> ethtool commands. We can add a new command to ethtool which uses the new API.
> 
> Question is, do we want to do this? Would we be introducing yet more
> confusion, rather than making the situation better?
> 
> 	Andrew
> 
I think it is likely to introduce more confusion.
-Doug

Re: [PATCH net-next 1/4] net: ethernet: validate pause autoneg setting

[Doug Berger]

On 5/13/2020 2:20 AM, Russell King - ARM Linux admin wrote:
> On Wed, May 13, 2020 at 06:34:05AM +0100, Russell King - ARM Linux admin wrote:
>> On Tue, May 12, 2020 at 08:48:22PM -0700, Doug Berger wrote:
>>> On 5/12/2020 11:55 AM, Russell King - ARM Linux admin wrote:
>>>> On Tue, May 12, 2020 at 11:31:39AM -0700, Doug Berger wrote:
>>>>> This was intended as a fix, but I thought it would be better to keep it
>>>>> as part of this set for context and since net-next is currently open.
>>>>>
>>>>> The context is trying to improve the phylib support for offloading
>>>>> ethtool pause configuration and this is something that could be checked
>>>>> in a single location rather than by individual drivers.
>>>>>
>>>>> I included it here to get feedback about its appropriateness as a common
>>>>> behavior. I should have been more explicit about that.
>>>>>
>>>>> Personally, I'm actually not that fond of this change since it can
>>>>> easily be a source of confusion with the ethtool interface because the
>>>>> link autonegotiation and the pause autonegotiation are controlled by
>>>>> different commands.
>>>>>
>>>>> Since the ethtool -A command performs a read/modify/write of pause
>>>>> parameters, you can get strange results like these:
>>>>> # ethtool -s eth0 speed 100 duplex full autoneg off
>>>>> # ethtool -A eth0 tx off
>>>>> Cannot set device pause parameters: Invalid argument
>>>>> #
>>>>> Because, the get read pause autoneg as enabled and only the tx_pause
>>>>> member of the structure was updated.
>>>>
>>>> This looks like the same argument I've been having with Heiner over
>>>> the EEE interface, except there's a difference here.
>>>>
>>>> # ethtool -A eth0 autoneg on
>>>> # ethtool -s eth0 autoneg off speed 100 duplex full
>>>>
>>>> After those two commands, what is the state of pause mode?  The answer
>>>> is, it's disabled.
>>>>
>>>> # ethtool -A eth0 autoneg off rx on tx on
>>>>
>>>> is perfectly acceptable, as we are forcing pause modes at the local
>>>> end of the link.
>>>>
>>>> # ethtool -A eth0 autoneg on
>>>>
>>>> Now, the question is whether that should be allowed or not - but this
>>>> is merely restoring the "pause" settings that were in effect prior
>>>> to the previous command.  It does not enable pause negotiation,
>>>> because autoneg as a whole is disabled, but it _allows_ pause
>>>> negotiation to occur when autoneg is enabled at some point in the
>>>> future.
>>>>
>>>> Also, allowing "ethtool -A eth0 autoneg on" when "ethtool -s eth0
>>>> autoneg off" means you can configure the negotiation parameters
>>>> _before_ triggering a negotiation cycle on the link.  In other words,
>>>> it would avoid:
>>>>
>>>> # ethtool -s eth0 autoneg on
>>>> # # Link renegotiates
>>>> # ethtool -A eth0 autoneg on
>>>> # # Link renegotiates a second time
>>>>
>>>> and it also means that if stuff has already been scripted to avoid
>>>> this, nothing breaks.
>>>>
>>>> If we start rejecting ethtool -A because autoneg is disabled, then
>>>> things get difficult to configure - we would need ethtool documentation
>>>> to state that autoneg must be enabled before configuration of pause
>>>> and EEE can be done.  IMHO, that hurts usability, and adds confusion.
>>>>
>>> Thanks for your input and I agree with what you have said here. I will
>>> remove this commit from the set when I resubmit and I assume that, like
>>> Michal, you would like to see the comment in ethtool.h revised.
>>>
>>> I think the crux of the matter is that the meaning of the autoneg pause
>>> parameter is not well specified, and that is fundamentally what I am
>>> trying to clarify in a common implementation that might help unify a
>>> consistent behavior across network drivers.
>>>
>>> My interpretation is that the link autonegotiation and the pause
>>> autonegotiation can be meaningfully set independently from each other
>>> and that the interplay between the two has easily overlooked subtleties.
>>>
>>> My opinion (which is at least in part drawn from my interpretation of
>>> your opinion) is as follows with regard to pause behaviors:
>>>
>>> The link autonegotiation parameter concerns itself with whether the
>>> Pause capabilities are advertised as part of autonegotiation of link
>>> parameters.
>>>
>>> The pause autonegotiation parameter concerns itself with whether the
>>> local node is willing to accept the advertised capabilities of its peer
>>> as input into its pause configuration.
>>>
>>> The Tx_Pause and Rx_Pause parameters indicate in which directions pause
>>> frames should be supported.
>>
>> This is where the ethtool interface breaks down - they are unable
>> to sanely define which should be supported, as what you end up with
>> could be wildly different from what you thought.  See the
>> documentation against linkmode_set_pause() where I detail the issues
>> in this API.
>>
>> For example, if you specify Tx_Pause = 0, Rx_Pause = 1, you can end
>> up with the pause negotiating transmit and receive pause.
>>
>> If you specify Tx_Pause = 1, Rx_Pause = 1, and the far end supports
>> only AsymPause, then you end up with pause disabled, despite the
>> link actually being able to support receive pause at the local end.
>> Whereas if you specified Tx_Pause = 0, Rx_Pause=1 in this scenario,
>> you would get receive pause.  That's very counter intuitive.
>>
>>> If the pause autonegotiation is off, the MAC is allowed to act
>>> exclusively according to the Tx_Pause and Rx_Pause parameters. If
>>> Tx_Pause is on the MAC should send pause control frames whenever it
>>> needs to assert back pressure to ease the load on its receiver. If
>>> Tx_Pause is off the MAC should not transmit any pause control frames. If
>>> Rx_Pause is on the MAC should delay its transmissions in response to any
>>> pause control frames it receives. If Rx_Pause is off received pause
>>> control frames should be ignored. If link autonegotiation is on the
>>> Tx_Pause and Rx_Pause values should be advertised in the PHY Pause and
>>> AsymPause bits for informational purposes according to the following
>>> mapping:
>>>     tx rx  Pause AsymPause
>>>     0  0   0     0
>>>     0  1   1     1
>>>     1  0   0     1
>>>     1  1   1     0
>>
>> That is what is presently implemented by the helpers, and leads to
>> the above counter intuitive behaviour.
>>
>>> If the pause autonegotiation is on, and the link autonegotiation is also
>>> on then the Tx_Pause and Rx_Pause values should be advertised in the PHY
>>> Pause and AsymPause bits according to the IEEE 802.3 spec according to
>>> the following mapping:
>>>     tx rx  Pause AsymPause
>>>     0  0   0     0
>>>     0  1   1     1
>>>     1  0   0     1
>>>     1  1   1     1
>>
>> That would be an API change - and note that in the case of 'tx=0
>> rx=1' and the result of negotiation being used, you can still end
>> up with transmit and receive pause being enabled.
>>
>> Basically, trying to define the pause advertisment in terms of
>> desired TX and RX pause enablement is *very* problematical - they
>> really do not mean anything as we can see if we work through the
>> various settings and results.
>>
>> You're much better using the raw advertisment mask to set the
>> pause and asym pause bits manually.
>>
>>> If link autonegotiation succeeds the peer's advertised Pause and
>>> AsymPause bits should be used in combination with the local Pause and
>>> Pause Asym bits to determine in which directions pause frames are
>>> supported. However, regardless of the negotiated result, if the Tx_Pause
>>> is off no pause frames should be sent and if the Rx_Pause is off
>>> received pause frames should be ignored. If Tx_Pause is on and the
>>> negotiated result allows pause frames to be sent then pause frames may
>>> be sent by the local node to apply back pressure to reduce the load on
>>> its receive path. If Rx_Pause is on and the negotiated result allows
>>> pause frames to be received then the local node should delay its
>>> transmission in response to received pause frames. In this way the local
>>> settings can only override the negotiated settings to disable the use of
>>> pause frames.
>>>
>>> If the pause autonegotiation is on, and the link autonegotiation is off
>>> then the values of the peer's Pause and AsymPause bits are forced to 0
>>> (because they can't be exchanged without link autonegotiation) which
>>> always produces the negotiated result of pause frame use being disabled
>>> in both directions. Since the local Tx_Pause and Rx_Pause parameters can
>>> only override the negotiation when they are off, pause frames should not
>>> be sent or received.
>>>
>>> This is the behavior I have attempted to implement by this patch set for
>>> the bcmgenet driver, but I see now that I made an error in this last
>>> case since I made the negotiation also dependent on the link
>>> autonegotiation being enabled. I will correct that in a re-submission.
>>>
>>> I would appreciate if you can confirm that you agree that this is a good
>>> general behavior for all network devices before I resubmit, or please
>>> help me understand what could be done better.
>>
>> It's gratifying that someone else has run into the same issue I did a
>> while back, has put thought into it, and come up with a similar idea
>> that I did.  You'll find your idea already spelt out in the comments
>> in phylink_ethtool_set_pauseparam().
>>
>> However, as I say, it's an API change.
>>
>> I've long considered the ethtool APIs to be very deficient in its
>> pause handling in many ways.  Another example is:
>>
>>         Supported pause frame use: Symmetric Receive-only
>>
>> which leads to the obvious observation: the link can negotiate that
>> this end should transmit only, but the terminology used here does
>> not seem to permit it (there's no "Transmit-only" indicated.) In
>> reality, one shold read "Asymmetric" for "Receive-only" in this
>> output, because that is exactly what the bit that controls that
>> indication is.
> 
> One thing I didn't mention is - although I identified the same problem
> w.r.t interpretation of pause tx/rx to advertisement, I regard that
> it's more important for a user interface to behave consistently.
> phylib implements generic code that converts the set_pauseparam method
> parameters to the PHY advertisement using the "very-odd" behaviour.
> 
> Since phylink uses phylib, phylink has to follow phylib, otherwise we
> end up with the API having different behaviours depending on which SFP
> modules are plugged in, or whether the network driver is connected to
> a PHY or SFP.
> 
> So, I think consistency of implementation is more important than fixing
> this; the current behaviour has been established for many years now.
> 
I agree that consistency is important. I had just hoped that since there
really is surprisingly little support for this feature among network
drivers (I assume largely because of this lack of basic support in the
phylib) that there might still be a chance to get it changed.

Re: [PATCH net-next 1/4] net: ethernet: validate pause autoneg setting

[Doug Berger]

On 5/12/2020 10:34 PM, Russell King - ARM Linux admin wrote:
> On Tue, May 12, 2020 at 08:48:22PM -0700, Doug Berger wrote:
>> On 5/12/2020 11:55 AM, Russell King - ARM Linux admin wrote:
>>> On Tue, May 12, 2020 at 11:31:39AM -0700, Doug Berger wrote:
>>>> This was intended as a fix, but I thought it would be better to keep it
>>>> as part of this set for context and since net-next is currently open.
>>>>
>>>> The context is trying to improve the phylib support for offloading
>>>> ethtool pause configuration and this is something that could be checked
>>>> in a single location rather than by individual drivers.
>>>>
>>>> I included it here to get feedback about its appropriateness as a common
>>>> behavior. I should have been more explicit about that.
>>>>
>>>> Personally, I'm actually not that fond of this change since it can
>>>> easily be a source of confusion with the ethtool interface because the
>>>> link autonegotiation and the pause autonegotiation are controlled by
>>>> different commands.
>>>>
>>>> Since the ethtool -A command performs a read/modify/write of pause
>>>> parameters, you can get strange results like these:
>>>> # ethtool -s eth0 speed 100 duplex full autoneg off
>>>> # ethtool -A eth0 tx off
>>>> Cannot set device pause parameters: Invalid argument
>>>> #
>>>> Because, the get read pause autoneg as enabled and only the tx_pause
>>>> member of the structure was updated.
>>>
>>> This looks like the same argument I've been having with Heiner over
>>> the EEE interface, except there's a difference here.
>>>
>>> # ethtool -A eth0 autoneg on
>>> # ethtool -s eth0 autoneg off speed 100 duplex full
>>>
>>> After those two commands, what is the state of pause mode?  The answer
>>> is, it's disabled.
>>>
>>> # ethtool -A eth0 autoneg off rx on tx on
>>>
>>> is perfectly acceptable, as we are forcing pause modes at the local
>>> end of the link.
>>>
>>> # ethtool -A eth0 autoneg on
>>>
>>> Now, the question is whether that should be allowed or not - but this
>>> is merely restoring the "pause" settings that were in effect prior
>>> to the previous command.  It does not enable pause negotiation,
>>> because autoneg as a whole is disabled, but it _allows_ pause
>>> negotiation to occur when autoneg is enabled at some point in the
>>> future.
>>>
>>> Also, allowing "ethtool -A eth0 autoneg on" when "ethtool -s eth0
>>> autoneg off" means you can configure the negotiation parameters
>>> _before_ triggering a negotiation cycle on the link.  In other words,
>>> it would avoid:
>>>
>>> # ethtool -s eth0 autoneg on
>>> # # Link renegotiates
>>> # ethtool -A eth0 autoneg on
>>> # # Link renegotiates a second time
>>>
>>> and it also means that if stuff has already been scripted to avoid
>>> this, nothing breaks.
>>>
>>> If we start rejecting ethtool -A because autoneg is disabled, then
>>> things get difficult to configure - we would need ethtool documentation
>>> to state that autoneg must be enabled before configuration of pause
>>> and EEE can be done.  IMHO, that hurts usability, and adds confusion.
>>>
>> Thanks for your input and I agree with what you have said here. I will
>> remove this commit from the set when I resubmit and I assume that, like
>> Michal, you would like to see the comment in ethtool.h revised.
>>
>> I think the crux of the matter is that the meaning of the autoneg pause
>> parameter is not well specified, and that is fundamentally what I am
>> trying to clarify in a common implementation that might help unify a
>> consistent behavior across network drivers.
>>
>> My interpretation is that the link autonegotiation and the pause
>> autonegotiation can be meaningfully set independently from each other
>> and that the interplay between the two has easily overlooked subtleties.
>>
>> My opinion (which is at least in part drawn from my interpretation of
>> your opinion) is as follows with regard to pause behaviors:
>>
>> The link autonegotiation parameter concerns itself with whether the
>> Pause capabilities are advertised as part of autonegotiation of link
>> parameters.
>>
>> The pause autonegotiation parameter concerns itself with whether the
>> local node is willing to accept the advertised capabilities of its peer
>> as input into its pause configuration.
>>
>> The Tx_Pause and Rx_Pause parameters indicate in which directions pause
>> frames should be supported.
> 
> This is where the ethtool interface breaks down - they are unable
> to sanely define which should be supported, as what you end up with
> could be wildly different from what you thought.  See the
> documentation against linkmode_set_pause() where I detail the issues
> in this API.
> 
> For example, if you specify Tx_Pause = 0, Rx_Pause = 1, you can end
> up with the pause negotiating transmit and receive pause.
> 
> If you specify Tx_Pause = 1, Rx_Pause = 1, and the far end supports
> only AsymPause, then you end up with pause disabled, despite the
> link actually being able to support receive pause at the local end.
> Whereas if you specified Tx_Pause = 0, Rx_Pause=1 in this scenario,
> you would get receive pause.  That's very counter intuitive.
Yes, your documentation of these deficiencies in the current
implementation are very helpful.

>> If the pause autonegotiation is off, the MAC is allowed to act
>> exclusively according to the Tx_Pause and Rx_Pause parameters. If
>> Tx_Pause is on the MAC should send pause control frames whenever it
>> needs to assert back pressure to ease the load on its receiver. If
>> Tx_Pause is off the MAC should not transmit any pause control frames. If
>> Rx_Pause is on the MAC should delay its transmissions in response to any
>> pause control frames it receives. If Rx_Pause is off received pause
>> control frames should be ignored. If link autonegotiation is on the
>> Tx_Pause and Rx_Pause values should be advertised in the PHY Pause and
>> AsymPause bits for informational purposes according to the following
>> mapping:
>>     tx rx  Pause AsymPause
>>     0  0   0     0
>>     0  1   1     1
>>     1  0   0     1
>>     1  1   1     0
> 
> That is what is presently implemented by the helpers, and leads to
> the above counter intuitive behaviour.
Exactly, and that is by intent. I have retained this to allow backward
compatibility for existing drivers that wish to retain the current
behavior. I believe this mapping is not compliant with the IEEE 802.3
standard because of the deficiencies you have documented. However, I
have tried to make an argument in patch 2 of this set for why it might
make sense to advertise this way for a local node that is unwilling to
negotiate its pause capabilities and doesn't care about standard compliance.

>> If the pause autonegotiation is on, and the link autonegotiation is also
>> on then the Tx_Pause and Rx_Pause values should be advertised in the PHY
>> Pause and AsymPause bits according to the IEEE 802.3 spec according to
>> the following mapping:
>>     tx rx  Pause AsymPause
>>     0  0   0     0
>>     0  1   1     1
>>     1  0   0     1
>>     1  1   1     1
> 
> That would be an API change - and note that in the case of 'tx=0
> rx=1' and the result of negotiation being used, you can still end
> up with transmit and receive pause being enabled.
Yes, that is the API change in patch 2 of this set.

> Basically, trying to define the pause advertisment in terms of
> desired TX and RX pause enablement is *very* problematical - they
> really do not mean anything as we can see if we work through the
> various settings and results.
Yes, there is conflation of meaning between these parameters.

> You're much better using the raw advertisment mask to set the
> pause and asym pause bits manually.
Perhaps, but because advertisement is handled by a different ethtool
command and it is very easy to get wrong it would be nice to have a
better default behavior.

>> If link autonegotiation succeeds the peer's advertised Pause and
>> AsymPause bits should be used in combination with the local Pause and
>> Pause Asym bits to determine in which directions pause frames are
>> supported. However, regardless of the negotiated result, if the Tx_Pause
>> is off no pause frames should be sent and if the Rx_Pause is off
>> received pause frames should be ignored. If Tx_Pause is on and the
>> negotiated result allows pause frames to be sent then pause frames may
>> be sent by the local node to apply back pressure to reduce the load on
>> its receive path. If Rx_Pause is on and the negotiated result allows
>> pause frames to be received then the local node should delay its
>> transmission in response to received pause frames. In this way the local
>> settings can only override the negotiated settings to disable the use of
>> pause frames.
>>
>> If the pause autonegotiation is on, and the link autonegotiation is off
>> then the values of the peer's Pause and AsymPause bits are forced to 0
>> (because they can't be exchanged without link autonegotiation) which
>> always produces the negotiated result of pause frame use being disabled
>> in both directions. Since the local Tx_Pause and Rx_Pause parameters can
>> only override the negotiation when they are off, pause frames should not
>> be sent or received.
>>
>> This is the behavior I have attempted to implement by this patch set for
>> the bcmgenet driver, but I see now that I made an error in this last
>> case since I made the negotiation also dependent on the link
>> autonegotiation being enabled. I will correct that in a re-submission.
>>
>> I would appreciate if you can confirm that you agree that this is a good
>> general behavior for all network devices before I resubmit, or please
>> help me understand what could be done better.
> 
> It's gratifying that someone else has run into the same issue I did a
> while back, has put thought into it, and come up with a similar idea
> that I did.  You'll find your idea already spelt out in the comments
> in phylink_ethtool_set_pauseparam().
Yes, I did find it there and actually included it verbatim in the cover
letter to this set.

> However, as I say, it's an API change.
> 
> I've long considered the ethtool APIs to be very deficient in its
> pause handling in many ways.  Another example is:
> 
>         Supported pause frame use: Symmetric Receive-only
> 
> which leads to the obvious observation: the link can negotiate that
> this end should transmit only, but the terminology used here does
> not seem to permit it (there's no "Transmit-only" indicated.) In
> reality, one shold read "Asymmetric" for "Receive-only" in this
> output, because that is exactly what the bit that controls that
> indication is.
Absolutely.

The Pause and AsymPause bits as defined by the IEEE 802.3 standard are
for the purpose of advertising a capability. While the Tx_Pause and
Rx_Pause parameters of ethtool allow a user to indicate whether the
feature should be used on a link that is capable of the feature.

When pause autonegotiation is enabled the local and peer Pause and
AsymPause bits should be used to negotiate the CAPABILITY of using the
pause feature for each direction. This is not the same as enabling pause
in those directions.

So for the problematic cases:

If you specify Tx_Pause = 0, Rx_Pause = 1 you advertise that the link is
capable of both Symmetric PAUSE and Asymmetric PAUSE toward local device
according to Table 37-2 in IEEE Std 802.3-2018. If the result of link
autonegotiation indicates that both directions are capable of supporting
pause control frames you choose not to send pause control frames because
the user asked you not to by setting Tx_Pause = 0.

If you specify Tx_Pause = 1, Rx_Pause = 1 you advertise that the link is
capable of both Symmetric PAUSE and Asymmetric PAUSE toward local device
according to Table 37-2 in IEEE Std 802.3-2018. If the far end supports
only AsymPause, then the link autonegotiation will indicate that only
the receive direction is capable of supporting the pause feature and you
should not send pause control frames to the peer even though the user
has set Tx_Pause = 1.

If link autonegotiation is disabled, then the capability of the link to
support pause frames cannot be negotiated and therefore pause control
frames should not be used.

When pause autonegotiation is disabled the local peer does not care what
its peer is capable of and it can choose to send and process pause
control frames based entirely, on the users requested Tx_Pause and
Rx_Pause parameters. However, if link autonegotiation is enabled it
might as well not be rude and should advertise its intended usage.
Admittedly, Tx_Pause = 1 and Rx_Pause = 1 setting Pause and clearing
AsymPause is still "weird" behavior, but I'm really only retaining it
for backward compatibility. I don't really know why someone thought that
would be a good mapping and why its use has propagated, but I am not
willing to implement that behavior in the bcmgenet driver.

I was considering including a patch in this set to modify the phylink
behavior to use this more rational interpretation as suggested by your
comment there, but it wouldn't allow users to opt-in to the new behavior
so I stopped short of that.

My secret hope was that you would be inspired by this implementation to
champion such an effort in the phylink. :)

I still would like to see support for this standard compliant mapping in
the phylib, but I can localize the changes to a custom implementation in
the bcmgenet driver if necessary.

Re: [PATCH net-next 1/4] net: ethernet: validate pause autoneg setting

[Michal Kubecek]

On Tue, May 12, 2020 at 11:31:39AM -0700, Doug Berger wrote:
> On 5/11/2020 5:47 PM, Andrew Lunn wrote:
> > On Mon, May 11, 2020 at 05:24:07PM -0700, Doug Berger wrote:
> >> A comment in uapi/linux/ethtool.h states "Drivers should reject a
> >> non-zero setting of @autoneg when autoneogotiation is disabled (or
> >> not supported) for the link".
> >>
> >> That check should be added to phy_validate_pause() to consolidate
> >> the code where possible.
> >>
> >> Fixes: 22b7d29926b5 ("net: ethernet: Add helper to determine if pause configuration is supported")
> > 
> > Hi Doug
> > 
> > If this is a real fix, please submit this to net, not net-next.
> > 
> >    Andrew
> > 
> This was intended as a fix, but I thought it would be better to keep it
> as part of this set for context and since net-next is currently open.
> 
> The context is trying to improve the phylib support for offloading
> ethtool pause configuration and this is something that could be checked
> in a single location rather than by individual drivers.
> 
> I included it here to get feedback about its appropriateness as a common
> behavior. I should have been more explicit about that.
> 
> Personally, I'm actually not that fond of this change since it can
> easily be a source of confusion with the ethtool interface because the
> link autonegotiation and the pause autonegotiation are controlled by
> different commands.
> 
> Since the ethtool -A command performs a read/modify/write of pause
> parameters, you can get strange results like these:
> # ethtool -s eth0 speed 100 duplex full autoneg off
> # ethtool -A eth0 tx off
> Cannot set device pause parameters: Invalid argument
> #
> Because, the get read pause autoneg as enabled and only the tx_pause
> member of the structure was updated.

This would be indeed unfortunate. We could use extack to make the error
message easier to understand but the real problem IMHO is that
ethtool_ops::get_pauseparam() returns value which is rejected by
ethtool_ops::set_pauseparam(). This is something we should avoid.

If we really wanted to reject ethtool_pauseparam::autoneg on when
general autonegotiation is off, we would have to disable pause
autonegotiation whenever general autonegotiation is disabled. I don't
like that idea, however, as that would mean that

  ethtool -s dev autoneg off ...
  ethtool -s dev autoneg on ...

would reset the setting of pause autonegotiation.

Therefore I believe the comment should be rather replaced by a warning
that even if ethtool_pauseparam::autoneg is enabled, pause
autonegotiation is only active if general autonegotiation is also
enabled.

Michal

Re: [PATCH net-next 1/4] net: ethernet: validate pause autoneg setting

[Doug Berger]

On 5/12/2020 12:08 PM, Michal Kubecek wrote:
> On Tue, May 12, 2020 at 11:31:39AM -0700, Doug Berger wrote:
>> On 5/11/2020 5:47 PM, Andrew Lunn wrote:
>>> On Mon, May 11, 2020 at 05:24:07PM -0700, Doug Berger wrote:
>>>> A comment in uapi/linux/ethtool.h states "Drivers should reject a
>>>> non-zero setting of @autoneg when autoneogotiation is disabled (or
>>>> not supported) for the link".
>>>>
>>>> That check should be added to phy_validate_pause() to consolidate
>>>> the code where possible.
>>>>
>>>> Fixes: 22b7d29926b5 ("net: ethernet: Add helper to determine if pause configuration is supported")
>>>
>>> Hi Doug
>>>
>>> If this is a real fix, please submit this to net, not net-next.
>>>
>>>    Andrew
>>>
>> This was intended as a fix, but I thought it would be better to keep it
>> as part of this set for context and since net-next is currently open.
>>
>> The context is trying to improve the phylib support for offloading
>> ethtool pause configuration and this is something that could be checked
>> in a single location rather than by individual drivers.
>>
>> I included it here to get feedback about its appropriateness as a common
>> behavior. I should have been more explicit about that.
>>
>> Personally, I'm actually not that fond of this change since it can
>> easily be a source of confusion with the ethtool interface because the
>> link autonegotiation and the pause autonegotiation are controlled by
>> different commands.
>>
>> Since the ethtool -A command performs a read/modify/write of pause
>> parameters, you can get strange results like these:
>> # ethtool -s eth0 speed 100 duplex full autoneg off
>> # ethtool -A eth0 tx off
>> Cannot set device pause parameters: Invalid argument
>> #
>> Because, the get read pause autoneg as enabled and only the tx_pause
>> member of the structure was updated.
> 
> This would be indeed unfortunate. We could use extack to make the error
> message easier to understand but the real problem IMHO is that
> ethtool_ops::get_pauseparam() returns value which is rejected by
> ethtool_ops::set_pauseparam(). This is something we should avoid.
> 
> If we really wanted to reject ethtool_pauseparam::autoneg on when
> general autonegotiation is off, we would have to disable pause
> autonegotiation whenever general autonegotiation is disabled. I don't
> like that idea, however, as that would mean that
> 
>   ethtool -s dev autoneg off ...
>   ethtool -s dev autoneg on ...
> 
> would reset the setting of pause autonegotiation.
> 
> Therefore I believe the comment should be rather replaced by a warning
> that even if ethtool_pauseparam::autoneg is enabled, pause
> autonegotiation is only active if general autonegotiation is also
> enabled.
> 
> Michal
> 
Thanks for your reply.

I agree with your concerns and will remove this commit from the set when
I resubmit. I also favor replacing the comment in ethtool.h.

-Doug

Re: [PATCH net-next 1/4] net: ethernet: validate pause autoneg setting

[Florian Fainelli]

On 5/11/2020 5:24 PM, Doug Berger wrote:
> A comment in uapi/linux/ethtool.h states "Drivers should reject a
> non-zero setting of @autoneg when autoneogotiation is disabled (or
> not supported) for the link".
> 
> That check should be added to phy_validate_pause() to consolidate
> the code where possible.
> 
> Fixes: 22b7d29926b5 ("net: ethernet: Add helper to determine if pause configuration is supported")
> Signed-off-by: Doug Berger <opendmb@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian

[PATCH net-next 2/4] net: add autoneg parameter to linkmode_set_pause

[Doug Berger]

This commit introduces a new parameter to linkmode_set_pause to
provide a mechanism to specify two alternative mappings of the
pause parameters for advertisement by the PHY.

An argument is made that the advertisement through Pause and
AsymPause of the desired Rx and Tx pause frame use should be
different depending on whether pause autonegotiation is enabled.

The existing behavior is kept unchanged by setting autoneg=0 in
this new parameter and existing users of the function are made
to do just that.

Fixes: 45c767faef15 ("net: add linkmode helper for setting flow control advertisement")
Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/net/phy/linkmode.c   | 104 +++++++++++++++++++++++++++++++++----------
 drivers/net/phy/phy_device.c |   3 +-
 drivers/net/phy/phylink.c    |   6 +--
 include/linux/linkmode.h     |   3 +-
 4 files changed, 88 insertions(+), 28 deletions(-)

diff --git a/drivers/net/phy/linkmode.c b/drivers/net/phy/linkmode.c
index f60560fe3499..5658ba9ba94d 100644
--- a/drivers/net/phy/linkmode.c
+++ b/drivers/net/phy/linkmode.c
@@ -48,48 +48,106 @@ EXPORT_SYMBOL_GPL(linkmode_resolve_pause);
  * @advertisement: advertisement in ethtool format
  * @tx: boolean from ethtool struct ethtool_pauseparam tx_pause member
  * @rx: boolean from ethtool struct ethtool_pauseparam rx_pause member
+ * @autoneg: boolean from ethtool struct ethtool_pauseparam autoneg member
  *
  * Configure the advertised Pause and Asym_Pause bits according to the
- * capabilities of provided in @tx and @rx.
+ * capabilities specified by @tx, @rx, and @autoneg.
  *
- * We convert as follows:
+ * If autoneg is true, we convert as follows:
+ *  tx rx  Pause AsymDir
+ *  0  0   0     0
+ *  0  1   1     1
+ *  1  0   0     1
+ *  1  1   1     1
+ *
+ * Otherwise, we convert as follows:
  *  tx rx  Pause AsymDir
  *  0  0   0     0
  *  0  1   1     1
  *  1  0   0     1
  *  1  1   1     0
  *
- * Note: this translation from ethtool tx/rx notation to the advertisement
- * is actually very problematical. Here are some examples:
+ * To the extent that pause frame generation and consumption are defined as
+ * MAC layer functionalities and that the PHY layer should only concern
+ * itself with the advertising of these capabilities, this implementation
+ * is intended to address the problematic behaviors of the previous version
+ * while allowing equivalent behavior when an implementation is unwilling
+ * to negotiate with its peer.
+ *
+ * When autoneg is enabled for pause parameters it indicates a willingness
+ * to negotiate the parameters with a peer. Negotiation implies that a node
+ * is willing to accept a subset of its requested parameters as long as it
+ * is compatible with those requested parameters. This mapping agrees with
+ * the encoding specified in IEEE Std 802.3-2018 by Table 37-2.
+ *
+ * The negotiation is specified in IEEE Std 802.3-2018 by Table 37-4 and
+ * is implemented here by linkmode_resolve_pause.
+ *
+ * Allowing <autoneg=1 tx=1 rx=1> to set both Pause and AsymDir prevents
+ * the previous problematic behaviors as follows:
  *
  * For tx=0 rx=1, meaning transmit is unsupported, receive is supported:
  *
  *  Local device  Link partner
- *  Pause AsymDir Pause AsymDir Result
- *    1     1       1     0     TX + RX - but we have no TX support.
- *    1     1       0     1	Only this gives RX only
+ *  Pause AsymDir Pause AsymDir Result of negotiation at local device
+ *    1     1       0     0	No pause frames allowed
+ *    1     1       0     1	Only RX pause frames allowed
+ *    1     1       1     0	TX + RX pause frames are allowed
+ *    1     1       1     1	TX + RX pause frames are allowed
+ *
+ * While the TX + RX results may seem problematic they are only an
+ * indication that a MAC that receives a pause MAC control frame in the
+ * specified direction will comply with the specified behavior. The tx=0
+ * parameter is made visible to the local MAC layer by the set_pauseparam
+ * ethtool method so it should disable the generation of pause frames at
+ * the MAC layer regardless of what the PHY negotiates.
  *
  * For tx=1 rx=1, meaning we have the capability to transmit and receive
- * pause frames:
+ * pause frames, the results are the same as above. However, now the PHY
+ * negotiation result that reports "Only Rx pause frames allowed" must be
+ * used by the MAC to override the fact that tx=1 so that pause frame
+ * generation is disabled for this combination.
+ *
+ * When autoneg is disabled for pause parameters, it indicates an
+ * unwillingness to negotiate the parameters with a peer. In this case,
+ * the advertisement is more informational and mapping tx=1 rx=1 to only
+ * Symmetric Pause may be a better communication of intent.
  *
  *  Local device  Link partner
- *  Pause AsymDir Pause AsymDir Result
- *    1     0       0     1     Disabled - but since we do support tx and rx,
- *				this should resolve to RX only.
- *
- * Hence, asking for:
- *  rx=1 tx=0 gives Pause+AsymDir advertisement, but we may end up
- *            resolving to tx+rx pause or only rx pause depending on
- *            the partners advertisement.
- *  rx=0 tx=1 gives AsymDir only, which will only give tx pause if
- *            the partners advertisement allows it.
- *  rx=1 tx=1 gives Pause only, which will only allow tx+rx pause
- *            if the other end also advertises Pause.
+ *  Pause AsymDir Pause AsymDir Result of negotiation
+ *    1     0       0     1     Disabled
+ *
+ * This may be considered problematic, since with negotiation it should
+ * be possible for the peer device to send pause frames. However, since
+ * the local device is not willing to negotiate, the result agrees with
+ * a philosophy that the local device wants Symmetric Pause or nothing.
+ * A peer device that is willing to negotiate in this circumstance will
+ * have to fall back to not sending pause frames which may be appropriate
+ * if the local device is not able to support Asymmetric Pause.
+ *
+ * A MAC that has autoneg disabled for pause parameters may choose not
+ * to comply with the mapping returned by linkmode_resolve_pause and rely
+ * purely on the tx and rx values set by the set_pauseparam ethtool
+ * method. However, there is no guarantee of proper functionality and the
+ * burden is placed on the user to "know what they are doing" as is always
+ * the case with manual configuration of network parameters.
+ *
+ * Note: A MAC that is only able to support Symmetric Pause should use the
+ * phy_support_sym_pause function to reflect that in the supported bits.
+ * It should use phy_validate_pause to validate the set_pauseparam values.
+ * If the parameters are valid, this function should be called with
+ * autoneg=0 to advertise its inability to properly negotiate regardless
+ * of the autoneg parameter in the ethtool_pauseparam structure.
  */
-void linkmode_set_pause(unsigned long *advertisement, bool tx, bool rx)
+void linkmode_set_pause(unsigned long *advertisement, bool tx, bool rx,
+			bool autoneg)
 {
 	linkmode_mod_bit(ETHTOOL_LINK_MODE_Pause_BIT, advertisement, rx);
-	linkmode_mod_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, advertisement,
-			 rx ^ tx);
+	if (autoneg)
+		linkmode_mod_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
+				 advertisement, rx | tx);
+	else
+		linkmode_mod_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
+				 advertisement, rx ^ tx);
 }
 EXPORT_SYMBOL_GPL(linkmode_set_pause);
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 5a9618ba232e..48ab9efa0166 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2604,7 +2604,8 @@ void phy_set_asym_pause(struct phy_device *phydev, bool rx, bool tx)
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(oldadv);
 
 	linkmode_copy(oldadv, phydev->advertising);
-	linkmode_set_pause(phydev->advertising, tx, rx);
+	/* autoneg=0 to preserve functionality for exisiting users */
+	linkmode_set_pause(phydev->advertising, tx, rx, 0);
 
 	if (!linkmode_equal(oldadv, phydev->advertising) &&
 	    phydev->autoneg)
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 0f23bec431c1..c544d25ad654 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1489,8 +1489,8 @@ int phylink_ethtool_set_pauseparam(struct phylink *pl,
 
 	/*
 	 * See the comments for linkmode_set_pause(), wrt the deficiencies
-	 * with the current implementation.  A solution to this issue would
-	 * be:
+	 * with the autoneg=0 implementation.  A solution to this issue would
+	 * be to set autoneg=1 to get:
 	 * ethtool  Local device
 	 *  rx  tx  Pause AsymDir
 	 *  0   0   0     0
@@ -1501,7 +1501,7 @@ int phylink_ethtool_set_pauseparam(struct phylink *pl,
 	 * rx/tx pause resolution.
 	 */
 	linkmode_set_pause(config->advertising, pause->tx_pause,
-			   pause->rx_pause);
+			   pause->rx_pause, 0);
 
 	/* If we have a PHY, phylib will call our link state function if the
 	 * mode has changed, which will trigger a resolve and update the MAC
diff --git a/include/linux/linkmode.h b/include/linux/linkmode.h
index c664c27a29a0..7b55a97de162 100644
--- a/include/linux/linkmode.h
+++ b/include/linux/linkmode.h
@@ -92,6 +92,7 @@ void linkmode_resolve_pause(const unsigned long *local_adv,
 			    const unsigned long *partner_adv,
 			    bool *tx_pause, bool *rx_pause);
 
-void linkmode_set_pause(unsigned long *advertisement, bool tx, bool rx);
+void linkmode_set_pause(unsigned long *advertisement, bool tx, bool rx,
+			bool autoneg);
 
 #endif /* __LINKMODE_H */
-- 
2.7.4

[PATCH net-next 3/4] net: ethernet: introduce phy_set_pause

[Doug Berger]

This commit introduces the phy_set_pause function to the phylib as
a helper to support the set_pauseparam ethtool method.

It is hoped that the new behavior introduced by this function will
be widely embraced and the phy_set_sym_pause and phy_set_asym_pause
functions can be deprecated. Those functions are retained for all
existing users and for any desenting opinions on my interpretation
of the functionality.

Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/net/phy/phy_device.c | 31 +++++++++++++++++++++++++++++++
 include/linux/phy.h          |  1 +
 2 files changed, 32 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 48ab9efa0166..e6dafb3c3e5f 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2614,6 +2614,37 @@ void phy_set_asym_pause(struct phy_device *phydev, bool rx, bool tx)
 EXPORT_SYMBOL(phy_set_asym_pause);
 
 /**
+ * phy_set_pause - Configure Pause and Asym Pause with autoneg
+ * @phydev: target phy_device struct
+ * @rx: Receiver Pause is supported
+ * @tx: Transmit Pause is supported
+ * @autoneg: Auto neg should be used
+ *
+ * Description: Configure advertised Pause support depending on if
+ * receiver pause and pause auto neg is supported. Generally called
+ * from the set_pauseparam ethtool_ops.
+ *
+ * Note: Since pause is really a MAC level function it should be
+ * notified via adjust_link to update its pause functions.
+ */
+void phy_set_pause(struct phy_device *phydev, bool rx, bool tx, bool autoneg)
+{
+	linkmode_set_pause(phydev->advertising, tx, rx, autoneg);
+
+	/* Reset the state of an already running link to force a new
+	 * link up event when advertising doesn't change or when PHY
+	 * autoneg is disabled.
+	 */
+	mutex_lock(&phydev->lock);
+	if (phydev->state == PHY_RUNNING)
+		phydev->state = PHY_UP;
+	mutex_unlock(&phydev->lock);
+
+	phy_start_aneg(phydev);
+}
+EXPORT_SYMBOL(phy_set_pause);
+
+/**
  * phy_validate_pause - Test if the PHY/MAC support the pause configuration
  * @phydev: phy_device struct
  * @pp: requested pause configuration
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 5d8ff5428010..71e484424e68 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1403,6 +1403,7 @@ void phy_support_asym_pause(struct phy_device *phydev);
 void phy_set_sym_pause(struct phy_device *phydev, bool rx, bool tx,
 		       bool autoneg);
 void phy_set_asym_pause(struct phy_device *phydev, bool rx, bool tx);
+void phy_set_pause(struct phy_device *phydev, bool rx, bool tx, bool autoneg);
 bool phy_validate_pause(struct phy_device *phydev,
 			struct ethtool_pauseparam *pp);
 void phy_get_pause(struct phy_device *phydev, bool *tx_pause, bool *rx_pause);
-- 
2.7.4

Re: [PATCH net-next 3/4] net: ethernet: introduce phy_set_pause

[Andrew Lunn]

On Mon, May 11, 2020 at 05:24:09PM -0700, Doug Berger wrote:
> This commit introduces the phy_set_pause function to the phylib as
> a helper to support the set_pauseparam ethtool method.
> 
> It is hoped that the new behavior introduced by this function will
> be widely embraced and the phy_set_sym_pause and phy_set_asym_pause
> functions can be deprecated. Those functions are retained for all
> existing users and for any desenting opinions on my interpretation
> of the functionality.

It would be good to add comments to phy_set_sym_pause and
phy_set_asym_pause indicating they are deprecated and point to
phy_set_pause().

	Andrew

Re: [PATCH net-next 3/4] net: ethernet: introduce phy_set_pause

[Doug Berger]

On 5/11/2020 5:51 PM, Andrew Lunn wrote:
> On Mon, May 11, 2020 at 05:24:09PM -0700, Doug Berger wrote:
>> This commit introduces the phy_set_pause function to the phylib as
>> a helper to support the set_pauseparam ethtool method.
>>
>> It is hoped that the new behavior introduced by this function will
>> be widely embraced and the phy_set_sym_pause and phy_set_asym_pause
>> functions can be deprecated. Those functions are retained for all
>> existing users and for any desenting opinions on my interpretation
>> of the functionality.
> 
> It would be good to add comments to phy_set_sym_pause and
> phy_set_asym_pause indicating they are deprecated and point to
> phy_set_pause().
> 
> 	Andrew
> 

To be clear, this patch set reflects the pauseparam implementation I
desire for the bcmgenet driver. I attempted to implement it as a common
phylib service with the hope that it would help other network driver
maintainers add support for pause in a common way.

I would like to get feedback/consensus that it is desirable behavior for
other drivers to implement before promoting the change of existing
implementations.

In particular, I would like to know Russell King's opinion since he has
clearly observed (and documented) the short comings of current
implementations as part of his phylink work.

If others agree with this being the way to move forward, I will submit
another revision with your suggested comments about deprecation within
the specified functions.

Thanks for the feedback,
    Doug

Re: [PATCH net-next 3/4] net: ethernet: introduce phy_set_pause

[Florian Fainelli]

On 5/11/2020 5:24 PM, Doug Berger wrote:
> This commit introduces the phy_set_pause function to the phylib as
> a helper to support the set_pauseparam ethtool method.
> 
> It is hoped that the new behavior introduced by this function will
> be widely embraced and the phy_set_sym_pause and phy_set_asym_pause
> functions can be deprecated. Those functions are retained for all
> existing users and for any desenting opinions on my interpretation
> of the functionality.
> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next 3/4] net: ethernet: introduce phy_set_pause

[Russell King - ARM Linux admin]

On Mon, May 11, 2020 at 05:24:09PM -0700, Doug Berger wrote:
> This commit introduces the phy_set_pause function to the phylib as
> a helper to support the set_pauseparam ethtool method.
> 
> It is hoped that the new behavior introduced by this function will
> be widely embraced and the phy_set_sym_pause and phy_set_asym_pause
> functions can be deprecated. Those functions are retained for all
> existing users and for any desenting opinions on my interpretation
> of the functionality.
> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>
> ---
>  drivers/net/phy/phy_device.c | 31 +++++++++++++++++++++++++++++++
>  include/linux/phy.h          |  1 +
>  2 files changed, 32 insertions(+)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 48ab9efa0166..e6dafb3c3e5f 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -2614,6 +2614,37 @@ void phy_set_asym_pause(struct phy_device *phydev, bool rx, bool tx)
>  EXPORT_SYMBOL(phy_set_asym_pause);
>  
>  /**
> + * phy_set_pause - Configure Pause and Asym Pause with autoneg
> + * @phydev: target phy_device struct
> + * @rx: Receiver Pause is supported
> + * @tx: Transmit Pause is supported
> + * @autoneg: Auto neg should be used
> + *
> + * Description: Configure advertised Pause support depending on if
> + * receiver pause and pause auto neg is supported. Generally called
> + * from the set_pauseparam ethtool_ops.
> + *
> + * Note: Since pause is really a MAC level function it should be
> + * notified via adjust_link to update its pause functions.
> + */
> +void phy_set_pause(struct phy_device *phydev, bool rx, bool tx, bool autoneg)
> +{
> +	linkmode_set_pause(phydev->advertising, tx, rx, autoneg);
> +
> +	/* Reset the state of an already running link to force a new
> +	 * link up event when advertising doesn't change or when PHY
> +	 * autoneg is disabled.
> +	 */
> +	mutex_lock(&phydev->lock);
> +	if (phydev->state == PHY_RUNNING)
> +		phydev->state = PHY_UP;
> +	mutex_unlock(&phydev->lock);

I wonder about this - will drivers cope with having two link-up events
via adjust_link without a corresponding link-down event?  What if they
touch registers that are only supposed to be touched while the link is
down?  Obviously, drivers have to opt-in to this interface, so it may
be okay provided we don't get wholesale changes.

> +
> +	phy_start_aneg(phydev);

Should we be making that conditional on something changing and autoneg
being enabled, like phy_set_asym_pause() does?  There is no point
interrupting an established link if the advertisement didn't change.

> +}
> +EXPORT_SYMBOL(phy_set_pause);
> +
> +/**
>   * phy_validate_pause - Test if the PHY/MAC support the pause configuration
>   * @phydev: phy_device struct
>   * @pp: requested pause configuration
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 5d8ff5428010..71e484424e68 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -1403,6 +1403,7 @@ void phy_support_asym_pause(struct phy_device *phydev);
>  void phy_set_sym_pause(struct phy_device *phydev, bool rx, bool tx,
>  		       bool autoneg);
>  void phy_set_asym_pause(struct phy_device *phydev, bool rx, bool tx);
> +void phy_set_pause(struct phy_device *phydev, bool rx, bool tx, bool autoneg);
>  bool phy_validate_pause(struct phy_device *phydev,
>  			struct ethtool_pauseparam *pp);
>  void phy_get_pause(struct phy_device *phydev, bool *tx_pause, bool *rx_pause);
> -- 
> 2.7.4
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH net-next 3/4] net: ethernet: introduce phy_set_pause

[Doug Berger]

On 5/13/2020 2:42 AM, Russell King - ARM Linux admin wrote:
> On Mon, May 11, 2020 at 05:24:09PM -0700, Doug Berger wrote:
>> This commit introduces the phy_set_pause function to the phylib as
>> a helper to support the set_pauseparam ethtool method.
>>
>> It is hoped that the new behavior introduced by this function will
>> be widely embraced and the phy_set_sym_pause and phy_set_asym_pause
>> functions can be deprecated. Those functions are retained for all
>> existing users and for any desenting opinions on my interpretation
>> of the functionality.
>>
>> Signed-off-by: Doug Berger <opendmb@gmail.com>
>> ---
>>  drivers/net/phy/phy_device.c | 31 +++++++++++++++++++++++++++++++
>>  include/linux/phy.h          |  1 +
>>  2 files changed, 32 insertions(+)
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 48ab9efa0166..e6dafb3c3e5f 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -2614,6 +2614,37 @@ void phy_set_asym_pause(struct phy_device *phydev, bool rx, bool tx)
>>  EXPORT_SYMBOL(phy_set_asym_pause);
>>  
>>  /**
>> + * phy_set_pause - Configure Pause and Asym Pause with autoneg
>> + * @phydev: target phy_device struct
>> + * @rx: Receiver Pause is supported
>> + * @tx: Transmit Pause is supported
>> + * @autoneg: Auto neg should be used
>> + *
>> + * Description: Configure advertised Pause support depending on if
>> + * receiver pause and pause auto neg is supported. Generally called
>> + * from the set_pauseparam ethtool_ops.
>> + *
>> + * Note: Since pause is really a MAC level function it should be
>> + * notified via adjust_link to update its pause functions.
>> + */
>> +void phy_set_pause(struct phy_device *phydev, bool rx, bool tx, bool autoneg)
>> +{
>> +	linkmode_set_pause(phydev->advertising, tx, rx, autoneg);
>> +
>> +	/* Reset the state of an already running link to force a new
>> +	 * link up event when advertising doesn't change or when PHY
>> +	 * autoneg is disabled.
>> +	 */
>> +	mutex_lock(&phydev->lock);
>> +	if (phydev->state == PHY_RUNNING)
>> +		phydev->state = PHY_UP;
>> +	mutex_unlock(&phydev->lock);
> 
> I wonder about this - will drivers cope with having two link-up events
> via adjust_link without a corresponding link-down event?  What if they
> touch registers that are only supposed to be touched while the link is
> down?  Obviously, drivers have to opt-in to this interface, so it may
> be okay provided we don't get wholesale changes.
I too wonder about this. That's why I brought it up in the cover letter
to this set. I would prefer a cleaner service interface for this kind of
behavior for the reasons described in the cover letter, but thought this
might be acceptable.

>> +
>> +	phy_start_aneg(phydev);
> 
> Should we be making that conditional on something changing and autoneg
> being enabled, like phy_set_asym_pause() does?  There is no point
> interrupting an established link if the advertisement didn't change.
Again this is described in the cover letter but repeated here:
"The third introduces the phy_set_pause() function based on the existing
phy_set_asym_pause() implementation. One aberration here is the direct
manipulation of the phy state machine to allow a new link up event to
notify the MAC that the pause parameters may have changed. This is a
convenience to simplify the MAC driver by allowing one implementation
of the pause configuration logic to be located in its adjust_link
callback. Otherwise, the MAC driver would need to handle configuring
the pause parameters for an already active PHY link which would likely
require additional synchronization logic to protect the logic from
asynchronous changes in the PHY state.

The logic in phy_set_asym_pause() that looks for a change in
advertising is not particularly helpful here since now a change from
tx=1 rx=1 to tx=0 rx=1 no longer changes the advertising if autoneg is
enabled so phy_start_aneg() would not be called. I took the alternate
approach of unconditionally calling phy_start_aneg() since it
accommodates both manual and autoneg configured links. The "aberrant"
logic allows manually configured and autonegotiated links that don't
change their advertised parameters to receive an adjust_link call to
act on pause parameters that have no effect on the PHY layer.

It seemed excessive to bring the PHY down and back up when nogotiation
is not necessary, but that could be an alternative approach. I am
certainly open to any suggestions on how to improve that portion of
the code if it is controversial and a consensus can be reached."

>> +}
>> +EXPORT_SYMBOL(phy_set_pause);
>> +
>> +/**
>>   * phy_validate_pause - Test if the PHY/MAC support the pause configuration
>>   * @phydev: phy_device struct
>>   * @pp: requested pause configuration
>> diff --git a/include/linux/phy.h b/include/linux/phy.h
>> index 5d8ff5428010..71e484424e68 100644
>> --- a/include/linux/phy.h
>> +++ b/include/linux/phy.h
>> @@ -1403,6 +1403,7 @@ void phy_support_asym_pause(struct phy_device *phydev);
>>  void phy_set_sym_pause(struct phy_device *phydev, bool rx, bool tx,
>>  		       bool autoneg);
>>  void phy_set_asym_pause(struct phy_device *phydev, bool rx, bool tx);
>> +void phy_set_pause(struct phy_device *phydev, bool rx, bool tx, bool autoneg);
>>  bool phy_validate_pause(struct phy_device *phydev,
>>  			struct ethtool_pauseparam *pp);
>>  void phy_get_pause(struct phy_device *phydev, bool *tx_pause, bool *rx_pause);
>> -- 
>> 2.7.4
>>
>>
> 

[PATCH net-next 4/4] net: bcmgenet: add support for ethtool flow control

[Doug Berger]

This commit extends the supported ethtool operations to allow MAC
level flow control to be configured for the bcmgenet driver. It
provides an example of how the new phy_set_pause function and the
phy_validate_pause function can be used to configure the desired
PHY advertising as well as how the phy_get_pause function can be
used for resolving negotiated pause modes which may be overridden
by the MAC.

The ethtool utility can be used to change the configuration to enable
auto-negotiated symmetric and asymmetric modes as well as manually
enabling support for RX and TX Pause frames individually.

Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 54 ++++++++++++++++++++++++++
 drivers/net/ethernet/broadcom/genet/bcmgenet.h |  4 ++
 drivers/net/ethernet/broadcom/genet/bcmmii.c   | 38 ++++++++++++++----
 3 files changed, 89 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index ff31da0ed846..c0e22da7ac53 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -1017,6 +1017,53 @@ static int bcmgenet_set_coalesce(struct net_device *dev,
 	return 0;
 }
 
+static void bcmgenet_get_pauseparam(struct net_device *dev,
+				    struct ethtool_pauseparam *epause)
+{
+	struct bcmgenet_priv *priv;
+	u32 umac_cmd;
+
+	priv = netdev_priv(dev);
+
+	epause->autoneg = priv->autoneg_pause;
+
+	if (priv->old_link > 0) {
+		/* report active state when link is up */
+		umac_cmd = bcmgenet_umac_readl(priv, UMAC_CMD);
+		epause->tx_pause = !(umac_cmd & CMD_TX_PAUSE_IGNORE);
+		epause->rx_pause = !(umac_cmd & CMD_RX_PAUSE_IGNORE);
+	} else {
+		/* otherwise report stored settings */
+		epause->tx_pause = priv->tx_pause;
+		epause->rx_pause = priv->rx_pause;
+	}
+}
+
+static int bcmgenet_set_pauseparam(struct net_device *dev,
+				   struct ethtool_pauseparam *epause)
+{
+	struct bcmgenet_priv *priv = netdev_priv(dev);
+
+	if (!dev->phydev)
+		return -ENODEV;
+
+	if (!phy_validate_pause(dev->phydev, epause))
+		return -EINVAL;
+
+	priv->autoneg_pause = !!epause->autoneg;
+	priv->tx_pause = !!epause->tx_pause;
+	priv->rx_pause = !!epause->rx_pause;
+
+	/* Restart the PHY */
+	if (netif_running(dev))
+		priv->old_link = -1;
+
+	phy_set_pause(dev->phydev, priv->rx_pause, priv->tx_pause,
+		      priv->autoneg_pause);
+
+	return 0;
+}
+
 /* standard ethtool support functions. */
 enum bcmgenet_stat_type {
 	BCMGENET_STAT_NETDEV = -1,
@@ -1670,6 +1717,8 @@ static const struct ethtool_ops bcmgenet_ethtool_ops = {
 	.get_ts_info		= ethtool_op_get_ts_info,
 	.get_rxnfc		= bcmgenet_get_rxnfc,
 	.set_rxnfc		= bcmgenet_set_rxnfc,
+	.get_pauseparam		= bcmgenet_get_pauseparam,
+	.set_pauseparam		= bcmgenet_set_pauseparam,
 };
 
 /* Power down the unimac, based on mode. */
@@ -4018,6 +4067,11 @@ static int bcmgenet_probe(struct platform_device *pdev)
 
 	spin_lock_init(&priv->lock);
 
+	/* Set default pause parameters */
+	priv->autoneg_pause = 1;
+	priv->tx_pause = 1;
+	priv->rx_pause = 1;
+
 	SET_NETDEV_DEV(dev, &pdev->dev);
 	dev_set_drvdata(&pdev->dev, dev);
 	dev->watchdog_timeo = 2 * HZ;
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
index a12cb59298f4..e44830b3aa4a 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
@@ -683,6 +683,10 @@ struct bcmgenet_priv {
 	/* HW descriptors/checksum variables */
 	bool crc_fwd_en;
 
+	unsigned autoneg_pause:1;
+	unsigned tx_pause:1;
+	unsigned rx_pause:1;
+
 	u32 dma_max_burst_length;
 
 	u32 msg_enable;
diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index 511d553a4d11..788da1ecea0c 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
@@ -25,6 +25,21 @@
 
 #include "bcmgenet.h"
 
+static u32 _flow_control_autoneg(struct phy_device *phydev)
+{
+	bool tx_pause, rx_pause;
+	u32 cmd_bits = 0;
+
+	phy_get_pause(phydev, &tx_pause, &rx_pause);
+
+	if (!tx_pause)
+		cmd_bits |= CMD_TX_PAUSE_IGNORE;
+	if (!rx_pause)
+		cmd_bits |= CMD_RX_PAUSE_IGNORE;
+
+	return cmd_bits;
+}
+
 /* setup netdev link state when PHY link status change and
  * update UMAC and RGMII block when link up
  */
@@ -71,12 +86,20 @@ void bcmgenet_mii_setup(struct net_device *dev)
 		cmd_bits <<= CMD_SPEED_SHIFT;
 
 		/* duplex */
-		if (phydev->duplex != DUPLEX_FULL)
-			cmd_bits |= CMD_HD_EN;
-
-		/* pause capability */
-		if (!phydev->pause)
-			cmd_bits |= CMD_RX_PAUSE_IGNORE | CMD_TX_PAUSE_IGNORE;
+		if (phydev->duplex != DUPLEX_FULL) {
+			cmd_bits |= CMD_HD_EN |
+				CMD_RX_PAUSE_IGNORE | CMD_TX_PAUSE_IGNORE;
+		} else {
+			/* pause capability defaults to Symmetric */
+			if (phydev->autoneg && priv->autoneg_pause)
+				cmd_bits |= _flow_control_autoneg(phydev);
+
+			/* Manual override */
+			if (!priv->rx_pause)
+				cmd_bits |= CMD_RX_PAUSE_IGNORE;
+			if (!priv->tx_pause)
+				cmd_bits |= CMD_TX_PAUSE_IGNORE;
+		}
 
 		/*
 		 * Program UMAC and RGMII block based on established
@@ -350,7 +373,8 @@ int bcmgenet_mii_probe(struct net_device *dev)
 		return ret;
 	}
 
-	linkmode_copy(phydev->advertising, phydev->supported);
+	phy_support_asym_pause(phydev);
+	phy_advertise_supported(phydev);
 
 	/* The internal PHY has its link interrupts routed to the
 	 * Ethernet MAC ISRs. On GENETv5 there is a hardware issue
-- 
2.7.4

Re: [PATCH net-next 4/4] net: bcmgenet: add support for ethtool flow control

[Florian Fainelli]

On 5/11/2020 5:24 PM, Doug Berger wrote:
> This commit extends the supported ethtool operations to allow MAC
> level flow control to be configured for the bcmgenet driver. It
> provides an example of how the new phy_set_pause function and the
> phy_validate_pause function can be used to configure the desired
> PHY advertising as well as how the phy_get_pause function can be
> used for resolving negotiated pause modes which may be overridden
> by the MAC.
> 
> The ethtool utility can be used to change the configuration to enable
> auto-negotiated symmetric and asymmetric modes as well as manually
> enabling support for RX and TX Pause frames individually.
> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>

[snip]

Only if you need to respin this patch series, I would rename
_flow_control_autoneg() to bcmgenet_flow_control_autoneg() or something
shorter that still contains bcmgenet_ as a prefix for consistency.

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next 4/4] net: bcmgenet: add support for ethtool flow control

[Russell King - ARM Linux admin]

On Mon, May 11, 2020 at 05:24:10PM -0700, Doug Berger wrote:
> diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
> index 511d553a4d11..788da1ecea0c 100644
> --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
> +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
> @@ -25,6 +25,21 @@
>  
>  #include "bcmgenet.h"
>  
> +static u32 _flow_control_autoneg(struct phy_device *phydev)
> +{
> +	bool tx_pause, rx_pause;
> +	u32 cmd_bits = 0;
> +
> +	phy_get_pause(phydev, &tx_pause, &rx_pause);
> +
> +	if (!tx_pause)
> +		cmd_bits |= CMD_TX_PAUSE_IGNORE;
> +	if (!rx_pause)
> +		cmd_bits |= CMD_RX_PAUSE_IGNORE;
> +
> +	return cmd_bits;
> +}
> +
>  /* setup netdev link state when PHY link status change and
>   * update UMAC and RGMII block when link up
>   */
> @@ -71,12 +86,20 @@ void bcmgenet_mii_setup(struct net_device *dev)
>  		cmd_bits <<= CMD_SPEED_SHIFT;
>  
>  		/* duplex */
> -		if (phydev->duplex != DUPLEX_FULL)
> -			cmd_bits |= CMD_HD_EN;
> -
> -		/* pause capability */
> -		if (!phydev->pause)
> -			cmd_bits |= CMD_RX_PAUSE_IGNORE | CMD_TX_PAUSE_IGNORE;
> +		if (phydev->duplex != DUPLEX_FULL) {
> +			cmd_bits |= CMD_HD_EN |
> +				CMD_RX_PAUSE_IGNORE | CMD_TX_PAUSE_IGNORE;

phy_get_pause() already takes account of whether the PHY is in half
duplex mode.  So:

		bool tx_pause, rx_pause;

		if (phydev->autoneg && priv->autoneg_pause) {
			phy_get_pause(phydev, &tx_pause, &rx_pause);
		} else if (phydev->duplex == DUPLEX_FULL) {
			tx_pause = priv->tx_pause;
			rx_pause = priv->rx_pause;
		} else {
			tx_pause = false;
			rx_pause = false;
		}

		if (!tx_pause)
			cmd_bits |= CMD_TX_PAUSE_IGNORE;
		if (!rx_pause)
			cmd_bits |= CMD_RX_PAUSE_IGNORE;

would be entirely sufficient here.

I wonder whether your implementation (which mine follows) is really
correct though.  Consider this:

# ethtool -A eth0 autoneg on tx on rx on
# ethtool -s eth0 autoneg off speed 1000 duplex full

At this point, what do you expect the resulting pause state to be?  It
may not be what you actually think it should be - it will be tx and rx
pause enabled (it's easier to see why that happens with my rewritten
version of your implementation, which is functionally identical.)

If we take the view that if link autoneg is disabled, and therefore the
link partner's advertisement is zero, shouldn't it result in tx and rx
pause being disabled?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH net-next 4/4] net: bcmgenet: add support for ethtool flow control

[Doug Berger]

On 5/13/2020 2:52 AM, Russell King - ARM Linux admin wrote:
> On Mon, May 11, 2020 at 05:24:10PM -0700, Doug Berger wrote:
>> diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
>> index 511d553a4d11..788da1ecea0c 100644
>> --- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
>> +++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
>> @@ -25,6 +25,21 @@
>>  
>>  #include "bcmgenet.h"
>>  
>> +static u32 _flow_control_autoneg(struct phy_device *phydev)
>> +{
>> +	bool tx_pause, rx_pause;
>> +	u32 cmd_bits = 0;
>> +
>> +	phy_get_pause(phydev, &tx_pause, &rx_pause);
>> +
>> +	if (!tx_pause)
>> +		cmd_bits |= CMD_TX_PAUSE_IGNORE;
>> +	if (!rx_pause)
>> +		cmd_bits |= CMD_RX_PAUSE_IGNORE;
>> +
>> +	return cmd_bits;
>> +}
>> +
>>  /* setup netdev link state when PHY link status change and
>>   * update UMAC and RGMII block when link up
>>   */
>> @@ -71,12 +86,20 @@ void bcmgenet_mii_setup(struct net_device *dev)
>>  		cmd_bits <<= CMD_SPEED_SHIFT;
>>  
>>  		/* duplex */
>> -		if (phydev->duplex != DUPLEX_FULL)
>> -			cmd_bits |= CMD_HD_EN;
>> -
>> -		/* pause capability */
>> -		if (!phydev->pause)
>> -			cmd_bits |= CMD_RX_PAUSE_IGNORE | CMD_TX_PAUSE_IGNORE;
>> +		if (phydev->duplex != DUPLEX_FULL) {
>> +			cmd_bits |= CMD_HD_EN |
>> +				CMD_RX_PAUSE_IGNORE | CMD_TX_PAUSE_IGNORE;
> 
> phy_get_pause() already takes account of whether the PHY is in half
> duplex mode.  So:
> 
> 		bool tx_pause, rx_pause;
> 
> 		if (phydev->autoneg && priv->autoneg_pause) {
> 			phy_get_pause(phydev, &tx_pause, &rx_pause);
> 		} else if (phydev->duplex == DUPLEX_FULL) {
> 			tx_pause = priv->tx_pause;
> 			rx_pause = priv->rx_pause;
> 		} else {
> 			tx_pause = false;
> 			rx_pause = false;
> 		}
> 
> 		if (!tx_pause)
> 			cmd_bits |= CMD_TX_PAUSE_IGNORE;
> 		if (!rx_pause)
> 			cmd_bits |= CMD_RX_PAUSE_IGNORE;
> 
> would be entirely sufficient here.
I need to check the duplex to configure the HD bit in the same register
with the pause controls so I am covering both with one compare.

This also includes a bug here that I mentioned in one of my responses to
the first patch of the set. The call to phy_get_pause() should only be
conditioned on phydev->autoneg_pause here.

The idea is that both directions of pause default to on, but if
autoneg_pause is on then phy_get_pause() has an opportunity to disable
any direction if the capability can't be negotiated for that direction.
Finally, the pause feature can be explicitly disabled by the tx_pause
and rx_pause parameters.
> I wonder whether your implementation (which mine follows) is really
> correct though.  Consider this:
> 
> # ethtool -A eth0 autoneg on tx on rx on
> # ethtool -s eth0 autoneg off speed 1000 duplex full
> 
> At this point, what do you expect the resulting pause state to be?  It
> may not be what you actually think it should be - it will be tx and rx
> pause enabled (it's easier to see why that happens with my rewritten
> version of your implementation, which is functionally identical.)
> 
> If we take the view that if link autoneg is disabled, and therefore the
> link partner's advertisement is zero, shouldn't it result in tx and rx
> pause being disabled?
So with the bug fixed as described above, after these commands
autoneg_pause would be on and autoneg would be off. The logic would call
phy_get_pause() which should return tx_pause = rx_pause = false turning
pause off in both directions.