[PATCH 1/2] net: fec: Propagate PTP initialization errors

[PATCH 1/2] net: fec: Propagate PTP initialization errors

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

Change fec_ptp_init() to return an error code instead of silently
ignoring failures during PTP initialization.

The PPS IRQ is not required for the FEC/PTP functionality, so its
absence should not make the probe fail. However, an unavailable
optional IRQ should be distinguished from an actual error returned
during the IRQ lookup.

If a platform does not support the PPS IRQ, it can omit the IRQ from
its device tree and the optional lookup will return -ENXIO. Propagate
other errors from the IRQ lookup instead of silently ignoring them.

Also propagate failures from devm_request_irq() and ptp_clock_register().

Update the function declaration in fec.h accordingly.

Found by manual code inspection.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 drivers/net/ethernet/freescale/fec.h     |  2 +-
 drivers/net/ethernet/freescale/fec_ptp.c | 19 +++++++++++--------
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index 7176803146f3..8831da37326b 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -692,7 +692,7 @@ struct fec_enet_private {
 	u64 ethtool_stats[];
 };
 
-void fec_ptp_init(struct platform_device *pdev, int irq_idx);
+int fec_ptp_init(struct platform_device *pdev, int irq_idx);
 void fec_ptp_restore_state(struct fec_enet_private *fep);
 void fec_ptp_save_state(struct fec_enet_private *fep);
 void fec_ptp_stop(struct platform_device *pdev);
diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index 56801c2009d5..8ad680411b0c 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -736,7 +736,7 @@ static irqreturn_t fec_pps_interrupt(int irq, void *dev_id)
  * cyclecounter init routine and exits.
  */
 
-void fec_ptp_init(struct platform_device *pdev, int irq_idx)
+int fec_ptp_init(struct platform_device *pdev, int irq_idx)
 {
 	struct net_device *ndev = platform_get_drvdata(pdev);
 	struct fec_enet_private *fep = netdev_priv(ndev);
@@ -779,26 +779,29 @@ void fec_ptp_init(struct platform_device *pdev, int irq_idx)
 		      HRTIMER_MODE_REL);
 
 	irq = platform_get_irq_byname_optional(pdev, "pps");
-	if (irq < 0)
+	if (irq < 0) {
 		irq = platform_get_irq_optional(pdev, irq_idx);
-	/* Failure to get an irq is not fatal,
-	 * only the PTP_CLOCK_PPS clock events should stop
-	 */
-	if (irq >= 0) {
+		if (irq < 0 && irq != -ENXIO)
+			return irq;
+	}
+
+	if (irq > 0) {
 		ret = devm_request_irq(&pdev->dev, irq, fec_pps_interrupt,
 				       0, pdev->name, ndev);
 		if (ret < 0)
-			dev_warn(&pdev->dev, "request for pps irq failed(%d)\n",
-				 ret);
+			return ret;
 	}
 
 	fep->ptp_clock = ptp_clock_register(&fep->ptp_caps, &pdev->dev);
 	if (IS_ERR(fep->ptp_clock)) {
 		fep->ptp_clock = NULL;
 		dev_err(&pdev->dev, "ptp_clock_register failed\n");
+		return PTR_ERR(fep->ptp_clock);
 	}
 
 	schedule_delayed_work(&fep->time_keep, HZ);
+
+	return 0;
 }
 
 void fec_ptp_save_state(struct fec_enet_private *fep)
-- 
2.43.0

[PATCH 2/2] net: fec: Handle PTP initialization errors in probe

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

Check the return value of fec_ptp_init() and abort the probe if PTP
initialization fails.

Only call fec_ptp_stop() when PTP support was initialized to avoid
stopping an uninitialized PTP instance during error handling.

Found by manual code inspection.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index ced4dbf8cd90..b6c22d6c1e69 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -5384,8 +5384,11 @@ fec_probe(struct platform_device *pdev)
 		goto failed_reset;
 
 	irq_cnt = fec_enet_get_irq_cnt(pdev);
-	if (fep->bufdesc_ex)
-		fec_ptp_init(pdev, irq_cnt);
+	if (fep->bufdesc_ex) {
+		ret = fec_ptp_init(pdev, irq_cnt);
+		if (ret)
+			goto failed_reset;
+	}
 
 	ret = fec_enet_init(ndev);
 	if (ret)
@@ -5457,7 +5460,8 @@ fec_probe(struct platform_device *pdev)
 failed_irq:
 	fec_enet_deinit(ndev);
 failed_init:
-	fec_ptp_stop(pdev);
+	if (fep->bufdesc_ex)
+		fec_ptp_stop(pdev);
 failed_reset:
 	pm_runtime_put_noidle(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
-- 
2.43.0

Re: [PATCH 1/2] net: fec: Propagate PTP initialization errors

[Paolo Abeni]

On 8/20/26 1:14 PM, phucduc.bui@gmail.com wrote:
> From: bui duc phuc <phucduc.bui@gmail.com>
> 
> Change fec_ptp_init() to return an error code instead of silently
> ignoring failures during PTP initialization.
> 
> The PPS IRQ is not required for the FEC/PTP functionality, so its
> absence should not make the probe fail. However, an unavailable
> optional IRQ should be distinguished from an actual error returned
> during the IRQ lookup.
> 
> If a platform does not support the PPS IRQ, it can omit the IRQ from
> its device tree and the optional lookup will return -ENXIO. Propagate
> other errors from the IRQ lookup instead of silently ignoring them.
> 
> Also propagate failures from devm_request_irq() and ptp_clock_register().
> 
> Update the function declaration in fec.h accordingly.
> 
> Found by manual code inspection.
> 
> Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
## Form letter - net-next-closed

We have already submitted our pull request with net-next material for v7.3,
and therefore net-next is closed for new drivers, features, code refactoring
and optimizations. We are currently accepting bug fixes only.

Please repost when net-next reopens after Aug 31st.

RFC patches sent for review only are obviously welcome at any time.

See: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#development-cycle

RE: [PATCH 1/2] net: fec: Propagate PTP initialization errors

[Wei Fang]

> -void fec_ptp_init(struct platform_device *pdev, int irq_idx)
> +int fec_ptp_init(struct platform_device *pdev, int irq_idx)
>  {
>  	struct net_device *ndev = platform_get_drvdata(pdev);
>  	struct fec_enet_private *fep = netdev_priv(ndev);
> @@ -779,26 +779,29 @@ void fec_ptp_init(struct platform_device *pdev, int
> irq_idx)
>  		      HRTIMER_MODE_REL);
> 
>  	irq = platform_get_irq_byname_optional(pdev, "pps");
> -	if (irq < 0)
> +	if (irq < 0) {
>  		irq = platform_get_irq_optional(pdev, irq_idx);
> -	/* Failure to get an irq is not fatal,
> -	 * only the PTP_CLOCK_PPS clock events should stop
> -	 */
> -	if (irq >= 0) {
> +		if (irq < 0 && irq != -ENXIO)
> +			return irq;
> +	}
> +
> +	if (irq > 0) {
>  		ret = devm_request_irq(&pdev->dev, irq, fec_pps_interrupt,
>  				       0, pdev->name, ndev);
>  		if (ret < 0)
> -			dev_warn(&pdev->dev, "request for pps irq failed(%d)\n",
> -				 ret);

Why remove the diagnostic log?

> +			return ret;
>  	}
> 
>  	fep->ptp_clock = ptp_clock_register(&fep->ptp_caps, &pdev->dev);
>  	if (IS_ERR(fep->ptp_clock)) {
>  		fep->ptp_clock = NULL;
>  		dev_err(&pdev->dev, "ptp_clock_register failed\n");
> +		return PTR_ERR(fep->ptp_clock);

fep->ptp_clock is set to NULL on the line immediately before
PTR_ERR(fep->ptp_clock) is evaluated, so PTR_ERR receives NULL rather
than the original ERR_PTR value.

>  	}
> 
>  	schedule_delayed_work(&fep->time_keep, HZ);
> +
> +	return 0;
>  }
> 
>  void fec_ptp_save_state(struct fec_enet_private *fep)
> --
> 2.43.0

Additionally, the second patch can be merged with this patch. There is no
need to use two separate patches.

Re: [PATCH 1/2] net: fec: Propagate PTP initialization errors

[Bui Duc Phuc]

Hi Wei Fang,

Thank you for your review.

> > +     if (irq > 0) {
> >               ret = devm_request_irq(&pdev->dev, irq, fec_pps_interrupt,
> >                                      0, pdev->name, ndev);
> >               if (ret < 0)
> > -                     dev_warn(&pdev->dev, "request for pps irq failed(%d)\n",
> > -                              ret);
>
> Why remove the diagnostic log?
>

The reason is that the error is already logged deeper in the call chain,
after devm_request_irq() goes through several layers, with sufficient details:

-----------------------------------------------------------------------------------------
return dev_err_probe(dev, rc, "request_irq(%u) %ps %ps %s\n",
                                    irq, handler, thread_fn, devname ? : "");
-----------------------------------------------------------------------------------------

Therefore, I removed the diagnostic in this function to avoid duplicate error
messages.
If you prefer, I can keep it.

> >       if (IS_ERR(fep->ptp_clock)) {
> >               fep->ptp_clock = NULL;
> >               dev_err(&pdev->dev, "ptp_clock_register failed\n");
> > +             return PTR_ERR(fep->ptp_clock);
>
> fep->ptp_clock is set to NULL on the line immediately before
> PTR_ERR(fep->ptp_clock) is evaluated, so PTR_ERR receives NULL rather
> than the original ERR_PTR value.
>

You're right. This was an oversight on my part.
The error value should be saved before clearing fep->ptp_clock:

---------------------------------------------------------------------------
if (IS_ERR(fep->ptp_clock)) {
        ret = PTR_ERR(fep->ptp_clock);
        fep->ptp_clock = NULL;
        dev_err(&pdev->dev, "ptp_clock_register failed\n");
        return ret;
}
--------------------------------------------------------------------------

>
> Additionally, the second patch can be merged with this patch. There is no
> need to use two separate patches.
>

The error propagation part in the second patch is directly related to
the first patch :

------------------------------------------------------------
-       if (fep->bufdesc_ex)
-               fec_ptp_init(pdev, irq_cnt);
+       if (fep->bufdesc_ex) {
+               ret = fec_ptp_init(pdev, irq_cnt);
+               if (ret)
+                       goto failed_reset;
+       }
------------------------------------------------------------

So I agree that this part can be merged into the first patch.
However, the change in failed_init is an independent pre-existing bug:

-------------------------------------------------
 failed_init:
-       fec_ptp_stop(pdev);
+       if (fep->bufdesc_ex)
+               fec_ptp_stop(pdev);
-------------------------------------------------

Therefore, I think it would be better to keep this part as a separate patch so
that the Fixes: tag can correctly identify the commit that introduced the bug.

What do you think?

Best regards,
Phuc

RE: [PATCH 1/2] net: fec: Propagate PTP initialization errors

[Wei Fang]

> > > +     if (irq > 0) {
> > >               ret = devm_request_irq(&pdev->dev, irq,
> fec_pps_interrupt,
> > >                                      0, pdev->name, ndev);
> > >               if (ret < 0)
> > > -                     dev_warn(&pdev->dev, "request for pps irq
> failed(%d)\n",
> > > -                              ret);
> >
> > Why remove the diagnostic log?
> >
> 
> The reason is that the error is already logged deeper in the call chain,
> after devm_request_irq() goes through several layers, with sufficient details:
> 
> -----------------------------------------------------------------------------------------
> return dev_err_probe(dev, rc, "request_irq(%u) %ps %ps %s\n",
>                                     irq, handler, thread_fn, devname ? :
> "");
> -----------------------------------------------------------------------------------------
> 
> Therefore, I removed the diagnostic in this function to avoid duplicate error
> messages.
> If you prefer, I can keep it.

That makes sense, you'd better add a description for removing the log in the
commit message. :)

> >
> > Additionally, the second patch can be merged with this patch. There is no
> > need to use two separate patches.
> >
> 
> The error propagation part in the second patch is directly related to
> the first patch :
> 
> ------------------------------------------------------------
> -       if (fep->bufdesc_ex)
> -               fec_ptp_init(pdev, irq_cnt);
> +       if (fep->bufdesc_ex) {
> +               ret = fec_ptp_init(pdev, irq_cnt);
> +               if (ret)
> +                       goto failed_reset;
> +       }
> ------------------------------------------------------------
> 
> So I agree that this part can be merged into the first patch.

Yes, this patch and the above part should be in a patch. And it should
target to net-next tree.

> However, the change in failed_init is an independent pre-existing bug:

You should add a separate patch to fix the pre-existing issue, and the
patch targets to net tree. Patches to net tree are welcome at any time.
And you need to add the target tree in the subject so that netdev
maintainers know which git tree the patch should be applied to. For
example:
    [PATCH net]        -> net tree
    [PATCH net-next]    -> net-next tree

See: https://elixir.bootlin.com/linux/v7.2/source/Documentation/process/maintainer-netdev.rst#L61

> 
> -------------------------------------------------
>  failed_init:
> -       fec_ptp_stop(pdev);
> +       if (fep->bufdesc_ex)
> +               fec_ptp_stop(pdev);
> -------------------------------------------------
> 
> Therefore, I think it would be better to keep this part as a separate patch so
> that the Fixes: tag can correctly identify the commit that introduced the bug.
> 
> What do you think?

I do not see the Fixes tag in the second patch, please add the Fixes tag and
send a separate patch to the net tree.

Re: [PATCH 1/2] net: fec: Propagate PTP initialization errors

[Bui Duc Phuc]

Hi Wei Fang,

Thank you for your feedback.


>
> You should add a separate patch to fix the pre-existing issue, and the
> patch targets to net tree. Patches to net tree are welcome at any time.
> And you need to add the target tree in the subject so that netdev
> maintainers know which git tree the patch should be applied to. For
> example:
>     [PATCH net]        -> net tree
>     [PATCH net-next]    -> net-next tree
>

Thanks for the clarification.
I will split this into two separate patches: one for the main issue
addressed by
the current series, which I will send to net-next after August 31, and another
patch to fix the pre-existing issue, with a Fixes tag, which I will
send to net soon.

Best regards,
Phuc