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.

Fixes: b86bcb299092 ("net: fec_ptp: Use platform_get_irq_xxx_optional() to avoid error message")
Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---

Link v1: 
https://lore.kernel.org/all/20260820111416.97917-1-phucduc.bui@gmail.com/
Link v2:
https://lore.kernel.org/all/20260903092430.354186-1-phucduc.bui@gmail.com/
Changes in v2:
 - Squash the two patches into one.
 - Add error handling for platform_get_irq_byname_optional().
 - Fix error handling for ptp_clock_register().
Changes in v3:
 - Retarget the patch to the net tree instead of net-next.
 - Add error handling for platform_get_irq_byname_optional() 
   in fec_probe() for the fec_enet_interrupt IRQ.

 drivers/net/ethernet/freescale/fec.h      |  2 +-
 drivers/net/ethernet/freescale/fec_main.c | 11 ++++++++---
 drivers/net/ethernet/freescale/fec_ptp.c  | 22 ++++++++++++++--------
 3 files changed, 23 insertions(+), 12 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_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 794ec427b0ee..a9a458703547 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)
@@ -5394,7 +5397,9 @@ fec_probe(struct platform_device *pdev)
 	for (i = 0; i < irq_cnt; i++) {
 		snprintf(irq_name, sizeof(irq_name), "int%d", i);
 		irq = platform_get_irq_byname_optional(pdev, irq_name);
-		if (irq < 0)
+		if (irq < 0 && irq != -ENXIO)
+			return irq;
+		if (irq == -ENXIO)
 			irq = platform_get_irq(pdev, i);
 		if (irq < 0) {
 			ret = irq;
diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index 56801c2009d5..557a76797eba 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,32 @@ 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 != -ENXIO)
+		return irq;
+	if (irq == -ENXIO) {
 		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)) {
+		ret = PTR_ERR(fep->ptp_clock);
 		fep->ptp_clock = NULL;
 		dev_err(&pdev->dev, "ptp_clock_register failed\n");
+		return ret;
 	}
 
 	schedule_delayed_work(&fep->time_keep, HZ);
+
+	return 0;
 }
 
 void fec_ptp_save_state(struct fec_enet_private *fep)
-- 
2.43.0

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev/#/patchset/20260904105542.70540-1-phucduc.bui@gmail.com?part=1
---
net: fec: Propagate PTP initialization errors

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

Link: https://lore.kernel.org/all/20260903092430.354186-1-phucduc.bui@gmail.com/

> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 794ec427b0ee6..a9a458703547d 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
[ ... ]
> @@ -5394,7 +5397,9 @@ fec_probe(struct platform_device *pdev)
>  	for (i = 0; i < irq_cnt; i++) {
>  		snprintf(irq_name, sizeof(irq_name), "int%d", i);
>  		irq = platform_get_irq_byname_optional(pdev, irq_name);
> -		if (irq < 0)
> +		if (irq < 0 && irq != -ENXIO)
> +			return irq;

[Severity: High]
Does this direct return bypass the error handling cleanup at the end of
fec_probe()?

By returning directly instead of jumping to the failed_irq label, the driver
bypasses fec_ptp_stop(). This leaves the fep->time_keep delayed work
(scheduled earlier in fec_ptp_init()) active.

It also bypasses free_netdev(ndev), which leaks the net_device structure.

When the driver core cleans up devres allocations on probe failure, it unmaps
fep->hwp. If the un-canceled delayed work subsequently fires, will it attempt
to access the unmapped memory at fep->hwp and cause a kernel panic?

[ ... ]
> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
> index 56801c2009d59..557a76797ebaa 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
[ ... ]
> @@ -779,26 +779,32 @@ 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) {
>  		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)) {
> +		ret = PTR_ERR(fep->ptp_clock);
>  		fep->ptp_clock = NULL;
>  		dev_err(&pdev->dev, "ptp_clock_register failed\n");
> +		return ret;

[Severity: High]
Does this error return leave the devm-managed IRQ active while propagating an
error back to the probe function?

If ptp_clock_register() fails, fec_ptp_init() returns an error which
propagates to fec_probe(), causing it to jump to failed_reset.

The error path in fec_probe() manually frees ndev via free_netdev(ndev).

However, the PPS interrupt was registered just prior using devm_request_irq(),
so it remains active until fec_probe() returns and devres tears it down.

If the PPS interrupt fires between the manual free of ndev and the devres
cleanup, will fec_pps_interrupt() cast dev_id to ndev and attempt to
dereference the already-freed ndev structure, causing a use-after-free crash?

> > -             if (irq < 0)
> > +             if (irq < 0 && irq != -ENXIO)
> > +                     return irq;
>
> [Severity: High]
> Does this direct return bypass the error handling cleanup at the end of
> fec_probe()?
>
> By returning directly instead of jumping to the failed_irq label, the driver
> bypasses fec_ptp_stop(). This leaves the fep->time_keep delayed work
> (scheduled earlier in fec_ptp_init()) active.
>
> It also bypasses free_netdev(ndev), which leaks the net_device structure.
>
> When the driver core cleans up devres allocations on probe failure, it unmaps
> fep->hwp. If the un-canceled delayed work subsequently fires, will it attempt
> to access the unmapped memory at fep->hwp and cause a kernel panic?
>


Yes, this was my mistake.

It should be:
+ ret = irq;
+ goto failed_irq;

instead of :
+ return irq;.

I will fix this in the next version.



> [Severity: High]
> Does this error return leave the devm-managed IRQ active while propagating an
> error back to the probe function?
>
> If ptp_clock_register() fails, fec_ptp_init() returns an error which
> propagates to fec_probe(), causing it to jump to failed_reset.
>
> The error path in fec_probe() manually frees ndev via free_netdev(ndev).
>
> However, the PPS interrupt was registered just prior using devm_request_irq(),
> so it remains active until fec_probe() returns and devres tears it down.
>
> If the PPS interrupt fires between the manual free of ndev and the devres
> cleanup, will fec_pps_interrupt() cast dev_id to ndev and attempt to
> dereference the already-freed ndev structure, causing a use-after-free crash?


This is a pre-existing issue:
https://lore.kernel.org/all/CAABR9nEkci1OFoHx2qmJWvKabUN=JSu5AcPynQ-tH02wxHEmOA@mail.gmail.com/

Previously, I suggested using request_irq() and free_irq() manually,
but that would add more code.
Another option is to replace alloc_etherdev_mqs() with
devm_alloc_etherdev_mqs(). With the LIFO cleanup order,
devm_request_irq() will be cleaned up before free_netdev(res->ndev),
which should resolve the issue.

Since this is a long-standing, pre-existing issue, I think it would be
better to handle it in a separate patch.