Received: by 2002:a05:6a10:22f:0:0:0:0 with SMTP id 15csp2744656pxk; Sun, 27 Sep 2020 21:00:09 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwl74RIadQUR22pNOxdYUGkiSGywOZyTh1fjlXB7JkDKlYG36h25kWne47Z8bZKJ6QHz7tM X-Received: by 2002:a50:e799:: with SMTP id b25mr13369752edn.225.1601265608841; Sun, 27 Sep 2020 21:00:08 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1601265608; cv=none; d=google.com; s=arc-20160816; b=hzM1mHk/oTFmqW9EwQWgoWOdc/ALP780eiTbeaP4CBTe0TC94Wbvpv1TRO2L68VXsg hzr2l42YPeolqXhJrnV5zf6aThNRChUth2TCThyPWHyfbsrZneejslYcseH9pem9Av2i cCYVa5nfFssFLRZA0OPhWpePCh8SAkE53xqUwTOAs7zuHZPvq6evDxrUsTYxlXQsJN1O hKGMjyno0/3CZuV8NvoRWCy+Qgs6IAWKJBoKqBUPfl++Kwn+J17S1KdjAYytfRc46pQH 3XoRgSjghfF25UdRa3PimbBEbVP/wjhTa3TPvCzP+svIYTjT7HRms2sxfoIQMKzO54ec N4/g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:organization:in-reply-to:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :ironport-sdr:ironport-sdr; bh=KjAbok1a6QK50T5/aJUiIvrfxSILuN8UvyT4ar2tW6g=; b=quhdw9iC3M3KQxftRvuwA12WyXIdZjthab5O7KuKyhBjoihgWbGo6yOdFLh5J+yAM3 I0o70e+45gHVJWR3agEObJ6fgQuqO3ZLPdAIQoe96xL5T/wyWEBQgzwSUlpeCW5m+i3y r1DD+ZoIbhzityQR1XC+wIL/3iIli87pYYqY83vt+GxtMxrk0xoU9bbJS3rzIB8rMM7D t9bDRFVB7nwYR45oqLfpL+Njwkq1Cp68MrjVmLeCizfIxfs+52hHZpO+WWbAyC/DMEVL kFGMlNg5CeqZO8DDYuWS1NvXLTmHBRT7acFS3dnwxOViwl4l0VkyiwgrxqOu2pOVGaT1 9dZg== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=intel.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id dp1si6952021ejc.307.2020.09.27.20.59.45; Sun, 27 Sep 2020 21:00:08 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726478AbgI1D6r (ORCPT + 99 others); Sun, 27 Sep 2020 23:58:47 -0400 Received: from mga04.intel.com ([192.55.52.120]:37232 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726380AbgI1D6r (ORCPT ); Sun, 27 Sep 2020 23:58:47 -0400 IronPort-SDR: H0KbsDqAC4HwYYWhF4B8HLoGQ+oTkMBuRrvak6uX5WY/Qv3m8uruD0OOvMLdtg2IQdr+5hwUB/ 6ncvDHrgCV1w== X-IronPort-AV: E=McAfee;i="6000,8403,9757"; a="159301856" X-IronPort-AV: E=Sophos;i="5.77,312,1596524400"; d="scan'208";a="159301856" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 27 Sep 2020 17:15:30 -0700 IronPort-SDR: G3bpv15SFxQpqfe6ZKKSZv8GJxri1qhrVgk4q5TGUyr/hE1AtVcKIlrwKvYKdU/UUfgru/zG9f 1Xvot87pKjGg== X-IronPort-AV: E=Sophos;i="5.77,312,1596524400"; d="scan'208";a="488343287" Received: from memara-mobl.ger.corp.intel.com (HELO localhost) ([10.252.49.157]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 27 Sep 2020 17:15:28 -0700 Date: Mon, 28 Sep 2020 03:15:30 +0300 From: Jarkko Sakkinen To: James Bottomley Cc: Stefan Berger , linux-integrity@vger.kernel.org, linux-kernel , Jerry Snitselaar Subject: Re: [PATCH 2/2] tpm: Revert "tpm_tis_core: Turn on the TPM before probing IRQ's" Message-ID: <20200928001530.GF5283@linux.intel.com> References: <20191126131753.3424363-1-stefanb@linux.vnet.ibm.com> <20191126131753.3424363-3-stefanb@linux.vnet.ibm.com> <1de642865a142dfbf9d7ef0da398c98d52228943.camel@HansenPartnership.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1de642865a142dfbf9d7ef0da398c98d52228943.camel@HansenPartnership.com> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Sep 27, 2020 at 01:06:03PM -0700, James Bottomley wrote: > On Tue, 2019-11-26 at 08:17 -0500, Stefan Berger wrote: > > From: Stefan Berger > > > > Revert the patch that was turning the TPM on before probing for IRQs. > > > > Fixes: 5b359c7c4372 ("tpm_tis_core: Turn on the TPM before probing > > IRQ's") > > Signed-off-by: Stefan Berger > > Reported-by: Jerry Snitselaar > > Cc: stable@vger.kernel.org > > --- > > drivers/char/tpm/tpm_tis_core.c | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/drivers/char/tpm/tpm_tis_core.c > > b/drivers/char/tpm/tpm_tis_core.c > > index 5dc52c4e2292..27c6ca031e23 100644 > > --- a/drivers/char/tpm/tpm_tis_core.c > > +++ b/drivers/char/tpm/tpm_tis_core.c > > @@ -1059,7 +1059,6 @@ int tpm_tis_core_init(struct device *dev, > > struct tpm_tis_data *priv, int irq, > > goto out_err; > > } > > > > - tpm_chip_start(chip); > > if (irq) { > > tpm_tis_probe_irq_single(chip, intmask, > > IRQF_SHARED, > > irq); > > @@ -1069,7 +1068,6 @@ int tpm_tis_core_init(struct device *dev, > > struct tpm_tis_data *priv, int irq, > > } else { > > tpm_tis_probe_irq(chip, intmask); > > } > > - tpm_chip_stop(chip); > > } > > > > rc = tpm_chip_register(chip); > > This patch is completely bogus: it's not a full revert of what it > claims to be. With this patch applied all my TIS TPMs are returning > 0xff to the status reads because the locality hasn't been properly > requested. The chip has to be started somewhere for the interrupt > probe to work on these TPMs ... what the original patch did was > eliminate a bunch of start/stops for a global one. However, if the > global one isn't working we should have gone back to the bunch of > smaller ones i.e. a full revert. > > The only real manifestation of the problems this patch causes is that > interrupts never get enabled on TIS TPMs that have this issue, but they > still work via polling. > > The below is what fixes this for me with the minimum possible extend of > additional chip start/stop in the code. This should be checked against > the previous failing laptops. > > James > > --- > > From: James Bottomley > Subject: [PATCH] tpm_tis: fix interrupt probing > > When we send a command into the TPM core, the TPM must be started > otherwise the register reads can be bogus. There have been several > bug reports about doing this inside the TIS core, so fix the issue by > adding an external version of the tpm2_get_tpm_pt() call which adds a > tpm ops get/put to set up the TPM correctly before the command is > sent. > > Fixes: aa4a63dd9816 (tpm: Revert "tpm_tis_core: Turn on the TPM before probing IRQ's") > Signed-off-by: James Bottomley > --- > drivers/char/tpm/tpm.h | 2 ++ > drivers/char/tpm/tpm2-cmd.c | 30 ++++++++++++++++++++++++++++++ > drivers/char/tpm/tpm_tis_core.c | 2 +- > 3 files changed, 33 insertions(+), 1 deletion(-) > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h > index 947d1db0a5cc..041b0b5bd2a5 100644 > --- a/drivers/char/tpm/tpm.h > +++ b/drivers/char/tpm/tpm.h > @@ -223,6 +223,8 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, > int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max); > ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id, > u32 *value, const char *desc); > +ssize_t tpm2_get_tpm_pt_cmd(struct tpm_chip *chip, u32 property_id, > + u32 *value, const char *desc); > > ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip); > int tpm2_auto_startup(struct tpm_chip *chip); > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c > index eff1f12d981a..9b84158c5a9e 100644 > --- a/drivers/char/tpm/tpm2-cmd.c > +++ b/drivers/char/tpm/tpm2-cmd.c > @@ -407,6 +407,36 @@ ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id, u32 *value, > } > EXPORT_SYMBOL_GPL(tpm2_get_tpm_pt); > > +/** > + * tpm2_get_tpm_pt_cmd() - get value of a TPM_CAP_TPM_PROPERTIES type property > + * @chip: a &tpm_chip instance > + * @property_id: property ID. > + * @value: output variable. > + * @desc: passed to tpm_transmit_cmd() > + * > + * This calls the necessary tpm_try_get_ops()/tpm_put_ops() around > + * tpm2_get_tpm_pt() and must be called where it is used stand alone > + * outside the core code. > + * > + * Return: > + * 0 on success, > + * -errno or a TPM return code otherwise > + */ > +ssize_t tpm2_get_tpm_pt_cmd(struct tpm_chip *chip, u32 property_id, u32 *value, > + const char *desc) > +{ > + ssize_t rc; > + > + rc = tpm_try_get_ops(chip); > + if (rc) > + return rc; > + rc = tpm2_get_tpm_pt(chip, property_id, value, desc); > + tpm_put_ops(chip); > + > + return rc; > +} > +EXPORT_SYMBOL_GPL(tpm2_get_tpm_pt_cmd); Hi, same comment as for the other, i.e. rename the function that does not have get/put_ops as __tpm2_get_tpm_pt(). Otherwise, fine. Thank you. /Jarkko