* Re: [PATCH] phy: fsl-lynx-10g: propagate PCVT enable errors
2026-07-20 10:19 [PATCH] phy: fsl-lynx-10g: propagate PCVT enable errors kr494167
@ 2026-07-20 12:14 ` Breno Leitao
2026-07-20 12:16 ` Vladimir Oltean
1 sibling, 0 replies; 3+ messages in thread
From: Breno Leitao @ 2026-07-20 12:14 UTC (permalink / raw)
To: kr494167
Cc: ioana.ciornei, vladimir.oltean, vkoul, neil.armstrong, netdev,
linux-phy, linux-kernel
On Mon, Jul 20, 2026 at 03:49:06PM +0530, kr494167@gmail.com wrote:
> Propagate the error and leave the previous lane mode intact so the caller
> can handle the failed reconfiguration.
Do you need to undo these operations that were done to the lane earlier?
lynx_10g_lane_change_proto_conf(lane, lane_mode);
lynx_10g_lane_remap_pll(lane, lane_mode);
--breno
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] phy: fsl-lynx-10g: propagate PCVT enable errors
2026-07-20 10:19 [PATCH] phy: fsl-lynx-10g: propagate PCVT enable errors kr494167
2026-07-20 12:14 ` Breno Leitao
@ 2026-07-20 12:16 ` Vladimir Oltean
1 sibling, 0 replies; 3+ messages in thread
From: Vladimir Oltean @ 2026-07-20 12:16 UTC (permalink / raw)
To: kr494167
Cc: ioana.ciornei, vkoul, neil.armstrong, netdev, linux-phy,
linux-kernel
Hi Surendra,
On Mon, Jul 20, 2026 at 03:49:06PM +0530, kr494167@gmail.com wrote:
> From: surendra <kr494167@gmail.com>
>
> lynx_10g_set_mode() currently ignores failures from
> lynx_10g_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.
>
> Signed-off-by: surendra <kr494167@gmail.com>
> ---
> drivers/phy/freescale/phy-fsl-lynx-10g.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/phy/freescale/phy-fsl-lynx-10g.c b/drivers/phy/freescale/phy-fsl-lynx-10g.c
> index 38def160ef1a..9740b08700c4 100644
> --- a/drivers/phy/freescale/phy-fsl-lynx-10g.c
> +++ b/drivers/phy/freescale/phy-fsl-lynx-10g.c
> @@ -1231,7 +1231,9 @@ static int lynx_10g_set_mode(struct phy *phy, enum phy_mode mode, int submode)
>
> lynx_10g_lane_change_proto_conf(lane, lane_mode);
> lynx_10g_lane_remap_pll(lane, lane_mode);
> - WARN_ON(lynx_10g_lane_enable_pcvt(lane, lane_mode));
> + err = lynx_10g_lane_enable_pcvt(lane, lane_mode);
> + if (err)
> + goto out;
>
> lane->mode = lane_mode;
>
> --
> 2.55.0
>
Thank you for the patch, however you are fixing a situation which will
never occur.
Assume lynx_10g_lane_enable_pcvt(lane, lane_mode) will fail.
It means one of the following functions inside of it returned a negative
return code:
- lynx_pcvt_rmw(lane, mode, ...)
- lynx_pccr_write(lane, mode, ...)
Let's take them one by one.
lynx_pcvt_rmw() fails if priv->info->get_pcvt_offset(lane->id, mode)
returns negative.
lynx_pccr_write() fails if priv->info->get_pccr(mode, lane->id, ...)
returns negative.
Had any of these two functions returned a negative error code, the
execution would not have reached the lynx_10g_lane_enable_pcvt() stage!
The following code path:
lynx_10g_set_mode()
-> lynx_10g_validate()
-> lynx_phy_mode_to_lane_mode()
-> lynx_lane_supports_mode()
-> priv->info->lane_supports_mode() // this is provided for lynx-28g
OR
-> lynx_lane_supports_mode_default() // all lynx-10g instances use this
-> checks priv->info->get_pccr() and
priv->info->get_pcvt_offset()
already fails much earlier if priv->info->get_pccr() or
priv->info->get_pcvt_offset() return negative (i.e. we don't know how to
handle the requested protocol).
The suggested placement of your error handling is extremely late anyway.
lynx_10g_lane_change_proto_conf() and lynx_10g_lane_remap_pll() have
already been executed, so the lane is reconfigured for the new protocol
and remapped to the new PLL. The "goto out" label doesn't undo that.
This is why I am reluctant to agree with your proposed change - it
doesn't make the code any more correct, it just complicates the handling
and actually makes that impossible case handled *incorrectly* (on
phy_set_mode_ext() failure, you don't expect the PHY to be configured
half way).
It would be very strange to proceed thus far if we didn't first make
sure that the protocol we are switching to is fully supported.
This is why the code has the WARN_ON(). The function _can_ fail, but we
don't need to check more than once for failures on the same mode with
the same protocol, since they are time-invariant.
^ permalink raw reply [flat|nested] 3+ messages in thread