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

[Paul Menzel]

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@xxxxxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxx>
> >
> > Please order it to not use the comma: Hong Aun Looi
>
> Will do, thanks.
>
> > > Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@xxxxxxxxx>
> > > Signed-off-by: Faizal Rahim <faizal.abdul.rahim@xxxxxxxxxxxxxxx>
> > > Signed-off-by: KhaiWenTan <khai.wen.tan@xxxxxxxxxxxxxxx>
> > > ---
> > >   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