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

On 28/4/2026 11:06 pm, Paul Menzel wrote:
> 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.

Will update.

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

Ohh okay, let me check, thanks!

> […]
> 
>>>>    * 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
>