netdev - Re: [PATCH v2 iwl-net] igc: power up the PHY before the 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] [day] [month] [year] [list]

Message-ID: <625ae122-7cfc-4297-9b98-8495c3d0f237@intel.com>
Date: Sun, 21 Sep 2025 09:41:43 +0300
From: "Lifshits, Vitaly" <vitaly.lifshits@...el.com>
To: Kohei Enju <enjuk@...zon.com>, <intel-wired-lan@...ts.osuosl.org>,
	<netdev@...r.kernel.org>
CC: Tony Nguyen <anthony.l.nguyen@...el.com>, Przemek Kitszel
	<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>,
	<aleksandr.loktionov@...el.com>, <kohei.enju@...il.com>
Subject: Re: [PATCH v2 iwl-net] igc: power up the PHY before the link test

On 9/20/2025 9:39 AM, Kohei Enju wrote:
> The current implementation of the igc driver doesn't power up the PHY
> before the link test in igc_ethtool_diag_test(), causing the link test
> to always report FAIL when admin state is down and the PHY is
> consequently powered down.
> 
> To test the link state regardless of admin state, power up the PHY
> before the link test in the offline test path. After the link test, the
> original PHY state is restored by igc_reset(), so additional code which
> explicitly restores the original state is not necessary.
> 
> Note that this change is applied only for the offline test path. This is
> because in the online path we shouldn't interrupt normal networking
> operation and powering up the PHY and restoring the original state would
> interrupt that.
> 
> This implementation also uses igc_power_up_phy_copper() without checking
> the media type, since igc devices are currently only copper devices and
> the function is called in other places without checking the media type.
> 
> Furthermore, the powering up is on a best-effort basis, that is, we
> don't handle failures of powering up (e.g. bus error) and just let the
> test report FAIL.
> 
> 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>
> ---
> Changes:
> v1->v2:
>    - rephrase commit message to clarify:
>      - applied only for offline test path
>      - original power state is restored by igc_reset()
>      - powering up the PHY is on a best-effort basis
> v1: https://lore.kernel.org/intel-wired-lan/20250830170656.61496-1-enjuk@amazon.com/
> ---
>   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);
> +
>   		/* Link test performed before hardware reset so autoneg doesn't
>   		 * interfere with test result
>   		 */

Reviewed-by: Vitaly Lifshits <vitaly.lifshits@...el.com>