netdev - Re: [PATCH 2/2] net: phy: marvell-88q2xxx: Prevent reading temperature with asserted reset

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: <48c4cd14-be56-438e-9561-c85b0245178c@lunn.ch>
Date: Wed, 19 Feb 2025 14:21:23 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Dimitri Fedrau <dima.fedrau@...il.com>
Cc: Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Niklas Söderlund <niklas.soderlund+renesas@...natech.se>,
	Gregor Herburger <gregor.herburger@...tq-group.com>,
	Stefan Eichenberger <eichest@...il.com>,
	Geert Uytterhoeven <geert@...ux-m68k.org>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] net: phy: marvell-88q2xxx: Prevent reading
 temperature with asserted reset

On Tue, Feb 18, 2025 at 07:33:10PM +0100, Dimitri Fedrau wrote:
> If the PHYs reset is asserted it returns 0xffff for any read operation.
> Prevent reading the temperature in this case and return with an I/O error.
> Write operations are ignored by the device.

I think the commit message could be improved. Explain why the PHY
reset would be asserted. You are saying it is because the interface is
admin down. That is a concept the user is more likely to understand.

> Fixes: a197004cf3c2 ("net: phy: marvell-88q2xxx: Fix temperature measurement with reset-gpios")

Is this really a fix? My personal reason for this change was
architecture, it seemed odd to probe the hwmon device in one spot and
then enable it later. But is it really broken? Stable rules say:

  It must either fix a real bug that bothers people or just add a device ID

> Signed-off-by: Dimitri Fedrau <dima.fedrau@...il.com>
> ---
>  drivers/net/phy/marvell-88q2xxx.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
> index 30d71bfc365597d77c34c48f05390db9d63c4af4..c1ae27057ee34feacb31c2e3c40b2b1769596408 100644
> --- a/drivers/net/phy/marvell-88q2xxx.c
> +++ b/drivers/net/phy/marvell-88q2xxx.c
> @@ -647,6 +647,12 @@ static int mv88q2xxx_hwmon_read(struct device *dev,
>  	struct phy_device *phydev = dev_get_drvdata(dev);
>  	int ret;
>  
> +	/* If the PHYs reset is asserted it returns 0xffff for any read
> +	 * operation. Return with an I/O error in this case.
> +	 */
> +	if (phydev->mdio.reset_state == 1)
> +		return -EIO;

Maybe ENETDOWN is better?

	Andrew