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

[Yao Zi <me@ziyao.cc>]

On Mon, Dec 15, 2025 at 12:20:43PM +0800, David Wang wrote:
> 
> At 2025-12-15 11:42:17, "Yao Zi" <me@ziyao.cc> 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"?

As I said earlier,

> However, it's likely a programming bug when gettime64()/settime64() is
> missing

but usually you know what exactly happens here when writing a stub
returning -EOPNOTSUPP. IMO this should be the difference.

Quoting Vadim in the original series,

> WARN_ON_ONCE is better in terms of reducing the amount of review work.
> Driver developers will be automatically notified about improper
> implementation while Junjie's patch will simply hide the problem.[2]

Regards,
Yao Zi

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


[2]: https://lore.kernel.org/all/b4d675a4-b7ad-4ecf-8d19-6bf08b452472@linux.dev/