Re: [Intel-wired-lan] [PATCH iwl-next v4 1/3] igc: remove unused autoneg_failed field

["Abdul Rahim, Faizal" <faizal.abdul.rahim@linux.intel.com>]

Hi Paul,

Thank you for your review.

On 28/4/2026 2:56 pm, Paul Menzel wrote:
> [Cc: Removed stray *Looi*]
> 
> Dear Khai Wen Tan,
> 
> 
> Thank you for your patch.
> 
> 
> Am 28.04.26 um 08:00 schrieb KhaiWenTan:
> 
> (Should spaces be added in your name?)
> 
>> From: Faizal Rahim <faizal.abdul.rahim@linux.intel.com>
>>
>> autoneg_failed in struct igc_mac_info is never set in the igc driver.
>> Remove the field and the dead code checking it in
>> igc_config_fc_after_link_up().
> 
> Could you please elaborate. Why is removal the correct fix, and it’s not an 
> incomplete feature? Does auto-negotiation always succeed?
> 

Auto-negotiation does not always succeed, but igc does not use
autoneg_failed to handle that case, the field was never set anywhere
in the igc driver.

Before this patch, the only igc references to autoneg_failed were
the struct member declaration and the read in
igc_config_fc_after_link_up(). No igc code ever assigned it to true,
and git history shows no commit that added a setter since the code
creation in 2018.

The field originates from the e1000/e1000e fiber/serdes forced-link
path: when MAC-level auto-negotiation on fiber times out, the driver
forces link up and sets autoneg_failed so the flow-control code knows
pause was not negotiated and must be forced. igc has no fiber or
serdes media, it only supports copper (igc_media_type_copper), so
the code that sets autoneg_failed was never ported.

On copper, PHY auto-negotiation failure is handled differently:
- No link at all: igc_check_for_copper_link() returns before reaching
   flow-control configuration, there's nothing to configure FC on.
- Link present but autoneg not yet complete:
   igc_config_fc_after_link_up() checks MII_SR_AUTONEG_COMPLETE and
   returns early without resolving pause. The next link-status event
   re-triggers the check.
- Autoneg completes (including via parallel detection fallback when
   the link partner doesn't autoneg): the PHY still sets
   AUTONEG_COMPLETE but LP_ABILITY won't have PAUSE bits since the
   partner never sent autoneg pages. The existing flow-control logic
   in igc_config_fc_after_link_up() handles that correctly, it falls
   through to igc_fc_none or igc_fc_rx_pause based on requested_mode.

None of these paths need autoneg_failed. Keeping the field would be 
misleading to reader.


>> Reviewed-by: Looi, Hong Aun <hong.aun.looi@intel.com>
> 
> Please order it to not use the comma: Hong Aun Looi
> 

Will do, thanks.

>> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
>> Signed-off-by: Faizal Rahim <faizal.abdul.rahim@linux.intel.com>
>> Signed-off-by: KhaiWenTan <khai.wen.tan@linux.intel.com>
>> ---
>>   drivers/net/ethernet/intel/igc/igc_hw.h  |  1 -
>>   drivers/net/ethernet/intel/igc/igc_mac.c | 16 +---------------
>>   2 files changed, 1 insertion(+), 16 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igc/igc_hw.h b/drivers/net/ 
>> ethernet/intel/igc/igc_hw.h
>> index be8a49a86d09..86ab8f566f44 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_hw.h
>> +++ b/drivers/net/ethernet/intel/igc/igc_hw.h
>> @@ -92,7 +92,6 @@ struct igc_mac_info {
>>       bool asf_firmware_present;
>>       bool arc_subsystem_valid;
>>
>> -    bool autoneg_failed;
>>       bool get_link_status;
>>   };
>>
>> diff --git a/drivers/net/ethernet/intel/igc/igc_mac.c b/drivers/net/ 
>> ethernet/intel/igc/igc_mac.c
>> index 7ac6637f8db7..142beb9ae557 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_mac.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_mac.c
>> @@ -438,28 +438,14 @@ void igc_config_collision_dist(struct igc_hw *hw)
>>    * Checks the status of auto-negotiation after link up to ensure that the
>>    * speed and duplex were not forced.  If the link needed to be forced, 
>> then
>>    * flow control needs to be forced also.  If auto-negotiation is enabled
>> - * and did not fail, then we configure flow control based on our link
>> - * partner.
>> + * then we configure flow control based on our link partner.
>>    */
>>   s32 igc_config_fc_after_link_up(struct igc_hw *hw)
>>   {
>>       u16 mii_status_reg, mii_nway_adv_reg, mii_nway_lp_ability_reg;
>> -    struct igc_mac_info *mac = &hw->mac;
>>       u16 speed, duplex;
>>       s32 ret_val = 0;
>>
>> -    /* Check for the case where we have fiber media and auto-neg failed
>> -     * so we had to force link.  In this case, we need to force the
>> -     * configuration of the MAC to match the "fc" parameter.
>> -     */
>> -    if (mac->autoneg_failed)
>> -        ret_val = igc_force_mac_fc(hw);
>> -
>> -    if (ret_val) {
>> -        hw_dbg("Error forcing flow control settings\n");
>> -        goto out;
>> -    }
>> -
>>       /* In auto-neg, we need to check and see if Auto-Neg has completed,
>>        * and if so, how the PHY and link partner has flow control
>>        * configured.
> 
> 
> Kind regards,
> 
> Paul
>