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

[Jarkko Sakkinen]

On Tue May 30, 2023 at 8:56 PM EEST, Jerry Snitselaar wrote:
> On Mon, May 29, 2023 at 09:46:08AM +0300, Péter Ujfalusi wrote:
> > Hi Lino,
> > 
> > On 23/05/2023 23:46, Lino Sanfilippo wrote:
> > >> On the other hand any new functionality is objectively a maintanance
> > >> burden of some measure (applies to any functionality). So how do we know
> > >> that taking this change is less of a maintenance burden than just add
> > >> new table entries, as they come up?
> > >>
> > > 
> > > Initially this set was created as a response to this 0-day bug report which you asked me
> > > to have a look at:
> > > 
> > > https://lore.kernel.org/linux-integrity/d80b180a569a9f068d3a2614f062cfa3a78af5a6.camel@xxxxxxxxxx/
> > > 
> > > My hope was that it could also avoid some of (existing or future) DMI entries. But even if it does not
> > > (e.g. the problem Péter Ujfalusi reported with the UPX-i11 cannot be fixed by this patch set and thus
> > > needs the DMI quirk) we may at least avoid more bug reports due to interrupt storms once
> > > 6.4 is released. 
> > 
> > I'm surprised that there is a need for a storm detection in the first
> > place... Do we have something else on the same IRQ line on the affected
> > devices which might have a bug or no driver at all?
> > It is hard to believe that a TPM (Trusted Platform Module) is integrated
> > so poorly ;)
> > 
> > But put that aside: I think the storm detection is good given that there
> > is no other way to know which machine have sloppy TPM integration.
> > There are machines where this happens, so it is a know integration
> > issue, right?
> > 
> > My only 'nitpick' is with the printk level to be used.
> > The ERR level is not correct as we know the issue and we handle it, so
> > all is under control.
> > If we want to add these machines to the quirk list then WARN is a good
> > level to gain attention but I'm not sure if a user will know how to get
> > the machine in the quirk (where to file a bug).
> > If we only want the quirk to be used for machines like UPX-i11 which
> > simply just have broken (likely floating) IRQ line then the WARN is too
> > high level, INFO or even DBG would be appropriate as you are not going
> > to update the quirk, it is just handled under the hood (which is a great
> > thing, but on the other hand you will have the storm never the less and
> > that is not a nice thing).
> > 
> > It is a matter on how this is going to be handled in a long term. Add
> > quirk for all the known machines with either stormy or plain broken IRQ
> > line or handle the stormy ones and quirk the broken ones only.
> > 
> > >>> Detect an interrupt storm by counting the number of unhandled interrupts
> > >>> within a 10 ms time interval. In case that more than 1000 were unhandled
> > >>> deactivate interrupts, deregister the handler and fall back to polling.
> > >>
> > >> I know it can be sometimes hard to evaluate but can you try to explain
> > >> how you came up to the 10 ms sampling period and 1000 interrupt
> > >> threshold? I just don't like abritrary numbers.
> > > 
> > > At least the 100 ms is not plucked out of thin air but its the same time period
> > > that the generic code in note_interrupt() uses - I assume for a good reason.
> > > Not only this number but the whole irq storm detection logic is taken from 
> > > there: 
> > > 
> > >>
> > >>> This equals the implementation that handles interrupt storms in
> > >>> note_interrupt() by means of timestamps and counters in struct irq_desc.
> > >> The number of 1000 unhandled interrupts is still far below the 99900
> > used in
> > > note_interrupt() but IMHO enough to indicate that there is something seriously
> > > wrong with interrupt processing and it is probably saver to fall back to polling.
> > 
> > Except that if the line got the spurious designation in core, the
> > interrupt line will be disabled while the TPM driver will think that it
> > is still using IRQ mode and will not switch to polling.
> > 
> > A storm of 1000 is better than a storm of 99900 for sure but quirking
> > these would be the desired final solution. imho
>
> If that is the case, then output could probably be sent to the console
> detailing the dmi info needed to update the table.

+1

Good idea.

BR, Jarkko