This is a set of 3 improvements to the 28G Lynx SerDes driver as found
on NXP Layerscape:
- avoid kernel hangs if lane resets/halts fail due to other bugs
- actually have phy_power_down() cut power from lanes, not just halt them
- allow consumers to call phy_exit(), to balance the phy->init_count

Especially change 3 will allow further development of the dpaa2-eth
consumer. To permit phy_exit() and other patches in net-next to be
submitted in this development cycle without functionally breaking
networking, please apply this change on top of v7.0-rc1 and provide
it as a stable tag to be pulled in netdev.

Vladimir Oltean (3):
  phy: lynx-28g: use timeouts when waiting for lane halt and reset
  phy: lynx-28g: truly power the lanes up or down
  phy: lynx-28g: implement phy_exit() operation

 drivers/phy/freescale/phy-fsl-lynx-28g.c | 190 +++++++++++++++++++----
 1 file changed, 157 insertions(+), 33 deletions(-)

-- 
2.34.1

On Sat, Mar 21, 2026 at 03:14:48AM +0200, Vladimir Oltean wrote:
> This is a set of 3 improvements to the 28G Lynx SerDes driver as found
> on NXP Layerscape:
> - avoid kernel hangs if lane resets/halts fail due to other bugs
> - actually have phy_power_down() cut power from lanes, not just halt them
> - allow consumers to call phy_exit(), to balance the phy->init_count
> 
> Especially change 3 will allow further development of the dpaa2-eth
> consumer. To permit phy_exit() and other patches in net-next to be
> submitted in this development cycle without functionally breaking
> networking, please apply this change on top of v7.0-rc1 and provide
> it as a stable tag to be pulled in netdev.
> 
> Vladimir Oltean (3):
>   phy: lynx-28g: use timeouts when waiting for lane halt and reset
>   phy: lynx-28g: truly power the lanes up or down
>   phy: lynx-28g: implement phy_exit() operation
> 
>  drivers/phy/freescale/phy-fsl-lynx-28g.c | 190 +++++++++++++++++++----
>  1 file changed, 157 insertions(+), 33 deletions(-)
> 
> -- 
> 2.34.1
>

I've analyzed the AI review output from
https://sashiko.dev/#/patchset/20260321011451.1557091-1-vladimir.oltean%40nxp.com
and I don't consider anything from it to be actionable.

> > @@ -1065,7 +1097,7 @@ static void lynx_28g_cdr_lock_check(struct work_struct *work)
> >  	struct lynx_28g_priv *priv = work_to_lynx(work);
> >  	struct lynx_28g_lane *lane;
> >  	u32 rrstctl;
> > -	int i;
> > +	int err, i;
> >
> >  	for (i = 0; i < LYNX_28G_NUM_LANE; i++) {
> >  		lane = &priv->lane[i];
> Can this dereference a NULL pointer for unpopulated lanes?
> If the device tree contains available child nodes, lynx_28g_probe_lane()
> is only called for lanes explicitly defined by the "reg" property, leaving
> lane->phy as NULL for the rest.
> Since the loop unconditionally iterates over all LYNX_28G_NUM_LANE (8) lanes:
> 	for (i = 0; i < LYNX_28G_NUM_LANE; i++) {
> 		lane = &priv->lane[i];
> 		mutex_lock(&lane->phy->mutex);
> 		if (!lane->init || !lane->powered_up) {
> Would it crash on mutex_lock(&lane->phy->mutex) before checking
> if the lane is initialized?

This is fixed as of commit
https://git.kernel.org/pub/scm/linux/kernel/git/phy/linux-phy.git/commit/?id=a258d843a3e4cb687da19437f8f81fee55ad7d35
not present in linux-phy/next.

> > @@ -1081,9 +1113,17 @@ static void lynx_28g_cdr_lock_check(struct work_struct *work)
> >  		if (!(rrstctl & LNaRRSTCTL_CDR_LOCK)) {
> >  			lynx_28g_lane_rmw(lane, LNaRRSTCTL, LNaRRSTCTL_RST_REQ,
> >  					  LNaRRSTCTL_RST_REQ);
> > -			do {
> > -				rrstctl = lynx_28g_lane_read(lane, LNaRRSTCTL);
> > -			} while (!(rrstctl & LNaRRSTCTL_RST_DONE));
> > +
> > +			err = read_poll_timeout(lynx_28g_lane_read, rrstctl,
> > +						!!(rrstctl & LNaRRSTCTL_RST_DONE),
> > +						LYNX_28G_LANE_RESET_SLEEP_US,
> > +						LYNX_28G_LANE_RESET_TIMEOUT_US,
> > +						false, lane, LNaRRSTCTL);
> > +			if (err) {
> > +				dev_warn_once(&lane->phy->dev,
> > +					      "Lane %c receiver reset failed: %pe\n",
> > +					      'A' + lane->id, ERR_PTR(err));
> > +			}
> Since dev_warn_once() uses a static boolean flag that is evaluated per call
> site rather than per lane, will this mask errors for subsequent lanes?
> If a receiver reset times out on any lane, the warning is printed and the
> static flag is set to true. If any other lane subsequently fails the reset,
> the warning will be silently suppressed.
> Would a per-lane state tracking mechanism or dev_warn_ratelimited() be more
> appropriate here to ensure multi-lane failures can be diagnosed?

Unjustified complexity for an error case that should never really trigger
in production environments.

> > diff --git a/drivers/phy/freescale/phy-fsl-lynx-28g.c b/drivers/phy/freescale/phy-fsl-lynx-28g.c
> > --- a/drivers/phy/freescale/phy-fsl-lynx-28g.c
> > +++ b/drivers/phy/freescale/phy-fsl-lynx-28g.c
> [ ... ]
> > +static int lynx_28g_power_on(struct phy *phy)
> > +{
> > +	struct lynx_28g_lane *lane = phy_get_drvdata(phy);
> > +	int err;
> > +
> > +	if (lane->powered_up)
> > +		return 0;
> > +
> > +	/* Power up the RX and TX portions of the lane */
> > +	lynx_28g_lane_rmw(lane, LNaRRSTCTL, 0, LNaRRSTCTL_DIS);
> > +	lynx_28g_lane_rmw(lane, LNaTRSTCTL, 0, LNaTRSTCTL_DIS);
> > +
> > +	err = lynx_28g_lane_reset(phy);
> > +	if (err)
> > +		return err;
> If lynx_28g_lane_reset() returns an error, the function returns immediately
> without setting lane->powered_up to true. Does this leave the hardware
> powered up while the software state considers it powered off?
> If a subsequent cleanup calls lynx_28g_power_off(), it will check
> if (!lane->powered_up) and return 0 immediately, leaving the DIS bits
> cleared permanently. Should the error path restore the DIS bits before
> returning?

Same. If lynx_28g_lane_reset() times out, there is really no way to keep
the software state in sync no matter whether lane->powered_up is true or
false. The lane is broken regardless.

> > @@ -1007,7 +1074,7 @@ static int lynx_28g_set_mode(struct phy *phy, enum phy_mode mode, int submode)
> >
> >  out:
> >  	if (powered_up)
> > -		lynx_28g_power_on(phy);
> > +		lynx_28g_lane_reset(phy);
> >
> >  	return err;
> >  }
> If lynx_28g_lane_reset() fails during the success path where err is 0,
> the return value of the reset call is discarded and the function still
> returns 0.
> Can this hide a hardware failure from the caller, misleading the MAC driver
> into thinking the link is successfully brought back up? Should err be
> updated with the return value of lynx_28g_lane_reset() here if err is
> currently 0?

As explained in the commit message, handling and propagating the
lynx_28g_lane_reset() error to the caller isn't really any better.
The caller would think that lynx_28g_set_mode() failed, thus that the
mode wasn't changed. But it was changed.

I do not plan to send a v2 to address any of the change suggestions above.