Re: [PATCH net-next 1/2] net: phylink: add functions to block/unblock rx clock stop

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hello Russell,

On Wed, 05 Mar 2025 18:00:39 +0000
"Russell King (Oracle)" <rmk+kernel@armlinux.org.uk> wrote:

> Some MACs require the PHY receive clock to be running to complete setup
> actions. This may fail if the PHY has negotiated EEE, the MAC supports
> receive clock stop, and the link has entered LPI state. Provide a pair
> of APIs that MAC drivers can use to temporarily block the PHY disabling
> the receive clock.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

I only have comments on the implementation, see below :)

> ---
>  drivers/net/phy/phylink.c | 50 +++++++++++++++++++++++++++++++++++++++
>  include/linux/phylink.h   |  3 +++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index a3b186ab3854..8f93b597d019 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -88,6 +88,7 @@ struct phylink {
>  	bool mac_enable_tx_lpi;
>  	bool mac_tx_clk_stop;
>  	u32 mac_tx_lpi_timer;
> +	u8 mac_rx_clk_stop_blocked;
>  
>  	struct sfp_bus *sfp_bus;
>  	bool sfp_may_have_phy;
> @@ -2592,6 +2593,55 @@ void phylink_stop(struct phylink *pl)
>  }
>  EXPORT_SYMBOL_GPL(phylink_stop);
>  
> +
> +void phylink_rx_clk_stop_block(struct phylink *pl)
> +{
> +	ASSERT_RTNL();
> +
> +	if (pl->mac_rx_clk_stop_blocked == U8_MAX) {
> +		phylink_warn(pl, "%s called too many times - ignoring\n",
> +			     __func__);
> +		dump_stack();
> +		return;
> +	}
> +
> +	/* Disable PHY receive clock stop if this is the first time this
> +	 * function has been called and clock-stop was previously enabled.
> +	 */
> +	if (pl->mac_rx_clk_stop_blocked++ == 0 &&
> +	    pl->mac_supports_eee_ops && pl->phydev)
> +	    pl->config->eee_rx_clk_stop_enable)

Looks like there's an extra closing ')' here

> +		phy_eee_rx_clock_stop(pl->phydev, false);
> +}

Do you need an EXPORT_SYMBOL_GPL here as this will be used by MAC
drivers?

> +
> +/**
> + * phylink_rx_clk_stop_unblock() - unblock PHY ability to stop receive clock
> + * @pl: a pointer to a &struct phylink returned from phylink_create()
> + *
> + * All calls to phylink_rx_clk_stop_block() must be balanced with a
> + * corresponding call to phylink_rx_clk_stop_unblock() to restore the PHYs
> + * clock stop ability.
> + */
> +void phylink_rx_clk_stop_unblock(struct phylink *pl)
> +{
> +	ASSERT_RTNL();
> +
> +	if (pl->mac_rx_clk_stop_blocked == 0) {
> +		phylink_warn(pl, "%s called too many times - ignoring\n",
> +			     __func__);
> +		dump_stack();
> +		return;
> +	}
> +
> +	/* Re-enable PHY receive clock stop if the number of unblocks matches
> +	 * the number of calls to the block function above.
> +	 */
> +	if (--pl->mac_rx_clk_stop_blocked == 0 &&
> +	    pl->mac_supports_eee_ops && pl->phydev &&
> +	    pl->config->eee_rx_clk_stop_enable)
> +		phy_eee_rx_clock_stop(pl->phydev, true);
> +}

Same for the EXPORT_SYMBOL_GPL

Thanks,

Maxime