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=-9.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED 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 DC6D4C4727C for ; Wed, 30 Sep 2020 02:39:28 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 8C6B720773 for ; Wed, 30 Sep 2020 02:39:28 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729470AbgI3Cj2 (ORCPT ); Tue, 29 Sep 2020 22:39:28 -0400 Received: from mga09.intel.com ([134.134.136.24]:7939 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729446AbgI3Cj2 (ORCPT ); Tue, 29 Sep 2020 22:39:28 -0400 IronPort-SDR: MaFcSTwrqyu6nIoJI6f2zQHgbFArYmCKRHcid23Xf/SkGTVd4F3D+uxUk3H2k/VTo2DtbzjCdZ g1lq3zEU2cwQ== X-IronPort-AV: E=McAfee;i="6000,8403,9759"; a="163212993" X-IronPort-AV: E=Sophos;i="5.77,320,1596524400"; d="scan'208";a="163212993" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 29 Sep 2020 19:39:23 -0700 IronPort-SDR: B/S/3r1QPhAXl46ZX2XOKLgHaGDCcZSaLVErrKccjlYLVx7qGLYlaiXYeXNHkBhnfCr63VKGbW J3SY5T4Fvl/A== X-IronPort-AV: E=Sophos;i="5.77,320,1596524400"; d="scan'208";a="494906258" Received: from xinpan-mobl.ger.corp.intel.com (HELO localhost) ([10.249.35.239]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 29 Sep 2020 19:39:22 -0700 Date: Wed, 30 Sep 2020 05:39:19 +0300 From: Jarkko Sakkinen To: James Bottomley Cc: linux-integrity@vger.kernel.org, Jason Gunthorpe , Jerry Snitselaar , Peter Huewe Subject: Re: [PATCH 2/4] tpm_tis: Fix interrupts for TIS TPMs without legacy cycles Message-ID: <20200930023919.GJ808399@linux.intel.com> References: <20200929223216.22584-1-James.Bottomley@HansenPartnership.com> <20200929223216.22584-3-James.Bottomley@HansenPartnership.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200929223216.22584-3-James.Bottomley@HansenPartnership.com> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo Precedence: bulk List-ID: X-Mailing-List: linux-integrity@vger.kernel.org On Tue, Sep 29, 2020 at 03:32:14PM -0700, James Bottomley wrote: > If a TIS TPM doesn't have legacy cycles, any write to the interrupt > registers is ignored unless a locality is active. This means even to > set up the interrupt vectors a locality must first be activated. Fix > this by activating the 0 locality in the interrupt probe setup. > > Since the TPM_EOI signalling also requires an active locality, the > interrupt routine cannot end an interrupt if the locality is released. > This can lead to a situation where the TPM goes command ready after > locality release and since the interrupt cannot be ended it refires > continuously. Fix this by disabling all interrupts except locality > change when a locality is released (this only fires when a locality > becomes active, meaning the TPM_EOI should work). > > Finally, since we now disable all status based interrupts in the > locality release, they must be re-enabled before waiting to check the > condition, so add interrupt enabling to the status wait. > > Signed-off-by: James Bottomley > --- > drivers/char/tpm/tpm_tis_core.c | 125 ++++++++++++++++++++++++++------ > 1 file changed, 101 insertions(+), 24 deletions(-) > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c > index a9fa40714c64..02cc384fdaea 100644 > --- a/drivers/char/tpm/tpm_tis_core.c > +++ b/drivers/char/tpm/tpm_tis_core.c > @@ -29,6 +29,46 @@ > > static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool value); > > +static void enable_interrupt(struct tpm_chip *chip, u8 mask) Even if this has not been followed before I'd prefeer that for new functions or when modifying the signature of function they'd be prefixed with tpm_tis_. It is is just more practical e.g. for grepping stuff. I'm fine with full 'interrupt' would but I also think that just 'int' would be perfectly fine. Not something I'm too opionated about. > +{ > + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > + u32 intmask; > + > + /* Take control of the TPM's interrupt hardware and shut it off */ > + tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask); > + > + intmask |= mask | TPM_GLOBAL_INT_ENABLE; > + > + tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask); > +} > + > +static void disable_interrupt(struct tpm_chip *chip, u8 mask) > +{ > + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > + u32 intmask; > + > + /* Take control of the TPM's interrupt hardware and shut it off */ > + tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask); > + > + intmask &= ~mask; > + > + tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask); > +} > + > +static void enable_stat_interrupt(struct tpm_chip *chip, u8 stat) I'd use plural (interrupts or ints). > +{ > + u32 mask = 0; > + > + if (stat & TPM_STS_COMMAND_READY) > + mask |= TPM_INTF_CMD_READY_INT; > + if (stat & TPM_STS_VALID) > + mask |= TPM_INTF_STS_VALID_INT; > + if (stat & TPM_STS_DATA_AVAIL) > + mask |= TPM_INTF_DATA_AVAIL_INT; > + > + enable_interrupt(chip, mask); > +} > + > static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask, > bool check_cancel, bool *canceled) > { > @@ -65,11 +105,14 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, > timeout = stop - jiffies; > if ((long)timeout <= 0) > return -ETIME; > + enable_stat_interrupt(chip, mask); > rc = wait_event_interruptible_timeout(*queue, > wait_for_tpm_stat_cond(chip, mask, check_cancel, > &canceled), > timeout); > if (rc > 0) { > + if (rc == 1) > + dev_err(&chip->dev, "Lost Interrupt waiting for TPM stat\n"); > if (canceled) > return -ECANCELED; > return 0; > @@ -137,6 +180,28 @@ static bool check_locality(struct tpm_chip *chip, int l) > static int release_locality(struct tpm_chip *chip, int l) > { > struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > + u32 int_status; > + int rc; > + > + /* > + * Note that once we relinquish the locality, all writes to > + * the interrupt registers become ineffective meaning we can't > + * do a TPM_EOI. This means we must disable every interrupt > + * except the locality change one to avoid interrupt > + * storms. > + */ > + disable_interrupt(chip, TPM_INTF_CMD_READY_INT > + | TPM_INTF_STS_VALID_INT > + | TPM_INTF_DATA_AVAIL_INT); > + > + rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &int_status); > + if (rc < 0) > + return rc; > + > + /* Clear all pending */ > + rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), int_status); > + if (rc < 0) > + return rc; > > tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY); > > @@ -163,12 +228,17 @@ static int request_locality(struct tpm_chip *chip, int l) > timeout = stop - jiffies; > if ((long)timeout <= 0) > return -1; > + > rc = wait_event_interruptible_timeout(priv->int_queue, > (check_locality > (chip, l)), > timeout); > - if (rc > 0) > + if (rc > 1) > + return l; > + if (rc == 1) { > + dev_info(&chip->dev, "Lost Interrupt waiting for locality\n"); > return l; > + } > if (rc == -ERESTARTSYS && freezing(current)) { > clear_thread_flag(TIF_SIGPENDING); > goto again; > @@ -464,6 +534,10 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len) > irq = priv->irq; > priv->irq = 0; > chip->flags &= ~TPM_CHIP_FLAG_IRQ; > + enable_interrupt(chip, TPM_INTF_CMD_READY_INT > + | TPM_INTF_LOCALITY_CHANGE_INT > + | TPM_INTF_DATA_AVAIL_INT > + | TPM_INTF_STS_VALID_INT); > rc = tpm_tis_send_main(chip, buf, len); > priv->irq = irq; > chip->flags |= TPM_CHIP_FLAG_IRQ; > @@ -718,7 +792,7 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip) > * irq is seen then leave the chip setup for IRQ operation, otherwise reverse > * everything and leave in polling mode. Returns 0 on success. > */ > -static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask, > +static int tpm_tis_probe_irq_single(struct tpm_chip *chip, > int flags, int irq) > { > struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > @@ -752,9 +826,11 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask, > if (rc < 0) > return rc; > > - /* Turn on */ > - rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), > - intmask | TPM_GLOBAL_INT_ENABLE); > + /* > + * Turn on. The locality change interrupt is the only one > + * always enabled > + */ > + enable_interrupt(chip, TPM_INTF_LOCALITY_CHANGE_INT); > if (rc < 0) > return rc; > > @@ -786,7 +862,7 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask, > * do not have ACPI/etc. We typically expect the interrupt to be declared if > * present. > */ > -static void tpm_tis_probe_irq(struct tpm_chip *chip, u32 intmask) > +static void tpm_tis_probe_irq(struct tpm_chip *chip) > { > struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > u8 original_int_vec; > @@ -800,11 +876,9 @@ static void tpm_tis_probe_irq(struct tpm_chip *chip, u32 intmask) > if (!original_int_vec) { > if (IS_ENABLED(CONFIG_X86)) > for (i = 3; i <= 15; i++) > - if (!tpm_tis_probe_irq_single(chip, intmask, 0, > - i)) > + if (!tpm_tis_probe_irq_single(chip, 0, i)) > return; > - } else if (!tpm_tis_probe_irq_single(chip, intmask, 0, > - original_int_vec)) > + } else if (!tpm_tis_probe_irq_single(chip, 0, original_int_vec)) > return; > } > > @@ -1029,8 +1103,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, > } > > if (irq) { > - tpm_tis_probe_irq_single(chip, intmask, IRQF_SHARED, > - irq); > + tpm_tis_probe_irq_single(chip, IRQF_SHARED, irq); > if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) { > dev_err(&chip->dev, FW_BUG > "TPM interrupt not working, polling instead\n"); > @@ -1038,7 +1111,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, > disable_interrupts(chip); > } > } else { > - tpm_tis_probe_irq(chip, intmask); > + tpm_tis_probe_irq(chip); > } > } > > @@ -1064,12 +1137,23 @@ EXPORT_SYMBOL_GPL(tpm_tis_core_init); > static void tpm_tis_reenable_interrupts(struct tpm_chip *chip) > { > struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > - u32 intmask; > int rc; > > if (chip->ops->clk_enable != NULL) > chip->ops->clk_enable(chip, true); > > + /* > + * must have the locality before we can enable interrupts, so > + * poll for the locality being ready > + */ > + chip->flags &= ~TPM_CHIP_FLAG_IRQ; > + if (request_locality(chip, 0) != 0) { > + dev_err(&chip->dev, "Failed to enable interrupts after suspend\n"); > + goto out; > + } > + chip->flags |= TPM_CHIP_FLAG_IRQ; > + > + > /* reenable interrupts that device may have lost or > * BIOS/firmware may have disabled > */ > @@ -1077,17 +1161,10 @@ static void tpm_tis_reenable_interrupts(struct tpm_chip *chip) > if (rc < 0) > goto out; > > - rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask); > - if (rc < 0) > - goto out; > - > - intmask |= TPM_INTF_CMD_READY_INT > - | TPM_INTF_LOCALITY_CHANGE_INT | TPM_INTF_DATA_AVAIL_INT > - | TPM_INTF_STS_VALID_INT | TPM_GLOBAL_INT_ENABLE; > + enable_interrupt(chip, TPM_INTF_LOCALITY_CHANGE_INT); > > - tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask); > - > -out: > + out: > + release_locality(chip, 0); > if (chip->ops->clk_enable != NULL) > chip->ops->clk_enable(chip, false); > > -- > 2.28.0 > Agree with the change minus the cosmetic stuff that I mentioned. /Jarko