[PATCH] Add intr_mask() / intr_unmask() interface to mask / unmask individual interrupt sources

[Jason Thorpe]

Folks --

A patch I need reviewed by x86 interrupt handling experts. It adds two new routines:

void intr_mask(void *ih);
void intr_unmask(void *ih);

That mask and unmask, respectively, individual interrupt sources. The "ih" argument is the interrupt cookie that's returned by intr_establish().

There are some corresponding changes to the ACPI interrupt functions that I haven't included here, but suffice to say they're basic wrappers.

These changes are needed in order to be able to defer processing of a hard interrupt to a softint in some circumstances, specifically in cases where it's not actually safe to access the interrupting device's registers while in interrupt context. Real world example: ihidev (hid-over-i2c driver). That driver currently does i2c access from the hardware interrupt, but it plays some unsavory tricks in order to do so, and it can't work in all cases as-written. Unfortunately, the hid-over-i2c spec specifies a level-triggered interrupt, hence the need to be able to mask it off after scheduling a softint. The interrupt can then be unmasked after processing all outstanding data from the input device.

Unfortunately, I don't have a uhidev device I can test this against directly. I know bouyer@ does, and I am hoping he can test the larger set of changes I'm working on. I have tested these patches at least in the sense that the system boots multi-user ... i.e. I didn't break anything :-)

I would appreciate review of these changes by someone who is very familiar with the x86 interrupt handling code. I am pretty sure I got this right, but it's a critical section of code so I want to be sure. The interrupt vector changes essentially check the "mask count" of the intrsource and treat it as an interrupt masked by splXXX() in two places:

1- Upon entry to an interrupt vector (either by the "interrupted" path or by the "resume" path).

2- Upon return from invoking all of the handlers for an intrsource ... because the hard interrupt handler will be the thing requesting the interrupt to be masked, we check here so we can void re-enabling the source, thus avoiding an extra interrupt just to mark it as pending.

Note that it's important for a masked interrupt source to always be marked as "pending" so that it can be reliably serviced again when unmasked.

Diff attached. Please let me know if it looks OK or if you have any questions.

Thanks!

[Jason Thorpe]

Anyone? Bueller?

Post by Jason Thorpe
Folks --
void intr_mask(void *ih);
void intr_unmask(void *ih);
That mask and unmask, respectively, individual interrupt sources. The "ih" argument is the interrupt cookie that's returned by intr_establish().
There are some corresponding changes to the ACPI interrupt functions that I haven't included here, but suffice to say they're basic wrappers.
These changes are needed in order to be able to defer processing of a hard interrupt to a softint in some circumstances, specifically in cases where it's not actually safe to access the interrupting device's registers while in interrupt context. Real world example: ihidev (hid-over-i2c driver). That driver currently does i2c access from the hardware interrupt, but it plays some unsavory tricks in order to do so, and it can't work in all cases as-written. Unfortunately, the hid-over-i2c spec specifies a level-triggered interrupt, hence the need to be able to mask it off after scheduling a softint. The interrupt can then be unmasked after processing all outstanding data from the input device.
1- Upon entry to an interrupt vector (either by the "interrupted" path or by the "resume" path).
2- Upon return from invoking all of the handlers for an intrsource ... because the hard interrupt handler will be the thing requesting the interrupt to be masked, we check here so we can void re-enabling the source, thus avoiding an extra interrupt just to mark it as pending.
Note that it's important for a masked interrupt source to always be marked as "pending" so that it can be reliably serviced again when unmasked.
Diff attached. Please let me know if it looks OK or if you have any questions.
Thanks!
<x86-intr-diffs.txt>
-- thorpej

-- thorpej


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Maxime Villard]

Post by Jason Thorpe
Anyone? Bueller?

Small notes:

+ const u_long psl = x86_read_psl();
+ x86_disable_intr();

Not sure, why disable interrupts? To prevent an interrupt between the
is_mask_count and pic_hw* changes?

+ if (mask) {
+ source->is_mask_count++;
+ KASSERT(source->is_mask_count != 0);
+ (*pic->pic_hwmask)(pic, ih->ih_pin);

Seems like pic_hwmask only needs to be called when is_mask_count==0.

Note that XENINTRSTUB will also need the same changes.

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Jason Thorpe]

Post by Maxime Villard
Seems like pic_hwmask only needs to be called when is_mask_count==0.

Yes, good catch. I also noticed a few other places where I should be checking is_mask_count before calling pic_hwunmask, and also realized that I needed to add some code to handle the set_affinity flow ... it's complicated by the fact that an intr_mask()'d interrupt will always be "pending", so some care is needed. Specifically, before waiting for "pending" to drain out, I now mark the interrupt sources as "distribution-pending" ... so when intr_unmask() comes along, it won't re-enable the source at the hardware if it sees that, but will still force processing of the source if it was marked as interrupt-pending.

Ok, I've made some tweaks -- I still need to do some more testing, but I think the basic x86 version of the changes is basically ready. Updated diff attached.

Post by Maxime Villard
Note that XENINTRSTUB will also need the same changes.

Thanks, I'll take a look at those, too.

So, I took a look at this, and I'm not entirely sure what can / should be done with them. They seem only superficially related to the regular interrupt stubs. They don't seem to have any notion of "pending". Can someone explain to me how these work?

-- thorpej

[Jason Thorpe]

Just please make sure that the i2c code (espeically hid over i2c) can
still run without the mask/unmask MD parts (this will probably require some
#ifdef unfortunably).

That's not possible -- the whole point is to get rid of i2c access in interrupt context (because it's not safe, even with I2C_F_POLL).

-- thorpej


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Jason Thorpe]

Note that usually, the bus acquire / release issue is not a problem
if there's a single slave on the bus, which remains the common case.

It might be a common case on x86, but it is definitely not the common case on ARM.

[...]

I still believe we'll need some #define to know if the MD interrupt code
provides working mask/unmask until all ports provide them.
This will be usefull for more hardware than i2c ...

I guess I'm fine with adding a #define __HAVE_INTR_MASK that can be tested (and I'll make ihidev.c require it).
But eventually a proper overhaul of the various interrupt APIs we have needs to be undertaken. Alas, that's just beyond the scope of what I'm trying to get done right now, and I don't really have the bandwidth to go down that rabbit hole.

Sure, I agree with this long-term goal. Beside ihidev I have another case
that would use it (on arm).
But this is lots of work, and we can't break existing code until all
ports have implemented it.

Well, the current ihidev really only works on x86 anyway, because it depends on ACPI. Yes, I know that aarch64 has ACPI, but I don't know if any such systems where i2c hid would be applicable at the moment (the i2c hid driver does not currently support FDT, which would be the more common way for i2c hid to be used on ARM right now anyway).

I suppose with my current set of changes, ihidev would be broken on a XEN Dom0 system. How common would that scenario be? Anyway, I'd argue that the current situation is equally broken, and I've already been waiting for 4 months for someone who understands the XEN interrupt code to help out with this. Am I supposed to just wait indefinitely?

-- thorpej


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Jason Thorpe]

+Andrew for his comments as well.

Ok, so I thought about this some more, and there is a problem there, but not the one you mentioned.
intr_mask() is intended to be callable from an interrupt handler, but you can't take the cpu_lock in that case, because that's a MUTEX_DEFAULT mutex. The code can be tweaked to address this issue, but yes, places where the mask count is checked do need to be protected in a block that disables interrupts.
I'll post a follow-up patch shortly.

Well, FSVO "shortly". Anyway, I *think* this is the final patch. The main change between this one and the previous is that this one also uses an xcall to perform the "hwunmask" operation. I added the relevant details in code comments, so please make sure to read them.

Again, to recap:

1- Adds intr_mask() and intr_unmask() functions that take as an argument the cookie returned by intr_establish().

2- intr_mask() calls can be nested (i.e. there is a mask count).

3- intr_mask() can be called from interrupt context; intr_unmask() cannot (softint context is OK).

4- Whereas spl*() masks off a logical interrupt level (e.g. IPL_NET), intr_mask() masks off an individual interrupt source / line.

5- Wrappers for ACPI are included (following the existing ACPI interrupt code layering model).

6- Adapts the hid-over-i2c driver, so as to avoid using i2c in interrupt context. I don't have such a device. Please ping me off-list if you have one and would be willing to test a kernel with these changes.

7- Making all of this stuff work with Xen is up to the people who maintain our Xen code. As of right now, it doesn't, and I don't know what will happen if code on a Xen system tries to call intr_mask().

[Jason Thorpe]

but my one comment is
that it sucks that you actually have a need to mask an interrupt source in
regular operation the first place. Doesn't sound like a fun problem.

Yes, this is particularly dumb item in the hid-over-i2c spec (level-sensitive interrupts).

-- thorpej


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de