Re: [PATCH iwlwifi-fixes] wifi: iwlwifi: Implement settime64 as stub for MVM/MLD PTP

[David Wang]

At 2025-12-15 11:42:17, "Yao Zi" <me@xxxxxxxx> wrote:
>On Sun, Dec 14, 2025 at 06:12:57PM +0800, David Wang wrote:
>> On Thu, Dec 04, 2025 at 12:32:04PM +0000, Yao Zi wrote:
>> > Since commit dfb073d32cac ("ptp: Return -EINVAL on ptp_clock_register if
>> > required ops are NULL"), PTP clock registered through ptp_clock_register
>> > is required to have ptp_clock_info.settime64 set, however, neither MVM
>> > nor MLD's PTP clock implementation sets it, resulting in warnings when
>> > the interface starts up, like
>> > 
>> > WARNING: drivers/ptp/ptp_clock.c:325 at ptp_clock_register+0x2c8/0x6b8, CPU#1: wpa_supplicant/469
>> > CPU: 1 UID: 0 PID: 469 Comm: wpa_supplicant Not tainted 6.18.0+ #101 PREEMPT(full)
>> > ra: ffff800002732cd4 iwl_mvm_ptp_init+0x114/0x188 [iwlmvm]
>> > ERA: 9000000002fdc468 ptp_clock_register+0x2c8/0x6b8
>> > iwlwifi 0000:01:00.0: Failed to register PHC clock (-22)
>> > 
>> > I don't find an appropriate firmware interface to implement settime64()
>> > for iwlwifi MLD/MVM, thus instead create a stub that returns
>> > -EOPTNOTSUPP only, suppressing the warning and allowing the PTP clock to
>> > be registered.
>> 
>> This seems disturbing....If a null settime64 deserve a kernel WARN dump, so should
>> a settime64 which returns error.
>
>They're separate things. A ptp clock implementing not provinding
>settime64() or gettime64()/gettimex64() callback will crash when
>userspace tries to call clock_gettime()/clock_settime() on it, since
>either ptp_clock_settime() or ptp_clock_gettime() invokes these
>callbacks unconditionally.
>
>However, failing with -ENOTSUPP/-EOPNOTSUPP when clock_settime() isn't
>supported by a dynamic POSIX clock device is a documented behavior, see
>man-page clock_getres(2).
>
>> Before fixing the warning, the expected behavior of settime64 should be specified clearly,
>
>I think failing with -EOPNOTSUPP (which is the same as -ENOTSUPP on
>Linux) when the operation isn't supported is well-documented, and is
>suitable for this case.
>
>One may argue that it'd be helpful for ptp_clock_register() to provide
>a default implementation of settime64() that always fails with
>-EOPNOTSUPP when the driver doesn't provide one.
>
>However, it's likely a programming bug when gettime64()/settime64() is
>missing, so the current behavior of warning sounds reasonable to me.
>
>> hence why the dfb073d32cac ("ptp: Return -EINVAL on ptp_clock_register if required ops are NULL")?
>
>You may be interested in the original series[1] where the idea of
>warning for missing settime64/gettime64/gettimex64 callbacks came up.

Thanks for the information, this link holds way more relevant information than the *Link* tag in dfb073d32cac.  :)

But isn't  the patch in [1]  and similar change to settime64 better? 
What is the  difference between a null callback and a callback returning error? aren't they saying the same
thing: "this device does not support it"?



David

>
>Also cc Kuniyuki, in case that I missed something or got it wrong.
>
>> 
>> David
>
>Best regards,
>Yao Zi
>
>> > 
>> > Reported-by: Nathan Chancellor <nathan@xxxxxxxxxx>
>> > Closes: https://lore.kernel.org/all/20251108044822.GA3262936@ax162/
>> > Signed-off-by: Yao Zi <ziyao@xxxxxxxxxxx>
>> > ---
>> 
>
>[1]: https://lore.kernel.org/all/20251028095143.396385-1-junjie.cao@xxxxxxxxx/