On Fri, Apr 16, 2021 at 10:10:51PM +0200, Thomas Gleixner wrote: > On Tue, Apr 13 2021 at 21:35, Paul E. McKenney wrote: > > > > +static int inject_delay_freq; > > +module_param(inject_delay_freq, int, 0644); > > +static int inject_delay_run = 1; > > +module_param(inject_delay_run, int, 0644); > > int? Can't we just make them 'unsigned int'? Negative values are not > that useful. > > > +static int max_read_retries = 3; > > +module_param(max_read_retries, int, 0644); > > max_read_retries is unused here. Should be in the patch which actually > uses it.
Good point, I will make all three unsigned int and move max_read_retries to 2/5 ("clocksource: Retry clock read if long delays detected"). > > +static void clocksource_watchdog_inject_delay(void) > > +{ > > + int i; > > + static int injectfail = -1; > > + > > + if (inject_delay_freq <= 0 || inject_delay_run <= 0) > > + return; > > + if (injectfail < 0 || injectfail > INT_MAX / 2) > > + injectfail = inject_delay_run; > > + if (!(++injectfail / inject_delay_run % inject_delay_freq)) { > > Operator precedence based cleverness is really easy to parse - NOT! > > > + pr_warn("%s(): Injecting delay.\n", __func__); > > + for (i = 0; i < 2 * WATCHDOG_THRESHOLD / NSEC_PER_MSEC; i++) > > + udelay(1000); > > + pr_warn("%s(): Done injecting delay.\n", __func__); > > + } > > + > > + WARN_ON_ONCE(injectfail < 0); > > +} > > Brain melt stage reached by now. > > static unsigned int invocations, injections; > > if (!inject_delay_period || !inject_delay_repeat) > return; > > if (!(invocations % inject_delay_period)) { > mdelay(2 * WATCHDOG_THRESHOLD / NSEC_PER_MSEC); > if (++injections < inject_delay_repeat) > return; > injections = 0; > } > > invocations++; > } > > Hmm? That is quite a bit nicer than the interacting parameters that I had. I will rework along these lines. Thanx, Paul