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: <20250830201549.16083-1-enjuk@amazon.com>
Date: Sun, 31 Aug 2025 05:14:41 +0900
From: Kohei Enju <enjuk@...zon.com>
To: <andrew@...n.ch>
CC: <andrew+netdev@...n.ch>, <anthony.l.nguyen@...el.com>,
	<davem@...emloft.net>, <edumazet@...gle.com>, <enjuk@...zon.com>,
	<intel-wired-lan@...ts.osuosl.org>, <kohei.enju@...il.com>,
	<kuba@...nel.org>, <netdev@...r.kernel.org>, <pabeni@...hat.com>,
	<przemyslaw.kitszel@...el.com>, <vitaly.lifshits@...el.com>
Subject: Re: [Intel-wired-lan] [PATCH v1 iwl-net] igc: power up PHY before link test

On Sat, 30 Aug 2025 21:27:33 +0200, Andrew Lunn wrote:

>On Sun, Aug 31, 2025 at 02:06:19AM +0900, Kohei Enju wrote:
>> 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.
>
>Maybe you should power the PHY down again after the test?

You're right about the concern, but it's already handled by the existing 
code flow:
    /* power up PHY for link test */
	igc_power_up_phy_copper(&adapter->hw);

    /* doing link test */

    if (if_running)
        igc_close(netdev);
    else
        igc_reset(adapter);
        
    /* other tests */
    igc_reset(adapter);

igc_reset() calls igc_power_down_phy_copper_base() when !netif_running(), 
so the PHY is properly powered down again.

>
>Alternatively, just return -ENOTDOWN is the network is admin down.

That would be simpler indeed. Since the callback returns void, we'd set 
the test result to indicate skip/fail.
However, I think checking actual physical connectivity even when admin 
down is valuable, which other Intel ethernet drivers (e.g., i40e, ixgbe, 
igb) also do.

>
>	Andrew