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

[luoxuanqiang <xuanqiang.luo@linux.dev>]

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