tpm_tis: Detect interrupt storms

To try and detect potential interrupt storms that have been occurring with tpm_tis devices it was suggested to use kstat_irqs() to get the number of interrupts. Since tpm_tis can be built as a module it needs kstat_irqs exported.

Reported-by kernel test robot <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Jarkko Sakkinen <[email protected]>

Cc Jason Gunthorpe <[email protected]>

Cc Peter Huewe <[email protected]>

Cc James Bottomley <[email protected]>

Cc Matthew Garrett <[email protected]>

Cc Hans de Goede <[email protected]>

Signed-off-by Jerry Snitselaar <[email protected]>

Quoted 5 lines · Jerry Snitselaar

To try and detect potential interrupt storms that have been occurring with tpm_tis devices it was suggested to use kstat_irqs() to get the number of interrupts. Since tpm_tis can be built as a module it needs kstat_irqs exported.

I think you should also have a paragraph explicitly stating that i915_pmu.c contains a duplicate of kstat_irqs() because it is not exported as of today. It adds a lot more weight to this given that there is already existing mainline usage (kind of).

Quoted 7 lines · Jarkko Sakkinen

Quoted 5 lines · Jerry Snitselaar

To try and detect potential interrupt storms that have been occurring with tpm_tis devices it was suggested to use kstat_irqs() to get the number of interrupts. Since tpm_tis can be built as a module it needs kstat_irqs exported.

Quoted 4 lines

I think you should also have a paragraph explicitly stating that i915_pmu.c contains a duplicate of kstat_irqs() because it is not exported as of today. It adds a lot more weight to this given that there is already existing mainline usage (kind of).

It's abusage and just the fact that it exists is not an argument by itself.

Quoted 13 lines · Thomas Gleixner

Quoted 11 lines · Jarkko Sakkinen

Quoted 5 lines · Jerry Snitselaar

To try and detect potential interrupt storms that have been occurring with tpm_tis devices it was suggested to use kstat_irqs() to get the number of interrupts. Since tpm_tis can be built as a module it needs kstat_irqs exported.

I think you should also have a paragraph explicitly stating that i915_pmu.c contains a duplicate of kstat_irqs() because it is not exported as of today. It adds a lot more weight to this given that there is already existing mainline usage (kind of).

Quoted 2 lines

It's abusage and just the fact that it exists is not an argument by itself.

What we want is a count of the interrupts to see if we're having an interrupt storm from the TPM device (some seem to be wired to fire the interrupt even when there's no event to warrant it). Since kstat_irqs_user() does the correct RCU locking, should we be using that instead?

To try and detect potential interrupt storms that have been occurring with tpm_tis devices it was suggested to use kstat_irqs() to get the number of interrupts. Since tpm_tis can be built as a module it needs kstat_irqs exported.

Jerry,

On Fri, Dec 04 2020 at 18:43, Jerry Snitselaar wrote:

The proper prefix is 'genirq:' git log kernel/irq/irqdesc.c would have told you.

Quoted 5 lines

To try and detect potential interrupt storms that have been occurring with tpm_tis devices it was suggested to use kstat_irqs() to get the number of interrupts. Since tpm_tis can be built as a module it needs kstat_irqs exported.

I'm not really enthused about exporting this without making it at least safe. Using it from an interrupt handler is obviously safe vs. concurrent removal, but the next driver writer who thinks this is cool is going to get it wrong for sure.

Though I still have to figure out what the advantage of invoking a function which needs to do a radix tree lookup over a device local counter is just to keep track of this.

I'll reply on the TPM part of this as well.

Thanks,

tglx

Now that kstat_irqs is exported, get rid of count_interrupts in i915_pmu.c

drivers/gpu/drm/i915/i915_pmu.c +0 -5

@@ -423,22 +423,6 @@ static enum hrtimer_restart i915_sample(struct hrtimer *hrtimer)
 	return HRTIMER_RESTART;
 }
 
-static u64 count_interrupts(struct drm_i915_private *i915)
-{
-	/* open-coded kstat_irqs() */
-	struct irq_desc *desc = irq_to_desc(i915->drm.pdev->irq);
-	u64 sum = 0;

Quoted 2 lines · Jerry Snitselaar

Now that kstat_irqs is exported, get rid of count_interrupts in i915_pmu.c

May I ask why this has been merged in the first place?

Nothing in a driver has ever to fiddle with the internals of an irq descriptor. We have functions for properly accessing them. Just because C allows to fiddle with everything is not a justification. If the required function is not exported then adding the export with a proper explanation is not asked too much.

Also this lacks protection or at least a comment why this can be called safely and is not subject to a concurrent removal of the irq descriptor. The same problem exists when calling kstat_irqs(). It's even documented at the top of the function.

Quoted 12 lines · Thomas Gleixner

Quoted 2 lines · Jerry Snitselaar

Now that kstat_irqs is exported, get rid of count_interrupts in i915_pmu.c

May I ask why this has been merged in the first place?

Nothing in a driver has ever to fiddle with the internals of an irq descriptor. We have functions for properly accessing them. Just because C allows to fiddle with everything is not a justification. If the required function is not exported then adding the export with a proper explanation is not asked too much.

Quoted 4 lines

Also this lacks protection or at least a comment why this can be called safely and is not subject to a concurrent removal of the irq descriptor. The same problem exists when calling kstat_irqs(). It's even documented at the top of the function.

And as pointed out vs. that TPM thing this really could have been a trivial

    i915->irqs++;

in the interrupt handler and a read of that instead of iterating over all possible cpus and summing it up. Oh well...

Thanks,

tglx

Quoted 14 lines

Now that kstat_irqs is exported, get rid of count_interrupts in i915_pmu.c

drivers/gpu/drm/i915/i915_pmu.c +0 -5

@@ -423,22 +423,6 @@ static enum hrtimer_restart i915_sample(struct hrtimer *hrtimer)
 	return HRTIMER_RESTART;
 }
 
-static u64 count_interrupts(struct drm_i915_private *i915)
-{
-	/* open-coded kstat_irqs() */
-	struct irq_desc *desc = irq_to_desc(i915->drm.pdev->irq);
-	u64 sum = 0;

drivers/gpu/drm/i915/i915_pmu.c static enum hrtimer_restart i915_sample(struct hrtimer *hrtimer) · line 440

Quoted 10 lines

drivers/gpu/drm/i915/i915_pmu.c +0 -10

-	int cpu;
-
-	if (!desc || !desc->kstat_irqs)
-		return 0;
-
-	for_each_possible_cpu(cpu)
-		sum += *per_cpu_ptr(desc->kstat_irqs, cpu);
-
-	return sum;
-}

May I ask why this has been merged in the first place?

Nothing in a driver has ever to fiddle with the internals of an irq descriptor. We have functions for properly accessing them. Just because C allows to fiddle with everything is not a justification. If the required function is not exported then adding the export with a proper explanation is not asked too much.

Also this lacks protection or at least a comment why this can be called safely and is not subject to a concurrent removal of the irq descriptor. The same problem exists when calling kstat_irqs(). It's even documented at the top of the function.

Thanks,

tglx

Thomas Gleixner @ 2020-12-06 09:38 MST:

I don't know the history behind this bit. I stumbled across it in cscope when looking for places using kstat_irqs.

Quoted 14 lines

Now that kstat_irqs is exported, get rid of count_interrupts in i915_pmu.c

drivers/gpu/drm/i915/i915_pmu.c +0 -5

@@ -423,22 +423,6 @@ static enum hrtimer_restart i915_sample(struct hrtimer *hrtimer)
      return HRTIMER_RESTART;
 }
 
-static u64 count_interrupts(struct drm_i915_private *i915)
-{
-     /* open-coded kstat_irqs() */
-     struct irq_desc *desc = irq_to_desc(i915->drm.pdev->irq);
-     u64 sum = 0;

drivers/gpu/drm/i915/i915_pmu.c static enum hrtimer_restart i915_sample(struct hrtimer *hrtimer) · line 440

Quoted 10 lines

drivers/gpu/drm/i915/i915_pmu.c +0 -10

-     int cpu;
-
-     if (!desc || !desc->kstat_irqs)
-             return 0;
-
-     for_each_possible_cpu(cpu)
-             sum += *per_cpu_ptr(desc->kstat_irqs, cpu);
-
-     return sum;
-}

May I ask why this has been merged in the first place?

Nothing in a driver has ever to fiddle with the internals of an irq descriptor. We have functions for properly accessing them. Just because C allows to fiddle with everything is not a justification. If the required function is not exported then adding the export with a proper explanation is not asked too much.

Also this lacks protection or at least a comment why this can be called safely and is not subject to a concurrent removal of the irq descriptor. The same problem exists when calling kstat_irqs(). It's even documented at the top of the function.

Thanks,

tglx

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

+ Tvrtko and Chris for comments

Code seems to be added in:

commit 0cd4684d6ea9a4ffec33fc19de4dd667bb90d0a5

Author: Tvrtko Ursulin <[email protected]>
Date:   Tue Nov 21 18:18:50 2017 +0000

drm/i915/pmu: Add interrupt count metric

I think later in the thread there was a suggestion to replace this with simple counter increment in IRQ handler.

Quoting Thomas Gleixner (2020-12-06 18:38:44)

Quoted 44 lines · Jerry Snitselaar

Quoted 14 lines

Now that kstat_irqs is exported, get rid of count_interrupts in i915_pmu.c

drivers/gpu/drm/i915/i915_pmu.c +0 -5

@@ -423,22 +423,6 @@ static enum hrtimer_restart i915_sample(struct hrtimer *hrtimer)
       return HRTIMER_RESTART;
  }
 
-static u64 count_interrupts(struct drm_i915_private *i915)
-{
-     /* open-coded kstat_irqs() */
-     struct irq_desc *desc = irq_to_desc(i915->drm.pdev->irq);
-     u64 sum = 0;

drivers/gpu/drm/i915/i915_pmu.c static enum hrtimer_restart i915_sample(struct hrtimer *hrtimer) · line 440

Quoted 10 lines

drivers/gpu/drm/i915/i915_pmu.c +0 -10

-     int cpu;
-
-     if (!desc || !desc->kstat_irqs)
-             return 0;
-
-     for_each_possible_cpu(cpu)
-             sum += *per_cpu_ptr(desc->kstat_irqs, cpu);
-
-     return sum;
-}

May I ask why this has been merged in the first place?

Nothing in a driver has ever to fiddle with the internals of an irq descriptor. We have functions for properly accessing them. Just because C allows to fiddle with everything is not a justification. If the required function is not exported then adding the export with a proper explanation is not asked too much.

Also this lacks protection or at least a comment why this can be called safely and is not subject to a concurrent removal of the irq descriptor. The same problem exists when calling kstat_irqs(). It's even documented at the top of the function.

Thanks,

tglx

Quoted 4 lines

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 2 lines · Joonas Lahtinen

I think later in the thread there was a suggestion to replace this with simple counter increment in IRQ handler.

It was indeed unsafe until recent b00bccb3f0bb ("drm/i915/pmu: Handle PCI unbind") but now should be fine.

If kstat_irqs does not get exported it is easy enough for i915 to keep a local counter. Reasoning was very infrequent per cpu summation is much cheaper than very frequent atomic add. Up to thousands of interrupts per second vs "once per second" PMU read kind of thing.

Quoted 11 lines · Tvrtko Ursulin

Quoted 2 lines · Joonas Lahtinen

I think later in the thread there was a suggestion to replace this with simple counter increment in IRQ handler.

It was indeed unsafe until recent b00bccb3f0bb ("drm/i915/pmu: Handle PCI unbind") but now should be fine.

If kstat_irqs does not get exported it is easy enough for i915 to keep a local counter. Reasoning was very infrequent per cpu summation is much cheaper than very frequent atomic add. Up to thousands of interrupts per second vs "once per second" PMU read kind of thing.

Why do you need a atomic_add? It's ONE interrupt which can only be executed on ONE CPU at a time. Interrupt handlers are non-reentrant.

The core code function will just return an accumulated counter nowadays which is only 32bit wide, which is what the interface provided forever. That needs to be fixed first.

Aside of that the accounting is wrong when the interrupt line is shared because the core accounts interrupt per line not per device sharing the line. Don't know whether you care or not.

I'll send out a series addressing irq_to_desc() (ab)use all over the place shortly. i915 is in there...

Quoted 2 lines · Thomas Gleixner

I'll send out a series addressing irq_to_desc() (ab)use all over the place shortly. i915 is in there...

Yep we don't need atomic, my bad. And we would care about the shared interrupt line. And without atomic the extra accounting falls way below noise.

So in the light of it all, it sounds best I just quickly replace our abuse with private counting and then you don't have to deal with it in your series.

Quoted 7 lines · Tvrtko Ursulin

Quoted 2 lines · Thomas Gleixner

I'll send out a series addressing irq_to_desc() (ab)use all over the place shortly. i915 is in there...

Yep we don't need atomic, my bad. And we would care about the shared interrupt line. And without atomic the extra accounting falls way below noise.

You have to be careful though. If you make the accumulated counter 64 bit wide then you need to be careful vs. 32bit machines.

Quoted 3 lines

So in the light of it all, it sounds best I just quickly replace our abuse with private counting and then you don't have to deal with it in your series.

I mostly have it. Still chewing on the 32bit vs. 64bit thing. And keeping it in my series allows me to remove the export of irq_to_desc() at the end without waiting for your tree to be merged.

Give me a few.

Just as a side note. I was looking at tpm_tis_probe_irq_single() and that function is leaking the interrupt request if any of the checks afterwards fails, except for the final interrupt probe check which does a cleanup. That means on fail before that the interrupt handler stays requested up to the point where the module is removed. If that's a shared interrupt and some other device is active on the same line, then each interrupt from that device will call into the TPM code. Something like the below is needed.

Also the X86 autoprobe mechanism is interesting:

	if (IS_ENABLED(CONFIG_X86))
		for (i = 3; i <= 15; i++)
			if (!tpm_tis_probe_irq_single(chip, intmask, 0, i))
				return;

The third argument is 'flags' which is handed to request_irq(). So that won't ever be able to probe a shared interrupt. But if an interrupt number > 0 is handed to tpm_tis_core_init() the interrupt is requested with IRQF_SHARED. Same issue when the chip has an interrupt number in the register. It's also requested exclusive which is pretty likely to fail on ancient x86 machines.

Quoted 17 lines · Thomas Gleixner

Just as a side note. I was looking at tpm_tis_probe_irq_single() and that function is leaking the interrupt request if any of the checks afterwards fails, except for the final interrupt probe check which does a cleanup. That means on fail before that the interrupt handler stays requested up to the point where the module is removed. If that's a shared interrupt and some other device is active on the same line, then each interrupt from that device will call into the TPM code. Something like the below is needed.

Also the X86 autoprobe mechanism is interesting:

	if (IS_ENABLED(CONFIG_X86))
		for (i = 3; i <= 15; i++)
			if (!tpm_tis_probe_irq_single(chip, intmask, 0,
i))
				return;

Quoted 6 lines

The third argument is 'flags' which is handed to request_irq(). So that won't ever be able to probe a shared interrupt. But if an interrupt number > 0 is handed to tpm_tis_core_init() the interrupt is requested with IRQF_SHARED. Same issue when the chip has an interrupt number in the register. It's also requested exclusive which is pretty likely to fail on ancient x86 machines.

It is very likely none of this works any more, it has been repeatedly reworked over the years and just left behind out of fear someone needs it. I've thought it should be deleted for a while now.

I suppose the original logic was to try and probe without SHARED because a probe would need exclusive access to the interrupt to tell if the TPM was actually the source, not some other device.

It is all very old and very out of step with current thinking, IMHO. I skeptical that TPM interrupts were ever valuable enough to deserve this in the first place.

Quoted 29 lines · Jason Gunthorpe

Quoted 17 lines · Thomas Gleixner

Just as a side note. I was looking at tpm_tis_probe_irq_single() and that function is leaking the interrupt request if any of the checks afterwards fails, except for the final interrupt probe check which does a cleanup. That means on fail before that the interrupt handler stays requested up to the point where the module is removed. If that's a shared interrupt and some other device is active on the same line, then each interrupt from that device will call into the TPM code. Something like the below is needed.

Also the X86 autoprobe mechanism is interesting:

	if (IS_ENABLED(CONFIG_X86))
		for (i = 3; i <= 15; i++)
			if (!tpm_tis_probe_irq_single(chip, intmask, 0,
i))
				return;

Quoted 6 lines

The third argument is 'flags' which is handed to request_irq(). So that won't ever be able to probe a shared interrupt. But if an interrupt number > 0 is handed to tpm_tis_core_init() the interrupt is requested with IRQF_SHARED. Same issue when the chip has an interrupt number in the register. It's also requested exclusive which is pretty likely to fail on ancient x86 machines.

It is very likely none of this works any more, it has been repeatedly reworked over the years and just left behind out of fear someone needs it. I've thought it should be deleted for a while now.

Quoted 7 lines

I suppose the original logic was to try and probe without SHARED because a probe would need exclusive access to the interrupt to tell if the TPM was actually the source, not some other device.

It is all very old and very out of step with current thinking, IMHO. I skeptical that TPM interrupts were ever valuable enough to deserve this in the first place.

For what it's worth, I agree. Trying to probe all 15 ISA interrupts is last millennium thinking we should completely avoid. If it's not described in ACPI then you don't get an interrupt full stop.

James

Quoted 16 lines · Thomas Gleixner

Just as a side note. I was looking at tpm_tis_probe_irq_single() and that function is leaking the interrupt request if any of the checks afterwards fails, except for the final interrupt probe check which does a cleanup. That means on fail before that the interrupt handler stays requested up to the point where the module is removed. If that's a shared interrupt and some other device is active on the same line, then each interrupt from that device will call into the TPM code. Something like the below is needed.

Also the X86 autoprobe mechanism is interesting:

	if (IS_ENABLED(CONFIG_X86))
		for (i = 3; i <= 15; i++)
			if (!tpm_tis_probe_irq_single(chip, intmask, 0, i))
				return;

Quoted 6 lines

The third argument is 'flags' which is handed to request_irq(). So that won't ever be able to probe a shared interrupt. But if an interrupt number > 0 is handed to tpm_tis_core_init() the interrupt is requested with IRQF_SHARED. Same issue when the chip has an interrupt number in the register. It's also requested exclusive which is pretty likely to fail on ancient x86 machines.

It is very likely none of this works any more, it has been repeatedly reworked over the years and just left behind out of fear someone needs it. I've thought it should be deleted for a while now.

I suppose the original logic was to try and probe without SHARED because a probe would need exclusive access to the interrupt to tell if the TPM was actually the source, not some other device.

It is all very old and very out of step with current thinking, IMHO. I skeptical that TPM interrupts were ever valuable enough to deserve this in the first place.

Jason