> -----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.
