[PATCH net] net: phylink: mac_link_(up|down)() clarifications

[PATCH net] net: phylink: mac_link_(up|down)() clarifications

[Russell King (Oracle)]

As a result of an email from the fbnic author, I reviewed the phylink
documentation, and I have decided to clarify the wording in the
mac_link_(up|down)() kernel documentation as this was written from the
point of view of mvneta/mvpp2 and is misleading.

The documentation talks about forcing the link - indeed, this is what
is done in the mvneta and mvpp2 drivers but not at the physical layer
but the MACs idea, which has the effect of only allowing or stopping
packet flow at the MAC. This "link" needs to be controlled when using
a PHY or fixed link to start or stop packet flow at the MAC. However,
as the MAC and PCS are tightly integrated, if the MACs idea of the
link is forced down, it has the side effect that there is no way to
determine that the media link has come up - in this mode, the MAC must
be allowed to follow its built-in PCS so we can read the link state.

Frame the documentation in more generic terms, to avoid the thought
that the physical media link to the partner needs in some way to be
forced up or down with these calls; it does not. If that were to be
done, it would be a self-fulfilling prophecy - e.g. if the media link
goes down, then mac_link_down() will be called, and if the media link
is then placed into a forced down state, there is no possibility
that the media link will ever come up again - clearly this is a wrong
interpretation.

These methods are notifications to the MAC about what has happened to
the media link state - either from the PHY, or a PCS, or whatever
mechanism fixed-link is using. Thus, reword them to get away from
talking about changing link state to avoid confusion with media link
state.

This is not a change of any requirements of these methods.

Also, remove the obsolete references to EEE for these methods, we now
have the LPI functions for configuring the EEE parameters which
renders this redundant, and also makes the passing of "phy" to the
mac_link_up() function obsolete.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 include/linux/phylink.h | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index 1f5773ab5660..30659b615fca 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -361,23 +361,29 @@ int mac_finish(struct phylink_config *config, unsigned int mode,
 		phy_interface_t iface);
 
 /**
- * mac_link_down() - take the link down
+ * mac_link_down() - notification that the link has gone down
  * @config: a pointer to a &struct phylink_config.
  * @mode: link autonegotiation mode
  * @interface: link &typedef phy_interface_t mode
  *
- * If @mode is not an in-band negotiation mode (as defined by
- * phylink_autoneg_inband()), force the link down and disable any
- * Energy Efficient Ethernet MAC configuration. Interface type
- * selection must be done in mac_config().
+ * Notifies the MAC that the link has gone down. This will not be called
+ * unless mac_link_up() has been previously called.
+ *
+ * The MAC should stop processing packets for transmission and reception.
+ * phylink will have called netif_carrier_off() to notify the networking
+ * stack that the link has gone down, so MAC drivers should not make this
+ * call.
+ *
+ * If @mode is %MLO_AN_INBAND, then this function must not prevent the
+ * link coming up.
  */
 void mac_link_down(struct phylink_config *config, unsigned int mode,
 		   phy_interface_t interface);
 
 /**
- * mac_link_up() - allow the link to come up
+ * mac_link_up() - notification that the link has come up
  * @config: a pointer to a &struct phylink_config.
- * @phy: any attached phy
+ * @phy: any attached phy (deprecated - please use LPI interfaces)
  * @mode: link autonegotiation mode
  * @interface: link &typedef phy_interface_t mode
  * @speed: link speed
@@ -385,7 +391,10 @@ void mac_link_down(struct phylink_config *config, unsigned int mode,
  * @tx_pause: link transmit pause enablement status
  * @rx_pause: link receive pause enablement status
  *
- * Configure the MAC for an established link.
+ * Notifies the MAC that the link has come up, and the parameters of the
+ * link as seen from the MACs point of view. If mac_link_up() has been
+ * called previously, there will be an intervening call to mac_link_down()
+ * before this method will be subsequently called.
  *
  * @speed, @duplex, @tx_pause and @rx_pause indicate the finalised link
  * settings, and should be used to configure the MAC block appropriately
@@ -397,9 +406,9 @@ void mac_link_down(struct phylink_config *config, unsigned int mode,
  * that the user wishes to override the pause settings, and this should
  * be allowed when considering the implementation of this method.
  *
- * If in-band negotiation mode is disabled, allow the link to come up. If
- * @phy is non-%NULL, configure Energy Efficient Ethernet by calling
- * phy_init_eee() and perform appropriate MAC configuration for EEE.
+ * Once configured, the MAC may begin to process packets for transmission
+ * and reception.
+ *
  * Interface type selection must be done in mac_config().
  */
 void mac_link_up(struct phylink_config *config, struct phy_device *phy,
-- 
2.30.2

Re: [PATCH net] net: phylink: mac_link_(up|down)() clarifications

[Maxime Chevallier]

Hello Russell,

On Wed, 16 Apr 2025 22:53:19 +0100
"Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> wrote:

> As a result of an email from the fbnic author, I reviewed the phylink
> documentation, and I have decided to clarify the wording in the
> mac_link_(up|down)() kernel documentation as this was written from the
> point of view of mvneta/mvpp2 and is misleading.
> 
> The documentation talks about forcing the link - indeed, this is what
> is done in the mvneta and mvpp2 drivers but not at the physical layer
> but the MACs idea, which has the effect of only allowing or stopping
> packet flow at the MAC. This "link" needs to be controlled when using
> a PHY or fixed link to start or stop packet flow at the MAC. However,
> as the MAC and PCS are tightly integrated, if the MACs idea of the
> link is forced down, it has the side effect that there is no way to
> determine that the media link has come up - in this mode, the MAC must
> be allowed to follow its built-in PCS so we can read the link state.
> 
> Frame the documentation in more generic terms, to avoid the thought
> that the physical media link to the partner needs in some way to be
> forced up or down with these calls; it does not. If that were to be
> done, it would be a self-fulfilling prophecy - e.g. if the media link
> goes down, then mac_link_down() will be called, and if the media link
> is then placed into a forced down state, there is no possibility
> that the media link will ever come up again - clearly this is a wrong
> interpretation.
> 
> These methods are notifications to the MAC about what has happened to
> the media link state - either from the PHY, or a PCS, or whatever
> mechanism fixed-link is using. Thus, reword them to get away from
> talking about changing link state to avoid confusion with media link
> state.
> 
> This is not a change of any requirements of these methods.
> 
> Also, remove the obsolete references to EEE for these methods, we now
> have the LPI functions for configuring the EEE parameters which
> renders this redundant, and also makes the passing of "phy" to the
> mac_link_up() function obsolete.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Thanks for that,

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime

Re: [PATCH net] net: phylink: mac_link_(up|down)() clarifications

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 16 Apr 2025 22:53:19 +0100 you wrote:
> As a result of an email from the fbnic author, I reviewed the phylink
> documentation, and I have decided to clarify the wording in the
> mac_link_(up|down)() kernel documentation as this was written from the
> point of view of mvneta/mvpp2 and is misleading.
> 
> The documentation talks about forcing the link - indeed, this is what
> is done in the mvneta and mvpp2 drivers but not at the physical layer
> but the MACs idea, which has the effect of only allowing or stopping
> packet flow at the MAC. This "link" needs to be controlled when using
> a PHY or fixed link to start or stop packet flow at the MAC. However,
> as the MAC and PCS are tightly integrated, if the MACs idea of the
> link is forced down, it has the side effect that there is no way to
> determine that the media link has come up - in this mode, the MAC must
> be allowed to follow its built-in PCS so we can read the link state.
> 
> [...]

Here is the summary with links:
  - [net] net: phylink: mac_link_(up|down)() clarifications
    https://git.kernel.org/netdev/net/c/ce6815585d46

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html