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 <[email protected]> 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/[email protected]?part=6
