On Tue, May 12, 2026 at 01:42:39PM +0000, Loktionov, Aleksandr wrote: > > > > -----Original Message----- > > From: Simon Horman <[email protected]> > > Sent: Monday, May 11, 2026 5:41 PM > > To: Loktionov, Aleksandr <[email protected]> > > Cc: [email protected]; Nguyen, Anthony L > > <[email protected]>; [email protected] > > 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 <[email protected]> > > > > > > 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 <[email protected]> > > > Signed-off-by: Aleksandr Loktionov <[email protected]> > > > > Reviewed-by: Simon Horman <[email protected]> > > > > 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().
