* [PATCH net-next 02/10] net: add helpers to resolve negotiated flow control
2020-02-15 15:48 [PATCH net-next 00/10] Pause updates for phylib and phylink Russell King - ARM Linux admin
@ 2020-02-15 15:49 ` Russell King
2020-02-15 18:49 ` Andrew Lunn
2020-02-15 15:49 ` [PATCH net-next 03/10] net: add linkmode helper for setting flow control advertisement Russell King
` (10 subsequent siblings)
11 siblings, 1 reply; 24+ messages in thread
From: Russell King @ 2020-02-15 15:49 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, Heiner Kallweit; +Cc: David S. Miller, netdev
Add a couple of helpers to resolve negotiated flow control. Two helpers
are provided:
- linkmode_resolve_pause() which takes the link partner and local
advertisements, and decodes whether we should enable TX or RX pause
at the MAC. This is useful outside of phylib, e.g. in phylink.
- phy_get_pause(), which returns the TX/RX enablement status for the
current negotiation results of the PHY.
This allows us to centralise the flow control resolution, rather than
spreading it around.
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
drivers/net/phy/Makefile | 3 ++-
drivers/net/phy/linkmode.c | 44 ++++++++++++++++++++++++++++++++++++
drivers/net/phy/phy_device.c | 26 +++++++++++++++++++++
include/linux/linkmode.h | 4 ++++
include/linux/phy.h | 3 +++
5 files changed, 79 insertions(+), 1 deletion(-)
create mode 100644 drivers/net/phy/linkmode.c
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index fe5badf13b65..d523fd5670e4 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -1,7 +1,8 @@
# SPDX-License-Identifier: GPL-2.0
# Makefile for Linux PHY drivers and MDIO bus drivers
-libphy-y := phy.o phy-c45.o phy-core.o phy_device.o
+libphy-y := phy.o phy-c45.o phy-core.o phy_device.o \
+ linkmode.o
mdio-bus-y += mdio_bus.o mdio_device.o
ifdef CONFIG_MDIO_DEVICE
diff --git a/drivers/net/phy/linkmode.c b/drivers/net/phy/linkmode.c
new file mode 100644
index 000000000000..969918795228
--- /dev/null
+++ b/drivers/net/phy/linkmode.c
@@ -0,0 +1,44 @@
+// SPDX-License-Identifier: GPL-2.0+
+#include <linux/linkmode.h>
+
+/**
+ * linkmode_resolve_pause - resolve the allowable pause modes
+ * @local_adv: local advertisement in ethtool format
+ * @partner_adv: partner advertisement in ethtool format
+ * @tx_pause: pointer to bool to indicate whether transmit pause should be
+ * enabled.
+ * @rx_pause: pointer to bool to indicate whether receive pause should be
+ * enabled.
+ *
+ * Flow control is resolved according to our and the link partners
+ * advertisements using the following drawn from the 802.3 specs:
+ * Local device Link partner
+ * Pause AsymDir Pause AsymDir Result
+ * 0 X 0 X Disabled
+ * 0 1 1 0 Disabled
+ * 0 1 1 1 TX
+ * 1 0 0 X Disabled
+ * 1 X 1 X TX+RX
+ * 1 1 0 1 RX
+ */
+void linkmode_resolve_pause(const unsigned long *local_adv,
+ const unsigned long *partner_adv,
+ bool *tx_pause, bool *rx_pause)
+{
+ __ETHTOOL_DECLARE_LINK_MODE_MASK(m);
+
+ linkmode_and(m, local_adv, partner_adv);
+ if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT, m)) {
+ *tx_pause = true;
+ *rx_pause = true;
+ } else if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, m)) {
+ *tx_pause = linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
+ partner_adv);
+ *rx_pause = linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
+ local_adv);
+ } else {
+ *tx_pause = false;
+ *rx_pause = false;
+ }
+}
+EXPORT_SYMBOL_GPL(linkmode_resolve_pause);
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 6a5056e0ae77..f5a7a077ec1f 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2409,6 +2409,32 @@ bool phy_validate_pause(struct phy_device *phydev,
}
EXPORT_SYMBOL(phy_validate_pause);
+/**
+ * phy_get_pause - resolve negotiated pause modes
+ * @phydev: phy_device struct
+ * @tx_pause: pointer to bool to indicate whether transmit pause should be
+ * enabled.
+ * @rx_pause: pointer to bool to indicate whether receive pause should be
+ * enabled.
+ *
+ * Resolve and return the flow control modes according to the negotiation
+ * result. This includes checking that we are operating in full duplex mode.
+ * See linkmode_resolve_pause() for further details.
+ */
+void phy_get_pause(struct phy_device *phydev, bool *tx_pause, bool *rx_pause)
+{
+ if (phydev->duplex != DUPLEX_FULL) {
+ *tx_pause = false;
+ *rx_pause = false;
+ return;
+ }
+
+ return linkmode_resolve_pause(phydev->advertising,
+ phydev->lp_advertising,
+ tx_pause, rx_pause);
+}
+EXPORT_SYMBOL(phy_get_pause);
+
static bool phy_drv_supports_irq(struct phy_driver *phydrv)
{
return phydrv->config_intr && phydrv->ack_interrupt;
diff --git a/include/linux/linkmode.h b/include/linux/linkmode.h
index 8e5b352e44f2..9ec210f31d06 100644
--- a/include/linux/linkmode.h
+++ b/include/linux/linkmode.h
@@ -88,4 +88,8 @@ static inline int linkmode_subset(const unsigned long *src1,
return bitmap_subset(src1, src2, __ETHTOOL_LINK_MODE_MASK_NBITS);
}
+void linkmode_resolve_pause(const unsigned long *local_adv,
+ const unsigned long *partner_adv,
+ bool *tx_pause, bool *rx_pause);
+
#endif /* __LINKMODE_H */
diff --git a/include/linux/phy.h b/include/linux/phy.h
index c570e162e05e..80f8b2158271 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1257,6 +1257,9 @@ void phy_set_sym_pause(struct phy_device *phydev, bool rx, bool tx,
void phy_set_asym_pause(struct phy_device *phydev, bool rx, bool tx);
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);
+void phy_resolve_pause(unsigned long *local_adv, unsigned long *partner_adv,
+ bool *tx_pause, bool *rx_pause);
int phy_register_fixup(const char *bus_id, u32 phy_uid, u32 phy_uid_mask,
int (*run)(struct phy_device *));
--
2.20.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH net-next 02/10] net: add helpers to resolve negotiated flow control
2020-02-15 15:49 ` [PATCH net-next 02/10] net: add helpers to resolve negotiated flow control Russell King
@ 2020-02-15 18:49 ` Andrew Lunn
2020-02-15 23:18 ` Russell King - ARM Linux admin
0 siblings, 1 reply; 24+ messages in thread
From: Andrew Lunn @ 2020-02-15 18:49 UTC (permalink / raw)
To: Russell King; +Cc: Florian Fainelli, Heiner Kallweit, David S. Miller, netdev
On Sat, Feb 15, 2020 at 03:49:27PM +0000, Russell King wrote:
> Add a couple of helpers to resolve negotiated flow control. Two helpers
> are provided:
>
> - linkmode_resolve_pause() which takes the link partner and local
> advertisements, and decodes whether we should enable TX or RX pause
> at the MAC. This is useful outside of phylib, e.g. in phylink.
> - phy_get_pause(), which returns the TX/RX enablement status for the
> current negotiation results of the PHY.
>
> This allows us to centralise the flow control resolution, rather than
> spreading it around.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
> drivers/net/phy/Makefile | 3 ++-
> drivers/net/phy/linkmode.c | 44 ++++++++++++++++++++++++++++++++++++
> drivers/net/phy/phy_device.c | 26 +++++++++++++++++++++
> include/linux/linkmode.h | 4 ++++
> include/linux/phy.h | 3 +++
> 5 files changed, 79 insertions(+), 1 deletion(-)
> create mode 100644 drivers/net/phy/linkmode.c
>
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index fe5badf13b65..d523fd5670e4 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -1,7 +1,8 @@
> # SPDX-License-Identifier: GPL-2.0
> # Makefile for Linux PHY drivers and MDIO bus drivers
>
> -libphy-y := phy.o phy-c45.o phy-core.o phy_device.o
> +libphy-y := phy.o phy-c45.o phy-core.o phy_device.o \
> + linkmode.o
> mdio-bus-y += mdio_bus.o mdio_device.o
>
> ifdef CONFIG_MDIO_DEVICE
> diff --git a/drivers/net/phy/linkmode.c b/drivers/net/phy/linkmode.c
> new file mode 100644
> index 000000000000..969918795228
> --- /dev/null
> +++ b/drivers/net/phy/linkmode.c
> @@ -0,0 +1,44 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +#include <linux/linkmode.h>
> +
> +/**
> + * linkmode_resolve_pause - resolve the allowable pause modes
> + * @local_adv: local advertisement in ethtool format
> + * @partner_adv: partner advertisement in ethtool format
> + * @tx_pause: pointer to bool to indicate whether transmit pause should be
> + * enabled.
> + * @rx_pause: pointer to bool to indicate whether receive pause should be
> + * enabled.
> + *
> + * Flow control is resolved according to our and the link partners
> + * advertisements using the following drawn from the 802.3 specs:
> + * Local device Link partner
> + * Pause AsymDir Pause AsymDir Result
> + * 0 X 0 X Disabled
> + * 0 1 1 0 Disabled
> + * 0 1 1 1 TX
> + * 1 0 0 X Disabled
> + * 1 X 1 X TX+RX
> + * 1 1 0 1 RX
> + */
> +void linkmode_resolve_pause(const unsigned long *local_adv,
> + const unsigned long *partner_adv,
> + bool *tx_pause, bool *rx_pause)
> +{
> + __ETHTOOL_DECLARE_LINK_MODE_MASK(m);
> +
> + linkmode_and(m, local_adv, partner_adv);
> + if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT, m)) {
> + *tx_pause = true;
> + *rx_pause = true;
> + } else if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, m)) {
> + *tx_pause = linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> + partner_adv);
> + *rx_pause = linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> + local_adv);
> + } else {
> + *tx_pause = false;
> + *rx_pause = false;
> + }
Hi Russell
It took me a while to check this. Maybe it is the way my brain works,
but to me, it is not obviously correct. I had to expand the table, and
they try all 16 combinations.
Maybe a lookup table would be more obvious?
However, now i spent the time:
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH net-next 02/10] net: add helpers to resolve negotiated flow control
2020-02-15 18:49 ` Andrew Lunn
@ 2020-02-15 23:18 ` Russell King - ARM Linux admin
0 siblings, 0 replies; 24+ messages in thread
From: Russell King - ARM Linux admin @ 2020-02-15 23:18 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Florian Fainelli, Heiner Kallweit, David S. Miller, netdev
On Sat, Feb 15, 2020 at 07:49:32PM +0100, Andrew Lunn wrote:
> On Sat, Feb 15, 2020 at 03:49:27PM +0000, Russell King wrote:
> > Add a couple of helpers to resolve negotiated flow control. Two helpers
> > are provided:
> >
> > - linkmode_resolve_pause() which takes the link partner and local
> > advertisements, and decodes whether we should enable TX or RX pause
> > at the MAC. This is useful outside of phylib, e.g. in phylink.
> > - phy_get_pause(), which returns the TX/RX enablement status for the
> > current negotiation results of the PHY.
> >
> > This allows us to centralise the flow control resolution, rather than
> > spreading it around.
> >
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> > drivers/net/phy/Makefile | 3 ++-
> > drivers/net/phy/linkmode.c | 44 ++++++++++++++++++++++++++++++++++++
> > drivers/net/phy/phy_device.c | 26 +++++++++++++++++++++
> > include/linux/linkmode.h | 4 ++++
> > include/linux/phy.h | 3 +++
> > 5 files changed, 79 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/net/phy/linkmode.c
> >
> > diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> > index fe5badf13b65..d523fd5670e4 100644
> > --- a/drivers/net/phy/Makefile
> > +++ b/drivers/net/phy/Makefile
> > @@ -1,7 +1,8 @@
> > # SPDX-License-Identifier: GPL-2.0
> > # Makefile for Linux PHY drivers and MDIO bus drivers
> >
> > -libphy-y := phy.o phy-c45.o phy-core.o phy_device.o
> > +libphy-y := phy.o phy-c45.o phy-core.o phy_device.o \
> > + linkmode.o
> > mdio-bus-y += mdio_bus.o mdio_device.o
> >
> > ifdef CONFIG_MDIO_DEVICE
> > diff --git a/drivers/net/phy/linkmode.c b/drivers/net/phy/linkmode.c
> > new file mode 100644
> > index 000000000000..969918795228
> > --- /dev/null
> > +++ b/drivers/net/phy/linkmode.c
> > @@ -0,0 +1,44 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +#include <linux/linkmode.h>
> > +
> > +/**
> > + * linkmode_resolve_pause - resolve the allowable pause modes
> > + * @local_adv: local advertisement in ethtool format
> > + * @partner_adv: partner advertisement in ethtool format
> > + * @tx_pause: pointer to bool to indicate whether transmit pause should be
> > + * enabled.
> > + * @rx_pause: pointer to bool to indicate whether receive pause should be
> > + * enabled.
> > + *
> > + * Flow control is resolved according to our and the link partners
> > + * advertisements using the following drawn from the 802.3 specs:
> > + * Local device Link partner
> > + * Pause AsymDir Pause AsymDir Result
> > + * 0 X 0 X Disabled
> > + * 0 1 1 0 Disabled
> > + * 0 1 1 1 TX
> > + * 1 0 0 X Disabled
> > + * 1 X 1 X TX+RX
> > + * 1 1 0 1 RX
> > + */
> > +void linkmode_resolve_pause(const unsigned long *local_adv,
> > + const unsigned long *partner_adv,
> > + bool *tx_pause, bool *rx_pause)
> > +{
> > + __ETHTOOL_DECLARE_LINK_MODE_MASK(m);
> > +
> > + linkmode_and(m, local_adv, partner_adv);
> > + if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT, m)) {
> > + *tx_pause = true;
> > + *rx_pause = true;
> > + } else if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, m)) {
> > + *tx_pause = linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> > + partner_adv);
> > + *rx_pause = linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT,
> > + local_adv);
> > + } else {
> > + *tx_pause = false;
> > + *rx_pause = false;
> > + }
>
> Hi Russell
>
> It took me a while to check this. Maybe it is the way my brain works,
> but to me, it is not obviously correct. I had to expand the table, and
> they try all 16 combinations.
The table is actually as given in 802.3, which is not expanded.
> Maybe a lookup table would be more obvious?
I feel that making a table of all 16 combinations is less obviously
correct.
I tend to work from the table as given, in this order:
Local device Link partner
Pause AsymDir Pause AsymDir Result
1 X 1 X TX+RX
If both pause bits are set, we don't care what the asym direction bits
say. Transmit and receive pause are enabled.
0 1 1 1 TX
If both asym direction bits are set, and the link partner pause bit is
set, then we want transmit pause but not receive pause.
1 1 0 1 RX
If both asym direction bits are set, and our pause bit is set, then
we want receive pause but not transmit pause.
These are the only three combinations that result in any pause settings
being enabled; all other combinations must result in both disabled.
So, working from that, the first test is fairly obvious - we want to
know if both pause bits are set: adv.pause & lpa.pause. If that
evaluates true, we set both.
The second and third have a common precondition, which is:
adv.asymdir & lpa.asymdir. If that is true, then to separate out
whether we enable transmit or receive pause becomes dependent on which
pause bit was set: lpa.pause tells us to enable transmit pause,
adv.pause tells us to enable receive pause. We can't get here if both
pause bits were set due to the first.
If neither pause bits are set, then neither pause gets enabled even
if both asymdir bits are set.
Otherwise, we simply force both transmit and receive pause off.
This kind of thought process seems quite logical to me, but then I've
had several years of logic design and analysis when I was young - so
sorry if it's not obvious to others!
> However, now i spent the time:
>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH net-next 03/10] net: add linkmode helper for setting flow control advertisement
2020-02-15 15:48 [PATCH net-next 00/10] Pause updates for phylib and phylink Russell King - ARM Linux admin
2020-02-15 15:49 ` [PATCH net-next 02/10] net: add helpers to resolve negotiated flow control Russell King
@ 2020-02-15 15:49 ` Russell King
2020-02-15 18:56 ` Andrew Lunn
2020-02-15 15:49 ` [PATCH net-next 04/10] net: phylink: remove pause mode ethtool setting for fixed links Russell King
` (9 subsequent siblings)
11 siblings, 1 reply; 24+ messages in thread
From: Russell King @ 2020-02-15 15:49 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, Heiner Kallweit; +Cc: David S. Miller, netdev
Add a linkmode helper to set the flow control advertisement in an
ethtool linkmode mask according to the tx/rx capabilities. This
implementation is moved from phylib, and documented with an
analysis of its shortcomings.
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
drivers/net/phy/linkmode.c | 51 ++++++++++++++++++++++++++++++++++++
drivers/net/phy/phy_device.c | 17 +-----------
include/linux/linkmode.h | 2 ++
3 files changed, 54 insertions(+), 16 deletions(-)
diff --git a/drivers/net/phy/linkmode.c b/drivers/net/phy/linkmode.c
index 969918795228..f60560fe3499 100644
--- a/drivers/net/phy/linkmode.c
+++ b/drivers/net/phy/linkmode.c
@@ -42,3 +42,54 @@ void linkmode_resolve_pause(const unsigned long *local_adv,
}
}
EXPORT_SYMBOL_GPL(linkmode_resolve_pause);
+
+/**
+ * linkmode_set_pause - set the pause mode advertisement
+ * @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
+ *
+ * Configure the advertised Pause and Asym_Pause bits according to the
+ * capabilities of provided in @tx and @rx.
+ *
+ * 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:
+ *
+ * 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
+ *
+ * For tx=1 rx=1, meaning we have the capability to transmit and receive
+ * pause frames:
+ *
+ * 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.
+ */
+void linkmode_set_pause(unsigned long *advertisement, bool tx, bool rx)
+{
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_Pause_BIT, advertisement, rx);
+ 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 f5a7a077ec1f..2a973265de80 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2361,22 +2361,7 @@ 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_clear_bit(ETHTOOL_LINK_MODE_Pause_BIT,
- phydev->advertising);
- linkmode_clear_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
- phydev->advertising);
-
- if (rx) {
- linkmode_set_bit(ETHTOOL_LINK_MODE_Pause_BIT,
- phydev->advertising);
- linkmode_set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
- phydev->advertising);
- }
-
- if (tx)
- linkmode_change_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
- phydev->advertising);
+ linkmode_set_pause(phydev->advertising, tx, rx);
if (!linkmode_equal(oldadv, phydev->advertising) &&
phydev->autoneg)
diff --git a/include/linux/linkmode.h b/include/linux/linkmode.h
index 9ec210f31d06..c664c27a29a0 100644
--- a/include/linux/linkmode.h
+++ b/include/linux/linkmode.h
@@ -92,4 +92,6 @@ 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);
+
#endif /* __LINKMODE_H */
--
2.20.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH net-next 03/10] net: add linkmode helper for setting flow control advertisement
2020-02-15 15:49 ` [PATCH net-next 03/10] net: add linkmode helper for setting flow control advertisement Russell King
@ 2020-02-15 18:56 ` Andrew Lunn
2020-02-15 23:54 ` Russell King - ARM Linux admin
0 siblings, 1 reply; 24+ messages in thread
From: Andrew Lunn @ 2020-02-15 18:56 UTC (permalink / raw)
To: Russell King; +Cc: Florian Fainelli, Heiner Kallweit, David S. Miller, netdev
On Sat, Feb 15, 2020 at 03:49:32PM +0000, Russell King wrote:
> Add a linkmode helper to set the flow control advertisement in an
> ethtool linkmode mask according to the tx/rx capabilities. This
> implementation is moved from phylib, and documented with an
> analysis of its shortcomings.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
> drivers/net/phy/linkmode.c | 51 ++++++++++++++++++++++++++++++++++++
> drivers/net/phy/phy_device.c | 17 +-----------
> include/linux/linkmode.h | 2 ++
> 3 files changed, 54 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/net/phy/linkmode.c b/drivers/net/phy/linkmode.c
> index 969918795228..f60560fe3499 100644
> --- a/drivers/net/phy/linkmode.c
> +++ b/drivers/net/phy/linkmode.c
> @@ -42,3 +42,54 @@ void linkmode_resolve_pause(const unsigned long *local_adv,
> }
> }
> EXPORT_SYMBOL_GPL(linkmode_resolve_pause);
> +
> +/**
> + * linkmode_set_pause - set the pause mode advertisement
> + * @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
> + *
> + * Configure the advertised Pause and Asym_Pause bits according to the
> + * capabilities of provided in @tx and @rx.
> + *
> + * 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:
> + *
> + * 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
> + *
> + * For tx=1 rx=1, meaning we have the capability to transmit and receive
> + * pause frames:
> + *
> + * 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.
> + */
It is good to document this.
With the change to netlink ethtool, we have the option to change the
interface to user space, or at least, easily add another way for
userspace to configure things. Maybe you can think of a better API?
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH net-next 03/10] net: add linkmode helper for setting flow control advertisement
2020-02-15 18:56 ` Andrew Lunn
@ 2020-02-15 23:54 ` Russell King - ARM Linux admin
0 siblings, 0 replies; 24+ messages in thread
From: Russell King - ARM Linux admin @ 2020-02-15 23:54 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Florian Fainelli, Heiner Kallweit, David S. Miller, netdev
On Sat, Feb 15, 2020 at 07:56:32PM +0100, Andrew Lunn wrote:
> On Sat, Feb 15, 2020 at 03:49:32PM +0000, Russell King wrote:
> > Add a linkmode helper to set the flow control advertisement in an
> > ethtool linkmode mask according to the tx/rx capabilities. This
> > implementation is moved from phylib, and documented with an
> > analysis of its shortcomings.
> >
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> > drivers/net/phy/linkmode.c | 51 ++++++++++++++++++++++++++++++++++++
> > drivers/net/phy/phy_device.c | 17 +-----------
> > include/linux/linkmode.h | 2 ++
> > 3 files changed, 54 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/net/phy/linkmode.c b/drivers/net/phy/linkmode.c
> > index 969918795228..f60560fe3499 100644
> > --- a/drivers/net/phy/linkmode.c
> > +++ b/drivers/net/phy/linkmode.c
> > @@ -42,3 +42,54 @@ void linkmode_resolve_pause(const unsigned long *local_adv,
> > }
> > }
> > EXPORT_SYMBOL_GPL(linkmode_resolve_pause);
> > +
> > +/**
> > + * linkmode_set_pause - set the pause mode advertisement
> > + * @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
> > + *
> > + * Configure the advertised Pause and Asym_Pause bits according to the
> > + * capabilities of provided in @tx and @rx.
> > + *
> > + * 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:
> > + *
> > + * 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
> > + *
> > + * For tx=1 rx=1, meaning we have the capability to transmit and receive
> > + * pause frames:
> > + *
> > + * 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.
> > + */
>
> It is good to document this.
>
> With the change to netlink ethtool, we have the option to change the
> interface to user space, or at least, easily add another way for
> userspace to configure things. Maybe you can think of a better API?
I don't think we even need "a better API" - we just need to be a
little smarter when it comes to the implementation.
Let me expand my table above with the possible link partner resolutions
based on the current local advertisement
(Pause = rx, AsymDir = tx ^ rx):
tx rx Pause AsymDir Possible partner resolutions
0 0 0 0 Disabled
0 1 1 1 TX only, TX+RX, Disabled
1 0 0 1 RX only, Disabled
1 1 1 0 TX+RX, Disabled
If we simply modify the logic such that
(Pause = rx, AsymDir = rx | tx):
tx rx Pause AsymDir Possible partner resolutions
0 0 0 0 Disabled
0 1 1 1 TX only, TX+RX, Disabled
1 0 0 1 RX, Disabled
1 1 1 1 TX only, TX+RX, Disabled
That changes one resolution possibility from where the link partner
resolves to disable pause completely, to enabling transmit pause for
the forced-tx+rx-enabled local state.
To pull out the lines from the 802.3 table:
Local device Link partner
Pause AsymDir Pause AsymDir Result
0 1 1 0 Disabled
becomes:
0 1 1 1 TX
to the link partner, which must surely be better when the link partner
is not being forced.
Now, if we want to say "if you force one end, you should force both
ends" then the immediate question then becomes, what is the point in
updating the advertisement at all? So, would it be better to drop
the requirement to manipulate the advertisement entirely, and only
allow ethtool -s able to change the advertisement?
I don't think there's a clear-cut answer to this, but I think adding
some documentation describing what we've decided to do and
acknowledging any short-comings is very worth while... and that
should probably find its way into ethtool's man page too.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH net-next 04/10] net: phylink: remove pause mode ethtool setting for fixed links
2020-02-15 15:48 [PATCH net-next 00/10] Pause updates for phylib and phylink Russell King - ARM Linux admin
2020-02-15 15:49 ` [PATCH net-next 02/10] net: add helpers to resolve negotiated flow control Russell King
2020-02-15 15:49 ` [PATCH net-next 03/10] net: add linkmode helper for setting flow control advertisement Russell King
@ 2020-02-15 15:49 ` Russell King
2020-02-15 18:57 ` Andrew Lunn
2020-02-15 15:49 ` [PATCH net-next 05/10] net: phylink: ensure manual flow control is selected appropriately Russell King
` (8 subsequent siblings)
11 siblings, 1 reply; 24+ messages in thread
From: Russell King @ 2020-02-15 15:49 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, Heiner Kallweit; +Cc: David S. Miller, netdev
Remove the ability for ethtool -A to change the pause settings for
fixed links; if this is really required, we can reinstate it later.
Andrew Lunn agrees: "So I think it is safe to not implement ethtool
-A, at least until somebody has a real use case for it."
Lets avoid making things too complex for use cases that aren't being
used.
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
drivers/net/phy/phylink.c | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 70b9a143db84..de7b7499ae38 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1373,6 +1373,9 @@ int phylink_ethtool_set_pauseparam(struct phylink *pl,
ASSERT_RTNL();
+ if (pl->cur_link_an_mode == MLO_AN_FIXED)
+ return -EOPNOTSUPP;
+
if (!phylink_test(pl->supported, Pause) &&
!phylink_test(pl->supported, Asym_Pause))
return -EOPNOTSUPP;
@@ -1399,18 +1402,8 @@ int phylink_ethtool_set_pauseparam(struct phylink *pl,
pause->tx_pause);
} else if (!test_bit(PHYLINK_DISABLE_STOPPED,
&pl->phylink_disable_state)) {
- switch (pl->cur_link_an_mode) {
- case MLO_AN_FIXED:
- /* Should we allow fixed links to change against the config? */
- phylink_resolve_flow(pl, config);
- phylink_mac_config(pl, config);
- break;
-
- case MLO_AN_INBAND:
- phylink_mac_config(pl, config);
- phylink_mac_an_restart(pl);
- break;
- }
+ phylink_mac_config(pl, &pl->link_config);
+ phylink_mac_an_restart(pl);
}
return 0;
--
2.20.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH net-next 04/10] net: phylink: remove pause mode ethtool setting for fixed links
2020-02-15 15:49 ` [PATCH net-next 04/10] net: phylink: remove pause mode ethtool setting for fixed links Russell King
@ 2020-02-15 18:57 ` Andrew Lunn
0 siblings, 0 replies; 24+ messages in thread
From: Andrew Lunn @ 2020-02-15 18:57 UTC (permalink / raw)
To: Russell King; +Cc: Florian Fainelli, Heiner Kallweit, David S. Miller, netdev
On Sat, Feb 15, 2020 at 03:49:37PM +0000, Russell King wrote:
> Remove the ability for ethtool -A to change the pause settings for
> fixed links; if this is really required, we can reinstate it later.
>
> Andrew Lunn agrees: "So I think it is safe to not implement ethtool
> -A, at least until somebody has a real use case for it."
>
> Lets avoid making things too complex for use cases that aren't being
> used.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH net-next 05/10] net: phylink: ensure manual flow control is selected appropriately
2020-02-15 15:48 [PATCH net-next 00/10] Pause updates for phylib and phylink Russell King - ARM Linux admin
` (2 preceding siblings ...)
2020-02-15 15:49 ` [PATCH net-next 04/10] net: phylink: remove pause mode ethtool setting for fixed links Russell King
@ 2020-02-15 15:49 ` Russell King
2020-02-15 19:00 ` Andrew Lunn
2020-02-15 15:49 ` [PATCH net-next 06/10] net: phylink: use phylib resolved flow control modes Russell King
` (7 subsequent siblings)
11 siblings, 1 reply; 24+ messages in thread
From: Russell King @ 2020-02-15 15:49 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, Heiner Kallweit; +Cc: David S. Miller, netdev
Split the application of manually controlled flow control modes from
phylink_resolve_flow(), so that we can use alternative providers of
flow control resolution.
We also want to clear the MLO_PAUSE_AN flag when autoneg is disabled,
since flow control can't be negotiated in this circumstance.
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
drivers/net/phy/phylink.c | 42 +++++++++++++++++++++++----------------
1 file changed, 25 insertions(+), 17 deletions(-)
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index de7b7499ae38..846aee591684 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -339,6 +339,18 @@ static int phylink_parse_mode(struct phylink *pl, struct fwnode_handle *fwnode)
return 0;
}
+static void phylink_apply_manual_flow(struct phylink *pl,
+ struct phylink_link_state *state)
+{
+ /* If autoneg is disabled, pause AN is also disabled */
+ if (!state->an_enabled)
+ state->pause &= ~MLO_PAUSE_AN;
+
+ /* Manual configuration of pause modes */
+ if (!(pl->link_config.pause & MLO_PAUSE_AN))
+ state->pause = pl->link_config.pause;
+}
+
static void phylink_mac_config(struct phylink *pl,
const struct phylink_link_state *state)
{
@@ -408,25 +420,20 @@ static void phylink_resolve_flow(struct phylink *pl,
struct phylink_link_state *state)
{
int new_pause = 0;
+ int pause = 0;
- if (pl->link_config.pause & MLO_PAUSE_AN) {
- int pause = 0;
-
- if (phylink_test(pl->link_config.advertising, Pause))
- pause |= MLO_PAUSE_SYM;
- if (phylink_test(pl->link_config.advertising, Asym_Pause))
- pause |= MLO_PAUSE_ASYM;
+ if (phylink_test(pl->link_config.advertising, Pause))
+ pause |= MLO_PAUSE_SYM;
+ if (phylink_test(pl->link_config.advertising, Asym_Pause))
+ pause |= MLO_PAUSE_ASYM;
- pause &= state->pause;
+ pause &= state->pause;
- if (pause & MLO_PAUSE_SYM)
- new_pause = MLO_PAUSE_TX | MLO_PAUSE_RX;
- else if (pause & MLO_PAUSE_ASYM)
- new_pause = state->pause & MLO_PAUSE_SYM ?
- MLO_PAUSE_TX : MLO_PAUSE_RX;
- } else {
- new_pause = pl->link_config.pause & MLO_PAUSE_TXRX_MASK;
- }
+ if (pause & MLO_PAUSE_SYM)
+ new_pause = MLO_PAUSE_TX | MLO_PAUSE_RX;
+ else if (pause & MLO_PAUSE_ASYM)
+ new_pause = state->pause & MLO_PAUSE_SYM ?
+ MLO_PAUSE_TX : MLO_PAUSE_RX;
state->pause &= ~MLO_PAUSE_TXRX_MASK;
state->pause |= new_pause;
@@ -494,6 +501,7 @@ static void phylink_resolve(struct work_struct *w)
case MLO_AN_PHY:
link_state = pl->phy_state;
phylink_resolve_flow(pl, &link_state);
+ phylink_apply_manual_flow(pl, &link_state);
phylink_mac_config_up(pl, &link_state);
break;
@@ -518,6 +526,7 @@ static void phylink_resolve(struct work_struct *w)
* the pause mode bits. */
link_state.pause |= pl->phy_state.pause;
phylink_resolve_flow(pl, &link_state);
+ phylink_apply_manual_flow(pl, &link_state);
phylink_mac_config(pl, &link_state);
}
break;
@@ -1006,7 +1015,6 @@ void phylink_start(struct phylink *pl)
* a fixed-link to start with the correct parameters, and also
* ensures that we set the appropriate advertisement for Serdes links.
*/
- phylink_resolve_flow(pl, &pl->link_config);
phylink_mac_config(pl, &pl->link_config);
/* Restart autonegotiation if using 802.3z to ensure that the link
--
2.20.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH net-next 06/10] net: phylink: use phylib resolved flow control modes
2020-02-15 15:48 [PATCH net-next 00/10] Pause updates for phylib and phylink Russell King - ARM Linux admin
` (3 preceding siblings ...)
2020-02-15 15:49 ` [PATCH net-next 05/10] net: phylink: ensure manual flow control is selected appropriately Russell King
@ 2020-02-15 15:49 ` Russell King
2020-02-15 15:49 ` [PATCH net-next 07/10] net: phylink: resolve fixed link flow control Russell King
` (6 subsequent siblings)
11 siblings, 0 replies; 24+ messages in thread
From: Russell King @ 2020-02-15 15:49 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, Heiner Kallweit; +Cc: David S. Miller, netdev
Use the new phy_get_pause() helper to get the resolved pause modes for
a PHY rather than resolving the pause modes ourselves. We temporarily
retain our pause mode resolution for causes where there is no PHY
attached, e.g. for fixed-link modes.
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
drivers/net/phy/phylink.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 846aee591684..e65e9c9dc759 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -500,7 +500,6 @@ static void phylink_resolve(struct work_struct *w)
switch (pl->cur_link_an_mode) {
case MLO_AN_PHY:
link_state = pl->phy_state;
- phylink_resolve_flow(pl, &link_state);
phylink_apply_manual_flow(pl, &link_state);
phylink_mac_config_up(pl, &link_state);
break;
@@ -523,9 +522,8 @@ static void phylink_resolve(struct work_struct *w)
link_state.interface = pl->phy_state.interface;
/* If we have a PHY, we need to update with
- * the pause mode bits. */
- link_state.pause |= pl->phy_state.pause;
- phylink_resolve_flow(pl, &link_state);
+ * the PHY flow control bits. */
+ link_state.pause = pl->phy_state.pause;
phylink_apply_manual_flow(pl, &link_state);
phylink_mac_config(pl, &link_state);
}
@@ -714,15 +712,18 @@ static void phylink_phy_change(struct phy_device *phydev, bool up,
bool do_carrier)
{
struct phylink *pl = phydev->phylink;
+ bool tx_pause, rx_pause;
+
+ phy_get_pause(phydev, &tx_pause, &rx_pause);
mutex_lock(&pl->state_mutex);
pl->phy_state.speed = phydev->speed;
pl->phy_state.duplex = phydev->duplex;
pl->phy_state.pause = MLO_PAUSE_NONE;
- if (phydev->pause)
- pl->phy_state.pause |= MLO_PAUSE_SYM;
- if (phydev->asym_pause)
- pl->phy_state.pause |= MLO_PAUSE_ASYM;
+ if (tx_pause)
+ pl->phy_state.pause |= MLO_PAUSE_TX;
+ if (rx_pause)
+ pl->phy_state.pause |= MLO_PAUSE_RX;
pl->phy_state.interface = phydev->interface;
pl->phy_state.link = up;
mutex_unlock(&pl->state_mutex);
--
2.20.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH net-next 07/10] net: phylink: resolve fixed link flow control
2020-02-15 15:48 [PATCH net-next 00/10] Pause updates for phylib and phylink Russell King - ARM Linux admin
` (4 preceding siblings ...)
2020-02-15 15:49 ` [PATCH net-next 06/10] net: phylink: use phylib resolved flow control modes Russell King
@ 2020-02-15 15:49 ` Russell King
2020-02-15 19:02 ` Andrew Lunn
2020-02-15 15:49 ` [PATCH net-next 08/10] net: phylink: allow ethtool -A to change flow control advertisement Russell King
` (5 subsequent siblings)
11 siblings, 1 reply; 24+ messages in thread
From: Russell King @ 2020-02-15 15:49 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, Heiner Kallweit; +Cc: David S. Miller, netdev
Resolve the fixed link flow control using the recently introduced
linkmode_resolve_pause() helper, which we use in
phylink_get_fixed_state() only when operating in full duplex mode.
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
drivers/net/phy/phylink.c | 70 +++++++++++++++++----------------------
include/linux/phylink.h | 8 ++---
2 files changed, 34 insertions(+), 44 deletions(-)
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index e65e9c9dc759..c29648b90ce7 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -181,9 +181,11 @@ static int phylink_parse_fixedlink(struct phylink *pl,
/* We treat the "pause" and "asym-pause" terminology as
* defining the link partner's ability. */
if (fwnode_property_read_bool(fixed_node, "pause"))
- pl->link_config.pause |= MLO_PAUSE_SYM;
+ __set_bit(ETHTOOL_LINK_MODE_Pause_BIT,
+ pl->link_config.lp_advertising);
if (fwnode_property_read_bool(fixed_node, "asym-pause"))
- pl->link_config.pause |= MLO_PAUSE_ASYM;
+ __set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
+ pl->link_config.lp_advertising);
if (ret == 0) {
desc = fwnode_gpiod_get_index(fixed_node, "link", 0,
@@ -215,9 +217,11 @@ static int phylink_parse_fixedlink(struct phylink *pl,
DUPLEX_FULL : DUPLEX_HALF;
pl->link_config.speed = prop[2];
if (prop[3])
- pl->link_config.pause |= MLO_PAUSE_SYM;
+ __set_bit(ETHTOOL_LINK_MODE_Pause_BIT,
+ pl->link_config.lp_advertising);
if (prop[4])
- pl->link_config.pause |= MLO_PAUSE_ASYM;
+ __set_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
+ pl->link_config.lp_advertising);
}
}
@@ -351,6 +355,22 @@ static void phylink_apply_manual_flow(struct phylink *pl,
state->pause = pl->link_config.pause;
}
+static void phylink_resolve_flow(struct phylink_link_state *state)
+{
+ bool tx_pause, rx_pause;
+
+ state->pause = MLO_PAUSE_NONE;
+ if (state->duplex == DUPLEX_FULL) {
+ linkmode_resolve_pause(state->advertising,
+ state->lp_advertising,
+ &tx_pause, &rx_pause);
+ if (tx_pause)
+ state->pause |= MLO_PAUSE_TX;
+ if (rx_pause)
+ state->pause |= MLO_PAUSE_RX;
+ }
+}
+
static void phylink_mac_config(struct phylink *pl,
const struct phylink_link_state *state)
{
@@ -399,44 +419,16 @@ static void phylink_mac_pcs_get_state(struct phylink *pl,
/* The fixed state is... fixed except for the link state,
* which may be determined by a GPIO or a callback.
*/
-static void phylink_get_fixed_state(struct phylink *pl, struct phylink_link_state *state)
+static void phylink_get_fixed_state(struct phylink *pl,
+ struct phylink_link_state *state)
{
*state = pl->link_config;
if (pl->get_fixed_state)
pl->get_fixed_state(pl->netdev, state);
else if (pl->link_gpio)
state->link = !!gpiod_get_value_cansleep(pl->link_gpio);
-}
-/* Flow control is resolved according to our and the link partners
- * advertisements using the following drawn from the 802.3 specs:
- * Local device Link partner
- * Pause AsymDir Pause AsymDir Result
- * 1 X 1 X TX+RX
- * 0 1 1 1 TX
- * 1 1 0 1 RX
- */
-static void phylink_resolve_flow(struct phylink *pl,
- struct phylink_link_state *state)
-{
- int new_pause = 0;
- int pause = 0;
-
- if (phylink_test(pl->link_config.advertising, Pause))
- pause |= MLO_PAUSE_SYM;
- if (phylink_test(pl->link_config.advertising, Asym_Pause))
- pause |= MLO_PAUSE_ASYM;
-
- pause &= state->pause;
-
- if (pause & MLO_PAUSE_SYM)
- new_pause = MLO_PAUSE_TX | MLO_PAUSE_RX;
- else if (pause & MLO_PAUSE_ASYM)
- new_pause = state->pause & MLO_PAUSE_SYM ?
- MLO_PAUSE_TX : MLO_PAUSE_RX;
-
- state->pause &= ~MLO_PAUSE_TXRX_MASK;
- state->pause |= new_pause;
+ phylink_resolve_flow(state);
}
static const char *phylink_pause_to_str(int pause)
@@ -1393,8 +1385,7 @@ int phylink_ethtool_set_pauseparam(struct phylink *pl,
!pause->autoneg && pause->rx_pause != pause->tx_pause)
return -EINVAL;
- config->pause &= ~(MLO_PAUSE_AN | MLO_PAUSE_TXRX_MASK);
-
+ config->pause = 0;
if (pause->autoneg)
config->pause |= MLO_PAUSE_AN;
if (pause->rx_pause)
@@ -1505,13 +1496,14 @@ static int phylink_mii_emul_read(unsigned int reg,
struct phylink_link_state *state)
{
struct fixed_phy_status fs;
+ unsigned long *lpa = state->lp_advertising;
int val;
fs.link = state->link;
fs.speed = state->speed;
fs.duplex = state->duplex;
- fs.pause = state->pause & MLO_PAUSE_SYM;
- fs.asym_pause = state->pause & MLO_PAUSE_ASYM;
+ fs.pause = test_bit(ETHTOOL_LINK_MODE_Pause_BIT, lpa);
+ fs.asym_pause = test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, lpa);
val = swphy_read_reg(reg, &fs);
if (reg == MII_BMSR) {
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 523209e70947..0d6073c2b2b7 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -12,12 +12,10 @@ struct net_device;
enum {
MLO_PAUSE_NONE,
- MLO_PAUSE_ASYM = BIT(0),
- MLO_PAUSE_SYM = BIT(1),
- MLO_PAUSE_RX = BIT(2),
- MLO_PAUSE_TX = BIT(3),
+ MLO_PAUSE_RX = BIT(0),
+ MLO_PAUSE_TX = BIT(1),
MLO_PAUSE_TXRX_MASK = MLO_PAUSE_TX | MLO_PAUSE_RX,
- MLO_PAUSE_AN = BIT(4),
+ MLO_PAUSE_AN = BIT(2),
MLO_AN_PHY = 0, /* Conventional PHY */
MLO_AN_FIXED, /* Fixed-link mode */
--
2.20.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH net-next 08/10] net: phylink: allow ethtool -A to change flow control advertisement
2020-02-15 15:48 [PATCH net-next 00/10] Pause updates for phylib and phylink Russell King - ARM Linux admin
` (5 preceding siblings ...)
2020-02-15 15:49 ` [PATCH net-next 07/10] net: phylink: resolve fixed link flow control Russell King
@ 2020-02-15 15:49 ` Russell King
2020-02-15 19:04 ` Andrew Lunn
2020-02-15 15:50 ` [PATCH net-next 09/10] net: phylink: improve initial mac configuration Russell King
` (4 subsequent siblings)
11 siblings, 1 reply; 24+ messages in thread
From: Russell King @ 2020-02-15 15:49 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, Heiner Kallweit; +Cc: David S. Miller, netdev
When ethtool -A is used to change the pause modes, the pause
advertisement is not being changed, but the documentation in
uapi/linux/ethtool.h says we should be. Add that capability to
phylink.
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
drivers/net/phy/phylink.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index c29648b90ce7..ab72bd1a7dca 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1385,6 +1385,7 @@ int phylink_ethtool_set_pauseparam(struct phylink *pl,
!pause->autoneg && pause->rx_pause != pause->tx_pause)
return -EINVAL;
+ mutex_lock(&pl->state_mutex);
config->pause = 0;
if (pause->autoneg)
config->pause |= MLO_PAUSE_AN;
@@ -1393,6 +1394,22 @@ int phylink_ethtool_set_pauseparam(struct phylink *pl,
if (pause->tx_pause)
config->pause |= MLO_PAUSE_TX;
+ /*
+ * 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.
+ */
+ linkmode_set_pause(config->advertising, pause->tx_pause,
+ pause->rx_pause);
+
/* 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
* configuration.
@@ -1405,6 +1422,7 @@ int phylink_ethtool_set_pauseparam(struct phylink *pl,
phylink_mac_config(pl, &pl->link_config);
phylink_mac_an_restart(pl);
}
+ mutex_unlock(&pl->state_mutex);
return 0;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH net-next 09/10] net: phylink: improve initial mac configuration
2020-02-15 15:48 [PATCH net-next 00/10] Pause updates for phylib and phylink Russell King - ARM Linux admin
` (6 preceding siblings ...)
2020-02-15 15:49 ` [PATCH net-next 08/10] net: phylink: allow ethtool -A to change flow control advertisement Russell King
@ 2020-02-15 15:50 ` Russell King
2020-02-15 19:06 ` Andrew Lunn
2020-02-15 15:50 ` [PATCH net-next 10/10] net: phylink: clarify flow control settings in documentation Russell King
` (3 subsequent siblings)
11 siblings, 1 reply; 24+ messages in thread
From: Russell King @ 2020-02-15 15:50 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, Heiner Kallweit; +Cc: David S. Miller, netdev
Improve the initial MAC configuration so we get a configuration which
more represents the final operating mode, in particular with respect
to the flow control settings.
We do this by:
1) more fully initialising our phy state, so we can use this as the
initial state for PHY based connections.
2) reading the fixed link state.
3) ensuring that in-band mode has sane pause settings for SGMII vs
802.3z negotiation modes.
In all three cases, we ensure that state->link is false, just in case
any MAC drivers have other ideas by mis-using this member, and we also
take account of manual pause mode configuration at this point.
This avoids MLO_PAUSE_AN being seen in mac_config() when operating in
PHY, fixed mode or inband SGMII mode, thereby giving cleaner semantics
to the pause flags. As a result of this, the pause flags now indicate
in a mode-independent way what is required from a mac_config()
implementation.
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
drivers/net/phy/phylink.c | 36 ++++++++++++++++++++++++++++++++++--
1 file changed, 34 insertions(+), 2 deletions(-)
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index ab72bd1a7dca..2899fbe699ab 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -431,6 +431,35 @@ static void phylink_get_fixed_state(struct phylink *pl,
phylink_resolve_flow(state);
}
+static void phylink_mac_initial_config(struct phylink *pl)
+{
+ struct phylink_link_state link_state;
+
+ switch (pl->cur_link_an_mode) {
+ case MLO_AN_PHY:
+ link_state = pl->phy_state;
+ break;
+
+ case MLO_AN_FIXED:
+ phylink_get_fixed_state(pl, &link_state);
+ break;
+
+ case MLO_AN_INBAND:
+ link_state = pl->link_config;
+ if (link_state.interface == PHY_INTERFACE_MODE_SGMII)
+ link_state.pause = MLO_PAUSE_NONE;
+ break;
+
+ default: /* can't happen */
+ return;
+ }
+
+ link_state.link = false;
+
+ phylink_apply_manual_flow(pl, &link_state);
+ phylink_mac_config(pl, &link_state);
+}
+
static const char *phylink_pause_to_str(int pause)
{
switch (pause & MLO_PAUSE_TXRX_MASK) {
@@ -779,6 +808,9 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy,
mutex_lock(&pl->state_mutex);
pl->phydev = phy;
pl->phy_state.interface = interface;
+ pl->phy_state.pause = MLO_PAUSE_NONE;
+ pl->phy_state.speed = SPEED_UNKNOWN;
+ pl->phy_state.duplex = DUPLEX_UNKNOWN;
linkmode_copy(pl->supported, supported);
linkmode_copy(pl->link_config.advertising, config.advertising);
@@ -1008,7 +1040,7 @@ void phylink_start(struct phylink *pl)
* a fixed-link to start with the correct parameters, and also
* ensures that we set the appropriate advertisement for Serdes links.
*/
- phylink_mac_config(pl, &pl->link_config);
+ phylink_mac_initial_config(pl);
/* Restart autonegotiation if using 802.3z to ensure that the link
* parameters are properly negotiated. This is necessary for DSA
@@ -1826,7 +1858,7 @@ static int phylink_sfp_config(struct phylink *pl, u8 mode,
if (changed && !test_bit(PHYLINK_DISABLE_STOPPED,
&pl->phylink_disable_state))
- phylink_mac_config(pl, &pl->link_config);
+ phylink_mac_initial_config(pl);
return ret;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH net-next 09/10] net: phylink: improve initial mac configuration
2020-02-15 15:50 ` [PATCH net-next 09/10] net: phylink: improve initial mac configuration Russell King
@ 2020-02-15 19:06 ` Andrew Lunn
0 siblings, 0 replies; 24+ messages in thread
From: Andrew Lunn @ 2020-02-15 19:06 UTC (permalink / raw)
To: Russell King; +Cc: Florian Fainelli, Heiner Kallweit, David S. Miller, netdev
On Sat, Feb 15, 2020 at 03:50:03PM +0000, Russell King wrote:
> Improve the initial MAC configuration so we get a configuration which
> more represents the final operating mode, in particular with respect
> to the flow control settings.
>
> We do this by:
> 1) more fully initialising our phy state, so we can use this as the
> initial state for PHY based connections.
> 2) reading the fixed link state.
> 3) ensuring that in-band mode has sane pause settings for SGMII vs
> 802.3z negotiation modes.
>
> In all three cases, we ensure that state->link is false, just in case
> any MAC drivers have other ideas by mis-using this member, and we also
> take account of manual pause mode configuration at this point.
>
> This avoids MLO_PAUSE_AN being seen in mac_config() when operating in
> PHY, fixed mode or inband SGMII mode, thereby giving cleaner semantics
> to the pause flags. As a result of this, the pause flags now indicate
> in a mode-independent way what is required from a mac_config()
> implementation.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH net-next 10/10] net: phylink: clarify flow control settings in documentation
2020-02-15 15:48 [PATCH net-next 00/10] Pause updates for phylib and phylink Russell King - ARM Linux admin
` (7 preceding siblings ...)
2020-02-15 15:50 ` [PATCH net-next 09/10] net: phylink: improve initial mac configuration Russell King
@ 2020-02-15 15:50 ` Russell King
2020-02-15 19:08 ` Andrew Lunn
2020-02-15 21:11 ` [PATCH net-next 00/10] Pause updates for phylib and phylink Florian Fainelli
` (2 subsequent siblings)
11 siblings, 1 reply; 24+ messages in thread
From: Russell King @ 2020-02-15 15:50 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, Heiner Kallweit; +Cc: netdev
Clarify the expected flow control settings operation in the phylink
documentation for each negotiation mode.
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
include/linux/phylink.h | 26 ++++++++++++++++++--------
1 file changed, 18 insertions(+), 8 deletions(-)
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 0d6073c2b2b7..812357c03df4 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -152,13 +152,20 @@ void mac_pcs_get_state(struct phylink_config *config,
* guaranteed to be correct, and so any mac_config() implementation must
* never reference these fields.
*
+ * In all negotiation modes, as defined by @mode, @state->pause indicates the
+ * pause settings which should be applied as follows. If %MLO_PAUSE_AN is not
+ * set, %MLO_PAUSE_TX and %MLO_PAUSE_RX indicate whether the MAC should send
+ * pause frames and/or act on received pause frames respectively. Otherwise,
+ * the results of in-band negotiation/status from the MAC PCS should be used
+ * to control the MAC pause mode settings.
+ *
* The action performed depends on the currently selected mode:
*
* %MLO_AN_FIXED, %MLO_AN_PHY:
- * Configure the specified @state->speed, @state->duplex and
- * @state->pause (%MLO_PAUSE_TX / %MLO_PAUSE_RX) modes over a link
- * specified by @state->interface. @state->advertising may be used,
- * but is not required. Other members of @state must be ignored.
+ * Configure the specified @state->speed and @state->duplex over a link
+ * specified by @state->interface. @state->advertising may be used, but
+ * is not required. Pause modes as above. Other members of @state must
+ * be ignored.
*
* Valid state members: interface, speed, duplex, pause, advertising.
*
@@ -170,11 +177,14 @@ void mac_pcs_get_state(struct phylink_config *config,
* mac_pcs_get_state() callback. Changes in link state must be made
* by calling phylink_mac_change().
*
+ * Interface mode specific details are mentioned below.
+ *
* If in 802.3z mode, the link speed is fixed, dependent on the
- * @state->interface. Duplex is negotiated, and pause is advertised
- * according to @state->an_enabled, @state->pause and
- * @state->advertising flags. Beware of MACs which only support full
- * duplex at gigabit and higher speeds.
+ * @state->interface. Duplex and pause modes are negotiated via
+ * the in-band configuration word. Advertised pause modes are set
+ * according to the @state->an_enabled and @state->advertising
+ * flags. Beware of MACs which only support full duplex at gigabit
+ * and higher speeds.
*
* If in Cisco SGMII mode, the link speed and duplex mode are passed
* in the serial bitstream 16-bit configuration word, and the MAC
--
2.20.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH net-next 00/10] Pause updates for phylib and phylink
2020-02-15 15:48 [PATCH net-next 00/10] Pause updates for phylib and phylink Russell King - ARM Linux admin
` (8 preceding siblings ...)
2020-02-15 15:50 ` [PATCH net-next 10/10] net: phylink: clarify flow control settings in documentation Russell King
@ 2020-02-15 21:11 ` Florian Fainelli
2020-02-16 0:00 ` Russell King - ARM Linux admin
2020-02-15 23:57 ` [PATCH net-next 01/10] net: linkmode: make linkmode_test_bit() take const pointer Russell King
2020-02-17 3:40 ` [PATCH net-next 00/10] Pause updates for phylib and phylink David Miller
11 siblings, 1 reply; 24+ messages in thread
From: Florian Fainelli @ 2020-02-15 21:11 UTC (permalink / raw)
To: Russell King - ARM Linux admin, Andrew Lunn, Heiner Kallweit
Cc: David S. Miller, netdev
On 2/15/2020 7:48 AM, Russell King - ARM Linux admin wrote:
> Currently, phylib resolves the speed and duplex settings, which MAC
> drivers use directly. phylib also extracts the "Pause" and "AsymPause"
> bits from the link partner's advertisement, and stores them in struct
> phy_device's pause and asym_pause members with no further processing.
> It is left up to each MAC driver to implement decoding for this
> information.
>
> phylink converted drivers are able to take advantage of code therein
> which resolves the pause advertisements for the MAC driver, but this
> does nothing for unconverted drivers. It also does not allow us to
> make use of hardware-resolved pause states offered by several PHYs.
>
> This series aims to address this by:
>
> 1. Providing a generic implementation, linkmode_resolve_pause(), that
> takes the ethtool linkmode arrays for the link partner and local
> advertisements, decoding the result to whether pause frames should
> be allowed to be transmitted or received and acted upon. I call
> this the pause enablement state.
>
> 2. Providing a phylib implementation, phy_get_pause(), which allows
> MAC drivers to request the pause enablement state from phylib.
>
> 3. Providing a generic linkmode_set_pause() for setting the pause
> advertisement according to the ethtool tx/rx flags - note that this
> design has some shortcomings, see the comments in the kerneldoc for
> this function.
>
> 4. Remove the ability in phylink to set the pause states for fixed
> links, which brings them into line with how we deal with the speed
> and duplex parameters; we can reintroduce this later if anyone
> requires it. This could be a userspace-visible change.
>
> 5. Split application of manual pause enablement state from phylink's
> resolution of the same to allow use of phylib's new phy_get_pause()
> interface by phylink, and make that switch.
>
> 6. Resolve the fixed-link pause enablement state using the generic
> linkmode_resolve_pause() helper introduced earlier. This, in
> connection with the previous commits, allows us to kill the
> MLO_PAUSE_SYM and MLO_PAUSE_ASYM flags.
>
> 7. make phylink's ethtool pause setting implementation update the
> pause advertisement in the same way that phylib does, with the
> same caveats that are present there (as mentioned above w.r.t
> linkmode_set_pause()).
>
> 8. create a more accurate initial configuration for MACs, used when
> phy_start() is called or a SFP is detected. In particular, this
> ensures that the pause bits seen by MAC drivers in state->pause
> are accurate for SGMII.
>
> 9. finally, update the kerneldoc descriptions for mac_config() for
> the above changes.
>
> This series has been build-tested against net-next; the boot tested
> patches are in my "phy" branch against v5.5 plus the queued phylink
> changes that were merged for 5.6.
>
> The next series will introduce the ability for phylib drivers to
> provide hardware resolved pause enablement state. These patches can
> be found in my "phy" branch.
I do not think that patch #1 made it to the mailing-list though, so if
nothing else you may want to resend it:
http://patchwork.ozlabs.org/project/netdev/list/?series=158739
>
> drivers/net/phy/Makefile | 3 +-
> drivers/net/phy/linkmode.c | 95 +++++++++++++++++++++++++
> drivers/net/phy/phy_device.c | 43 +++++++-----
> drivers/net/phy/phylink.c | 162 +++++++++++++++++++++++++++----------------
> include/linux/linkmode.h | 8 ++-
> include/linux/phy.h | 3 +
> include/linux/phylink.h | 34 +++++----
> 7 files changed, 258 insertions(+), 90 deletions(-)
> create mode 100644 drivers/net/phy/linkmode.c
>
--
Florian
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH net-next 00/10] Pause updates for phylib and phylink
2020-02-15 21:11 ` [PATCH net-next 00/10] Pause updates for phylib and phylink Florian Fainelli
@ 2020-02-16 0:00 ` Russell King - ARM Linux admin
0 siblings, 0 replies; 24+ messages in thread
From: Russell King - ARM Linux admin @ 2020-02-16 0:00 UTC (permalink / raw)
To: Florian Fainelli; +Cc: Andrew Lunn, Heiner Kallweit, David S. Miller, netdev
On Sat, Feb 15, 2020 at 01:11:16PM -0800, Florian Fainelli wrote:
>
>
> On 2/15/2020 7:48 AM, Russell King - ARM Linux admin wrote:
> > Currently, phylib resolves the speed and duplex settings, which MAC
> > drivers use directly. phylib also extracts the "Pause" and "AsymPause"
> > bits from the link partner's advertisement, and stores them in struct
> > phy_device's pause and asym_pause members with no further processing.
> > It is left up to each MAC driver to implement decoding for this
> > information.
> >
> > phylink converted drivers are able to take advantage of code therein
> > which resolves the pause advertisements for the MAC driver, but this
> > does nothing for unconverted drivers. It also does not allow us to
> > make use of hardware-resolved pause states offered by several PHYs.
> >
> > This series aims to address this by:
> >
> > 1. Providing a generic implementation, linkmode_resolve_pause(), that
> > takes the ethtool linkmode arrays for the link partner and local
> > advertisements, decoding the result to whether pause frames should
> > be allowed to be transmitted or received and acted upon. I call
> > this the pause enablement state.
> >
> > 2. Providing a phylib implementation, phy_get_pause(), which allows
> > MAC drivers to request the pause enablement state from phylib.
> >
> > 3. Providing a generic linkmode_set_pause() for setting the pause
> > advertisement according to the ethtool tx/rx flags - note that this
> > design has some shortcomings, see the comments in the kerneldoc for
> > this function.
> >
> > 4. Remove the ability in phylink to set the pause states for fixed
> > links, which brings them into line with how we deal with the speed
> > and duplex parameters; we can reintroduce this later if anyone
> > requires it. This could be a userspace-visible change.
> >
> > 5. Split application of manual pause enablement state from phylink's
> > resolution of the same to allow use of phylib's new phy_get_pause()
> > interface by phylink, and make that switch.
> >
> > 6. Resolve the fixed-link pause enablement state using the generic
> > linkmode_resolve_pause() helper introduced earlier. This, in
> > connection with the previous commits, allows us to kill the
> > MLO_PAUSE_SYM and MLO_PAUSE_ASYM flags.
> >
> > 7. make phylink's ethtool pause setting implementation update the
> > pause advertisement in the same way that phylib does, with the
> > same caveats that are present there (as mentioned above w.r.t
> > linkmode_set_pause()).
> >
> > 8. create a more accurate initial configuration for MACs, used when
> > phy_start() is called or a SFP is detected. In particular, this
> > ensures that the pause bits seen by MAC drivers in state->pause
> > are accurate for SGMII.
> >
> > 9. finally, update the kerneldoc descriptions for mac_config() for
> > the above changes.
> >
> > This series has been build-tested against net-next; the boot tested
> > patches are in my "phy" branch against v5.5 plus the queued phylink
> > changes that were merged for 5.6.
> >
> > The next series will introduce the ability for phylib drivers to
> > provide hardware resolved pause enablement state. These patches can
> > be found in my "phy" branch.
>
> I do not think that patch #1 made it to the mailing-list though, so if
> nothing else you may want to resend it:
>
> http://patchwork.ozlabs.org/project/netdev/list/?series=158739
You're right, it was missing the Cc line. Resent just that - which
makes the series complete in patchwork, although not sure if the
order is correct. Does it sort a patch series by sent date?
>
> >
> > drivers/net/phy/Makefile | 3 +-
> > drivers/net/phy/linkmode.c | 95 +++++++++++++++++++++++++
> > drivers/net/phy/phy_device.c | 43 +++++++-----
> > drivers/net/phy/phylink.c | 162 +++++++++++++++++++++++++++----------------
> > include/linux/linkmode.h | 8 ++-
> > include/linux/phy.h | 3 +
> > include/linux/phylink.h | 34 +++++----
> > 7 files changed, 258 insertions(+), 90 deletions(-)
> > create mode 100644 drivers/net/phy/linkmode.c
> >
>
> --
> Florian
>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH net-next 01/10] net: linkmode: make linkmode_test_bit() take const pointer
2020-02-15 15:48 [PATCH net-next 00/10] Pause updates for phylib and phylink Russell King - ARM Linux admin
` (9 preceding siblings ...)
2020-02-15 21:11 ` [PATCH net-next 00/10] Pause updates for phylib and phylink Florian Fainelli
@ 2020-02-15 23:57 ` Russell King
2020-02-17 3:40 ` [PATCH net-next 00/10] Pause updates for phylib and phylink David Miller
11 siblings, 0 replies; 24+ messages in thread
From: Russell King @ 2020-02-15 23:57 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, Heiner Kallweit; +Cc: David S. Miller, netdev
linkmode_test_bit() does not modify the address; test_bit() is also
declared const volatile for the same reason. There's no need for
linkmode_test_bit() to be any different, and allows implementation of
helpers that take a const linkmode pointer.
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
include/linux/linkmode.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/linkmode.h b/include/linux/linkmode.h
index fe740031339d..8e5b352e44f2 100644
--- a/include/linux/linkmode.h
+++ b/include/linux/linkmode.h
@@ -71,7 +71,7 @@ static inline void linkmode_change_bit(int nr, volatile unsigned long *addr)
__change_bit(nr, addr);
}
-static inline int linkmode_test_bit(int nr, volatile unsigned long *addr)
+static inline int linkmode_test_bit(int nr, const volatile unsigned long *addr)
{
return test_bit(nr, addr);
}
--
2.20.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH net-next 00/10] Pause updates for phylib and phylink
2020-02-15 15:48 [PATCH net-next 00/10] Pause updates for phylib and phylink Russell King - ARM Linux admin
` (10 preceding siblings ...)
2020-02-15 23:57 ` [PATCH net-next 01/10] net: linkmode: make linkmode_test_bit() take const pointer Russell King
@ 2020-02-17 3:40 ` David Miller
11 siblings, 0 replies; 24+ messages in thread
From: David Miller @ 2020-02-17 3:40 UTC (permalink / raw)
To: linux; +Cc: andrew, f.fainelli, hkallweit1, netdev
From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
Date: Sat, 15 Feb 2020 15:48:39 +0000
> Currently, phylib resolves the speed and duplex settings, which MAC
> drivers use directly. phylib also extracts the "Pause" and "AsymPause"
> bits from the link partner's advertisement, and stores them in struct
> phy_device's pause and asym_pause members with no further processing.
> It is left up to each MAC driver to implement decoding for this
> information.
>
> phylink converted drivers are able to take advantage of code therein
> which resolves the pause advertisements for the MAC driver, but this
> does nothing for unconverted drivers. It also does not allow us to
> make use of hardware-resolved pause states offered by several PHYs.
>
> This series aims to address this by:
...
> This series has been build-tested against net-next; the boot tested
> patches are in my "phy" branch against v5.5 plus the queued phylink
> changes that were merged for 5.6.
>
> The next series will introduce the ability for phylib drivers to
> provide hardware resolved pause enablement state. These patches can
> be found in my "phy" branch.
Series applied to net-next, thank you.
^ permalink raw reply [flat|nested] 24+ messages in thread