* Re: [PATCH] phy: fsl-lynx-28g: propagate PCVT enable errors
2026-07-22 3:36 [PATCH] phy: fsl-lynx-28g: propagate PCVT enable errors kr494167
@ 2026-07-22 8:00 ` Vladimir Oltean
2026-07-22 13:37 ` Andrew Lunn
2026-07-23 3:36 ` sashiko-bot
2 siblings, 0 replies; 4+ messages in thread
From: Vladimir Oltean @ 2026-07-22 8:00 UTC (permalink / raw)
To: kr494167
Cc: ioana.ciornei, vkoul, neil.armstrong, davem, netdev, linux-phy,
linux-kernel
Hi Surendra,
On Wed, Jul 22, 2026 at 09:06:19AM +0530, kr494167@gmail.com wrote:
> From: surendra <kr494167@gmail.com>
>
> lynx_28g_set_mode() currently ignores failures from
> lynx_28g_lane_enable_pcvt(). It then updates the lane mode and reports
> success even though the protocol converter may remain disabled.
>
> Propagate the error and leave the previous lane mode intact so the caller
> can handle the failed reconfiguration.
>
> Fixes: 8f73b37cf3fb ("phy: add support for the Layerscape SerDes 28G")
> Signed-off-by: surendra <kr494167@gmail.com>
> ---
> drivers/phy/freescale/phy-fsl-lynx-28g.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/phy/freescale/phy-fsl-lynx-28g.c b/drivers/phy/freescale/phy-fsl-lynx-28g.c
> index 38afcd081a2a..1fe406adc83c 100644
> --- a/drivers/phy/freescale/phy-fsl-lynx-28g.c
> +++ b/drivers/phy/freescale/phy-fsl-lynx-28g.c
> @@ -1003,7 +1003,9 @@ static int lynx_28g_set_mode(struct phy *phy, enum phy_mode mode, int submode)
>
> lynx_28g_lane_change_proto_conf(lane, lane_mode);
> lynx_28g_lane_remap_pll(lane, lane_mode);
> - WARN_ON(lynx_28g_lane_enable_pcvt(lane, lane_mode));
> + err = lynx_28g_lane_enable_pcvt(lane, lane_mode);
> + if (err)
> + goto out;
>
> lane->mode = lane_mode;
>
> --
> 2.55.0
>
Did you draw any conclusions from the feedback you received on the
lynx-10g patch and decided that it doesn't apply to lynx-28g?
https://lore.kernel.org/linux-phy/20260720101906.80584-1-kr494167@gmail.com/
Hint: it does.
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] phy: fsl-lynx-28g: propagate PCVT enable errors
2026-07-22 3:36 [PATCH] phy: fsl-lynx-28g: propagate PCVT enable errors kr494167
2026-07-22 8:00 ` Vladimir Oltean
@ 2026-07-22 13:37 ` Andrew Lunn
2026-07-23 3:36 ` sashiko-bot
2 siblings, 0 replies; 4+ messages in thread
From: Andrew Lunn @ 2026-07-22 13:37 UTC (permalink / raw)
To: kr494167
Cc: ioana.ciornei, vladimir.oltean, vkoul, neil.armstrong, davem,
netdev, linux-phy, linux-kernel
On Wed, Jul 22, 2026 at 09:06:19AM +0530, kr494167@gmail.com wrote:
> From: surendra <kr494167@gmail.com>
>
> lynx_28g_set_mode() currently ignores failures from
> lynx_28g_lane_enable_pcvt(). It then updates the lane mode and reports
> success even though the protocol converter may remain disabled.
>
> Propagate the error and leave the previous lane mode intact so the caller
> can handle the failed reconfiguration.
>
> Fixes: 8f73b37cf3fb ("phy: add support for the Layerscape SerDes 28G")
> Signed-off-by: surendra <kr494167@gmail.com>
> ---
> drivers/phy/freescale/phy-fsl-lynx-28g.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/phy/freescale/phy-fsl-lynx-28g.c b/drivers/phy/freescale/phy-fsl-lynx-28g.c
> index 38afcd081a2a..1fe406adc83c 100644
> --- a/drivers/phy/freescale/phy-fsl-lynx-28g.c
> +++ b/drivers/phy/freescale/phy-fsl-lynx-28g.c
> @@ -1003,7 +1003,9 @@ static int lynx_28g_set_mode(struct phy *phy, enum phy_mode mode, int submode)
>
> lynx_28g_lane_change_proto_conf(lane, lane_mode);
> lynx_28g_lane_remap_pll(lane, lane_mode);
> - WARN_ON(lynx_28g_lane_enable_pcvt(lane, lane_mode));
A WARN_ON() produces a very noticeable splat, which will aid
debugging. Just returning the error code makes is easy to miss what
causes the issue. Maybe the WARN_ON() should be kept? At least, i
would add justification to the commit message for removing it.
Andrew
---
pw-bot: cr
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] phy: fsl-lynx-28g: propagate PCVT enable errors
2026-07-22 3:36 [PATCH] phy: fsl-lynx-28g: propagate PCVT enable errors kr494167
2026-07-22 8:00 ` Vladimir Oltean
2026-07-22 13:37 ` Andrew Lunn
@ 2026-07-23 3:36 ` sashiko-bot
2 siblings, 0 replies; 4+ messages in thread
From: sashiko-bot @ 2026-07-23 3:36 UTC (permalink / raw)
To: kr494167; +Cc: neil.armstrong, vkoul, linux-phy, olteanv
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Hardware and software state desynchronization on PCVT enable failure prevents mode recovery.
--
--- Patch [1]: [PATCH] phy: fsl-lynx-28g: propagate PCVT enable errors ---
commit 6193a7adcb18a183b9462da919d480f3a6949562
Author: surendra <kr494167@gmail.com>
phy: fsl-lynx-28g: propagate PCVT enable errors
This commit updates lynx_28g_set_mode() to check the return value of
lynx_28g_lane_enable_pcvt(). If an error occurs, the code now propagates
the error and exits before updating the software lane mode tracking.
> diff --git a/drivers/phy/freescale/phy-fsl-lynx-28g.c b/drivers/phy/freescale/phy-fsl-lynx-28g.c
> index 38afcd081a2a7..1fe406adc83cb 100644
> --- a/drivers/phy/freescale/phy-fsl-lynx-28g.c
> +++ b/drivers/phy/freescale/phy-fsl-lynx-28g.c
> @@ -1003,7 +1003,9 @@ static int lynx_28g_set_mode(struct phy *phy, enum phy_mode mode, int submode)
>
> lynx_28g_lane_change_proto_conf(lane, lane_mode);
> lynx_28g_lane_remap_pll(lane, lane_mode);
> - WARN_ON(lynx_28g_lane_enable_pcvt(lane, lane_mode));
> + err = lynx_28g_lane_enable_pcvt(lane, lane_mode);
> + if (err)
> + goto out;
[Severity: Medium]
Does this new error path leave the software and hardware states
desynchronized?
Prior to this point, the hardware has already been reprogrammed to the
new mode by lynx_28g_lane_change_proto_conf() and
lynx_28g_lane_remap_pll(). If lynx_28g_lane_enable_pcvt() fails and takes
the goto out path, lane->mode remains set to the old mode.
If a caller later attempts to recover by setting the PHY back to the old
mode, lynx_28g_set_mode() will see:
if (lane_mode == lane->mode)
return 0;
and short-circuit, preventing the hardware from actually being reverted.
Should the error path either revert the hardware changes or set
lane->mode = LANE_MODE_UNKNOWN to ensure subsequent recovery attempts bypass
the short-circuit check?
>
> lane->mode = lane_mode;
>
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260722033619.10647-1-kr494167@gmail.com?part=1
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply [flat|nested] 4+ messages in thread