All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] net: fec: Propagate PTP initialization errors
@ 2026-08-20 11:14 phucduc.bui
  2026-08-20 11:14 ` [PATCH 2/2] net: fec: Handle PTP initialization errors in probe phucduc.bui
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: phucduc.bui @ 2026-08-20 11:14 UTC (permalink / raw)
  To: Wei Fang, Frank Li, Shenwei Wang
  Cc: Andrew Lunn, davem, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Richard Cochran, imx, linux-kernel, netdev, bui duc phuc

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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/2] net: fec: Handle PTP initialization errors in probe
  2026-08-20 11:14 [PATCH 1/2] net: fec: Propagate PTP initialization errors phucduc.bui
@ 2026-08-20 11:14 ` phucduc.bui
  2026-08-21 11:14   ` sashiko-bot
  2026-08-20 13:28 ` [PATCH 1/2] net: fec: Propagate PTP initialization errors Paolo Abeni
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: phucduc.bui @ 2026-08-20 11:14 UTC (permalink / raw)
  To: Wei Fang, Frank Li, Shenwei Wang
  Cc: Andrew Lunn, davem, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Richard Cochran, imx, linux-kernel, netdev, bui duc phuc

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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] net: fec: Propagate PTP initialization errors
  2026-08-20 11:14 [PATCH 1/2] net: fec: Propagate PTP initialization errors phucduc.bui
  2026-08-20 11:14 ` [PATCH 2/2] net: fec: Handle PTP initialization errors in probe phucduc.bui
@ 2026-08-20 13:28 ` Paolo Abeni
  2026-08-21 10:13 ` Wei Fang
  2026-08-21 11:14 ` sashiko-bot
  3 siblings, 0 replies; 10+ messages in thread
From: Paolo Abeni @ 2026-08-20 13:28 UTC (permalink / raw)
  To: phucduc.bui, Wei Fang, Frank Li, Shenwei Wang
  Cc: Andrew Lunn, davem, Eric Dumazet, Jakub Kicinski, Richard Cochran,
	imx, linux-kernel, netdev

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH 1/2] net: fec: Propagate PTP initialization errors
  2026-08-20 11:14 [PATCH 1/2] net: fec: Propagate PTP initialization errors phucduc.bui
  2026-08-20 11:14 ` [PATCH 2/2] net: fec: Handle PTP initialization errors in probe phucduc.bui
  2026-08-20 13:28 ` [PATCH 1/2] net: fec: Propagate PTP initialization errors Paolo Abeni
@ 2026-08-21 10:13 ` Wei Fang
  2026-08-22  9:07   ` Bui Duc Phuc
  2026-08-21 11:14 ` sashiko-bot
  3 siblings, 1 reply; 10+ messages in thread
From: Wei Fang @ 2026-08-21 10:13 UTC (permalink / raw)
  To: phucduc.bui@gmail.com, Frank Li, Shenwei Wang
  Cc: Andrew Lunn, davem@davemloft.net, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Richard Cochran, imx@lists.linux.dev,
	linux-kernel@vger.kernel.org, netdev@vger.kernel.org

> -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.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] net: fec: Handle PTP initialization errors in probe
  2026-08-20 11:14 ` [PATCH 2/2] net: fec: Handle PTP initialization errors in probe phucduc.bui
@ 2026-08-21 11:14   ` sashiko-bot
  0 siblings, 0 replies; 10+ messages in thread
From: sashiko-bot @ 2026-08-21 11:14 UTC (permalink / raw)
  To: phucduc.bui; +Cc: Frank.Li, imx

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] The patch fixes an unconditional `fec_ptp_stop()` in the probe error path but misses the identical bug in `fec_drv_remove()`, leading to a NULL pointer dereference.
- [High] Use-after-free in devm-managed interrupt handlers during driver removal due to premature `free_netdev()`.
- [High] PTP device is exposed to userspace before probe completes, leading to potential bus faults on probe error paths.
- [High] `INIT_WORK(&fep->tx_timeout_work)` is called after `register_netdev()`, creating a race window where an uninitialized work queue can be scheduled.
--

--- Patch [2]: [PATCH 2/2] net: fec: Handle PTP initialization errors in probe ---
commit 41e2a49ed28a7c17df87f2ff80ee17b2b80b3801
Author: bui duc phuc <phucduc.bui@gmail.com>

net: fec: Handle PTP initialization errors in probe
    
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.

> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index ced4dbf8cd90f..b6c22d6c1e69a 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;
> +	}

[Severity: High]
This is a pre-existing issue, but does fec_ptp_init() expose the PTP device
to userspace before the probe completes?

drivers/net/ethernet/freescale/fec_ptp.c:fec_ptp_init() {
    ...
    fep->ptp_clock = ptp_clock_register(&fep->ptp_caps, &pdev->dev);
    ...
}

This makes the character device accessible to userspace before network device
registration completes. If register_netdev() fails later in fec_probe(), the
error path shuts down hardware clocks.

If userspace accesses the PTP device concurrently during this window (e.g. by
issuing an ioctl), could the PTP callbacks attempt to access disabled
hardware registers?

>  
>  	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);

[Severity: High]
This isn't a bug introduced by this patch, but since you are fixing the
unconditional call to fec_ptp_stop() here in the probe error path, should we
also fix the exact same issue in fec_drv_remove()?

drivers/net/ethernet/freescale/fec_main.c:fec_drv_remove() {
    ...
    cancel_work_sync(&fep->tx_timeout_work);
    fec_ptp_stop(pdev);
    unregister_netdev(ndev);
    ...
}

When the driver is unbound from a device without PTP support,
fec_drv_remove() unconditionally calls fec_ptp_stop(). Since PTP was never
initialized, hrtimer_cancel() inside fec_ptp_stop() will read from a
zero-initialized timer, dereferencing a null base pointer.

[Severity: High]
This is a pre-existing issue, but does fec_drv_remove() have a use-after-free
risk with devm-managed interrupts?

drivers/net/ethernet/freescale/fec_main.c:fec_drv_remove() {
    ...
    fec_enet_deinit(ndev);
    free_netdev(ndev);
}

The interrupt is requested with devm_request_irq() in fec_probe(), meaning
its lifetime extends beyond fec_drv_remove(). However, fec_drv_remove()
explicitly calls free_netdev(), freeing the ndev and fep structures.

If a spurious or shared interrupt fires after free_netdev() returns but
before devres executes free_irq(), could the interrupt handler dereference
the freed memory?

[Severity: High]
This isn't a bug introduced by this patch, but does the initialization order
in fec_probe() create a race window for tx_timeout_work?

drivers/net/ethernet/freescale/fec_main.c:fec_probe() {
    ...
    ret = register_netdev(ndev);
    if (ret)
        goto failed_register;
    ...
    INIT_WORK(&fep->tx_timeout_work, fec_enet_timeout_work);
    ...
}

Calling register_netdev() makes the network device visible to the system, so
userspace can configure the interface and start transmitting traffic. 

If a hardware stall causes the ndo_tx_timeout callback to be invoked before
INIT_WORK() executes, could schedule_work(&fep->tx_timeout_work) operate on
an uninitialized work_struct?

>  failed_reset:
>  	pm_runtime_put_noidle(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260820111416.97917-1-phucduc.bui@gmail.com?part=2

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] net: fec: Propagate PTP initialization errors
  2026-08-20 11:14 [PATCH 1/2] net: fec: Propagate PTP initialization errors phucduc.bui
                   ` (2 preceding siblings ...)
  2026-08-21 10:13 ` Wei Fang
@ 2026-08-21 11:14 ` sashiko-bot
  2026-08-24  6:11   ` Bui Duc Phuc
  3 siblings, 1 reply; 10+ messages in thread
From: sashiko-bot @ 2026-08-21 11:14 UTC (permalink / raw)
  To: phucduc.bui; +Cc: imx, Frank.Li

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] `fec_ptp_init()` incorrectly returns 0 (success) instead of propagating the actual error code when `ptp_clock_register()` fails.

Pre-existing issues:
- [High] Concurrent execution of the PPS interrupt handler and device teardown causes a use-after-free on `fep->ptp_clock`.
--

--- Patch [1]: [PATCH 1/2] net: fec: Propagate PTP initialization errors ---
commit 72db297960b04e49a3fe9e579f0ca646d2fb313b
Author: bui duc phuc <phucduc.bui@gmail.com>

net: fec: Propagate PTP initialization errors

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

> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
> index 56801c2009d59..8ad680411b0c3 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c

[ ... ]

> @@ -779,26 +779,29 @@ void fec_ptp_init(struct platform_device *pdev, int irq_idx)

[ ... ]

>  	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;
>  	}

[Severity: High]
This is a pre-existing issue, but is it safe to use devm_request_irq() here
without synchronizing during teardown?

During module removal, fec_drv_remove() calls fec_ptp_stop(), which sets
fep->pps_enable = 0 and unregisters fep->ptp_clock. However, since the
interrupt is managed by devres, it isn't freed or synchronized until after
the remove function completes.

If the interrupt fires concurrently on another CPU:

CPU 1 (fec_ptp_stop)
    ptp_clock_unregister(fep->ptp_clock);

CPU 2 (fec_pps_interrupt)
    if (fep->pps_enable) {
        event.type = PTP_CLOCK_PPS;
        ptp_clock_event(fep->ptp_clock, &event);
    }

If CPU 2 reads fep->pps_enable as true just before CPU 1 clears it, could it
then dereference the freed fep->ptp_clock?

>  
>  	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);

[Severity: Medium]
Since fep->ptp_clock is set to NULL on the line directly above this one,
doesn't PTR_ERR(fep->ptp_clock) evaluate to 0 here?

This would cause the function to return success instead of propagating the
negative error code from ptp_clock_register().

>  	}
>  
>  	schedule_delayed_work(&fep->time_keep, HZ);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260820111416.97917-1-phucduc.bui@gmail.com?part=1

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] net: fec: Propagate PTP initialization errors
  2026-08-21 10:13 ` Wei Fang
@ 2026-08-22  9:07   ` Bui Duc Phuc
  2026-08-24  2:28     ` Wei Fang
  0 siblings, 1 reply; 10+ messages in thread
From: Bui Duc Phuc @ 2026-08-22  9:07 UTC (permalink / raw)
  To: Wei Fang
  Cc: Frank Li, Shenwei Wang, Andrew Lunn, davem@davemloft.net,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Richard Cochran,
	imx@lists.linux.dev, linux-kernel@vger.kernel.org,
	netdev@vger.kernel.org

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH 1/2] net: fec: Propagate PTP initialization errors
  2026-08-22  9:07   ` Bui Duc Phuc
@ 2026-08-24  2:28     ` Wei Fang
  2026-08-24  5:33       ` Bui Duc Phuc
  0 siblings, 1 reply; 10+ messages in thread
From: Wei Fang @ 2026-08-24  2:28 UTC (permalink / raw)
  To: Bui Duc Phuc
  Cc: Frank Li, Shenwei Wang, Andrew Lunn, davem@davemloft.net,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Richard Cochran,
	imx@lists.linux.dev, linux-kernel@vger.kernel.org,
	netdev@vger.kernel.org

> > > +     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.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] net: fec: Propagate PTP initialization errors
  2026-08-24  2:28     ` Wei Fang
@ 2026-08-24  5:33       ` Bui Duc Phuc
  0 siblings, 0 replies; 10+ messages in thread
From: Bui Duc Phuc @ 2026-08-24  5:33 UTC (permalink / raw)
  To: Wei Fang
  Cc: Frank Li, Shenwei Wang, Andrew Lunn, davem@davemloft.net,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Richard Cochran,
	imx@lists.linux.dev, linux-kernel@vger.kernel.org,
	netdev@vger.kernel.org

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] net: fec: Propagate PTP initialization errors
  2026-08-21 11:14 ` sashiko-bot
@ 2026-08-24  6:11   ` Bui Duc Phuc
  0 siblings, 0 replies; 10+ messages in thread
From: Bui Duc Phuc @ 2026-08-24  6:11 UTC (permalink / raw)
  To: sashiko-reviews; +Cc: imx, Frank.Li

>
> New issues:
> - [Medium] `fec_ptp_init()` incorrectly returns 0 (success) instead of propagating the actual error code when `ptp_clock_register()` fails.
>

The ptp_clock_register() error handling issue will be addressed when I
resend the patch to net-next after August 31.


> Pre-existing issues:
> - [High] Concurrent execution of the PPS interrupt handler and device teardown causes a use-after-free on `fep->ptp_clock`.
> --
>

This series fixes the error path when devm_request_irq() fails by
propagating the error to the probe function.
The reported race between the PPS interrupt handler and device
teardown is a pre-existing issue and is unrelated to
the error propagation changes in this series.
I would like to leave this issue out of the current series.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2026-08-24  6:11 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-20 11:14 [PATCH 1/2] net: fec: Propagate PTP initialization errors phucduc.bui
2026-08-20 11:14 ` [PATCH 2/2] net: fec: Handle PTP initialization errors in probe phucduc.bui
2026-08-21 11:14   ` sashiko-bot
2026-08-20 13:28 ` [PATCH 1/2] net: fec: Propagate PTP initialization errors Paolo Abeni
2026-08-21 10:13 ` Wei Fang
2026-08-22  9:07   ` Bui Duc Phuc
2026-08-24  2:28     ` Wei Fang
2026-08-24  5:33       ` Bui Duc Phuc
2026-08-21 11:14 ` sashiko-bot
2026-08-24  6:11   ` Bui Duc Phuc

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.