Re: [PATCH v6 15/29] x86/hpet: Add helper function hpet_set_comparator_periodic()
On Sat, May 14, 2022 at 10:17:38AM +0200, Thomas Gleixner wrote:
> On Fri, May 13 2022 at 14:19, Ricardo Neri wrote:
> > On Fri, May 06, 2022 at 11:41:13PM +0200, Thomas Gleixner wrote:
> >> The argument about not bloating the code
> >> with an "obvious???" function which is quite small is slightly beyond my
> >> comprehension level.
> >
> > That obvious function would look like this:
> >
> > void hpet_set_comparator_one_shot(int channel, u32 delta)
> > {
> > u32 count;
> >
> > count = hpet_readl(HPET_COUNTER);
> > count += delta;
> > hpet_writel(count, HPET_Tn_CMP(channel));
> > }
>
> This function only works reliably when the delta is large. See
> hpet_clkevt_set_next_event().
That is a good point. One more reason to not have a
hpet_set_comparator_one_shot(), IMO.
Thanks and BR,
Ricardo
Re: [PATCH v6 15/29] x86/hpet: Add helper function hpet_set_comparator_periodic()
On Fri, May 13 2022 at 14:19, Ricardo Neri wrote:
> On Fri, May 06, 2022 at 11:41:13PM +0200, Thomas Gleixner wrote:
>> The argument about not bloating the code
>> with an "obvious???" function which is quite small is slightly beyond my
>> comprehension level.
>
> That obvious function would look like this:
>
> void hpet_set_comparator_one_shot(int channel, u32 delta)
> {
> u32 count;
>
> count = hpet_readl(HPET_COUNTER);
> count += delta;
> hpet_writel(count, HPET_Tn_CMP(channel));
> }
This function only works reliably when the delta is large. See
hpet_clkevt_set_next_event().
Thanks,
tglx
Re: [PATCH v6 15/29] x86/hpet: Add helper function hpet_set_comparator_periodic()
On Fri, May 06, 2022 at 11:51:52PM +0200, Thomas Gleixner wrote: > On Fri, May 06 2022 at 23:41, Thomas Gleixner wrote: > > On Thu, May 05 2022 at 16:59, Ricardo Neri wrote: > >> Programming an HPET channel as periodic requires setting the > >> HPET_TN_SETVAL bit in the channel configuration. Plus, the comparator > >> register must be written twice (once for the comparator value and once for > >> the periodic value). Since this programming might be needed in several > >> places (e.g., the HPET clocksource and the HPET-based hardlockup detector), > >> add a helper function for this purpose. > >> > >> A helper function hpet_set_comparator_oneshot() could also be implemented. > >> However, such function would only program the comparator register and the > >> function would be quite small. Hence, it is better to not bloat the code > >> with such an obvious function. > > > > This word salad above does not provide a single reason why the periodic > > programming function is required and better suited for the NMI watchdog > > case and then goes on and blurbs about why a function which is not > > required is not implemented. The argument about not bloating the code > > with an "obvious???" function which is quite small is slightly beyond my > > comprehension level. > > What's even more uncomprehensible is that the patch which actually sets > up that NMI watchdog cruft has: > > > + if (hc->boot_cfg & HPET_TN_PERIODIC_CAP) > > + hld_data->has_periodic = true; > > So how the heck does that work with a HPET which does not support > periodic mode? If the HPET channel does not support periodic mode (as indicated by the flag above), it will read the HPET counter into a local variable, increment that local variable, and write comparator of the HPET channel. If the HPET channel does support periodic mode, it will not kick it again. It will only kick a periodic HPET channel if needed (e.g., if the NMI watchdog was idle of watchdog_thresh changed its value). > > That watchdog muck will still happily invoke that set periodic function > in the hope that it works by chance? It will not. It will check hld_data->has_periodic and act accordingly. FWIW, I have tested this NMI watchdog with periodic and non-periodic HPET channels. Thanks and BR, Ricardo
Re: [PATCH v6 15/29] x86/hpet: Add helper function hpet_set_comparator_periodic()
On Fri, May 06, 2022 at 11:41:13PM +0200, Thomas Gleixner wrote:
> On Thu, May 05 2022 at 16:59, Ricardo Neri wrote:
> > Programming an HPET channel as periodic requires setting the
> > HPET_TN_SETVAL bit in the channel configuration. Plus, the comparator
> > register must be written twice (once for the comparator value and once for
> > the periodic value). Since this programming might be needed in several
> > places (e.g., the HPET clocksource and the HPET-based hardlockup detector),
> > add a helper function for this purpose.
> >
> > A helper function hpet_set_comparator_oneshot() could also be implemented.
> > However, such function would only program the comparator register and the
> > function would be quite small. Hence, it is better to not bloat the code
> > with such an obvious function.
>
> This word salad above does not provide a single reason why the periodic
> programming function is required and better suited for the NMI watchdog
> case
The goal of hpet_set_comparator_periodic() is to avoid code duplication. The
functions hpet_clkevt_set_state_periodic() and kick_timer() in the HPET NMI
watchdog need to program a periodic HPET channel when supported.
> and then goes on and blurbs about why a function which is not
> required is not implemented.
I can remove this.
> The argument about not bloating the code
> with an "obvious???" function which is quite small is slightly beyond my
> comprehension level.
That obvious function would look like this:
void hpet_set_comparator_one_shot(int channel, u32 delta)
{
u32 count;
count = hpet_readl(HPET_COUNTER);
count += delta;
hpet_writel(count, HPET_Tn_CMP(channel));
}
It involves one register read, one addition and one register write. IMO, this
code is sufficiently simple and small to allow duplication.
Programming a periodic HPET channel is not as straightforward, IMO. It involves
handling two different values (period and comparator) written in a specific
sequence, one configuration bit, and one delay. It also involves three register
writes and one register read.
Thanks and BR,
Ricardo
Re: [PATCH v6 15/29] x86/hpet: Add helper function hpet_set_comparator_periodic()
On Fri, May 06 2022 at 23:41, Thomas Gleixner wrote: > On Thu, May 05 2022 at 16:59, Ricardo Neri wrote: >> Programming an HPET channel as periodic requires setting the >> HPET_TN_SETVAL bit in the channel configuration. Plus, the comparator >> register must be written twice (once for the comparator value and once for >> the periodic value). Since this programming might be needed in several >> places (e.g., the HPET clocksource and the HPET-based hardlockup detector), >> add a helper function for this purpose. >> >> A helper function hpet_set_comparator_oneshot() could also be implemented. >> However, such function would only program the comparator register and the >> function would be quite small. Hence, it is better to not bloat the code >> with such an obvious function. > > This word salad above does not provide a single reason why the periodic > programming function is required and better suited for the NMI watchdog > case and then goes on and blurbs about why a function which is not > required is not implemented. The argument about not bloating the code > with an "obvious???" function which is quite small is slightly beyond my > comprehension level. What's even more uncomprehensible is that the patch which actually sets up that NMI watchdog cruft has: > + if (hc->boot_cfg & HPET_TN_PERIODIC_CAP) > + hld_data->has_periodic = true; So how the heck does that work with a HPET which does not support periodic mode? That watchdog muck will still happily invoke that set periodic function in the hope that it works by chance? Thanks, tglx
Re: [PATCH v6 15/29] x86/hpet: Add helper function hpet_set_comparator_periodic()
On Thu, May 05 2022 at 16:59, Ricardo Neri wrote: > Programming an HPET channel as periodic requires setting the > HPET_TN_SETVAL bit in the channel configuration. Plus, the comparator > register must be written twice (once for the comparator value and once for > the periodic value). Since this programming might be needed in several > places (e.g., the HPET clocksource and the HPET-based hardlockup detector), > add a helper function for this purpose. > > A helper function hpet_set_comparator_oneshot() could also be implemented. > However, such function would only program the comparator register and the > function would be quite small. Hence, it is better to not bloat the code > with such an obvious function. This word salad above does not provide a single reason why the periodic programming function is required and better suited for the NMI watchdog case and then goes on and blurbs about why a function which is not required is not implemented. The argument about not bloating the code with an "obvious???" function which is quite small is slightly beyond my comprehension level. Thanks, tglx
