[PATCH v1 iwl-net] igc: power up PHY before link test

[PATCH v1 iwl-net] igc: power up PHY before link test

[Kohei Enju]

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@amazon.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);
+
 		/* Link test performed before hardware reset so autoneg doesn't
 		 * interfere with test result
 		 */
-- 
2.48.1

Re: [PATCH v1 iwl-net] igc: power up PHY before link test

[Andrew Lunn]

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?

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

	Andrew

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

[Kohei Enju]

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

Re: [PATCH v1 iwl-net] igc: power up PHY before link test

[Lifshits, Vitaly]

> ---
>   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);

I suggest moving this to igc_link_test functionn igc_diags.c as it 
relates only to the link test.

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


Thank you for this patch.

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

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
> Of Kohei Enju
> Sent: Saturday, August 30, 2025 7:06 PM
> To: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org
> Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel,
> Przemyslaw <przemyslaw.kitszel@intel.com>; Andrew Lunn
> <andrew+netdev@lunn.ch>; David S. Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
> Abeni <pabeni@redhat.com>; Lifshits, Vitaly
> <vitaly.lifshits@intel.com>; kohei.enju@gmail.com; Kohei Enju
> <enjuk@amazon.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@amazon.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

Re: [PATCH v1 iwl-net] igc: power up PHY before link test

[Kohei Enju]

On Mon, 1 Sep 2025 07:36:21 +0300, Lifshits, Vitaly wrote:

>
>> ---
>>   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);
>
>I suggest moving this to igc_link_test functionn igc_diags.c as it 
>relates only to the link test.

Thank you for taking a look, Lifshits.

Now I'm considering changing only offline test path, not including
online test path.
This is because I think online test path should not trigger any
interruption and power down/up PHY may cause disruption.

So, I forgo the online path and my intention for this patch is to power
up PHY state only in offline test path.
I think introducing igc_power_up_phy_copper() in igc_link_test()
needs careful consideration in online test path.

>
>> +
>>   		/* Link test performed before hardware reset so autoneg doesn't
>>   		 * interfere with test result
>>   		 */
>
>
>Thank you for this patch.

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

[Kohei Enju]

On Mon, 1 Sep 2025 07:03:34 +0000, Loktionov, Aleksandr wrote:

[...]
>>  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.

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.

>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...

Hmm, indeed I didn't check the status, but should I tweak `void
igc_power_up_phy_copper(struct igc_hw *hw)` itself?

My intention was to try to power up PHY if possible, but give it up when
not possible (e.g., bus issue). When powering up is not successful, that
is, PHY is still powered down, igc_link_test() should fail as it does so
now.

>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

Got it. I'll do that in v2.

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

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

[Lifshits, Vitaly]

On 9/6/2025 6:49 AM, Kohei Enju wrote:
> On Mon, 1 Sep 2025 07:36:21 +0300, Lifshits, Vitaly wrote:
> 
>>
>>> ---
>>>    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);
>>
>> I suggest moving this to igc_link_test functionn igc_diags.c as it
>> relates only to the link test.
> 
> Thank you for taking a look, Lifshits.
> 
> Now I'm considering changing only offline test path, not including
> online test path.
> This is because I think online test path should not trigger any
> interruption and power down/up PHY may cause disruption.
> 
> So, I forgo the online path and my intention for this patch is to power
> up PHY state only in offline test path.
> I think introducing igc_power_up_phy_copper() in igc_link_test()
> needs careful consideration in online test path.

Yes, I see that now.
Then it's ok to leave it as is.

Regarding the question whether to wrap igc_power_up_phy_copper with an 
if (hw->phy.media_type == igc_media_type_copper), I think that it's not 
necessary. First of all, igc devices are only copper devices. Secondly, 
in the other place where we call this function (in igc_main), we don't 
check the media type, therefore I suggest being consistent with the 
already existing code and leaving it as it is now.

> 
>>
>>> +
>>>    		/* Link test performed before hardware reset so autoneg doesn't
>>>    		 * interfere with test result
>>>    		 */
>>
>>
>> Thank you for this patch.

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

[Kohei Enju]

+ Alex

On Sun, 7 Sep 2025 16:51:53 +0300, Lifshits, Vitaly wrote:

>On 9/6/2025 6:49 AM, Kohei Enju wrote:
>> On Mon, 1 Sep 2025 07:36:21 +0300, Lifshits, Vitaly wrote:
>> 
>>>
>>>> ---
>>>>    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);
>>>
>>> I suggest moving this to igc_link_test functionn igc_diags.c as it
>>> relates only to the link test.
>> 
>> Thank you for taking a look, Lifshits.
>> 
>> Now I'm considering changing only offline test path, not including
>> online test path.
>> This is because I think online test path should not trigger any
>> interruption and power down/up PHY may cause disruption.
>> 
>> So, I forgo the online path and my intention for this patch is to power
>> up PHY state only in offline test path.
>> I think introducing igc_power_up_phy_copper() in igc_link_test()
>> needs careful consideration in online test path.
>
>Yes, I see that now.
>Then it's ok to leave it as is.

Sorry for late response. Thank you for taking a look, Vitaly!

>
>Regarding the question whether to wrap igc_power_up_phy_copper with an 
>if (hw->phy.media_type == igc_media_type_copper), I think that it's not 
>necessary. First of all, igc devices are only copper devices. Secondly, 
>in the other place where we call this function (in igc_main), we don't 
>check the media type, therefore I suggest being consistent with the 
>already existing code and leaving it as it is now.

Indeed, I think you're right.

I looked at the existing code and found that indeed there are codes
which are assuming only copper devices at least for now. [1, 2, 3]

So, I'll rephrase the commit message in v2 to clarify:
- This change is applied only for offline testing, forgoing online
  testing.
- In this case, original power state is restored in igc_reset()
  afterwards.

and leave the diff as it is in V2 patch.

>
>> 
>>>
>>>> +
>>>>    		/* Link test performed before hardware reset so autoneg doesn't
>>>>    		 * interfere with test result
>>>>    		 */
>>>
>>>
>>> Thank you for this patch.

Thanks, 
Kohei

[1] igc_main.c
```
void igc_reset(struct igc_adapter *adapter)
{
  ...
  if (!netif_running(adapter->netdev))
      igc_power_down_phy_copper_base(&adapter->hw);
```

[2] igc_main.c
```
static void igc_power_up_link(struct igc_adapter *adapter)
{
  ...
  igc_power_up_phy_copper(&adapter->hw);
```

[3] igc_main.c
```
static int __igc_open(struct net_device *netdev, bool resuming)
{
  ...
  err_req_irq:
  igc_release_hw_control(adapter);
  igc_power_down_phy_copper_base(&adapter->hw);
```