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

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

Hi Vadim,

在 2026/8/7 22:51, 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?
>
>
Unfortunately, I do not have the hardware. While fixing the clear issue
in patch 1, Jakub forwarded a Sashiko report that flagged this race, which
I have only verified through static code review.

Thanks,
Xuanqiang