[External] Re: [PATCH v4 1/2] watchdog: move arm64 watchdog_hld into common code

[yunhui cui]

Hi Will,

On Fri, Nov 7, 2025 at 9:11 PM Will Deacon <will at kernel.org> wrote:
>
> On Fri, Nov 07, 2025 at 10:42:25AM +0800, yunhui cui wrote:
> > On Mon, Nov 3, 2025 at 9:44 PM Will Deacon <will at kernel.org> wrote:
> > >
> > > On Tue, Oct 14, 2025 at 11:14:24AM +0800, Yunhui Cui wrote:
> > > > @@ -306,3 +307,85 @@ void __init hardlockup_config_perf_event(const char *str)
> > > >       wd_hw_attr.type = PERF_TYPE_RAW;
> > > >       wd_hw_attr.config = config;
> > > >  }
> > > > +
> > > > +#ifdef CONFIG_WATCHDOG_PERF_ADJUST_PERIOD
> > > > +/*
> > > > + * Safe maximum CPU frequency in case a particular platform doesn't implement
> > > > + * cpufreq driver. Although, architecture doesn't put any restrictions on
> > > > + * maximum frequency but 5 GHz seems to be safe maximum given the available
> > > > + * CPUs in the market which are clocked much less than 5 GHz. On the other
> > > > + * hand, we can't make it much higher as it would lead to a large hard-lockup
> > > > + * detection timeout on parts which are running slower (eg. 1GHz on
> > > > + * Developerbox) and doesn't possess a cpufreq driver.
> > > > + */
> > > > +#define SAFE_MAX_CPU_FREQ    5000000000UL // 5 GHz
> > > > +__weak u64 hw_nmi_get_sample_period(int watchdog_thresh)
> > > > +{
> > > > +     unsigned int cpu = smp_processor_id();
> > > > +     unsigned long max_cpu_freq;
> > > > +
> > > > +     max_cpu_freq = cpufreq_get_hw_max_freq(cpu) * 1000UL;
> > > > +     if (!max_cpu_freq)
> > > > +             max_cpu_freq = SAFE_MAX_CPU_FREQ;
> > > > +
> > > > +     return (u64)max_cpu_freq * watchdog_thresh;
> > > > +}
> > >
> > > Why does this function become __weak? Neither arm64 nor riscv override
> > > it afaict.
> >
> > Would you say there’s any particular issue here? If some architectures
> > might need to override the hw_nmi_get_sample_period() function later
> > on, wouldn’t __weak be a more reasonable choice?
>
> __weak is pretty brittle (it can depend on link order if you have multiple
> targets) and I suspect it prevents inlining when LTO isn't enabled. It's
> cleaner and more robust for architectures to provide their hooks by
> #defining the symbol, as is done commonly in other parts of the kernel.
>
> But in this particular case, it's completely unnecessary because there
> isn't an architectural override and so this function should simply be
> static.

Since the function is declared as u64 hw_nmi_get_sample_period(int
watchdog_thresh) in nmi.h, it cannot be static.
I will remove the __weak modifier in the next version.

>
> Will

Thanks,
Yunhui