Re: [PATCH] tpm_tis_core: Disable broken IRQ handling code

From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
To: Hans de Goede <hdegoede@redhat.com>
Cc: Peter Huewe <peterhuewe@gmx.de>, Jason Gunthorpe <jgg@ziepe.ca>,
	Jerry Snitselaar <jsnitsel@redhat.com>,
	Stefan Berger <stefanb@linux.ibm.com>,
	Arnd Bergmann <arnd@arndb.de>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-integrity@vger.kernel.org
Subject: Re: [PATCH] tpm_tis_core: Disable broken IRQ handling code
Date: Sun, 12 Apr 2020 17:55:09 +0300	[thread overview]
Message-ID: <20200412145509.GA74087@linux.intel.com> (raw)
In-Reply-To: <20200410210936.GB16905@linux.intel.com>

On Sat, Apr 11, 2020 at 12:09:36AM +0300, Jarkko Sakkinen wrote:
> On Sat, Apr 11, 2020 at 12:07:02AM +0300, Jarkko Sakkinen wrote:
> > On Thu, Apr 09, 2020 at 11:10:44PM +0200, Hans de Goede wrote:
> > > Since commit dda8b2af395b ("tpm: Revert "tpm_tis_core: Set
> > > TPM_CHIP_FLAG_IRQ before probing for interrupts"") we no longer set
> > > the TPM_CHIP_FLAG_IRQ ever.
> > > 
> > > So the whole IRQ probing code is not useful, worse we rely on the
> > > IRQ-test path of tpm_tis_send() to call disable_interrupts() if
> > > interrupts do not work, but that path never gets entered because we
> > > never set the TPM_CHIP_FLAG_IRQ.
> > > 
> > > So the remaining IRQ probe code calls request_irq() and never calls
> > > free_irq() even when the interrupt is not working.
> > > 
> > > On some systems, e.g. the Lenovo X1 8th gen,  the interrupt we try
> > > to use and never free creates an interrupt storm followed by
> > > an "irq XX: nobody cared" oops.
> > > 
> > > Since it is non-functional at the moment anyways, lets just completely
> > > disable the IRQ code in tpm_tis_core for now.
> > > 
> > > Fixes: dda8b2af395b ("tpm: Revert "tpm_tis_core: Set TPM_CHIP_FLAG_IRQ before probing for interrupts"")
> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > ---
> > > Note I'm working with Lenovo to try and get to the bottom of this.
> > > ---
> > 
> > OK if I recall correctly the reason for reverting was that the fixes
> > Stefan was sending were broken and no access to hardware were the
> > issues would be visible. The reason for not doing anything til this
> > day is that we don't have T490 available.
> > 
> > The lack of devm_free_irq() is an by itself, so please instead send
> > a fix that adds that to the code instead of "#if 0" crap.
> 
> If we were to go with your proposal, then you'd have to do "if 0" to
> bunch of other places. Otherwise, the change would be incomplete. Thus,
> it is now more sane just to free that IRQ.

Also, already the commit that has the fixes tag disables IRQ handling
and that was for the reason that some hardware that we (basically me
and Jerry) did not have access to. That's why there is no proper fix.

/Jarkko