linux-kernel - Re: [PATCH net 2/2] net: phy: aquantia: fix applying active

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: <Zume2obQ6b28QMDj@makrotopia.org>
Date: Tue, 17 Sep 2024 16:23:06 +0100
From: Daniel Golle <daniel@...rotopia.org>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
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>,
	Christian Marangi <ansuelsmth@...il.com>,
	Bartosz Golaszewski <bartosz.golaszewski@...aro.org>,
	Robert Marko <robimarko@...il.com>, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net 2/2] net: phy: aquantia: fix applying active_low bit
 after reset

Hi Russell,

On Tue, Sep 17, 2024 at 03:41:11PM +0100, Russell King (Oracle) wrote:
> On Tue, Sep 17, 2024 at 02:49:55PM +0100, Daniel Golle wrote:
> > for_each_set_bit was used wrongly in aqr107_config_init() when iterating
> > over LEDs. Drop misleading 'index' variable and call
> > aqr_phy_led_active_low_set() for each set bit representing an LED which
> > is driven by VDD instead of GND pin.
> 
> Assuming that the intention is only to set LEDs active-low that were
> previously configured to be active-low, then:

Exactly. That was supposedly also the original intention.

> 
> Reviewed-by: Russell King (Oracle) <rmk+kernel@...linux.org.uk>
> 
> It's good that we don't call aqr_phy_led_active_low_set() for every LED
> in the .config_init method because we don't know whether the LED
> outputs for this PHY are used on SFPs to drive e.g. the SFP LOS pin,
> and changing LED settings in such a case could cause incorrect
> signalling. If this ever changes, then this code needs to be
> conditional on !phy_on_sfp(phydev).

Ack. The post-reset default is active-high and the only case we need to
cover here are LEDs for which VDD is driven instead of GND, so
supposedly if any of those signals are used as LOS in a SFP module it
would be wired in a way which uses the post-reset default of the PHY
anyway which were aren't changing in this case.

Thank you for the quick review!


Daniel