Re: [PATCH iwl-next 6/8] ixgbe: extract ixgbe_restart_auto_neg() to avoid code duplication

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unchecked register read failures cause cross-register PHY corruption.
--

commit bb1b6f6954d948eeebea8f8bf0a9c8bd82cf6d54
Author: Jakub Chylkowski <jakubx.chylkowski@intel.com>

ixgbe: extract ixgbe_restart_auto_neg() to avoid code duplication

This commit factors out the PHY autonegotiation restart sequence into a
helper function to avoid code duplication. It also updates the sequence to
check the return value of read_reg() to avoid writing stale values on read
failure.

> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_phy.c
[ ... ]
> @@ -1089,6 +1089,26 @@ int ixgbe_mii_bus_init(struct ixgbe_hw *hw)
>  	return mdiobus_register(bus);
>  }
>  
> +/**
> + * ixgbe_restart_auto_neg - restart PHY autonegotiation
> + * @hw: pointer to hardware structure
> + *
> + * Sets the restart autoneg bit in MDIO_CTRL1 to trigger a new
> + * autonegotiation cycle.
> + **/
> +static void ixgbe_restart_auto_neg(struct ixgbe_hw *hw)
> +{
> +	u16 autoneg_reg;
> +	int status;
> +
> +	status = hw->phy.ops.read_reg(hw, MDIO_CTRL1, MDIO_MMD_AN,
> +				      &autoneg_reg);
> +	if (status)
> +		return;
> +	autoneg_reg |= MDIO_AN_CTRL1_RESTART;
> +	hw->phy.ops.write_reg(hw, MDIO_CTRL1, MDIO_MMD_AN, autoneg_reg);
> +}

The commit message correctly notes that ignoring the read_reg() result can
cause a stale autoneg_reg value from a prior register access to be
unconditionally written back.

Since autoneg_reg is shared across multiple operations earlier in the caller
functions, does this same cross-register corruption still occur if one of
those prior reads fails?

For example, earlier in ixgbe_setup_phy_link_generic():

ixgbe_setup_phy_link_generic() {
        u16 autoneg_reg = IXGBE_MII_AUTONEG_REG;
        ...
        hw->phy.ops.read_reg(hw, MDIO_AN_10GBT_CTRL, MDIO_MMD_AN, &autoneg_reg);
        ...
        hw->phy.ops.write_reg(hw, MDIO_AN_10GBT_CTRL, MDIO_MMD_AN, autoneg_reg);

        hw->phy.ops.read_reg(hw, IXGBE_MII_AUTONEG_VENDOR_PROVISION_1_REG,
                             MDIO_MMD_AN, &autoneg_reg);
        ...
        hw->phy.ops.write_reg(hw, IXGBE_MII_AUTONEG_VENDOR_PROVISION_1_REG,
                              MDIO_MMD_AN, autoneg_reg);
        ...
}

If the second read fails, won't it retain the value read from
MDIO_AN_10GBT_CTRL and blindly write it into
IXGBE_MII_AUTONEG_VENDOR_PROVISION_1_REG?

Should these earlier register accesses also check the return value of
read_reg() to prevent similar corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508031226.3601800-1-aleksandr.loktionov@intel.com?part=6