From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
To: Scot Doyle <lkml14@scotdoyle.com>
Cc: Peter Huewe <peterhuewe@gmx.de>,
Ashley Lai <ashley@ashleylai.com>,
Marcel Selhorst <tpmdd@selhorst.net>,
Stefan Berger <stefanb@linux.vnet.ibm.com>,
Luigi Semenzato <semenzato@google.com>,
tpmdd-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH v5] tpm_tis: verify interrupt during init
Date: Tue, 2 Sep 2014 11:20:15 -0600 [thread overview]
Message-ID: <20140902172015.GD13956@obsidianresearch.com> (raw)
In-Reply-To: <alpine.LNX.2.11.1408302229001.579@localhost.localdomain>
On Sat, Aug 30, 2014 at 11:23:56PM +0000, Scot Doyle wrote:
> On Sat, 30 Aug 2014, Jason Gunthorpe wrote:
>
> > On Fri, Aug 29, 2014 at 11:59:32PM +0000, Scot Doyle wrote:
> >
> >> I tried calling tpm_get_timeouts only during the interrupt test, but again
> >> was timed out after 30 seconds. The interrupt wait in tis_send calls
> >> tpm_calc_ordinal_duration, which uses a default timeout of two minutes
> >> when chip->vendor.duration[duration_idx] hasn't been set. Thus the second
> >> call to tpm_get_timeouts in tpm_tis_init.
> >
> > So the strategy is to read the timeouts and hope that the chip reports
> > something small and reasonable, then do a second read?
> >
> > Seems reasonable, but with this new arrangement we could also use an
> > alternate polling logic for 'testing_int' that did the normal polling
> > loop unconditionally and then checked if the interrupt was
> > delivered. This would give a minimal dealy.
>
> I like the idea. And then tpm_do_selftest could be used for the interrupt
> verification instead of a second tpm_get_timeouts?
Yes, or the first tpm_get_timeouts can be used - Long term I would
like to see the entire tpm_get_timeouts,self_test,startup, etc
sequence moved into core code, so I don't really want to see drivers
splitting the sequence up.
Ideally the driver will just automatically test the IRQ on the very
first command it executes. That is now a very small easy step, so lets
just do that..
> The output is now
> [ 1.526798] tpm_tis 00:08: 1.2 TPM (device-id 0xB, rev-id 16)
> [ 5.914732] tpm_tis 00:08: [Firmware Bug]: TPM interrupt not working, polling instead
Cool, why did it take 4 seconds though?
> +struct priv_data {
> + int test_irq;
Probably don't need this...
> @@ -358,13 +379,27 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
>
And this can probably just become:
bool test_irq = priv->int_count == 0;
int oldirq = chip->vendor.irq;
> + ((struct priv_data*)chip->vendor.priv)->int_count++;
.. Seems like there was no need for it to count, this can just be =
true?
> - if (tpm_do_selftest(chip)) {
> - dev_err(dev, "TPM self test failed\n");
> - rc = -ENODEV;
> - goto out_err;
> - }
And move tpm_get_timeouts down too.. Keep the sequence together.
Looks really good to me, I can try and test the next version here this
week.
Jason
next prev parent reply other threads:[~2014-09-02 17:20 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-08-22 0:58 [PATCH] tpm_tis: Verify ACPI-specified interrupt Scot Doyle
2014-08-22 16:06 ` Jason Gunthorpe
2014-08-22 20:17 ` Scot Doyle
2014-08-22 20:32 ` Jason Gunthorpe
2014-08-22 22:48 ` Peter Hüwe
2014-08-25 6:38 ` Scot Doyle
2014-08-25 18:24 ` Jason Gunthorpe
2014-08-27 4:31 ` [RFC PATCH v2] tpm_tis: verify interrupt during init Scot Doyle
2014-08-27 17:31 ` Jason Gunthorpe
2014-08-27 21:32 ` [RFC PATCH v3] " Scot Doyle
2014-08-27 21:47 ` Jason Gunthorpe
2014-08-28 0:35 ` Scot Doyle
2014-08-28 16:53 ` Jason Gunthorpe
2014-08-29 23:59 ` [RFC PATCH v4] " Scot Doyle
2014-08-30 17:49 ` Jason Gunthorpe
2014-08-30 23:23 ` [RFC PATCH v5] " Scot Doyle
2014-09-02 17:20 ` Jason Gunthorpe [this message]
2014-09-02 20:22 ` [RFC PATCH v6] " Scot Doyle
2014-09-08 22:02 ` Jason Gunthorpe
2014-09-09 2:13 ` [PATCH v7] " Scot Doyle
2014-09-09 3:12 ` Scot Doyle
2014-09-11 0:50 ` [RFC PATCH v8] " Scot Doyle
2014-09-16 23:36 ` Scot Doyle
2014-09-22 17:13 ` Jason Gunthorpe
2014-09-22 19:01 ` Peter Hüwe
2014-10-19 20:08 ` Scot Doyle
2014-09-23 2:44 ` Scot Doyle
2014-09-23 2:51 ` [PATCH v9] " Scot Doyle
2014-09-23 11:55 ` Scot Doyle
2014-09-23 17:12 ` [tpmdd-devel] " Stefan Berger
2014-09-24 19:38 ` Scot Doyle
2014-09-24 19:41 ` Stefan Berger
2014-09-24 22:41 ` [PATCH v10] " Scot Doyle
2014-09-29 17:24 ` Jason Gunthorpe
2014-11-30 14:24 ` Peter Hüwe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20140902172015.GD13956@obsidianresearch.com \
--to=jgunthorpe@obsidianresearch.com \
--cc=ashley@ashleylai.com \
--cc=linux-kernel@vger.kernel.org \
--cc=lkml14@scotdoyle.com \
--cc=peterhuewe@gmx.de \
--cc=semenzato@google.com \
--cc=stefanb@linux.vnet.ibm.com \
--cc=tpmdd-devel@lists.sourceforge.net \
--cc=tpmdd@selhorst.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).