Re: [PATCH v3 1/4] irq: export kstat_irqs
On Sun, Dec 06 2020 at 09:40, James Bottomley wrote: > On Sun, 2020-12-06 at 17:40 +0100, Thomas Gleixner wrote: >> On Sat, Dec 05 2020 at 12:39, Jarkko Sakkinen wrote: >> > On Fri, Dec 04, 2020 at 06:43:37PM -0700, Jerry Snitselaar wrote: >> > > 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). >> >> 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? If we need to export it, yes. But I still have to understand the value. See my other reply. Thanks, tglx ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 1/4] irq: export kstat_irqs
On Sat, Dec 05 2020 at 12:39, Jarkko Sakkinen wrote: > On Fri, Dec 04, 2020 at 06:43:37PM -0700, Jerry Snitselaar wrote: >> 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). It's abusage and just the fact that it exists is not an argument by itself. Thanks, tglx ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 1/4] irq: export kstat_irqs
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. > 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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 1/4] irq: export kstat_irqs
Thomas Gleixner @ 2020-12-06 10:54 MST: > 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. > >> 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 I can rework it to use a device local counter. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 1/4] irq: export kstat_irqs
On Sun, 2020-12-06 at 17:40 +0100, Thomas Gleixner wrote: > On Sat, Dec 05 2020 at 12:39, Jarkko Sakkinen wrote: > > On Fri, Dec 04, 2020 at 06:43:37PM -0700, Jerry Snitselaar wrote: > > > 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). > > 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? James ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 1/4] irq: export kstat_irqs
On Fri, Dec 04, 2020 at 06:43:37PM -0700, Jerry Snitselaar wrote: > 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). > > Reported-by: kernel test robot I'm not sure if this makes much sense. > Cc: Thomas Gleixner > Cc: Jarkko Sakkinen > Cc: Jason Gunthorpe > Cc: Peter Huewe > Cc: James Bottomley > Cc: Matthew Garrett > Cc: Hans de Goede > Signed-off-by: Jerry Snitselaar /Jarkko ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
