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

[Abdul Rahim, Faizal]

On 13/4/2026 10:52 pm, Simon Horman wrote:
> From: 'Simon Horman' <horms@xxxxxxxxxx>
>
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://sashiko.dev
> ---
> igc: add support for forcing link speed without autonegotiation
>
> This commit allows forcing 10/100 Mb/s link speed and duplex settings via
> ethtool when autonegotiation is disabled. It configures the MAC CTRL and
> PHY registers to bypass autonegotiation and apply the requested settings.
>
> > diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> > index 0122009bedd03..e14771532dad8 100644
> > --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
> > +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> [ ... ]
> > +static int igc_handle_autoneg_disabled(struct net_device *netdev, u32 speed,
> > +				       u8 duplex)
> > +{
> > +	struct igc_adapter *adapter = netdev_priv(netdev);
> > +	struct igc_mac_info *mac = &adapter->hw.mac;
> > +	enum igc_forced_speed_duplex forced_speed_duplex;
> >   
> > +	switch (speed) {
> > +	case SPEED_10:
> > +		forced_speed_duplex = (duplex == DUPLEX_FULL) ? IGC_FORCED_10F : IGC_FORCED_10H;
> > +		break;
> > +	case SPEED_100:
> > +		forced_speed_duplex = (duplex == DUPLEX_FULL) ? IGC_FORCED_100F : IGC_FORCED_100H;
> > +		break;
>
> Does forcing a half-duplex configuration here also require setting flow
> control to none (e.g., hw->fc.requested_mode = igc_fc_none)?
>
> > diff --git a/drivers/net/ethernet/intel/igc/igc_mac.c b/drivers/net/ethernet/intel/igc/igc_mac.c
> > index 7ac6637f8db73..16b0f2db06896 100644
> > --- a/drivers/net/ethernet/intel/igc/igc_mac.c
> > +++ b/drivers/net/ethernet/intel/igc/igc_mac.c
> > @@ -452,7 +452,7 @@ s32 igc_config_fc_after_link_up(struct igc_hw *hw)
> >   	 * 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)
> > +	if (mac->autoneg_failed || !mac->autoneg_enabled)
> >   		ret_val = igc_force_mac_fc(hw);
>
> If mac->autoneg_enabled is false, igc_force_mac_fc() will configure the MAC
> using the existing hw->fc.current_mode. If current_mode defaults to full flow
> control, this will enable PAUSE frames.
>
> Since autonegotiation is disabled, the MII status check further down in this
> same function will fail and return early:
>
> drivers/net/ethernet/intel/igc/igc_mac.c:igc_config_fc_after_link_up() {
>      ...
>      if (!(mii_status_reg & MII_SR_AUTONEG_COMPLETE)) {
>          hw_dbg("Copper PHY and Auto Neg has not completed.\n");
>          goto out;
>      }
>      ...
> }
>
> This early return skips the half-duplex safety check at the end of
> igc_config_fc_after_link_up() which would normally clear the flow control
> settings:
>
> drivers/net/ethernet/intel/igc/igc_mac.c:igc_config_fc_after_link_up() {
>      ...
>      if (duplex == HALF_DUPLEX)
>          hw->fc.current_mode = igc_fc_none;
>
>      ret_val = igc_force_mac_fc(hw);
>      ...
> }
>
> Does this leave the MAC incorrectly configured to send and receive PAUSE
> frames on a half-duplex link, violating the IEEE 802.3 specification?

Hi Simon,
Thanks for the review.

Yes, you're right, I missed that interaction.

I’ll update the code to also set the following in 
igc_handle_autoneg_disabled() when forcing half‑duplex:
hw->fc.requested_mode = igc_fc_none

I’ll test it and, if everything looks good, send out v2.

Thanks again!