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

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

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
>