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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250306092857.5cafd6d4@fedora.home>
Date: Thu, 6 Mar 2025 09:28:57 +0100
From: Maxime Chevallier <maxime.chevallier@...tlin.com>
To: "Russell King (Oracle)" <rmk+kernel@...linux.org.uk>
Cc: Jon Hunter <jonathanh@...dia.com>, Thierry Reding <treding@...dia.com>,
 "Lad,  Prabhakar" <prabhakar.csengg@...il.com>, Alexandre Torgue
 <alexandre.torgue@...s.st.com>, Andrew Lunn <andrew@...n.ch>, Andrew Lunn
 <andrew+netdev@...n.ch>, "David S. Miller" <davem@...emloft.net>, Eric
 Dumazet <edumazet@...gle.com>, Heiner Kallweit <hkallweit1@...il.com>,
 Jakub Kicinski <kuba@...nel.org>, linux-arm-kernel@...ts.infradead.org,
 linux-stm32@...md-mailman.stormreply.com, Maxime Coquelin
 <mcoquelin.stm32@...il.com>, netdev@...r.kernel.org, Paolo Abeni
 <pabeni@...hat.com>
Subject: Re: [PATCH net-next 1/2] net: phylink: add functions to
 block/unblock rx clock stop

Hello Russell,

On Wed, 05 Mar 2025 18:00:39 +0000
"Russell King (Oracle)" <rmk+kernel@...linux.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@...linux.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