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

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

[Abdul Rahim, Faizal]

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
> 

Re: [PATCH iwl-next v4 0/3] igc: add support for forcing link speed without autonegotiation

[Abdul Rahim, Faizal]

On 30/4/2026 10:41 pm, David Laight wrote:
> On Tue, 28 Apr 2026 14:00:06 +0800
> KhaiWenTan <khai.wen.tan@linux.intel.com> wrote:
> 
>> From: Faizal Rahim <faizal.abdul.rahim@linux.intel.com>
>>
>> This series adds support for forcing 10/100 Mb/s link speed via ethtool
>> when autonegotiation is disabled on the igc driver.
> 
> I'll ask 'why' ?
> 
> In particular forcing half/full duplex has always been a very good way
> of 'breaking' a network connection.
> 
> It really is much better to restrict the advertised link modes and let
> the autodetect/autonegotiation logic in the phy/mac do its job.
> 
> About the only think I can think of is to force 10M HDX when connected
> to a remote system that supports 10M/100M HDX.
> In that case you need to send out single link test pulses, not the
> burst used to identify 100M HDX, or the pattern encoded on the burst
> used by autonegotiation.
> But you need to got back to the mid 1990s to find such systems.
> Anything that supports FDX will do autonegotiation.
> 
> 	David
> 

There's a use case requested:

Profinet Certification tool reports that forcing a link speed without
auto-negotiation is not working.
Forcing the link speed is a critical feature for the industrial automation
"fast-start" use case. When there is a connection lost, the system must
come back up as fast as possible. In PROFINET, that means to force the
speed and rejoin the controller loops. Without supporting forcing the speed
to 100M in Foxville, the certification tool would not be able to certify
the availability of this feature.

I'm hoping this context is enough to justify the need?

Re: [PATCH iwl-next v4 0/3] igc: add support for forcing link speed without autonegotiation

[David Laight]

On Wed, 6 May 2026 14:21:59 +0800
"Abdul Rahim, Faizal" <faizal.abdul.rahim@linux.intel.com> wrote:

> On 30/4/2026 10:41 pm, David Laight wrote:
> > On Tue, 28 Apr 2026 14:00:06 +0800
> > KhaiWenTan <khai.wen.tan@linux.intel.com> wrote:
> >   
> >> From: Faizal Rahim <faizal.abdul.rahim@linux.intel.com>
> >>
> >> This series adds support for forcing 10/100 Mb/s link speed via ethtool
> >> when autonegotiation is disabled on the igc driver.  
> > 
> > I'll ask 'why' ?
> > 
> > In particular forcing half/full duplex has always been a very good way
> > of 'breaking' a network connection.
> > 
> > It really is much better to restrict the advertised link modes and let
> > the autodetect/autonegotiation logic in the phy/mac do its job.
> > 
> > About the only think I can think of is to force 10M HDX when connected
> > to a remote system that supports 10M/100M HDX.
> > In that case you need to send out single link test pulses, not the
> > burst used to identify 100M HDX, or the pattern encoded on the burst
> > used by autonegotiation.
> > But you need to got back to the mid 1990s to find such systems.
> > Anything that supports FDX will do autonegotiation.
> > 
> > 	David
> >   
> 
> There's a use case requested:
> 
> Profinet Certification tool reports that forcing a link speed without
> auto-negotiation is not working.
> Forcing the link speed is a critical feature for the industrial automation
> "fast-start" use case. When there is a connection lost, the system must
> come back up as fast as possible. In PROFINET, that means to force the
> speed and rejoin the controller loops. Without supporting forcing the speed
> to 100M in Foxville, the certification tool would not be able to certify
> the availability of this feature.
> 
> I'm hoping this context is enough to justify the need?

Is auto-negotiation of the 'low' speed actually that slow?
IIRC detecting 10G and above requires a lot of signal processing.
But 10/100 and hdx/fdx just uses the ANAR register value sent in the
link test pulses.
(IIRC 1G uses the ANAR pattern, but requires extra signal processing as well.
The higher speeds didn't exist when I was writing ethernet drivers.)

I've been on the 'wrong end' of hdx/fdx mismatches - you really don't
want to let people get there, it is terribly confusing.

There actually ought to be a way of setting the auto-negotiation
registers to 100M (HDX and/or FDX) and then transmitting as (say) 100M HDX
even before negotiation completes.
Then correcting hdx/fdx based on the received ANAR register.
Or, at least, sending out an ANAR that only contains what you are using.

The problem I always had was that the actual operating mode of the phy
wasn't in one of the standard registers.
So if you connected to a system that didn't do auto-negotiation the
phy would be using (say) 10M HDX, but the received ANAR register would
still contain a value from an earlier connection.
If the driver read that register from the phy it used the wrong duplex mode.
(The speed for 10/100 doesn't matter, the phy clocks the interface to the
mac at the right speed and the mac doesn't care.)

	David

Re: [PATCH iwl-next v4 0/3] igc: add support for forcing link speed without autonegotiation

[Abdul Rahim, Faizal]

+ Hector

On 6/5/2026 5:40 pm, David Laight wrote:
> On Wed, 6 May 2026 14:21:59 +0800
> "Abdul Rahim, Faizal" <faizal.abdul.rahim@linux.intel.com> wrote:
> 
>> On 30/4/2026 10:41 pm, David Laight wrote:
>>> On Tue, 28 Apr 2026 14:00:06 +0800
>>> KhaiWenTan <khai.wen.tan@linux.intel.com> wrote:
>>>   
>>>> From: Faizal Rahim <faizal.abdul.rahim@linux.intel.com>
>>>>
>>>> This series adds support for forcing 10/100 Mb/s link speed via ethtool
>>>> when autonegotiation is disabled on the igc driver.  
>>>
>>> I'll ask 'why' ?
>>>
>>> In particular forcing half/full duplex has always been a very good way
>>> of 'breaking' a network connection.
>>>
>>> It really is much better to restrict the advertised link modes and let
>>> the autodetect/autonegotiation logic in the phy/mac do its job.
>>>
>>> About the only think I can think of is to force 10M HDX when connected
>>> to a remote system that supports 10M/100M HDX.
>>> In that case you need to send out single link test pulses, not the
>>> burst used to identify 100M HDX, or the pattern encoded on the burst
>>> used by autonegotiation.
>>> But you need to got back to the mid 1990s to find such systems.
>>> Anything that supports FDX will do autonegotiation.
>>>
>>> 	David
>>>   
>>
>> There's a use case requested:
>>
>> Profinet Certification tool reports that forcing a link speed without
>> auto-negotiation is not working.
>> Forcing the link speed is a critical feature for the industrial automation
>> "fast-start" use case. When there is a connection lost, the system must
>> come back up as fast as possible. In PROFINET, that means to force the
>> speed and rejoin the controller loops. Without supporting forcing the speed
>> to 100M in Foxville, the certification tool would not be able to certify
>> the availability of this feature.
>>
>> I'm hoping this context is enough to justify the need?
> 
> Is auto-negotiation of the 'low' speed actually that slow?
> IIRC detecting 10G and above requires a lot of signal processing.
> But 10/100 and hdx/fdx just uses the ANAR register value sent in the
> link test pulses.
> (IIRC 1G uses the ANAR pattern, but requires extra signal processing as well.
> The higher speeds didn't exist when I was writing ethernet drivers.)
> 
> I've been on the 'wrong end' of hdx/fdx mismatches - you really don't
> want to let people get there, it is terribly confusing.
> 

Thanks for the information.

I agree that for normal Ethernet use, auto-negotiation on both link
partners is safer and avoids the issues you mentioned.

The reason for this patch is the more specific PROFINET Fast Start Up
(FSU) use case. For FSU, the requirement is different from normal Ethernet
use. It is intended for deterministic startup, for example in industrial
robot/tool-change applications.

One of the startup optimizations is to use "fixed transmission parameters"
instead of automatic detection in the profinet specification:
  https://us.profinet.com/profinet_tech/fast-start-up/

I understand your point that 10/100 auto-negotiation is faster than
higher-speed link training. I don't have a detailed timing breakdown for
the FSU case comparing 10/100 startup with auto-negotiation enabled versus
disabled, or enough visibility into the certification criteria to comment
on additional determinism requirements.

But keeping AutoNeg enabled, even with only specific speed advertised,
would not cover the same requirement.

This is only meant as an explicit link configuration for controlled
industrial deployments where both link partners are configured
consistently. It's not intended as a recommended default for general
networking.

Also, ethtool already allows users to request speed/duplex configuration
with auto-negotiation disabled, and some drivers already support this, for
example igb. This patch just reuses that existing interface and enables igc
to support the forced modes supported by this hardware.

> There actually ought to be a way of setting the auto-negotiation
> registers to 100M (HDX and/or FDX) and then transmitting as (say) 100M HDX
> even before negotiation completes.
> Then correcting hdx/fdx based on the received ANAR register.
> Or, at least, sending out an ANAR that only contains what you are using.
> 
> The problem I always had was that the actual operating mode of the phy
> wasn't in one of the standard registers.
> So if you connected to a system that didn't do auto-negotiation the
> phy would be using (say) 10M HDX, but the received ANAR register would
> still contain a value from an earlier connection.
> If the driver read that register from the phy it used the wrong duplex mode.
> (The speed for 10/100 doesn't matter, the phy clocks the interface to the
> mac at the right speed and the mac doesn't care.)
> 
> 	David
> 
> 
> 
> 
> 

Re: [PATCH iwl-next v4 0/3] igc: add support for forcing link speed without autonegotiation

[Andrew Lunn]

> I agree that for normal Ethernet use, auto-negotiation on both link
> partners is safer and avoids the issues you mentioned.
> 
> The reason for this patch is the more specific PROFINET Fast Start Up
> (FSU) use case. For FSU, the requirement is different from normal Ethernet
> use. It is intended for deterministic startup, for example in industrial
> robot/tool-change applications.
> 
> One of the startup optimizations is to use "fixed transmission parameters"
> instead of automatic detection in the profinet specification:
>   https://us.profinet.com/profinet_tech/fast-start-up/

Automotive also have similar requirements. For them, Autoneg is slow,
takes around 1 second, where as a fixed link is operative very
fast. There are even some automotive ethernet devices which don't even
support autoneg.

So this use case makes sense to me, for embedded systems. If the
hardware can do it, any most PHYs can, i see no reason not to support
it. And anything using phylib gets it for free.

	Andrew