| Message ID | 1448026354-6807-2-git-send-email-martin.wilck@ts.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Nov 20, 2015 at 02:32:30PM +0100, martin.wilck@ts.fujitsu.com wrote: > From: Martin Wilck <Martin.Wilck@ts.fujitsu.com> > > After initialization of the tpm_chip data structure, > chip->vendor.duration[i] is zero == TPM_SHORT. Thus there is > a risk that the probing code doesn't wait long enough for > completion. This patch moves the initialization of the expected > command durations before the probing code, eliminating that > problem. > > v2: split out this patch and the next one ("tpm_tis: add a short > wait in IRQ probing routine"), edit commit msg. > > Signed-off-by: Martin Wilck <Martin.Wilck@ts.fujitsu.com> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> /Jarkko > --- > drivers/char/tpm/tpm_tis.c | 36 +++++++++++++++++++----------------- > 1 file changed, 19 insertions(+), 17 deletions(-) > > diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c > index 65f7eec..f7f9039 100644 > --- a/drivers/char/tpm/tpm_tis.c > +++ b/drivers/char/tpm/tpm_tis.c > @@ -731,6 +731,25 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info, > if (intfcaps & TPM_INTF_DATA_AVAIL_INT) > dev_dbg(dev, "\tData Avail Int Support\n"); > > + if (chip->flags & TPM_CHIP_FLAG_TPM2) { > + chip->vendor.timeout_a = msecs_to_jiffies(TPM2_TIMEOUT_A); > + chip->vendor.timeout_b = msecs_to_jiffies(TPM2_TIMEOUT_B); > + chip->vendor.timeout_c = msecs_to_jiffies(TPM2_TIMEOUT_C); > + chip->vendor.timeout_d = msecs_to_jiffies(TPM2_TIMEOUT_D); > + chip->vendor.duration[TPM_SHORT] = > + msecs_to_jiffies(TPM2_DURATION_SHORT); > + chip->vendor.duration[TPM_MEDIUM] = > + msecs_to_jiffies(TPM2_DURATION_MEDIUM); > + chip->vendor.duration[TPM_LONG] = > + msecs_to_jiffies(TPM2_DURATION_LONG); > + } else { > + if (tpm_get_timeouts(chip)) { > + dev_err(dev, "Could not get TPM timeouts and durations\n"); > + rc = -ENODEV; > + goto out_err; > + } > + } > + > /* INTERRUPT Setup */ > init_waitqueue_head(&chip->vendor.read_queue); > init_waitqueue_head(&chip->vendor.int_queue); > @@ -840,17 +859,6 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info, > TPM_INT_VECTOR(chip->vendor.locality)); > > if (chip->flags & TPM_CHIP_FLAG_TPM2) { > - chip->vendor.timeout_a = msecs_to_jiffies(TPM2_TIMEOUT_A); > - chip->vendor.timeout_b = msecs_to_jiffies(TPM2_TIMEOUT_B); > - chip->vendor.timeout_c = msecs_to_jiffies(TPM2_TIMEOUT_C); > - chip->vendor.timeout_d = msecs_to_jiffies(TPM2_TIMEOUT_D); > - chip->vendor.duration[TPM_SHORT] = > - msecs_to_jiffies(TPM2_DURATION_SHORT); > - chip->vendor.duration[TPM_MEDIUM] = > - msecs_to_jiffies(TPM2_DURATION_MEDIUM); > - chip->vendor.duration[TPM_LONG] = > - msecs_to_jiffies(TPM2_DURATION_LONG); > - > rc = tpm2_do_selftest(chip); > if (rc == TPM2_RC_INITIALIZE) { > dev_warn(dev, "Firmware has not started TPM\n"); > @@ -866,12 +874,6 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info, > goto out_err; > } > } else { > - if (tpm_get_timeouts(chip)) { > - dev_err(dev, "Could not get TPM timeouts and durations\n"); > - rc = -ENODEV; > - goto out_err; > - } > - > if (tpm_do_selftest(chip)) { > dev_err(dev, "TPM self test failed\n"); > rc = -ENODEV; > -- > 1.8.3.1 > > > ------------------------------------------------------------------------------ > _______________________________________________ > tpmdd-devel mailing list > tpmdd-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/tpmdd-devel ------------------------------------------------------------------------------
On Sat, Nov 21, 2015 at 03:05:44PM +0200, Jarkko Sakkinen wrote: > On Fri, Nov 20, 2015 at 02:32:30PM +0100, martin.wilck@ts.fujitsu.com wrote: > > From: Martin Wilck <Martin.Wilck@ts.fujitsu.com> > > > > After initialization of the tpm_chip data structure, > > chip->vendor.duration[i] is zero == TPM_SHORT. Thus there is > > a risk that the probing code doesn't wait long enough for > > completion. This patch moves the initialization of the expected > > command durations before the probing code, eliminating that > > problem. > > > > v2: split out this patch and the next one ("tpm_tis: add a short > > wait in IRQ probing routine"), edit commit msg. > > > > Signed-off-by: Martin Wilck <Martin.Wilck@ts.fujitsu.com> > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> Applied to https://github.com/jsakkine/linux-tpmdd/commits/master /Jarkko ------------------------------------------------------------------------------
On Fri, Nov 20, 2015 at 02:32:30PM +0100, martin.wilck@ts.fujitsu.com wrote: > From: Martin Wilck <Martin.Wilck@ts.fujitsu.com> > > After initialization of the tpm_chip data structure, > chip->vendor.duration[i] is zero == TPM_SHORT. Thus there is > a risk that the probing code doesn't wait long enough for > completion. This patch moves the initialization of the expected > command durations before the probing code, eliminating that > problem. This is a regression, it needs a Fixes line in the commit message. Looks like the irq probing patch is at fault? Jason ------------------------------------------------------------------------------ Go from Idea to Many App Stores Faster with Intel(R) XDK Give your users amazing mobile app experiences with Intel(R) XDK. Use one codebase in this all-in-one HTML5 development environment. Design, debug & build mobile apps & 2D/3D high-impact games for multiple OSs. http://pubads.g.doubleclick.net/gampad/clk?id=254741551&iu=/4140
On Mon, Nov 23, 2015 at 04:19:31PM -0700, Jason Gunthorpe wrote: > On Fri, Nov 20, 2015 at 02:32:30PM +0100, martin.wilck@ts.fujitsu.com wrote: > > From: Martin Wilck <Martin.Wilck@ts.fujitsu.com> > > > > After initialization of the tpm_chip data structure, > > chip->vendor.duration[i] is zero == TPM_SHORT. Thus there is > > a risk that the probing code doesn't wait long enough for > > completion. This patch moves the initialization of the expected > > command durations before the probing code, eliminating that > > problem. > > This is a regression, it needs a Fixes line in the commit > message. Looks like the irq probing patch is at fault? I'll add the Fixes-line include it to pull request for 4.5. How you want to be tagged? > Jason /Jarkko ------------------------------------------------------------------------------ Go from Idea to Many App Stores Faster with Intel(R) XDK Give your users amazing mobile app experiences with Intel(R) XDK. Use one codebase in this all-in-one HTML5 development environment. Design, debug & build mobile apps & 2D/3D high-impact games for multiple OSs. http://pubads.g.doubleclick.net/gampad/clk?id=254741551&iu=/4140
On Tue, Nov 24, 2015 at 07:42:36AM +0200, Jarkko Sakkinen wrote: > On Mon, Nov 23, 2015 at 04:19:31PM -0700, Jason Gunthorpe wrote: > > On Fri, Nov 20, 2015 at 02:32:30PM +0100, martin.wilck@ts.fujitsu.com wrote: > > > From: Martin Wilck <Martin.Wilck@ts.fujitsu.com> > > > > > > After initialization of the tpm_chip data structure, > > > chip->vendor.duration[i] is zero == TPM_SHORT. Thus there is > > > a risk that the probing code doesn't wait long enough for > > > completion. This patch moves the initialization of the expected > > > command durations before the probing code, eliminating that > > > problem. > > > > This is a regression, it needs a Fixes line in the commit > > message. Looks like the irq probing patch is at fault? > > I'll add the Fixes-line include it to pull request for 4.5. How you > want to be tagged? Looks like this one: Fixes: 448e9c55c12d6 ("tpm_tis: verify interrupt during init") The ordering was originally corrected in a7b66822b2, but 448 moved tpm_get_timeouts to after tpm_gen_interrupts. Jason ------------------------------------------------------------------------------ Go from Idea to Many App Stores Faster with Intel(R) XDK Give your users amazing mobile app experiences with Intel(R) XDK. Use one codebase in this all-in-one HTML5 development environment. Design, debug & build mobile apps & 2D/3D high-impact games for multiple OSs. http://pubads.g.doubleclick.net/gampad/clk?id=254741551&iu=/4140
On Tue, Nov 24, 2015 at 11:31:42AM -0700, Jason Gunthorpe wrote: > On Tue, Nov 24, 2015 at 07:42:36AM +0200, Jarkko Sakkinen wrote: > > On Mon, Nov 23, 2015 at 04:19:31PM -0700, Jason Gunthorpe wrote: > > > On Fri, Nov 20, 2015 at 02:32:30PM +0100, martin.wilck@ts.fujitsu.com wrote: > > > > From: Martin Wilck <Martin.Wilck@ts.fujitsu.com> > > > > > > > > After initialization of the tpm_chip data structure, > > > > chip->vendor.duration[i] is zero == TPM_SHORT. Thus there is > > > > a risk that the probing code doesn't wait long enough for > > > > completion. This patch moves the initialization of the expected > > > > command durations before the probing code, eliminating that > > > > problem. > > > > > > This is a regression, it needs a Fixes line in the commit > > > message. Looks like the irq probing patch is at fault? > > > > I'll add the Fixes-line include it to pull request for 4.5. How you > > want to be tagged? > > Looks like this one: > > Fixes: 448e9c55c12d6 ("tpm_tis: verify interrupt during init") > > The ordering was originally corrected in a7b66822b2, but 448 moved > tpm_get_timeouts to after tpm_gen_interrupts. Thanks! I actually meant by that question whether you want Reviewed-by or Acked-by but I guess Reviewed-by is a better choice here :) > Jason /Jarkko ------------------------------------------------------------------------------ Go from Idea to Many App Stores Faster with Intel(R) XDK Give your users amazing mobile app experiences with Intel(R) XDK. Use one codebase in this all-in-one HTML5 development environment. Design, debug & build mobile apps & 2D/3D high-impact games for multiple OSs. http://pubads.g.doubleclick.net/gampad/clk?id=254741551&iu=/4140
On Tue, Nov 24, 2015 at 09:50:38PM +0200, Jarkko Sakkinen wrote: > On Tue, Nov 24, 2015 at 11:31:42AM -0700, Jason Gunthorpe wrote: > > On Tue, Nov 24, 2015 at 07:42:36AM +0200, Jarkko Sakkinen wrote: > > > On Mon, Nov 23, 2015 at 04:19:31PM -0700, Jason Gunthorpe wrote: > > > > On Fri, Nov 20, 2015 at 02:32:30PM +0100, martin.wilck@ts.fujitsu.com wrote: > > > > > From: Martin Wilck <Martin.Wilck@ts.fujitsu.com> > > > > > > > > > > After initialization of the tpm_chip data structure, > > > > > chip->vendor.duration[i] is zero == TPM_SHORT. Thus there is > > > > > a risk that the probing code doesn't wait long enough for > > > > > completion. This patch moves the initialization of the expected > > > > > command durations before the probing code, eliminating that > > > > > problem. > > > > > > > > This is a regression, it needs a Fixes line in the commit > > > > message. Looks like the irq probing patch is at fault? > > > > > > I'll add the Fixes-line include it to pull request for 4.5. How you > > > want to be tagged? > > > > Looks like this one: > > > > Fixes: 448e9c55c12d6 ("tpm_tis: verify interrupt during init") > > > > The ordering was originally corrected in a7b66822b2, but 448 moved > > tpm_get_timeouts to after tpm_gen_interrupts. > > Thanks! I actually meant by that question whether you want Reviewed-by > or Acked-by but I guess Reviewed-by is a better choice here :) Sure Reviewed-by Do we understand why 448e9c55c12d6 moved them in the first place? Better check with Scot, I vaguely recall there was a reason for this... Jason ------------------------------------------------------------------------------ Go from Idea to Many App Stores Faster with Intel(R) XDK Give your users amazing mobile app experiences with Intel(R) XDK. Use one codebase in this all-in-one HTML5 development environment. Design, debug & build mobile apps & 2D/3D high-impact games for multiple OSs. http://pubads.g.doubleclick.net/gampad/clk?id=254741551&iu=/4140
On Tue, Nov 24, 2015 at 12:58:03PM -0700, Jason Gunthorpe wrote: > On Tue, Nov 24, 2015 at 09:50:38PM +0200, Jarkko Sakkinen wrote: > > On Tue, Nov 24, 2015 at 11:31:42AM -0700, Jason Gunthorpe wrote: > > > On Tue, Nov 24, 2015 at 07:42:36AM +0200, Jarkko Sakkinen wrote: > > > > On Mon, Nov 23, 2015 at 04:19:31PM -0700, Jason Gunthorpe wrote: > > > > > On Fri, Nov 20, 2015 at 02:32:30PM +0100, martin.wilck@ts.fujitsu.com wrote: > > > > > > From: Martin Wilck <Martin.Wilck@ts.fujitsu.com> > > > > > > > > > > > > After initialization of the tpm_chip data structure, > > > > > > chip->vendor.duration[i] is zero == TPM_SHORT. Thus there is > > > > > > a risk that the probing code doesn't wait long enough for > > > > > > completion. This patch moves the initialization of the expected > > > > > > command durations before the probing code, eliminating that > > > > > > problem. > > > > > > > > > > This is a regression, it needs a Fixes line in the commit > > > > > message. Looks like the irq probing patch is at fault? > > > > > > > > I'll add the Fixes-line include it to pull request for 4.5. How you > > > > want to be tagged? > > > > > > Looks like this one: > > > > > > Fixes: 448e9c55c12d6 ("tpm_tis: verify interrupt during init") > > > > > > The ordering was originally corrected in a7b66822b2, but 448 moved > > > tpm_get_timeouts to after tpm_gen_interrupts. > > > > Thanks! I actually meant by that question whether you want Reviewed-by > > or Acked-by but I guess Reviewed-by is a better choice here :) > > Sure Reviewed-by > > Do we understand why 448e9c55c12d6 moved them in the first place? > Better check with Scot, I vaguely recall there was a reason for > this... Good catch. The commit message does not explain any reason for doing this. CC'ing to Scot. > Jason /Jarkko ------------------------------------------------------------------------------ Go from Idea to Many App Stores Faster with Intel(R) XDK Give your users amazing mobile app experiences with Intel(R) XDK. Use one codebase in this all-in-one HTML5 development environment. Design, debug & build mobile apps & 2D/3D high-impact games for multiple OSs. http://pubads.g.doubleclick.net/gampad/clk?id=254741551&iu=/4140
On Wed, 25 Nov 2015, Jarkko Sakkinen wrote: > On Tue, Nov 24, 2015 at 12:58:03PM -0700, Jason Gunthorpe wrote: > > On Tue, Nov 24, 2015 at 09:50:38PM +0200, Jarkko Sakkinen wrote: > > > On Tue, Nov 24, 2015 at 11:31:42AM -0700, Jason Gunthorpe wrote: > > > > On Tue, Nov 24, 2015 at 07:42:36AM +0200, Jarkko Sakkinen wrote: > > > > > On Mon, Nov 23, 2015 at 04:19:31PM -0700, Jason Gunthorpe wrote: > > > > > > On Fri, Nov 20, 2015 at 02:32:30PM +0100, martin.wilck@ts.fujitsu.com wrote: > > > > > > > From: Martin Wilck <Martin.Wilck@ts.fujitsu.com> > > > > > > > > > > > > > > After initialization of the tpm_chip data structure, > > > > > > > chip->vendor.duration[i] is zero == TPM_SHORT. Thus there is > > > > > > > a risk that the probing code doesn't wait long enough for > > > > > > > completion. This patch moves the initialization of the expected > > > > > > > command durations before the probing code, eliminating that > > > > > > > problem. > > > > > > > > > > > > This is a regression, it needs a Fixes line in the commit > > > > > > message. Looks like the irq probing patch is at fault? > > > > > > > > > > I'll add the Fixes-line include it to pull request for 4.5. How you > > > > > want to be tagged? > > > > > > > > Looks like this one: > > > > > > > > Fixes: 448e9c55c12d6 ("tpm_tis: verify interrupt during init") > > > > > > > > The ordering was originally corrected in a7b66822b2, but 448 moved > > > > tpm_get_timeouts to after tpm_gen_interrupts. > > > > > > Thanks! I actually meant by that question whether you want Reviewed-by > > > or Acked-by but I guess Reviewed-by is a better choice here :) > > > > Sure Reviewed-by > > > > Do we understand why 448e9c55c12d6 moved them in the first place? > > Better check with Scot, I vaguely recall there was a reason for > > this... > > Good catch. The commit message does not explain any reason for doing > this. CC'ing to Scot. tpm_tis from Jarkko's master branch (29bf5ad) still correctly drops back to polling mode and suspends/resumes on a Toshiba CB35-A3120 Chromebook: [ 0.236097] tpm_tis 00:08: 1.2 TPM (device-id 0xB, rev-id 16) [ 0.412828] tpm_tis 00:08: [Firmware Bug]: TPM interrupt not working, polling instead Tested-by: Scot Doyle <lkml14@scotdoyle.com> ------------------------------------------------------------------------------ Go from Idea to Many App Stores Faster with Intel(R) XDK Give your users amazing mobile app experiences with Intel(R) XDK. Use one codebase in this all-in-one HTML5 development environment. Design, debug & build mobile apps & 2D/3D high-impact games for multiple OSs. http://pubads.g.doubleclick.net/gampad/clk?id=254741911&iu=/4140
On Wed, Dec 02, 2015 at 04:39:33PM +0000, Scot Doyle wrote: > > Good catch. The commit message does not explain any reason for doing > > this. CC'ing to Scot. > > tpm_tis from Jarkko's master branch (29bf5ad) still correctly drops back > to polling mode and suspends/resumes on a Toshiba CB35-A3120 Chromebook: > > [ 0.236097] tpm_tis 00:08: 1.2 TPM (device-id 0xB, rev-id 16) > [ 0.412828] tpm_tis 00:08: [Firmware Bug]: TPM interrupt not working, polling instead > > Tested-by: Scot Doyle <lkml14@scotdoyle.com> Thanks Scot! /Jarkko ------------------------------------------------------------------------------ Go from Idea to Many App Stores Faster with Intel(R) XDK Give your users amazing mobile app experiences with Intel(R) XDK. Use one codebase in this all-in-one HTML5 development environment. Design, debug & build mobile apps & 2D/3D high-impact games for multiple OSs. http://pubads.g.doubleclick.net/gampad/clk?id=254741911&iu=/4140
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c index 65f7eec..f7f9039 100644 --- a/drivers/char/tpm/tpm_tis.c +++ b/drivers/char/tpm/tpm_tis.c @@ -731,6 +731,25 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info, if (intfcaps & TPM_INTF_DATA_AVAIL_INT) dev_dbg(dev, "\tData Avail Int Support\n"); + if (chip->flags & TPM_CHIP_FLAG_TPM2) { + chip->vendor.timeout_a = msecs_to_jiffies(TPM2_TIMEOUT_A); + chip->vendor.timeout_b = msecs_to_jiffies(TPM2_TIMEOUT_B); + chip->vendor.timeout_c = msecs_to_jiffies(TPM2_TIMEOUT_C); + chip->vendor.timeout_d = msecs_to_jiffies(TPM2_TIMEOUT_D); + chip->vendor.duration[TPM_SHORT] = + msecs_to_jiffies(TPM2_DURATION_SHORT); + chip->vendor.duration[TPM_MEDIUM] = + msecs_to_jiffies(TPM2_DURATION_MEDIUM); + chip->vendor.duration[TPM_LONG] = + msecs_to_jiffies(TPM2_DURATION_LONG); + } else { + if (tpm_get_timeouts(chip)) { + dev_err(dev, "Could not get TPM timeouts and durations\n"); + rc = -ENODEV; + goto out_err; + } + } + /* INTERRUPT Setup */ init_waitqueue_head(&chip->vendor.read_queue); init_waitqueue_head(&chip->vendor.int_queue); @@ -840,17 +859,6 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info, TPM_INT_VECTOR(chip->vendor.locality)); if (chip->flags & TPM_CHIP_FLAG_TPM2) { - chip->vendor.timeout_a = msecs_to_jiffies(TPM2_TIMEOUT_A); - chip->vendor.timeout_b = msecs_to_jiffies(TPM2_TIMEOUT_B); - chip->vendor.timeout_c = msecs_to_jiffies(TPM2_TIMEOUT_C); - chip->vendor.timeout_d = msecs_to_jiffies(TPM2_TIMEOUT_D); - chip->vendor.duration[TPM_SHORT] = - msecs_to_jiffies(TPM2_DURATION_SHORT); - chip->vendor.duration[TPM_MEDIUM] = - msecs_to_jiffies(TPM2_DURATION_MEDIUM); - chip->vendor.duration[TPM_LONG] = - msecs_to_jiffies(TPM2_DURATION_LONG); - rc = tpm2_do_selftest(chip); if (rc == TPM2_RC_INITIALIZE) { dev_warn(dev, "Firmware has not started TPM\n"); @@ -866,12 +874,6 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info, goto out_err; } } else { - if (tpm_get_timeouts(chip)) { - dev_err(dev, "Could not get TPM timeouts and durations\n"); - rc = -ENODEV; - goto out_err; - } - if (tpm_do_selftest(chip)) { dev_err(dev, "TPM self test failed\n"); rc = -ENODEV;