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 <[email protected]>
>>> ---
>>> 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 <[email protected]>
>>
>>
>> Kind regards,
>>
>> Paul
Thank you for the review, Paul.
