* [PATCH] phy: fsl-lynx-10g: propagate PCVT enable errors @ 2026-07-20 10:19 ` kr494167 0 siblings, 0 replies; 6+ messages in thread From: kr494167 @ 2026-07-20 10:19 UTC (permalink / raw) To: ioana.ciornei, vladimir.oltean, vkoul Cc: neil.armstrong, netdev, linux-phy, linux-kernel, surendra 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] phy: fsl-lynx-10g: propagate PCVT enable errors @ 2026-07-20 10:19 ` kr494167 0 siblings, 0 replies; 6+ messages in thread From: kr494167 @ 2026-07-20 10:19 UTC (permalink / raw) To: ioana.ciornei, vladimir.oltean, vkoul Cc: neil.armstrong, netdev, linux-phy, linux-kernel, surendra 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] phy: fsl-lynx-10g: propagate PCVT enable errors 2026-07-20 10:19 ` kr494167 @ 2026-07-20 12:14 ` Breno Leitao -1 siblings, 0 replies; 6+ 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] 6+ messages in thread
* Re: [PATCH] phy: fsl-lynx-10g: propagate PCVT enable errors @ 2026-07-20 12:14 ` Breno Leitao 0 siblings, 0 replies; 6+ 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 -- 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
* Re: [PATCH] phy: fsl-lynx-10g: propagate PCVT enable errors 2026-07-20 10:19 ` kr494167 @ 2026-07-20 12:16 ` Vladimir Oltean -1 siblings, 0 replies; 6+ 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] 6+ messages in thread
* Re: [PATCH] phy: fsl-lynx-10g: propagate PCVT enable errors @ 2026-07-20 12:16 ` Vladimir Oltean 0 siblings, 0 replies; 6+ 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. -- 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
end of thread, other threads:[~2026-07-20 12:16 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-07-20 10:19 [PATCH] phy: fsl-lynx-10g: propagate PCVT enable errors kr494167 2026-07-20 10:19 ` kr494167 2026-07-20 12:14 ` Breno Leitao 2026-07-20 12:14 ` Breno Leitao 2026-07-20 12:16 ` Vladimir Oltean 2026-07-20 12:16 ` Vladimir Oltean
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.