[PATCH] phy: fsl-lynx-10g: propagate PCVT enable errors

[PATCH] phy: fsl-lynx-10g: propagate PCVT enable errors

[kr494167]

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


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH] phy: fsl-lynx-10g: propagate PCVT enable errors

[kr494167]

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

Re: [PATCH] phy: fsl-lynx-10g: propagate PCVT enable errors

[Breno Leitao]

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

Re: [PATCH] phy: fsl-lynx-10g: propagate PCVT enable errors

[Breno Leitao]

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

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH] phy: fsl-lynx-10g: propagate PCVT enable errors

[Vladimir Oltean]

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.

Re: [PATCH] phy: fsl-lynx-10g: propagate PCVT enable errors

[Vladimir Oltean]

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.

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy