On Fri 2023-05-19 10:18:37, Douglas Anderson wrote:
> The fact that there watchdog_hardlockup_enable(),
> watchdog_hardlockup_disable(), and watchdog_hardlockup_probe() are
> declared __weak means that the configured hardlockup detector can
> define non-weak versions of those functions if it needs to. Instead of
> doing this, the perf hardlockup detector hooked itself into the
> default __weak implementation, which was a bit awkward. Clean this up.
>
> >From comments, it looks as if the original design was done because the
> __weak function were expected to implemented by the architecture and
> not by the configured hardlockup detector. This got awkward when we
> tried to add the buddy lockup detector which was not arch-specific but
> wanted to hook into those same functions.
>
> This is not expected to have any functional impact.
>
> @@ -187,27 +187,33 @@ static inline void watchdog_hardlockup_kick(void) { }
> #endif /* !CONFIG_HARDLOCKUP_DETECTOR_PERF */
>
> /*
> - * These functions can be overridden if an architecture implements its
> - * own hardlockup detector.
> + * These functions can be overridden based on the configured hardlockdup
> detector.
> *
> * watchdog_hardlockup_enable/disable can be implemented to start and stop
> when
> - * softlockup watchdog start and stop. The arch must select the
> + * softlockup watchdog start and stop. The detector must select the
> * SOFTLOCKUP_DETECTOR Kconfig.
> */
> -void __weak watchdog_hardlockup_enable(unsigned int cpu)
> -{
> - hardlockup_detector_perf_enable();
> -}
> +void __weak watchdog_hardlockup_enable(unsigned int cpu) { }
>
> -void __weak watchdog_hardlockup_disable(unsigned int cpu)
> -{
> - hardlockup_detector_perf_disable();
> -}
> +void __weak watchdog_hardlockup_disable(unsigned int cpu) { }
>
> /* Return 0, if a hardlockup watchdog is available. Error code otherwise */
> int __weak __init watchdog_hardlockup_probe(void)
> {
> - return hardlockup_detector_perf_init();
> + /*
> + * If CONFIG_HAVE_NMI_WATCHDOG is defined then an architecture
> + * is assumed to have the hard watchdog available and we return 0.
> + */
> + if (IS_ENABLED(CONFIG_HAVE_NMI_WATCHDOG))
> + return 0;
> +
> + /*
> + * Hardlockup detectors other than those using CONFIG_HAVE_NMI_WATCHDOG
> + * are required to implement a non-weak version of this probe function
> + * to tell whether they are available. If they don't override then
> + * we'll return -ENODEV.
> + */
> + return -ENODEV;
> }
When thinking more about it. It is weird that we need to handle
CONFIG_HAVE_NMI_WATCHDOG in this default week function.
It should be handled in watchdog_hardlockup_probe() implemented
in kernel/watchdog_perf.c.
IMHO, the default __weak function could always return -ENODEV;
Would it make sense, please?
Best Regards,
Petr
