linux-kernel - Re: [PATCH net v3 2/3] net: phy: allow drivers to disable polling via get_next_update

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250712131848.GA721198@horms.kernel.org>
Date: Sat, 12 Jul 2025 14:18:48 +0100
From: Simon Horman <horms@...nel.org>
To: Oleksij Rempel <o.rempel@...gutronix.de>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	kernel@...gutronix.de, linux-kernel@...r.kernel.org,
	Russell King <linux@...linux.org.uk>, netdev@...r.kernel.org,
	Andre Edich <andre.edich@...rochip.com>,
	Lukas Wunner <lukas@...ner.de>
Subject: Re: [PATCH net v3 2/3] net: phy: allow drivers to disable polling
 via get_next_update_time()

On Fri, Jul 11, 2025 at 11:49:08AM +0200, Oleksij Rempel wrote:

...

> @@ -1575,16 +1573,31 @@ static enum phy_state_work _phy_state_machine(struct phy_device *phydev)
>  	phy_process_state_change(phydev, old_state);
>  
>  	/* Only re-schedule a PHY state machine change if we are polling the
> -	 * PHY, if PHY_MAC_INTERRUPT is set, then we will be moving
> -	 * between states from phy_mac_interrupt().
> +	 * PHY. If PHY_MAC_INTERRUPT is set or get_next_update_time() returns
> +	 * PHY_STATE_IRQ, then we rely on interrupts for state changes.
>  	 *
>  	 * In state PHY_HALTED the PHY gets suspended, so rescheduling the
>  	 * state machine would be pointless and possibly error prone when
>  	 * called from phy_disconnect() synchronously.
>  	 */
> -	if (phy_polling_mode(phydev) && phy_is_started(phydev))
> -		phy_queue_state_machine(phydev,
> -					phy_get_next_update_time(phydev));
> +	if (phy_polling_mode(phydev) && phy_is_started(phydev)) {
> +		unsigned int next_time = phy_get_next_update_time(phydev);
> +
> +		if (next_time == PHY_STATE_IRQ) {
> +			/* A driver requesting IRQ mode while also needing
> +			 * polling for stats has a conflicting configuration.
> +			 * Warn about this buggy driver and fall back to
> +			 * polling to ensure stats are updated.
> +			 */
> +			if (phydev->drv->update_stats) {
> +				WARN_ONCE("phy: %s: driver requested IRQ mode but needs polling for stats\n",
> +					  phydev_name(phydev));

The first argument to WARN_ONCE() should be a condition, not a format string.

Flagged by GCC and Clang builds with KCFLAGS=-Wformat-security

> +				phy_queue_state_machine(phydev, PHY_STATE_TIME);
> +			}
> +		} else {
> +			phy_queue_state_machine(phydev, next_time);
> +		}
> +	}
>  
>  	return state_work;
>  }

...

-- 
pw-bot: changes-requested