Messages in this thread | | | Date | Wed, 24 May 2023 04:46:57 +0300 | Subject | Re: [PATCH 1/2] tpm, tpm_tis: Handle interrupt storm | From | "Jarkko Sakkinen" <> |
| |
On Tue May 23, 2023 at 10:37 PM EEST, Lukas Wunner wrote: > > > +unhandled: > > > + tpm_tis_process_unhandled_interrupt(chip); > > > + return IRQ_HANDLED; > > > > Shouldn't the return value be IRQ_NONE? > > No, absolutely not. If you return IRQ_NONE here then genirq code > will increase the spurious interrupt counter. That's bad because > the IRQ storm detection tpm_tis_core.c would race with the IRQ storm > detection in genirq code: > > Note that disablement of the interrupt must happen in a work_struct > here to avoid a deadlock. (The deadlock would occur because > devm_free_irq() waits for the interrupt handler to finish.) > > Now, let's say the 1000 unhandled interrupts limit has been reached > and the work_struct is scheduled. If the work_struct isn't run > quickly enough, you may reach the 99900 limit in note_interrupt() > (see kernel/irq/spurious.c) and then genirq code will force the > interrupt off completely. > > To avoid that you *have* to return IRQ_HANDLED here and thus pretend > towards genirq code that the interrupt was not spurious.
This would deserve an inline comment.
> > > struct tpm_tis_data { > > > + struct tpm_chip *chip; > > > u16 manufacturer_id; > > > struct mutex locality_count_mutex; > > > unsigned int locality_count; > > > int locality; > > > + /* Interrupts */ > > > > Not relevant change for a bug fix. > > But not harmful either, is it?
No but it is still spurious change in this context.
> Thanks, > > Lukas
BR, Jarkko
| |