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

[Simon Horman <horms@kernel.org>]

On Tue, May 12, 2026 at 01:42:39PM +0000, Loktionov, Aleksandr wrote:
> 
> 
> > -----Original Message-----
> > From: Simon Horman <horms@kernel.org>
> > Sent: Monday, May 11, 2026 5:41 PM
> > To: Loktionov, Aleksandr <aleksandr.loktionov@intel.com>
> > Cc: intel-wired-lan@lists.osuosl.org; Nguyen, Anthony L
> > <anthony.l.nguyen@intel.com>; netdev@vger.kernel.org
> > Subject: Re: [PATCH iwl-next 6/8] ixgbe: extract
> > ixgbe_restart_auto_neg() to avoid code duplication
> > 
> > On Fri, May 08, 2026 at 05:12:24AM +0200, Aleksandr Loktionov wrote:
> > > From: Jakub Chylkowski <jakubx.chylkowski@intel.com>
> > >
> > > Both ixgbe_setup_phy_link_generic() and ixgbe_setup_phy_link_tnx()
> > end
> > > with the same three-line sequence that reads MDIO_CTRL1, sets the
> > > MDIO_AN_CTRL1_RESTART bit, and writes MDIO_CTRL1 back.
> > >
> > > Factor it out into a static helper ixgbe_restart_auto_neg() and call
> > > it from both sites.
> > >
> > > While at it, also check the return value of phy.ops.read_reg() in
> > the
> > > helper and skip the write on failure.  The original inlined code
> > > ignored the read result and would OR MDIO_AN_CTRL1_RESTART into a
> > > stale autoneg_reg value (left over from the prior MDIO_AN_ADVERTISE
> > > write) and unconditionally write it back to MDIO_CTRL1 if the read
> > > failed.  This is a small behavioral change: on read_reg() failure
> > the
> > > restart write is now skipped instead of being issued with a
> > > potentially garbage value.
> > >
> > > Signed-off-by: Jakub Chylkowski <jakubx.chylkowski@intel.com>
> > > Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> > 
> > Reviewed-by: Simon Horman <horms@kernel.org>
> > 
> > FWIIW, the AI-generated review of this patch available on sashiko.dev
> > flags that similar problems wrt write on failre exist earlier on in
> > ixgbe_setup_phy_link_generic(). It may be good to address this area
> > more holistically as a follow-up. (I am not suggesting increasing the
> > scope of this patch/patch-set.)
> > 
> > ...
> 
> Thanks for the review!
> 
> On the min() nit - the operation here is "clamp the new (smaller) itr
> at a floor of prev - IXGBE_ITR_ADAPTIVE_MIN_INC", which max_t()
> expresses directly. Rewriting with min() would need to flip the
> reference point, e.g.
> 
> itr = ring_container->itr - min_t(unsigned int,
> ring_container->itr - itr,
> IXGBE_ITR_ADAPTIVE_MIN_INC);
> 
> which adds a subtraction and reads less naturally than the floor form.
> I'd prefer to keep max_t() here unless you feel strongly - your
> Reviewed-by stands either way, and I've added it for v5. Thanks again.

Thanks, I don't feel strongly about max_t().