On 23/06/2022, 19:28:34, Nathan Lynch wrote:
> Laurent Dufour <lduf...@linux.ibm.com> writes:
>> diff --git a/arch/powerpc/platforms/pseries/mobility.c
>> b/arch/powerpc/platforms/pseries/mobility.c
>> index 179bbd4ae881..4284ceaf9060 100644
>> --- a/arch/powerpc/platforms/pseries/mobility.c
>> +++ b/arch/powerpc/platforms/pseries/mobility.c
>> @@ -48,6 +48,39 @@ struct update_props_workarea {
>> #define MIGRATION_SCOPE (1)
>> #define PRRN_SCOPE -2
>>
>> +#ifdef CONFIG_PPC_WATCHDOG
>> +static unsigned int lpm_nmi_wd_factor = 200;
>> +
>> +#ifdef CONFIG_SYSCTL
>> +static struct ctl_table lpm_nmi_wd_factor_ctl_table[] = {
>> + {
>> + .procname = "lpm_nmi_watchdog_factor",
>
> Assuming the basic idea is acceptable, I suggest making the user-visible
> name more generic (e.g. "nmi_watchdog_factor") in case it makes sense to
> apply this to other contexts in the future.
Fair enough, indeed, I was wondering if "lpm" is meaningful.
>
>> + .data = &lpm_nmi_wd_factor,
>> + .maxlen = sizeof(int),
>> + .mode = 0644,
>> + .proc_handler = proc_douintvec_minmax,
>> + },
>> + {}
>> +};
>> +static struct ctl_table lpm_nmi_wd_factor_sysctl_root[] = {
>> + {
>> + .procname = "kernel",
>> + .mode = 0555,
>> + .child = lpm_nmi_wd_factor_ctl_table,
>> + },
>> + {}
>> +};
>> +
>> +static int __init register_lpm_nmi_wd_factor_sysctl(void)
>> +{
>> + register_sysctl_table(lpm_nmi_wd_factor_sysctl_root);
>> +
>> + return 0;
>> +}
>> +device_initcall(register_lpm_nmi_wd_factor_sysctl);
>> +#endif /* CONFIG_SYSCTL */
>> +#endif /* CONFIG_PPC_WATCHDOG */
>> +
>> static int mobility_rtas_call(int token, char *buf, s32 scope)
>> {
>> int rc;
>> @@ -702,6 +735,7 @@ static int pseries_suspend(u64 handle)
>> static int pseries_migrate_partition(u64 handle)
>> {
>> int ret;
>> + unsigned int factor = lpm_nmi_wd_factor;
>>
>> ret = wait_for_vasi_session_suspending(handle);
>> if (ret)
>> @@ -709,6 +743,13 @@ static int pseries_migrate_partition(u64 handle)
>>
>> vas_migration_handler(VAS_SUSPEND);
>>
>> +#ifdef CONFIG_PPC_WATCHDOG
>> + if (factor) {
>> + pr_info("Set the NMI watchdog factor to %u%%\n", factor);
>> + watchdog_nmi_set_lpm_factor(factor);
>> + }
>> +#endif /* CONFIG_PPC_WATCHDOG */
>> +
>> ret = pseries_suspend(handle);
>> if (ret == 0) {
>> post_mobility_fixup();
>> @@ -716,6 +757,13 @@ static int pseries_migrate_partition(u64 handle)
>> } else
>> pseries_cancel_migration(handle, ret);
>>
>> +#ifdef CONFIG_PPC_WATCHDOG
>> + if (factor) {
>> + pr_info("Restoring NMI watchdog timer\n");
>> + watchdog_nmi_set_lpm_factor(0);
>> + }
>> +#endif /* CONFIG_PPC_WATCHDOG */
>> +
>
> A couple more suggestions:
>
> * Move the prints into a single statement in watchdog_nmi_set_lpm_factor().
You're right that sounds a better place.
>
> * Add no-op versions of watchdog_nmi_set_lpm_factor for
> !CONFIG_PPC_WATCHDOG so we can minimize the #ifdef here.
>
Furthermore, this breaks compilation when !CONFIG_PPC_WATCHDOG because
lpm_nmi_wd_factor is not defined. I'll rework that part.
> Otherwise this looks fine to me.
Thanks,
Laurent.
