intr/x86: merge pic_{dis,en}able_source() call into pic_{dis,en}able_intr()

[ehem_freebsd_m5p.com]

Summary

When pic_{dis,en}able_intr() are called, they are paired with
pic_{dis,en}able_source() calls. Let the PIC implementations handle
calling of the second function.

Significantly, the pic_disable_source() calls paired with the
pic_disable_intr() calls are the only time PIC_NO_EOI was used. This
means the enum can be removed and pic_disable_source()'s prototype can
be simplified. Crucially, this means pic_disable_source() matches the
(*pre_ithread)() function of intr_event_create().

[ehem_freebsd_m5p.com]

Mostly looking for a review from @royger I'm unsure whether this additional call really needs to be retained, versus being unnecessary.

For for information on where this is headed: https://github.com/freebsd/freebsd-src/pull/1457

sys/x86/xen/xen_arch_intr.c

191

I'm unsure of this. intr_remove_handler() would previously end up calling xen_intr_pic_disable_source() after the xen_intr_pic_disable_intr() call, but I'm unsure whether this is actually needed. Does the source really need to be masked before the port is?

[ehem_freebsd_m5p.com]

Note to anyone else who happens to run across this. Indeed, (mask_fn)isrc->is_pic->pic_disable_source could be substituted for the intr_disable_srcargument to intr_event_create() inside intr_register_source(). Issue is after this commit, #1457 first modifies intr_disable_src and then later completely removes it. At the point where this is the wrapper still needs to be retained.

[ehem_freebsd_m5p.com]

Then when looking for something else discover sys/x86/isa/clock.c has the call i8254_intsrc->is_pic->pic_enable_intr(i8254_intsrc); without the ->pic_enable_source(). I lack hardware to test this, so I'm unsure of the effect.

[ehem_freebsd_m5p.com]

Seems the issue in sys/x86/isa/clock.c originates from rG:875b8844bec. I'm unsure how to work around the issue, rG:49ed68bbf3c makes the situation worse.

Is my understanding is correct the ISA clock is always paired with the AT PIC? If so then it is notable the AT PIC pic_enable_intr() function is a NOP before D47745. In which case the call to ->pic_enable_intr() in attimer_attach() can simply be removed. Then the call to ->pic_enable_source() is the only operational one and it handles everything.

@mav any insight into the effect of rG:875b8844bec, rG:49ed68bbf3c, and D47745?

[ehem_freebsd_m5p.com]

Updating, point to possible solution for what I need @mav to review (remove ->pic_enable_intr() call from clock.c/875b8844bec hack).

[ehem_freebsd_m5p.com]

sys/x86/isa/clock.c

608–609 ↗

(On Diff #147427)

@mav is the one to look at this (since @mav created it several years ago). I note if the ISA clock is paired with the AT PIC, the AT PIC's atpic_enable_intr() function is a NOP. My concern is, was the ISA clock ever paired with a different PIC driver?

I do note most ->pic_enable_source() implementations do a superset of what ->pic_enable_intr() does (not just the AT PIC). In which case removing this call and only doing ->pic_enable_source() will function correctly anyway.

[mav]

sys/x86/isa/clock.c

608–609 ↗

(On Diff #147427)

14 years is a bit more than several. ;) I am not sure I can review this without much deeper look, other than sharing some memories.

What I can recall is that this timer is 16-bit and has very short wraparound time. It means to use it as a timecounter we must track its interrupts to handle overflows. On the other side we might or might not need the timer interrupts themselves to do anything, since we might have more functional eventtimers to run hardclock(). You may see clkintr_pending = 1 hack in intr_execute_handlers(), which I think might explain these manipulations with enabling interrupts vs sources -- we wants interrupts enabled, but we want them as cheap and invisible as possible unless we really need them to play as eventtimer.

I don't remember this driver worked with different interrupt controller. Mentioned HPET can just steal the IRQ0 from this timer, but after that this driver can not function and should be disabled, so they don't really interoperate. There was also such thing as PC98 platform at that time that had its own oddities that could requite some hacks, but it was almost dead even back then and I don't remember much now.