[PATCH v1 iwl-net] igb: fix link test skipping when interface is admin down

[PATCH v1 iwl-net] igb: fix link test skipping when interface is admin down

[Kohei Enju]

The igb driver incorrectly skips the link test when the network
interface is admin down (if_running == false), causing the test to
always report PASS regardless of the actual physical link state.

This behavior is inconsistent with other drivers (e.g. i40e, ice, ixgbe,
etc.) which correctly test the physical link state regardless of admin
state.
Remove the if_running check to ensure link test always reflects the
physical link state.

Fixes: 8d420a1b3ea6 ("igb: correct link test not being run when link is down")
Signed-off-by: Kohei Enju <enjuk@amazon.com>
---
 drivers/net/ethernet/intel/igb/igb_ethtool.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index ca6ccbc13954..6412c84e2d17 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
@@ -2081,11 +2081,8 @@ static void igb_diag_test(struct net_device *netdev,
 	} else {
 		dev_info(&adapter->pdev->dev, "online testing starting\n");
 
-		/* PHY is powered down when interface is down */
-		if (if_running && igb_link_test(adapter, &data[TEST_LINK]))
+		if (igb_link_test(adapter, &data[TEST_LINK]))
 			eth_test->flags |= ETH_TEST_FL_FAILED;
-		else
-			data[TEST_LINK] = 0;
 
 		/* Online tests aren't run; pass by default */
 		data[TEST_REG] = 0;
-- 
2.48.1

Re: [Intel-wired-lan] [PATCH v1 iwl-net] igb: fix link test skipping when interface is admin down

[Paul Menzel]

Dear Kohei,


Thank you very much for your patch.

Am 15.08.25 um 08:26 schrieb Kohei Enju:
> The igb driver incorrectly skips the link test when the network
> interface is admin down (if_running == false), causing the test to
> always report PASS regardless of the actual physical link state.
> 
> This behavior is inconsistent with other drivers (e.g. i40e, ice, ixgbe,
> etc.) which correctly test the physical link state regardless of admin
> state.

I’d collapse the above two sentences into one paragraph and add an empty 
line here to visually separate the paragraph below.

> Remove the if_running check to ensure link test always reflects the
> physical link state.

Please add how to verify your change, that means the command to run.

> Fixes: 8d420a1b3ea6 ("igb: correct link test not being run when link is down")
> Signed-off-by: Kohei Enju <enjuk@amazon.com>
> ---
>   drivers/net/ethernet/intel/igb/igb_ethtool.c | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> index ca6ccbc13954..6412c84e2d17 100644
> --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
> +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> @@ -2081,11 +2081,8 @@ static void igb_diag_test(struct net_device *netdev,
>   	} else {
>   		dev_info(&adapter->pdev->dev, "online testing starting\n");
>   
> -		/* PHY is powered down when interface is down */
> -		if (if_running && igb_link_test(adapter, &data[TEST_LINK]))
> +		if (igb_link_test(adapter, &data[TEST_LINK]))
>   			eth_test->flags |= ETH_TEST_FL_FAILED;
> -		else
> -			data[TEST_LINK] = 0;
>   
>   		/* Online tests aren't run; pass by default */
>   		data[TEST_REG] = 0;

Regardless of my style comments above:

Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul

Re: [Intel-wired-lan] [PATCH v1 iwl-net] igb: fix link test skipping when interface is admin down

[Paul Menzel]

[Correct Alexander’s email address]

Am 15.08.25 um 09:38 schrieb Paul Menzel:
> Dear Kohei,
> 
> 
> Thank you very much for your patch.
> 
> Am 15.08.25 um 08:26 schrieb Kohei Enju:
>> The igb driver incorrectly skips the link test when the network
>> interface is admin down (if_running == false), causing the test to
>> always report PASS regardless of the actual physical link state.
>>
>> This behavior is inconsistent with other drivers (e.g. i40e, ice, ixgbe,
>> etc.) which correctly test the physical link state regardless of admin
>> state.
> 
> I’d collapse the above two sentences into one paragraph and add an empty 
> line here to visually separate the paragraph below.
> 
>> Remove the if_running check to ensure link test always reflects the
>> physical link state.
> 
> Please add how to verify your change, that means the command to run.
> 
>> Fixes: 8d420a1b3ea6 ("igb: correct link test not being run when link is down")
>> Signed-off-by: Kohei Enju <enjuk@amazon.com>
>> ---
>>   drivers/net/ethernet/intel/igb/igb_ethtool.c | 5 +----
>>   1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/ 
>> net/ethernet/intel/igb/igb_ethtool.c
>> index ca6ccbc13954..6412c84e2d17 100644
>> --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
>> +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
>> @@ -2081,11 +2081,8 @@ static void igb_diag_test(struct net_device 
>> *netdev,
>>       } else {
>>           dev_info(&adapter->pdev->dev, "online testing starting\n");
>> -        /* PHY is powered down when interface is down */
>> -        if (if_running && igb_link_test(adapter, &data[TEST_LINK]))
>> +        if (igb_link_test(adapter, &data[TEST_LINK]))
>>               eth_test->flags |= ETH_TEST_FL_FAILED;
>> -        else
>> -            data[TEST_LINK] = 0;
>>           /* Online tests aren't run; pass by default */
>>           data[TEST_REG] = 0;
> 
> Regardless of my style comments above:
> 
> Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
> 
> 
> Kind regards,
> 
> Paul

Re: [Intel-wired-lan] [PATCH v1 iwl-net] igb: fix link test skipping when interface is admin down

[Kohei Enju]

On Fri, 15 Aug 2025 09:41:28 +0200, Paul Menzel wrote:

>[Correct Alexander’s email address]

Thank you for the correction.
I'll use the correct address going forward.

>
>Am 15.08.25 um 09:38 schrieb Paul Menzel:
>> Dear Kohei,
>> 
>> 
>> Thank you very much for your patch.
>> 
>> Am 15.08.25 um 08:26 schrieb Kohei Enju:
>>> The igb driver incorrectly skips the link test when the network
>>> interface is admin down (if_running == false), causing the test to
>>> always report PASS regardless of the actual physical link state.
>>>
>>> This behavior is inconsistent with other drivers (e.g. i40e, ice, ixgbe,
>>> etc.) which correctly test the physical link state regardless of admin
>>> state.
>> 
>> I’d collapse the above two sentences into one paragraph and add an empty 
>> line here to visually separate the paragraph below.

Yes, that makes sense. I'll do that.

>> 
>>> Remove the if_running check to ensure link test always reflects the
>>> physical link state.
>> 
>> Please add how to verify your change, that means the command to run.

Sure, I tested the change using the ip and ethtool commands, with the
physical link down. 
Should I include the following test steps in the commit message?

Before:
  $ sudo ip link set dev enp7s0f0 down
  
  $ ip --json link show enp7s0f0 | jq -c ".[].flags"
  ["BROADCAST","MULTICAST"]
  
  $ sudo ethtool --test enp7s0f0 online
  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 # <- Not expected

After:
  $ sudo ip link set dev enp7s0f0 down
  
  $ ip --json link show enp7s0f0 | jq -c ".[].flags"
  ["BROADCAST","MULTICAST"]
  
  $ sudo ethtool --test enp7s0f0 online
  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 # <- Expected

>> 
>>> Fixes: 8d420a1b3ea6 ("igb: correct link test not being run when link is down")
>>> Signed-off-by: Kohei Enju <enjuk@amazon.com>
>>> ---
>>>   drivers/net/ethernet/intel/igb/igb_ethtool.c | 5 +----
>>>   1 file changed, 1 insertion(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/ 
>>> net/ethernet/intel/igb/igb_ethtool.c
>>> index ca6ccbc13954..6412c84e2d17 100644
>>> --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
>>> +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
>>> @@ -2081,11 +2081,8 @@ static void igb_diag_test(struct net_device 
>>> *netdev,
>>>       } else {
>>>           dev_info(&adapter->pdev->dev, "online testing starting\n");
>>> -        /* PHY is powered down when interface is down */
>>> -        if (if_running && igb_link_test(adapter, &data[TEST_LINK]))
>>> +        if (igb_link_test(adapter, &data[TEST_LINK]))
>>>               eth_test->flags |= ETH_TEST_FL_FAILED;
>>> -        else
>>> -            data[TEST_LINK] = 0;
>>>           /* Online tests aren't run; pass by default */
>>>           data[TEST_REG] = 0;
>> 
>> Regardless of my style comments above:
>> 
>> Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
>> 
>> 
>> Kind regards,
>> 
>> Paul

Thank you for the review, Paul.

RE: [Intel-wired-lan] [PATCH v1 iwl-net] igb: fix link test skipping when interface is admin down

[Rinitha, SX]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Kohei Enju
> Sent: 15 August 2025 11:57
> 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>; Alexander Duyck <alexander.h.duyck@intel.com>; kohei.enju@gmail.com; Kohei Enju <enjuk@amazon.com>
> Subject: [Intel-wired-lan] [PATCH v1 iwl-net] igb: fix link test skipping when interface is admin down
>
> The igb driver incorrectly skips the link test when the network interface is admin down (if_running == false), causing the test to always report PASS regardless of the actual physical link state.
>
> This behavior is inconsistent with other drivers (e.g. i40e, ice, ixgbe,
etc.) which correctly test the physical link state regardless of admin state.
> Remove the if_running check to ensure link test always reflects the physical link state.
>
> Fixes: 8d420a1b3ea6 ("igb: correct link test not being run when link is down")
> Signed-off-by: Kohei Enju <enjuk@amazon.com>
> ---
> drivers/net/ethernet/intel/igb/igb_ethtool.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>

Tested-by: Rinitha S <sx.rinitha@intel.com> (A Contingent worker at Intel)