[PATCH net v3 0/2] net: ravb: fix PTP clock lifetime

[PATCH net v3 0/2] net: ravb: fix PTP clock lifetime

[xuanqiang.luo]

From: Xuanqiang Luo <xuanqiang.luo@linux.dev>

This series fixes RAVB PTP clock lifetime handling. It handles unavailable
clocks and synchronizes clock access with publication and teardown.

Patch 1 handles unavailable PTP clocks.

Patch 2 synchronizes PTP clock access with publication and teardown.

---
Changes:
v3:
  Patch 1:
  - Omit Niklas Söderlund's Reviewed-by tag because the implementation he
    reviewed has changed.
  - Describe the NULL pointer dereference before the first open as the most
    likely failure mode in the commit message. (Jakub Kicinski, Sashiko)
  - Normalize PTP clock registration failures to NULL.
    (Jakub Kicinski, Sashiko)
  - Keep hardware timestamping capabilities independent of PHC
    availability. (Jakub Kicinski, Sashiko)
  Patch 2 (new):
  - Serialize PTP clock publication and access with priv->lock, and detach
    the clock before unregistering it. (Jakub Kicinski, Sashiko)

v2: https://lore.kernel.org/all/20260802090750.116215-1-xuanqiang.luo@linux.dev/
  - Only advertise hardware timestamping support when a PHC is available
    (Niklas Söderlund).

v1: https://lore.kernel.org/all/20260731063254.71260-1-xuanqiang.luo@linux.dev/

Xuanqiang Luo (2):
  net: ravb: handle unavailable PTP clock
  net: ravb: serialize PTP clock teardown

 drivers/net/ethernet/renesas/ravb_main.c | 15 +++++++++++---
 drivers/net/ethernet/renesas/ravb_ptp.c  | 25 ++++++++++++++++++++----
 2 files changed, 33 insertions(+), 7 deletions(-)


base-commit: 11028ab62899e4191e074ee364c712b77823a9c4
-- 
2.43.0

[PATCH net v3 1/2] net: ravb: handle unavailable PTP clock

[xuanqiang.luo]

From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>

The PTP clock is registered by ravb_open(), not ravb_probe(). Therefore,
priv->ptp.clock is NULL between register_netdev() and the first open, and
ethtool -T triggers a NULL dereference in ptp_clock_index().

ptp_clock_register() may also return an error pointer, which can reach
ptp_clock_index() or ptp_clock_unregister().

Normalize registration errors to NULL and only query or unregister the
clock when it is present. Leave phc_index at -1 when no PHC is registered,
while preserving the static hardware timestamping capabilities.

Fixes: a0d2f20650e8 ("Renesas Ethernet AVB PTP clock driver")
Cc: stable@vger.kernel.org
Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
---
 drivers/net/ethernet/renesas/ravb_main.c |  3 ++-
 drivers/net/ethernet/renesas/ravb_ptp.c  | 12 ++++++++++--
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 5f88733094d0f..3a9d9f8718216 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1779,7 +1779,8 @@ static int ravb_get_ts_info(struct net_device *ndev,
 			(1 << HWTSTAMP_FILTER_NONE) |
 			(1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) |
 			(1 << HWTSTAMP_FILTER_ALL);
-		info->phc_index = ptp_clock_index(priv->ptp.clock);
+		if (priv->ptp.clock)
+			info->phc_index = ptp_clock_index(priv->ptp.clock);
 	}
 
 	return 0;
diff --git a/drivers/net/ethernet/renesas/ravb_ptp.c b/drivers/net/ethernet/renesas/ravb_ptp.c
index 226c6c0ab945b..eaf07cb8eadfa 100644
--- a/drivers/net/ethernet/renesas/ravb_ptp.c
+++ b/drivers/net/ethernet/renesas/ravb_ptp.c
@@ -315,6 +315,7 @@ void ravb_ptp_interrupt(struct net_device *ndev)
 void ravb_ptp_init(struct net_device *ndev, struct platform_device *pdev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
+	struct ptp_clock *clock;
 	unsigned long flags;
 
 	priv->ptp.info = ravb_ptp_info;
@@ -327,7 +328,13 @@ void ravb_ptp_init(struct net_device *ndev, struct platform_device *pdev)
 	ravb_modify(ndev, GCCR, GCCR_TCSS, GCCR_TCSS_ADJGPTP);
 	spin_unlock_irqrestore(&priv->lock, flags);
 
-	priv->ptp.clock = ptp_clock_register(&priv->ptp.info, &pdev->dev);
+	clock = ptp_clock_register(&priv->ptp.info, &pdev->dev);
+	if (IS_ERR(clock)) {
+		netdev_err(ndev, "failed to register PTP clock: %pe\n", clock);
+		clock = NULL;
+	}
+
+	priv->ptp.clock = clock;
 }
 
 void ravb_ptp_stop(struct net_device *ndev)
@@ -337,5 +344,6 @@ void ravb_ptp_stop(struct net_device *ndev)
 	ravb_write(ndev, 0, GIC);
 	ravb_write(ndev, 0, GIS);
 
-	ptp_clock_unregister(priv->ptp.clock);
+	if (priv->ptp.clock)
+		ptp_clock_unregister(priv->ptp.clock);
 }
-- 
2.43.0

Re: [PATCH net v3 1/2] net: ravb: handle unavailable PTP clock

[Vadim Fedorenko]

On 06/08/2026 10:51, xuanqiang.luo@linux.dev wrote:
> From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
> 
> The PTP clock is registered by ravb_open(), not ravb_probe(). Therefore,
> priv->ptp.clock is NULL between register_netdev() and the first open, and
> ethtool -T triggers a NULL dereference in ptp_clock_index().
> 
> ptp_clock_register() may also return an error pointer, which can reach
> ptp_clock_index() or ptp_clock_unregister().
> 
> Normalize registration errors to NULL and only query or unregister the
> clock when it is present. Leave phc_index at -1 when no PHC is registered,
> while preserving the static hardware timestamping capabilities.
> 
> Fixes: a0d2f20650e8 ("Renesas Ethernet AVB PTP clock driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
> ---
>   drivers/net/ethernet/renesas/ravb_main.c |  3 ++-
>   drivers/net/ethernet/renesas/ravb_ptp.c  | 12 ++++++++++--
>   2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 5f88733094d0f..3a9d9f8718216 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1779,7 +1779,8 @@ static int ravb_get_ts_info(struct net_device *ndev,
>   			(1 << HWTSTAMP_FILTER_NONE) |
>   			(1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) |
>   			(1 << HWTSTAMP_FILTER_ALL);
> -		info->phc_index = ptp_clock_index(priv->ptp.clock);
> +		if (priv->ptp.clock)
> +			info->phc_index = ptp_clock_index(priv->ptp.clock);
>   	}
>   
>   	return 0;
> diff --git a/drivers/net/ethernet/renesas/ravb_ptp.c b/drivers/net/ethernet/renesas/ravb_ptp.c
> index 226c6c0ab945b..eaf07cb8eadfa 100644
> --- a/drivers/net/ethernet/renesas/ravb_ptp.c
> +++ b/drivers/net/ethernet/renesas/ravb_ptp.c
> @@ -315,6 +315,7 @@ void ravb_ptp_interrupt(struct net_device *ndev)
>   void ravb_ptp_init(struct net_device *ndev, struct platform_device *pdev)
>   {
>   	struct ravb_private *priv = netdev_priv(ndev);
> +	struct ptp_clock *clock;
>   	unsigned long flags;
>   
>   	priv->ptp.info = ravb_ptp_info;
> @@ -327,7 +328,13 @@ void ravb_ptp_init(struct net_device *ndev, struct platform_device *pdev)
>   	ravb_modify(ndev, GCCR, GCCR_TCSS, GCCR_TCSS_ADJGPTP);
>   	spin_unlock_irqrestore(&priv->lock, flags);
>   
> -	priv->ptp.clock = ptp_clock_register(&priv->ptp.info, &pdev->dev);
> +	clock = ptp_clock_register(&priv->ptp.info, &pdev->dev);
> +	if (IS_ERR(clock)) {
> +		netdev_err(ndev, "failed to register PTP clock: %pe\n", clock);
> +		clock = NULL;
> +	}
> +
> +	priv->ptp.clock = clock;
>   }
>   
>   void ravb_ptp_stop(struct net_device *ndev)
> @@ -337,5 +344,6 @@ void ravb_ptp_stop(struct net_device *ndev)
>   	ravb_write(ndev, 0, GIC);
>   	ravb_write(ndev, 0, GIS);
>   
> -	ptp_clock_unregister(priv->ptp.clock);
> +	if (priv->ptp.clock)
> +		ptp_clock_unregister(priv->ptp.clock);
>   }

Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>

[PATCH net v3 2/2] net: ravb: serialize PTP clock teardown

[xuanqiang.luo]

From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>

ravb_get_ts_info() can run without RTNL while ravb_ptp_stop() unregisters
the PHC. The PTP interrupt handler can race with the same teardown, so both
paths may access the clock while it is being freed.

Protect the clock pointer with priv->lock, clear it before unregistering
the PHC, and unregister the detached clock outside the lock.

Fixes: a0d2f20650e8 ("Renesas Ethernet AVB PTP clock driver")
Cc: stable@vger.kernel.org
Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
---
 drivers/net/ethernet/renesas/ravb_main.c | 16 ++++++++++++----
 drivers/net/ethernet/renesas/ravb_ptp.c  | 17 +++++++++++++----
 2 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 3a9d9f8718216..c2220a54a2bf4 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1765,8 +1765,13 @@ static int ravb_set_ringparam(struct net_device *ndev,
 static int ravb_get_ts_info(struct net_device *ndev,
 			    struct kernel_ethtool_ts_info *info)
 {
-	struct ravb_private *priv = netdev_priv(ndev);
-	const struct ravb_hw_info *hw_info = priv->info;
+	const struct ravb_hw_info *hw_info;
+	struct ravb_private *priv;
+	struct ptp_clock *clock;
+	unsigned long flags;
+
+	priv = netdev_priv(ndev);
+	hw_info = priv->info;
 
 	if (hw_info->gptp || hw_info->ccc_gac) {
 		info->so_timestamping =
@@ -1779,8 +1784,11 @@ static int ravb_get_ts_info(struct net_device *ndev,
 			(1 << HWTSTAMP_FILTER_NONE) |
 			(1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) |
 			(1 << HWTSTAMP_FILTER_ALL);
-		if (priv->ptp.clock)
-			info->phc_index = ptp_clock_index(priv->ptp.clock);
+		spin_lock_irqsave(&priv->lock, flags);
+		clock = priv->ptp.clock;
+		if (clock)
+			info->phc_index = ptp_clock_index(clock);
+		spin_unlock_irqrestore(&priv->lock, flags);
 	}
 
 	return 0;
diff --git a/drivers/net/ethernet/renesas/ravb_ptp.c b/drivers/net/ethernet/renesas/ravb_ptp.c
index eaf07cb8eadfa..530902a99d5ba 100644
--- a/drivers/net/ethernet/renesas/ravb_ptp.c
+++ b/drivers/net/ethernet/renesas/ravb_ptp.c
@@ -289,16 +289,17 @@ static const struct ptp_clock_info ravb_ptp_info = {
 void ravb_ptp_interrupt(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
+	struct ptp_clock *clock = priv->ptp.clock;
 	u32 gis = ravb_read(ndev, GIS);
 
 	gis &= ravb_read(ndev, GIC);
-	if (gis & GIS_PTCF) {
+	if ((gis & GIS_PTCF) && clock) {
 		struct ptp_clock_event event;
 
 		event.type = PTP_CLOCK_EXTTS;
 		event.index = 0;
 		event.timestamp = ravb_read(ndev, GCPT);
-		ptp_clock_event(priv->ptp.clock, &event);
+		ptp_clock_event(clock, &event);
 	}
 	if (gis & GIS_PTMF) {
 		struct ravb_ptp_perout *perout = priv->ptp.perout;
@@ -334,16 +335,24 @@ void ravb_ptp_init(struct net_device *ndev, struct platform_device *pdev)
 		clock = NULL;
 	}
 
+	spin_lock_irqsave(&priv->lock, flags);
 	priv->ptp.clock = clock;
+	spin_unlock_irqrestore(&priv->lock, flags);
 }
 
 void ravb_ptp_stop(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
+	struct ptp_clock *clock;
+	unsigned long flags;
 
+	spin_lock_irqsave(&priv->lock, flags);
 	ravb_write(ndev, 0, GIC);
 	ravb_write(ndev, 0, GIS);
+	clock = priv->ptp.clock;
+	priv->ptp.clock = NULL;
+	spin_unlock_irqrestore(&priv->lock, flags);
 
-	if (priv->ptp.clock)
-		ptp_clock_unregister(priv->ptp.clock);
+	if (clock)
+		ptp_clock_unregister(clock);
 }
-- 
2.43.0

Re: [PATCH net v3 2/2] net: ravb: serialize PTP clock teardown

[Vadim Fedorenko]

On 06/08/2026 10:51, xuanqiang.luo@linux.dev wrote:
> From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
> 
> ravb_get_ts_info() can run without RTNL while ravb_ptp_stop() unregisters
> the PHC. The PTP interrupt handler can race with the same teardown, so both
> paths may access the clock while it is being freed.
> 
> Protect the clock pointer with priv->lock, clear it before unregistering
> the PHC, and unregister the detached clock outside the lock.

I think adding spinlock here is a bit of overkill. For
ravb_get_ts_info() you can simply extend struct ravb_ptp to store clock
index on init and reset it to -1 on ptp stop.

For interrupt race I think ravb_ptp_stop() can be split into disable
interrupt function (like ravb_ptp_disable()) and actual clock free. In
this case ravb_ptp_disable() can be run before synchronize_irq() with
ptp_clock_unregister() afterwards in teardown callbacks, like it's
already done for ring allocations.

Re: [PATCH net v3 2/2] net: ravb: serialize PTP clock teardown

[luoxuanqiang]

Hi Vadim,

在 2026/8/7 06:34, Vadim Fedorenko 写道:
> On 06/08/2026 10:51, xuanqiang.luo@linux.dev wrote:
>> From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
>>
>> ravb_get_ts_info() can run without RTNL while ravb_ptp_stop() 
>> unregisters
>> the PHC. The PTP interrupt handler can race with the same teardown, 
>> so both
>> paths may access the clock while it is being freed.
>>
>> Protect the clock pointer with priv->lock, clear it before unregistering
>> the PHC, and unregister the detached clock outside the lock.
>
> I think adding spinlock here is a bit of overkill. For
> ravb_get_ts_info() you can simply extend struct ravb_ptp to store clock
> index on init and reset it to -1 on ptp stop.
>
Thanks! That is a good suggestion.

I will do this in the next version.

> For interrupt race I think ravb_ptp_stop() can be split into disable
> interrupt function (like ravb_ptp_disable()) and actual clock free. In
> this case ravb_ptp_disable() can be run before synchronize_irq() with
> ptp_clock_unregister() afterwards in teardown callbacks, like it's
> already done for ring allocations.
>
I see the approach you described. I assume you are referring to the
handling in ravb_set_ringparam().

One detail I would like to clarify is which IRQs need to be synchronized.
When err_mgmt_irqs is set, ravb_multi_interrupt() is registered for dia,
err_a and mgmt_a, while only dia is stored in ndev->irq. Each handler
checks ISS and may call ravb_ptp_interrupt().

Is there a hardware routing guarantee that gPTP interrupts are only
delivered through dia, making synchronize_irq(ndev->irq) sufficient?
Otherwise, it seems that the driver needs to retain the err_a and mgmt_a
IRQ numbers and synchronize all three before unregistering the clock.

The additional users of priv->lock would not be on particularly hot
paths, but avoiding them still seems preferable if draining the
interrupts fully closes the lifetime race. I would just like to confirm
the required IRQ scope first. Tracking and synchronizing all three IRQs
would also make the change larger, so the interrupt teardown patch may
no longer be suitable for Cc: stable.

In any case, thanks for the suggestion.

Thanks,
Xuanqiang

Re: [PATCH net v3 2/2] net: ravb: serialize PTP clock teardown

[Vadim Fedorenko]

On 07/08/2026 11:14, luoxuanqiang wrote:
> Hi Vadim,
> 
> 在 2026/8/7 06:34, Vadim Fedorenko 写道:
>> On 06/08/2026 10:51, xuanqiang.luo@linux.dev wrote:
>>> From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
>>>
>>> ravb_get_ts_info() can run without RTNL while ravb_ptp_stop() 
>>> unregisters
>>> the PHC. The PTP interrupt handler can race with the same teardown, 
>>> so both
>>> paths may access the clock while it is being freed.
>>>
>>> Protect the clock pointer with priv->lock, clear it before unregistering
>>> the PHC, and unregister the detached clock outside the lock.
>>
>> I think adding spinlock here is a bit of overkill. For
>> ravb_get_ts_info() you can simply extend struct ravb_ptp to store clock
>> index on init and reset it to -1 on ptp stop.
>>
> Thanks! That is a good suggestion.
> 
> I will do this in the next version.
> 
>> For interrupt race I think ravb_ptp_stop() can be split into disable
>> interrupt function (like ravb_ptp_disable()) and actual clock free. In
>> this case ravb_ptp_disable() can be run before synchronize_irq() with
>> ptp_clock_unregister() afterwards in teardown callbacks, like it's
>> already done for ring allocations.
>>
> I see the approach you described. I assume you are referring to the
> handling in ravb_set_ringparam().
> 
> One detail I would like to clarify is which IRQs need to be synchronized.
> When err_mgmt_irqs is set, ravb_multi_interrupt() is registered for dia,
> err_a and mgmt_a, while only dia is stored in ndev->irq. Each handler
> checks ISS and may call ravb_ptp_interrupt().
> 
> Is there a hardware routing guarantee that gPTP interrupts are only
> delivered through dia, making synchronize_irq(ndev->irq) sufficient?
> Otherwise, it seems that the driver needs to retain the err_a and mgmt_a
> IRQ numbers and synchronize all three before unregistering the clock.

I don't have access to the datasheet, so I cannot be sure, but from the 
quick look at the handler, there are 3 types of interrupt-handling
functions, and 3 different interrupt vectors...

But it's still better to ask people who have access to datasheet, and
implement things in correct way rather than throwing spinlocks in the
code.

btw, do you have a HW to reproduce the issue and check the fix?

> 
> The additional users of priv->lock would not be on particularly hot
> paths, but avoiding them still seems preferable if draining the
> interrupts fully closes the lifetime race. I would just like to confirm
> the required IRQ scope first. Tracking and synchronizing all three IRQs
> would also make the change larger, so the interrupt teardown patch may
> no longer be suitable for Cc: stable.
> 
> In any case, thanks for the suggestion.
> 
> Thanks,
> Xuanqiang
>