netdev - RE: [Intel-wired-lan] [PATCH v1 iwl-net] igc: power up PHY before link test

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: <IA3PR11MB8986CD55611A08A9AB8B6DF1E507A@IA3PR11MB8986.namprd11.prod.outlook.com>
Date: Mon, 1 Sep 2025 07:03:34 +0000
From: "Loktionov, Aleksandr" <aleksandr.loktionov@...el.com>
To: Kohei Enju <enjuk@...zon.com>, "intel-wired-lan@...ts.osuosl.org"
	<intel-wired-lan@...ts.osuosl.org>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>
CC: "Nguyen, Anthony L" <anthony.l.nguyen@...el.com>, "Kitszel, Przemyslaw"
	<przemyslaw.kitszel@...el.com>, Andrew Lunn <andrew+netdev@...n.ch>, "David
 S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, "Jakub
 Kicinski" <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, "Lifshits,
 Vitaly" <vitaly.lifshits@...el.com>, "kohei.enju@...il.com"
	<kohei.enju@...il.com>
Subject: RE: [Intel-wired-lan] [PATCH v1 iwl-net] igc: power up PHY before
 link test



> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@...osl.org> On Behalf
> Of Kohei Enju
> Sent: Saturday, August 30, 2025 7:06 PM
> To: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org
> Cc: Nguyen, Anthony L <anthony.l.nguyen@...el.com>; Kitszel,
> Przemyslaw <przemyslaw.kitszel@...el.com>; Andrew Lunn
> <andrew+netdev@...n.ch>; David S. Miller <davem@...emloft.net>; Eric
> Dumazet <edumazet@...gle.com>; Jakub Kicinski <kuba@...nel.org>; Paolo
> Abeni <pabeni@...hat.com>; Lifshits, Vitaly
> <vitaly.lifshits@...el.com>; kohei.enju@...il.com; Kohei Enju
> <enjuk@...zon.com>
> Subject: [Intel-wired-lan] [PATCH v1 iwl-net] igc: power up PHY before
> link test
> 
> The current implementation of igc driver doesn't power up PHY before
> link test in igc_ethtool_diag_test(), causing the link test to always
> report FAIL when admin state is down and PHY is consequently powered
> down.
> 
> To test the link state regardless of admin state, let's power up PHY
> in
> case of PHY down before link test.
> 
> Tested on Intel Corporation Ethernet Controller I226-V (rev 04) with
> cable connected and link available.
> 
> Set device down and do ethtool test.
>   # ip link set dev enp0s5 down
> 
> Without patch:
>   # ethtool --test enp0s5
>   The test result is FAIL
>   The test extra info:
>   Register test  (offline)         0
>   Eeprom test    (offline)         0
>   Interrupt test (offline)         0
>   Loopback test  (offline)         0
>   Link test   (on/offline)         1
> 
> With patch:
>   # ethtool --test enp0s5
>   The test result is PASS
>   The test extra info:
>   Register test  (offline)         0
>   Eeprom test    (offline)         0
>   Interrupt test (offline)         0
>   Loopback test  (offline)         0
>   Link test   (on/offline)         0
> 
> Fixes: f026d8ca2904 ("igc: add support to eeprom, registers and link
> self-tests")
> Signed-off-by: Kohei Enju <enjuk@...zon.com>
> ---
> This patch uses igc_power_up_phy_copper() instead of
> igc_power_up_link()
> to avoid PHY reset. The function only clears MII_CR_POWER_DOWN bit
> without performing PHY reset, so it should not cause the autoneg
> interference issue explained in the following comment:
>     /* Link test performed before hardware reset so autoneg doesn't
>      * interfere with test result
>      */
> ---
>  drivers/net/ethernet/intel/igc/igc_ethtool.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c
> b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> index f3e7218ba6f3..ca93629b1d3a 100644
> --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
> +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> @@ -2094,6 +2094,9 @@ static void igc_ethtool_diag_test(struct
> net_device *netdev,
>  		netdev_info(adapter->netdev, "Offline testing
> starting");
>  		set_bit(__IGC_TESTING, &adapter->state);
> 
> +		/* power up PHY for link test */
> +		igc_power_up_phy_copper(&adapter->hw);
> +
1.You unconditionally power the PHY up but you don't restore the previous power state at the end of the test.
2. igc_power_up_phy_copper() returns a status, but you ignore it. If the MDIO access fails (e.g., bus issue), you should mark the link test as failed and continue with the rest...
n. igc is predominantly copper, but it's still best practice to guard with: 	if (hw->phy.media_type == igc_media_type_copper) or or check that hw->phy.type != igc_phy_none

>  		/* Link test performed before hardware reset so autoneg
> doesn't
>  		 * interfere with test result
>  		 */
> --
> 2.48.1