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
