* [PATCH phy-next 1/3] phy: lynx-28g: use timeouts when waiting for lane halt and reset
2026-03-21 1:14 [PATCH phy-next 0/3] Lynx 28G: better init(), exit(), power_on(), power_off() Vladimir Oltean
@ 2026-03-21 1:14 ` Vladimir Oltean
2026-03-21 15:48 ` Andrew Lunn
2026-03-21 1:14 ` [PATCH phy-next 2/3] phy: lynx-28g: truly power the lanes up or down Vladimir Oltean
` (2 subsequent siblings)
3 siblings, 1 reply; 6+ messages in thread
From: Vladimir Oltean @ 2026-03-21 1:14 UTC (permalink / raw)
To: linux-phy
Cc: netdev, Ioana Ciornei, Vinod Koul, Neil Armstrong, Josua Mayer,
linux-kernel
There are various circumstances in which a lane halt, or a lane reset,
will fail to complete. If this happens, it will hang the kernel, which
only implements a busy loop with no timeout.
The circumstances in which this will happen are all bugs in nature:
- if we try to power off a powered off lane
- if we try to power off a lane that uses a PLL locked onto the wrong
refclk frequency (wrong RCW, but SoC boots anyway)
Actually, unbounded loops in the kernel are a bad practice, so let's use
read_poll_timeout() with a custom function that reads both LNaTRSTCTL
(lane transmit control register) and LNaRRSTCTL (lane receive control
register) and returns true when the request is done in both directions.
The HLT_REQ bit has to clear, whereas the RST_DONE bit has to get set.
Any time such an error happens, it is catastrophic and there is no point
in trying to propagate it to our callers:
- if lynx_28g_set_mode() -> lynx_28g_power_on() times out, we have
already reconfigured the lane, but returning an error would tell the
caller that we didn't
- if lynx_28g_power_off() times out, again not much for the consumer to
do to help get out of this situation - the phy_power_off() call is
probably made from a context that the consumer can't cancel, or it is
making it to return to a known state from a previous failure.
So just print an error if timeouts happen and let the driver control
flow continue. The entire point is just to not let the kernel freeze.
Suggested-by: Josua Mayer <josua@solid-run.com>
Link: https://lore.kernel.org/lkml/d0c8bbf8-a0c5-469f-a148-de2235948c0f@solid-run.com/
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Previously submitted as part of larger set:
https://lore.kernel.org/linux-phy/20260114152111.625350-7-vladimir.oltean@nxp.com/
Changes:
- Stop propagating the read_poll_timeout() errors to callers
- Adjust commit message to explain that decision
---
drivers/phy/freescale/phy-fsl-lynx-28g.c | 72 ++++++++++++++++++------
1 file changed, 56 insertions(+), 16 deletions(-)
diff --git a/drivers/phy/freescale/phy-fsl-lynx-28g.c b/drivers/phy/freescale/phy-fsl-lynx-28g.c
index 2b0fd95ba62f..3debf4131e0f 100644
--- a/drivers/phy/freescale/phy-fsl-lynx-28g.c
+++ b/drivers/phy/freescale/phy-fsl-lynx-28g.c
@@ -249,6 +249,12 @@
#define CR(x) ((x) * 4)
+#define LYNX_28G_LANE_HALT_SLEEP_US 100
+#define LYNX_28G_LANE_HALT_TIMEOUT_US 1000000
+
+#define LYNX_28G_LANE_RESET_SLEEP_US 100
+#define LYNX_28G_LANE_RESET_TIMEOUT_US 1000000
+
enum lynx_28g_eq_type {
EQ_TYPE_NO_EQ = 0,
EQ_TYPE_2TAP = 1,
@@ -600,10 +606,29 @@ static void lynx_28g_lane_set_pll(struct lynx_28g_lane *lane,
}
}
+static bool lynx_28g_lane_halt_done(struct lynx_28g_lane *lane)
+{
+ u32 trstctl = lynx_28g_lane_read(lane, LNaTRSTCTL);
+ u32 rrstctl = lynx_28g_lane_read(lane, LNaRRSTCTL);
+
+ return !(trstctl & LNaTRSTCTL_HLT_REQ) &&
+ !(rrstctl & LNaRRSTCTL_HLT_REQ);
+}
+
+static bool lynx_28g_lane_reset_done(struct lynx_28g_lane *lane)
+{
+ u32 trstctl = lynx_28g_lane_read(lane, LNaTRSTCTL);
+ u32 rrstctl = lynx_28g_lane_read(lane, LNaRRSTCTL);
+
+ return (trstctl & LNaTRSTCTL_RST_DONE) &&
+ (rrstctl & LNaRRSTCTL_RST_DONE);
+}
+
static int lynx_28g_power_off(struct phy *phy)
{
struct lynx_28g_lane *lane = phy_get_drvdata(phy);
- u32 trstctl, rrstctl;
+ bool done;
+ int err;
if (!lane->powered_up)
return 0;
@@ -615,11 +640,14 @@ static int lynx_28g_power_off(struct phy *phy)
LNaRRSTCTL_HLT_REQ);
/* Wait until the halting process is complete */
- do {
- trstctl = lynx_28g_lane_read(lane, LNaTRSTCTL);
- rrstctl = lynx_28g_lane_read(lane, LNaRRSTCTL);
- } while ((trstctl & LNaTRSTCTL_HLT_REQ) ||
- (rrstctl & LNaRRSTCTL_HLT_REQ));
+ err = read_poll_timeout(lynx_28g_lane_halt_done, done, done,
+ LYNX_28G_LANE_HALT_SLEEP_US,
+ LYNX_28G_LANE_HALT_TIMEOUT_US,
+ false, lane);
+ if (err) {
+ dev_err(&phy->dev, "Lane %c halt failed: %pe\n",
+ 'A' + lane->id, ERR_PTR(err));
+ }
lane->powered_up = false;
@@ -629,7 +657,8 @@ static int lynx_28g_power_off(struct phy *phy)
static int lynx_28g_power_on(struct phy *phy)
{
struct lynx_28g_lane *lane = phy_get_drvdata(phy);
- u32 trstctl, rrstctl;
+ bool done;
+ int err;
if (lane->powered_up)
return 0;
@@ -641,11 +670,14 @@ static int lynx_28g_power_on(struct phy *phy)
LNaRRSTCTL_RST_REQ);
/* Wait until the reset sequence is completed */
- do {
- trstctl = lynx_28g_lane_read(lane, LNaTRSTCTL);
- rrstctl = lynx_28g_lane_read(lane, LNaRRSTCTL);
- } while (!(trstctl & LNaTRSTCTL_RST_DONE) ||
- !(rrstctl & LNaRRSTCTL_RST_DONE));
+ err = read_poll_timeout(lynx_28g_lane_reset_done, done, done,
+ LYNX_28G_LANE_RESET_SLEEP_US,
+ LYNX_28G_LANE_RESET_TIMEOUT_US,
+ false, lane);
+ if (err) {
+ dev_err(&phy->dev, "Lane %c reset failed: %pe\n",
+ 'A' + lane->id, ERR_PTR(err));
+ }
lane->powered_up = true;
@@ -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];
@@ -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));
+ }
}
mutex_unlock(&lane->phy->mutex);
--
2.34.1
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH phy-next 1/3] phy: lynx-28g: use timeouts when waiting for lane halt and reset
2026-03-21 1:14 ` [PATCH phy-next 1/3] phy: lynx-28g: use timeouts when waiting for lane halt and reset Vladimir Oltean
@ 2026-03-21 15:48 ` Andrew Lunn
0 siblings, 0 replies; 6+ messages in thread
From: Andrew Lunn @ 2026-03-21 15:48 UTC (permalink / raw)
To: Vladimir Oltean
Cc: linux-phy, netdev, Ioana Ciornei, Vinod Koul, Neil Armstrong,
Josua Mayer, linux-kernel
On Sat, Mar 21, 2026 at 03:14:49AM +0200, Vladimir Oltean wrote:
> There are various circumstances in which a lane halt, or a lane reset,
> will fail to complete. If this happens, it will hang the kernel, which
> only implements a busy loop with no timeout.
>
> The circumstances in which this will happen are all bugs in nature:
> - if we try to power off a powered off lane
> - if we try to power off a lane that uses a PLL locked onto the wrong
> refclk frequency (wrong RCW, but SoC boots anyway)
>
> Actually, unbounded loops in the kernel are a bad practice, so let's use
> read_poll_timeout() with a custom function that reads both LNaTRSTCTL
> (lane transmit control register) and LNaRRSTCTL (lane receive control
> register) and returns true when the request is done in both directions.
>
> The HLT_REQ bit has to clear, whereas the RST_DONE bit has to get set.
>
> Any time such an error happens, it is catastrophic and there is no point
> in trying to propagate it to our callers:
> - if lynx_28g_set_mode() -> lynx_28g_power_on() times out, we have
> already reconfigured the lane, but returning an error would tell the
> caller that we didn't
> - if lynx_28g_power_off() times out, again not much for the consumer to
> do to help get out of this situation - the phy_power_off() call is
> probably made from a context that the consumer can't cancel, or it is
> making it to return to a known state from a previous failure.
>
> So just print an error if timeouts happen and let the driver control
> flow continue. The entire point is just to not let the kernel freeze.
>
> Suggested-by: Josua Mayer <josua@solid-run.com>
> Link: https://lore.kernel.org/lkml/d0c8bbf8-a0c5-469f-a148-de2235948c0f@solid-run.com/
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH phy-next 2/3] phy: lynx-28g: truly power the lanes up or down
2026-03-21 1:14 [PATCH phy-next 0/3] Lynx 28G: better init(), exit(), power_on(), power_off() Vladimir Oltean
2026-03-21 1:14 ` [PATCH phy-next 1/3] phy: lynx-28g: use timeouts when waiting for lane halt and reset Vladimir Oltean
@ 2026-03-21 1:14 ` Vladimir Oltean
2026-03-21 1:14 ` [PATCH phy-next 3/3] phy: lynx-28g: implement phy_exit() operation Vladimir Oltean
2026-03-30 11:46 ` [PATCH phy-next 0/3] Lynx 28G: better init(), exit(), power_on(), power_off() Vladimir Oltean
3 siblings, 0 replies; 6+ messages in thread
From: Vladimir Oltean @ 2026-03-21 1:14 UTC (permalink / raw)
To: linux-phy
Cc: netdev, Ioana Ciornei, Vinod Koul, Neil Armstrong, Josua Mayer,
linux-kernel
The current procedure for power_off() and power_on() is the same as the
one used for major lane reconfiguration, aka halting. But this is
incorrect.
One would expect that a powered off lane causes the CDR (clock and data
recovery) loop of the link partner to lose lock onto its RX stream
(which suggests there are no longer any bit transitions => the channel
is inactive). However, it can be observed that this does not take place
(the CDR lock is still there), which means that a halted lane is not
powered off.
Implement the procedure mentioned in the block guide for powering down
a lane, and then back on. This means:
- lynx_28g_power_off() currently emits a HLT_REQ and waits for it to
clear. Rename it to lynx_28g_lane_halt() and keep using it for
lynx_28g_set_mode().
- lynx_28g_power_on() currently emits a RST_REQ and waits for it to
clear. This is the reset procedure - rename it to
lynx_28g_lane_reset(), and keep using it for lynx_28g_set_mode().
The "real" lynx_28g_power_off() should emit a STP_REQ and wait for it to
clear. And the "real" lynx_28g_power_on() should clear the DIS bit and
then perform a lane reset. Hook these methods to the phy_ops ::
power_off() and power_on() instead.
As opposed to lynx_28g_power_off() which is also directly hooked to the
PHY API, lynx_28g_lane_halt() isn't; just to lynx_28g_set_mode(), and
the call is being made in a state where we haven't yet made any change
to the lane. So it does make some limited amount of sense to code this
up such that if it fails, we make an attempt to reset the lane anyway
and let the consumer know that phy_set_mode_ext() failed.
Sort the field definitions in LNaTRSTCTL and LNaRRSTCTL in descending
order, and add new definitions for STP_REQ and DIS, previously unused.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Previously submitted as part of larger set:
https://lore.kernel.org/linux-phy/20260114152111.625350-8-vladimir.oltean@nxp.com/
Changes:
- stop propagating read_poll_timeout() error code to the new
lynx_28g_power_off() implementation, for the same reason as the
previous patch.
- do propagate the read_poll_timeout() error code to
lynx_28g_lane_halt(), and make an attempt to reset the lane.
- more detailed explanation in commit message
---
drivers/phy/freescale/phy-fsl-lynx-28g.c | 103 +++++++++++++++++++----
1 file changed, 85 insertions(+), 18 deletions(-)
diff --git a/drivers/phy/freescale/phy-fsl-lynx-28g.c b/drivers/phy/freescale/phy-fsl-lynx-28g.c
index 3debf4131e0f..b056951506dc 100644
--- a/drivers/phy/freescale/phy-fsl-lynx-28g.c
+++ b/drivers/phy/freescale/phy-fsl-lynx-28g.c
@@ -70,9 +70,11 @@
/* Lane a Tx Reset Control Register */
#define LNaTRSTCTL(lane) (0x800 + (lane) * 0x100 + 0x20)
-#define LNaTRSTCTL_HLT_REQ BIT(27)
-#define LNaTRSTCTL_RST_DONE BIT(30)
#define LNaTRSTCTL_RST_REQ BIT(31)
+#define LNaTRSTCTL_RST_DONE BIT(30)
+#define LNaTRSTCTL_HLT_REQ BIT(27)
+#define LNaTRSTCTL_STP_REQ BIT(26)
+#define LNaTRSTCTL_DIS BIT(24)
/* Lane a Tx General Control Register */
#define LNaTGCR0(lane) (0x800 + (lane) * 0x100 + 0x24)
@@ -98,9 +100,11 @@
/* Lane a Rx Reset Control Register */
#define LNaRRSTCTL(lane) (0x800 + (lane) * 0x100 + 0x40)
-#define LNaRRSTCTL_HLT_REQ BIT(27)
-#define LNaRRSTCTL_RST_DONE BIT(30)
#define LNaRRSTCTL_RST_REQ BIT(31)
+#define LNaRRSTCTL_RST_DONE BIT(30)
+#define LNaRRSTCTL_HLT_REQ BIT(27)
+#define LNaRRSTCTL_STP_REQ BIT(26)
+#define LNaRRSTCTL_DIS BIT(24)
#define LNaRRSTCTL_CDR_LOCK BIT(12)
/* Lane a Rx General Control Register */
@@ -255,6 +259,9 @@
#define LYNX_28G_LANE_RESET_SLEEP_US 100
#define LYNX_28G_LANE_RESET_TIMEOUT_US 1000000
+#define LYNX_28G_LANE_STOP_SLEEP_US 100
+#define LYNX_28G_LANE_STOP_TIMEOUT_US 1000000
+
enum lynx_28g_eq_type {
EQ_TYPE_NO_EQ = 0,
EQ_TYPE_2TAP = 1,
@@ -615,6 +622,15 @@ static bool lynx_28g_lane_halt_done(struct lynx_28g_lane *lane)
!(rrstctl & LNaRRSTCTL_HLT_REQ);
}
+static bool lynx_28g_lane_stop_done(struct lynx_28g_lane *lane)
+{
+ u32 trstctl = lynx_28g_lane_read(lane, LNaTRSTCTL);
+ u32 rrstctl = lynx_28g_lane_read(lane, LNaRRSTCTL);
+
+ return !(trstctl & LNaTRSTCTL_STP_REQ) &&
+ !(rrstctl & LNaRRSTCTL_STP_REQ);
+}
+
static bool lynx_28g_lane_reset_done(struct lynx_28g_lane *lane)
{
u32 trstctl = lynx_28g_lane_read(lane, LNaTRSTCTL);
@@ -624,15 +640,13 @@ static bool lynx_28g_lane_reset_done(struct lynx_28g_lane *lane)
(rrstctl & LNaRRSTCTL_RST_DONE);
}
-static int lynx_28g_power_off(struct phy *phy)
+/* Halting puts the lane in a mode in which it can be reconfigured */
+static int lynx_28g_lane_halt(struct phy *phy)
{
struct lynx_28g_lane *lane = phy_get_drvdata(phy);
bool done;
int err;
- if (!lane->powered_up)
- return 0;
-
/* Issue a halt request */
lynx_28g_lane_rmw(lane, LNaTRSTCTL, LNaTRSTCTL_HLT_REQ,
LNaTRSTCTL_HLT_REQ);
@@ -649,20 +663,15 @@ static int lynx_28g_power_off(struct phy *phy)
'A' + lane->id, ERR_PTR(err));
}
- lane->powered_up = false;
-
- return 0;
+ return err;
}
-static int lynx_28g_power_on(struct phy *phy)
+static int lynx_28g_lane_reset(struct phy *phy)
{
struct lynx_28g_lane *lane = phy_get_drvdata(phy);
bool done;
int err;
- if (lane->powered_up)
- return 0;
-
/* Issue a reset request on the lane */
lynx_28g_lane_rmw(lane, LNaTRSTCTL, LNaTRSTCTL_RST_REQ,
LNaTRSTCTL_RST_REQ);
@@ -679,6 +688,61 @@ static int lynx_28g_power_on(struct phy *phy)
'A' + lane->id, ERR_PTR(err));
}
+ return err;
+}
+
+static int lynx_28g_power_off(struct phy *phy)
+{
+ struct lynx_28g_lane *lane = phy_get_drvdata(phy);
+ bool done;
+ int err;
+
+ if (!lane->powered_up)
+ return 0;
+
+ /* Issue a stop request */
+ lynx_28g_lane_rmw(lane, LNaTRSTCTL, LNaTRSTCTL_STP_REQ,
+ LNaTRSTCTL_STP_REQ);
+ lynx_28g_lane_rmw(lane, LNaRRSTCTL, LNaRRSTCTL_STP_REQ,
+ LNaRRSTCTL_STP_REQ);
+
+ /* Wait until the stop process is complete */
+ err = read_poll_timeout(lynx_28g_lane_stop_done, done, done,
+ LYNX_28G_LANE_STOP_SLEEP_US,
+ LYNX_28G_LANE_STOP_TIMEOUT_US,
+ false, lane);
+ if (err) {
+ dev_err(&phy->dev, "Lane %c stop failed: %pe\n",
+ 'A' + lane->id, ERR_PTR(err));
+ }
+
+ /* Power down the RX and TX portions of the lane */
+ lynx_28g_lane_rmw(lane, LNaRRSTCTL, LNaRRSTCTL_DIS,
+ LNaRRSTCTL_DIS);
+ lynx_28g_lane_rmw(lane, LNaTRSTCTL, LNaTRSTCTL_DIS,
+ LNaTRSTCTL_DIS);
+
+ lane->powered_up = false;
+
+ return 0;
+}
+
+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;
+
lane->powered_up = true;
return 0;
@@ -992,8 +1056,11 @@ static int lynx_28g_set_mode(struct phy *phy, enum phy_mode mode, int submode)
/* If the lane is powered up, put the lane into the halt state while
* the reconfiguration is being done.
*/
- if (powered_up)
- lynx_28g_power_off(phy);
+ if (powered_up) {
+ err = lynx_28g_lane_halt(phy);
+ if (err)
+ goto out;
+ }
err = lynx_28g_lane_disable_pcvt(lane, lane->mode);
if (err)
@@ -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;
}
--
2.34.1
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH phy-next 3/3] phy: lynx-28g: implement phy_exit() operation
2026-03-21 1:14 [PATCH phy-next 0/3] Lynx 28G: better init(), exit(), power_on(), power_off() Vladimir Oltean
2026-03-21 1:14 ` [PATCH phy-next 1/3] phy: lynx-28g: use timeouts when waiting for lane halt and reset Vladimir Oltean
2026-03-21 1:14 ` [PATCH phy-next 2/3] phy: lynx-28g: truly power the lanes up or down Vladimir Oltean
@ 2026-03-21 1:14 ` Vladimir Oltean
2026-03-30 11:46 ` [PATCH phy-next 0/3] Lynx 28G: better init(), exit(), power_on(), power_off() Vladimir Oltean
3 siblings, 0 replies; 6+ messages in thread
From: Vladimir Oltean @ 2026-03-21 1:14 UTC (permalink / raw)
To: linux-phy
Cc: netdev, Ioana Ciornei, Vinod Koul, Neil Armstrong, Josua Mayer,
linux-kernel
On Layerscape, some Lynx SerDes consumers go through the PHY framework
(we call these 'managed' for the sake of this discussion; they are some
Ethernet lanes), and some consumers don't go through the PHY framework
(we call these 'unmanaged'; they are some unconverted Ethernet lanes,
plus all SATA and PCIe controllers).
A lane is initially unmanaged, and becomes managed when phy_init() is
called on it. Similarly, it becomes unmanaged again when phy_exit() is
called.
Managed lanes are supposed to have power management through
phy_power_on() and phy_power_off().
The lynx-28g SerDes driver, when it probes, probes on all lanes, but
needs to be careful to keep the unmanaged lanes powered on, because
those might be in use by consumers unaware that they need to call
phy_init() and phy_power_on(). This also applies after phy_exit() is
called - no guarantee is made about how the port may be used
afterwards - may be DPDK, may be something else which is unaware of
the PHY framework.
Given this state table:
State | Consumer calls phy_exit()? | Provider implements phy_exit()?
-------+----------------------------+--------------------------------
(a) | no | no
(b) | no | yes
(c) | yes | no
(d) | yes | yes
we are currently in state (a). This has the problem that when the
consumer fails to probe with -EPROBE_DEFER or is otherwise unbound,
the phy->init_count remains elevated and the lane never returns to the
unmanaged state (temporarily or not) as it should. Furthermore, the
second and subsequent phy_init() consumer calls are never passed on to
the provider driver.
We solve the above problem by implementing phy_exit() in the consumer,
and that moves us to state (c). But this creates the problem that a
balanced set of phy_init() and phy_exit() calls from the consumer will
effectively result in multiple lynx_28g_init() calls as seen by the
SerDes and nothing else - as the optional phy_ops :: exit() is not
implemented. But that actually doesn't work - the 28G Lynx SerDes can't
power down a lane which is already powered down; that call sequence
would time out and fail.
So actually we want to be in state (d), where both the provider and the
consumer implement phy_exit(). But we can only do that safely through
intermediary state (b), where the provider implements it first. This
effectively behaves just like (a), except it offers a safe migration
path for the consumer to call phy_exit() as mandated by the Generic PHY
API.
Extra development notes: ignoring the lynx_28g_power_on() error in
lynx_28g_exit() is a deliberate decision. The consumer can't deal with a
teardown path that is not error-free. Ignoring the error is not silent:
lynx_28g_power_on() -> lynx_28g_lane_reset() will print on failure.
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Previously submitted as part of larger set:
https://lore.kernel.org/linux-phy/20260114152111.625350-9-vladimir.oltean@nxp.com/
Changes:
- Stop propagating lynx_28g_power_on() error code to lynx_28g_exit().
- Add more detailed explanation in commit message
---
drivers/phy/freescale/phy-fsl-lynx-28g.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/phy/freescale/phy-fsl-lynx-28g.c b/drivers/phy/freescale/phy-fsl-lynx-28g.c
index b056951506dc..dd54d82a1e1f 100644
--- a/drivers/phy/freescale/phy-fsl-lynx-28g.c
+++ b/drivers/phy/freescale/phy-fsl-lynx-28g.c
@@ -1113,8 +1113,25 @@ static int lynx_28g_init(struct phy *phy)
return 0;
}
+static int lynx_28g_exit(struct phy *phy)
+{
+ struct lynx_28g_lane *lane = phy_get_drvdata(phy);
+
+ /* The lane returns to the state where it isn't managed by the
+ * consumer, so we must treat is as if it isn't initialized, and
+ * always powered on.
+ */
+ lane->init = false;
+ lane->powered_up = false;
+
+ lynx_28g_power_on(phy);
+
+ return 0;
+}
+
static const struct phy_ops lynx_28g_ops = {
.init = lynx_28g_init,
+ .exit = lynx_28g_exit,
.power_on = lynx_28g_power_on,
.power_off = lynx_28g_power_off,
.set_mode = lynx_28g_set_mode,
--
2.34.1
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH phy-next 0/3] Lynx 28G: better init(), exit(), power_on(), power_off()
2026-03-21 1:14 [PATCH phy-next 0/3] Lynx 28G: better init(), exit(), power_on(), power_off() Vladimir Oltean
` (2 preceding siblings ...)
2026-03-21 1:14 ` [PATCH phy-next 3/3] phy: lynx-28g: implement phy_exit() operation Vladimir Oltean
@ 2026-03-30 11:46 ` Vladimir Oltean
3 siblings, 0 replies; 6+ messages in thread
From: Vladimir Oltean @ 2026-03-30 11:46 UTC (permalink / raw)
To: linux-phy
Cc: netdev, Ioana Ciornei, Vinod Koul, Neil Armstrong, Josua Mayer,
linux-kernel
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.
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply [flat|nested] 6+ messages in thread