From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2ECDCC56201 for ; Thu, 19 Nov 2020 17:06:19 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id A2DBF21D7F for ; Thu, 19 Nov 2020 17:06:18 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="eAFuNuIq" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728962AbgKSRFi (ORCPT ); Thu, 19 Nov 2020 12:05:38 -0500 Received: from us-smtp-delivery-124.mimecast.com ([216.205.24.124]:35503 "EHLO us-smtp-delivery-124.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727677AbgKSRFh (ORCPT ); Thu, 19 Nov 2020 12:05:37 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1605805535; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=7noBqjP7X7X7F1znylhK1IyBB+iXa4PXKrYGSK1I+Q4=; b=eAFuNuIqfn7+JfsZ26YSpy+TJyYK+mM4deUrVLapxMXI/HEqJXmLs/mO06oJ5oheGbUDm0 GqBRXwCagmJalFSa+SQTqpRjHvQH2vYtPMtQEFtnmcXQ6zkWukEUDeh5uKP9cQwpRDuvSR oeBTbqwea8EpeAvPSLaAdyDbMcp1lTU= Received: from mail-qk1-f200.google.com (mail-qk1-f200.google.com [209.85.222.200]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-249-JW9HQrUIOCifTqBZ-e__nw-1; Thu, 19 Nov 2020 12:05:33 -0500 X-MC-Unique: JW9HQrUIOCifTqBZ-e__nw-1 Received: by mail-qk1-f200.google.com with SMTP id f9so5540697qkg.13 for ; Thu, 19 Nov 2020 09:05:33 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:references:user-agent:from:to:cc:subject :in-reply-to:date:message-id:mime-version; bh=7noBqjP7X7X7F1znylhK1IyBB+iXa4PXKrYGSK1I+Q4=; b=mEkxoZljQNwGiyJBeQfZNzefmhMjwDna1IEcyBf7Ato0Nv5GeMHMrQ0J8APkNp8u82 vjj1lxnhcdMF1Mub2sPf6gIVB+6CSV2j/UGfpte8n9KUH4OLiN2J7XuIquokAq7lcUhh PQzbfqHZUqEd0E2QwXvqou/Lrqwwh6ivDxo20J2bWWzXlnYCYA+1nCLb+9pFRHD7n6z4 4n7VbbXaqMuXeh9Zhvo73LcaUt9LQH7s5XbYp1fSB88ONvssKBd4HvIhVl6mHjwcWITo 74ZumCqXZzCoWLIcD65DooyC3iAPXAEn67MTKPDcb+xBgPdLLMNLow3mZ1W+sLnELmYg yDwg== X-Gm-Message-State: AOAM531EtufLnu0YH4CYIIvKlxb+dNMcdstvlQip1sxsXkBar5TumkxB 1nLh3jMnmEdmwuBaYD9MVZ4MDPwfR4BqGaGS07Y30Y4RS1r/RUVpIMvhl1h1rhG1wud90cfpNT/ UGsJbDJ3j5N82Z2t9w/QtNyfH X-Received: by 2002:ac8:4914:: with SMTP id e20mr11079619qtq.210.1605805533030; Thu, 19 Nov 2020 09:05:33 -0800 (PST) X-Google-Smtp-Source: ABdhPJxAdwWhaOkY9QamylGcjAqWf3ynrNhCJQ9K9PX56a0MzYhN2QPwW4BWl+yyq6EB/3J0DYRhsw== X-Received: by 2002:ac8:4914:: with SMTP id e20mr11079589qtq.210.1605805532728; Thu, 19 Nov 2020 09:05:32 -0800 (PST) Received: from localhost (ip98-179-76-75.ph.ph.cox.net. [98.179.76.75]) by smtp.gmail.com with ESMTPSA id c199sm226227qke.111.2020.11.19.09.05.31 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 19 Nov 2020 09:05:31 -0800 (PST) References: <20201015214430.17937-1-jsnitsel@redhat.com> <87d009c0pn.fsf@redhat.com> <77498b10-cf2c-690b-8dad-78cbd61712ba@redhat.com> User-agent: mu4e 1.4.10; emacs 27.1 From: Jerry Snitselaar To: Hans de Goede Cc: Matthew Garrett , linux-integrity , Linux Kernel Mailing List , jarkko@kernel.org, Peter Huewe , Jason Gunthorpe , James Bottomley Subject: Re: [PATCH] tpm_tis: Disable interrupts on ThinkPad T490s In-reply-to: <77498b10-cf2c-690b-8dad-78cbd61712ba@redhat.com> Date: Thu, 19 Nov 2020 10:05:29 -0700 Message-ID: <87a6vdb7l2.fsf@redhat.com> MIME-Version: 1.0 Content-Type: text/plain Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hans de Goede @ 2020-11-19 07:42 MST: > Hi, > > On 11/19/20 7:36 AM, Jerry Snitselaar wrote: >> >> Matthew Garrett @ 2020-10-15 15:39 MST: >> >>> On Thu, Oct 15, 2020 at 2:44 PM Jerry Snitselaar wrote: >>>> >>>> There is a misconfiguration in the bios of the gpio pin used for the >>>> interrupt in the T490s. When interrupts are enabled in the tpm_tis >>>> driver code this results in an interrupt storm. This was initially >>>> reported when we attempted to enable the interrupt code in the tpm_tis >>>> driver, which previously wasn't setting a flag to enable it. Due to >>>> the reports of the interrupt storm that code was reverted and we went back >>>> to polling instead of using interrupts. Now that we know the T490s problem >>>> is a firmware issue, add code to check if the system is a T490s and >>>> disable interrupts if that is the case. This will allow us to enable >>>> interrupts for everyone else. If the user has a fixed bios they can >>>> force the enabling of interrupts with tpm_tis.interrupts=1 on the >>>> kernel command line. >>> >>> I think an implication of this is that systems haven't been >>> well-tested with interrupts enabled. In general when we've found a >>> firmware issue in one place it ends up happening elsewhere as well, so >>> it wouldn't surprise me if there are other machines that will also be >>> unhappy with interrupts enabled. Would it be possible to automatically >>> detect this case (eg, if we get more than a certain number of >>> interrupts in a certain timeframe immediately after enabling the >>> interrupt) and automatically fall back to polling in that case? It >>> would also mean that users with fixed firmware wouldn't need to pass a >>> parameter. >> >> I believe Matthew is correct here. I found another system today >> with completely different vendor for both the system and the tpm chip. >> In addition another Lenovo model, the L490, has the issue. >> >> This initial attempt at a solution like Matthew suggested works on >> the system I found today, but I imagine it is all sorts of wrong. >> In the 2 systems where I've seen it, there are about 100000 interrupts >> in around 1.5 seconds, and then the irq code shuts down the interrupt >> because they aren't being handled. > > Is that with your patch? The IRQ should be silenced as soon as > devm_free_irq(chip->dev.parent, priv->irq, chip); is called. > No that is just with James' patchset that enables interrupts for tpm_tis. It looks like the irq is firing, but the tpm's int_status register is clear, so the handler immediately returns IRQ_NONE. After that happens 100000 times the core irq code shuts down the irq, but it isn't released so I could still see the stats in /proc/interrupts. With my attempt below on top of that patchset it releases the irq. I had to stick the check prior to it checking the int_status register because it is cleared and the handler returns, and I couldn't do the devm_free_irq directly in tis_int_handler, so I tried sticking it in tpm_tis_send where the other odd irq testing code was already located. I'm not sure if there is another place that would work better for calling the devm_free_irq. > Depending on if we can get your storm-detection to work or not, > we might also choose to just never try to use the IRQ (at least on > x86 systems). AFAIK the TPM is never used for high-throughput stuff > so the polling overhead should not be a big deal (and I'm getting the feeling > that Windows always polls). > I was wondering about Windows as well. In addition to the Lenovo systems which the T490s had Nuvoton tpm, the system I found yesterday was a development system we have from a partner with an Infineon tpm. Dan Williams has seen it internally at Intel as well on some system. > Regards, > > Hans > > > >> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c >> index 49ae09ac604f..478e9d02a3fa 100644 >> --- a/drivers/char/tpm/tpm_tis_core.c >> +++ b/drivers/char/tpm/tpm_tis_core.c >> @@ -27,6 +27,11 @@ >> #include "tpm.h" >> #include "tpm_tis_core.h" >> >> +static unsigned int time_start = 0; >> +static bool storm_check = true; >> +static bool storm_killed = false; >> +static u32 irqs_fired = 0; >> + >> static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool value); >> >> static void tpm_tis_enable_interrupt(struct tpm_chip *chip, u8 mask) >> @@ -464,25 +469,31 @@ static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len) >> return rc; >> } >> >> -static void disable_interrupts(struct tpm_chip *chip) >> +static void __disable_interrupts(struct tpm_chip *chip) >> { >> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); >> u32 intmask; >> int rc; >> >> - if (priv->irq == 0) >> - return; >> - >> rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask); >> if (rc < 0) >> intmask = 0; >> >> intmask &= ~TPM_GLOBAL_INT_ENABLE; >> rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask); >> + chip->flags &= ~TPM_CHIP_FLAG_IRQ; >> +} >> + >> +static void disable_interrupts(struct tpm_chip *chip) >> +{ >> + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); >> >> + if (priv->irq == 0) >> + return; >> + >> + __disable_interrupts(chip); >> devm_free_irq(chip->dev.parent, priv->irq, chip); >> priv->irq = 0; >> - chip->flags &= ~TPM_CHIP_FLAG_IRQ; >> } >> >> /* >> @@ -528,6 +539,12 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len) >> int rc, irq; >> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); >> >> + if (unlikely(storm_killed)) { >> + devm_free_irq(chip->dev.parent, priv->irq, chip); >> + priv->irq = 0; >> + storm_killed = false; >> + } >> + >> if (!(chip->flags & TPM_CHIP_FLAG_IRQ) || priv->irq_tested) >> return tpm_tis_send_main(chip, buf, len); >> >> @@ -748,6 +765,21 @@ static irqreturn_t tis_int_handler(int dummy, void *dev_id) >> u32 interrupt; >> int i, rc; >> >> + if (storm_check) { >> + irqs_fired++; >> + >> + if (!time_start) { >> + time_start = jiffies_to_msecs(jiffies); >> + } else if ((irqs_fired > 1000) && (jiffies_to_msecs(jiffies) - jiffies < 500)) { >> + __disable_interrupts(chip); >> + storm_check = false; >> + storm_killed = true; >> + return IRQ_HANDLED; >> + } else if ((jiffies_to_msecs(jiffies) - time_start > 500) && (irqs_fired < 1000)) { >> + storm_check = false; >> + } >> + } >> + >> rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &interrupt); >> if (rc < 0) >> return IRQ_NONE; >>