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

[Paul Menzel <pmenzel@molgen.mpg.de>]

Dear Faizal,


Am 28.04.26 um 12:39 schrieb Abdul Rahim, Faizal:

> On 28/4/2026 2:56 pm, Paul Menzel wrote:

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

Thank you. For me the information about just supporting copper would be 
great to have in the commit message.

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

Just for your information, that your mailer wraps the lines of the quotes.

[…]

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