Re: [PATCH 1/2] tpm, tpm_tis: Handle interrupt storm

[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