| Message ID | 20180304121205.16934-1-tomas.winkler@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Mar 04, 2018 at 02:12:03PM +0200, Tomas Winkler wrote: > TPM2_CC_Create(0x153) and TPM2_CC_CreatePrimary (0x131) involve generation > of crypto keys which can be a computationally intensive task. > The timeout is set to 3min. > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com> Where is the cover letter? Please send separate patches if they are unrelated *or* add a cover letter that describes what they do as a whole. I will not review the next version if it does not have cover letter describing the high level change and containing the change log. > --- > drivers/char/tpm/tpm-interface.c | 4 ++++ > drivers/char/tpm/tpm.h | 27 ++++++++++++++++----------- > drivers/char/tpm/tpm2-cmd.c | 8 +++++--- > 3 files changed, 25 insertions(+), 14 deletions(-) > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c > index 85bdfa8c3348..c0aa9d11ec7a 100644 > --- a/drivers/char/tpm/tpm-interface.c > +++ b/drivers/char/tpm/tpm-interface.c > @@ -699,6 +699,10 @@ int tpm_get_timeouts(struct tpm_chip *chip) > msecs_to_jiffies(TPM2_DURATION_MEDIUM); > chip->duration[TPM_LONG] = > msecs_to_jiffies(TPM2_DURATION_LONG); > + chip->duration[TPM_LONG_LONG] = > + msecs_to_jiffies(TPM2_DURATION_LONG_LONG); > + chip->duration[TPM_UNDEFINED] = > + msecs_to_jiffies(TPM2_DURATION_DEFAULT); > > chip->flags |= TPM_CHIP_FLAG_HAVE_TIMEOUTS; > return 0; > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h > index f895fba4e20d..192ba68b39c2 100644 > --- a/drivers/char/tpm/tpm.h > +++ b/drivers/char/tpm/tpm.h > @@ -67,7 +67,9 @@ enum tpm_duration { > TPM_SHORT = 0, > TPM_MEDIUM = 1, > TPM_LONG = 2, > - TPM_UNDEFINED, > + TPM_LONG_LONG = 3, > + TPM_UNDEFINED = 4, > + TPM_DURATION_MAX, This is starting to rotten to become unmaintainable. Here is what I suggest to move forward: * Have essentially two duration types: 1. Default 2. Long 'default' is the old long duration i.e. two seconds. 'long' is a We should probably have two durations: enum tpm_duration { TPM_DURATION_DEFAULT = 2000, TPM_DURATION_LONG = 300000, }; These would be both for TPM 1.2 and TPM 2.0. Instead of having table for every ordinal there should be a small tables describing commands that require long timeout. > - duration = 2 * 60 * HZ; > + duration = msecs_to_jiffies(TPM2_DURATION_DEFAULT); NAK for this change. /Jarkko
> -----Original Message----- > From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com] > Sent: Monday, March 05, 2018 14:57 > To: Winkler, Tomas <tomas.winkler@intel.com> > Cc: Jason Gunthorpe <jgg@ziepe.ca>; Usyskin, Alexander > <alexander.usyskin@intel.com>; linux-integrity@vger.kernel.org; linux- > security-module@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH 1/3 RESEND] tpm: add longer timeouts for creation > commands. > > On Sun, Mar 04, 2018 at 02:12:03PM +0200, Tomas Winkler wrote: > > TPM2_CC_Create(0x153) and TPM2_CC_CreatePrimary (0x131) involve > > generation of crypto keys which can be a computationally intensive task. > > The timeout is set to 3min. > > > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com> > > Where is the cover letter? Please send separate patches if they are unrelated > *or* add a cover letter that describes what they do as a whole. > Why you need cover letter? What are u missing in the patch description > I will not review the next version if it does not have cover letter describing > the high level change and containing the change log. Don't follow. > > > --- > > drivers/char/tpm/tpm-interface.c | 4 ++++ > > drivers/char/tpm/tpm.h | 27 ++++++++++++++++----------- > > drivers/char/tpm/tpm2-cmd.c | 8 +++++--- > > 3 files changed, 25 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/char/tpm/tpm-interface.c > > b/drivers/char/tpm/tpm-interface.c > > index 85bdfa8c3348..c0aa9d11ec7a 100644 > > --- a/drivers/char/tpm/tpm-interface.c > > +++ b/drivers/char/tpm/tpm-interface.c > > @@ -699,6 +699,10 @@ int tpm_get_timeouts(struct tpm_chip *chip) > > msecs_to_jiffies(TPM2_DURATION_MEDIUM); > > chip->duration[TPM_LONG] = > > msecs_to_jiffies(TPM2_DURATION_LONG); > > + chip->duration[TPM_LONG_LONG] = > > + msecs_to_jiffies(TPM2_DURATION_LONG_LONG); > > + chip->duration[TPM_UNDEFINED] = > > + msecs_to_jiffies(TPM2_DURATION_DEFAULT); > > > > chip->flags |= TPM_CHIP_FLAG_HAVE_TIMEOUTS; > > return 0; > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index > > f895fba4e20d..192ba68b39c2 100644 > > --- a/drivers/char/tpm/tpm.h > > +++ b/drivers/char/tpm/tpm.h > > @@ -67,7 +67,9 @@ enum tpm_duration { > > TPM_SHORT = 0, > > TPM_MEDIUM = 1, > > TPM_LONG = 2, > > - TPM_UNDEFINED, > > + TPM_LONG_LONG = 3, > > + TPM_UNDEFINED = 4, > > + TPM_DURATION_MAX, > > This is starting to rotten to become unmaintainable. > Here is what I suggest to move forward: I fixed that in next patch, but this also moves the code to new spec, so I didn't want to make too much noise in this one. > * Have essentially two duration types: > 1. Default > 2. Long > 'default' is the old long duration i.e. two seconds. 'long' is a > > We should probably have two durations: > > enum tpm_duration { > TPM_DURATION_DEFAULT = 2000, > TPM_DURATION_LONG = 300000, > }; > How is this aligned with the spec PTP spec? > These would be both for TPM 1.2 and TPM 2.0. Instead of having table for > every ordinal there should be a small tables describing commands that > require long timeout. Yeah I didn't cover the 1.2. > > > - duration = 2 * 60 * HZ; > > + duration = msecs_to_jiffies(TPM2_DURATION_DEFAULT); > > NAK for this change. You should explain your NAKs, .... in general, doesn't look good. Thanks Tomas
On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote: > > enum tpm_duration { > > TPM_DURATION_DEFAULT = 2000, > > TPM_DURATION_LONG = 300000, > > }; > > > How is this aligned with the spec PTP spec? For TPM 2.0 that spec only partially defines durations for CCs and thus our look up table is already kind "flakky". In a sense that the default duration is upper limit for spec defined durations. > > These would be both for TPM 1.2 and TPM 2.0. Instead of having table for > > every ordinal there should be a small tables describing commands that > > require long timeout. > > Yeah I didn't cover the 1.2. I could probably help with TPM 1.2 changes if required. /Jarkko
> > On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote: > > > enum tpm_duration { > > > TPM_DURATION_DEFAULT = 2000, > > > TPM_DURATION_LONG = 300000, > > > }; > > > > > How is this aligned with the spec PTP spec? > > For TPM 2.0 that spec only partially defines durations for CCs and thus our > look up table is already kind "flakky". In a sense that the default duration is > upper limit for spec defined durations. The timeouts for LONG and MEDIUM is defined by the PTP spec, we need to maintain those as those effect the system. The UNDEFINED and LONG LONG is the implementation choice we driver from empirical data we have so far. > > > > These would be both for TPM 1.2 and TPM 2.0. Instead of having table > > > for every ordinal there should be a small tables describing commands > > > that require long timeout. > > > > Yeah I didn't cover the 1.2. > > I could probably help with TPM 1.2 changes if required. In middle of it, will send for review in few. Thanks Tomas
On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote:
> Why you need cover letter? What are u missing in the patch description
If you submit a *patch set* I *require* a cover letter, yes.
/Jarkko
On Mon, Mar 05, 2018 at 06:04:56PM +0000, Winkler, Tomas wrote: > > > > On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote: > > > > enum tpm_duration { > > > > TPM_DURATION_DEFAULT = 2000, > > > > TPM_DURATION_LONG = 300000, > > > > }; > > > > > > > How is this aligned with the spec PTP spec? > > > > For TPM 2.0 that spec only partially defines durations for CCs and thus our > > look up table is already kind "flakky". In a sense that the default duration is > > upper limit for spec defined durations. > > The timeouts for LONG and MEDIUM is defined by the PTP spec, we need to maintain those as those effect the system. > The UNDEFINED and LONG LONG is the implementation choice we driver from empirical data we have so far. Where can be get this empirical data? You are not only adding 30s delay but also turning the 2s delay to 12s delay. IMHO we could very well use PTP LONG for all commands as the timeout. Why that would not work? /Jarkko
> > On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote: > > Why you need cover letter? What are u missing in the patch description > > If you submit a *patch set* I *require* a cover letter, yes. It's good but it is not must, you are inventing your own rules. Thanks Tomas
> > On Mon, Mar 05, 2018 at 06:04:56PM +0000, Winkler, Tomas wrote: > > > > > > On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote: > > > > > enum tpm_duration { > > > > > TPM_DURATION_DEFAULT = 2000, > > > > > TPM_DURATION_LONG = 300000, > > > > > }; > > > > > > > > > How is this aligned with the spec PTP spec? > > > > > > For TPM 2.0 that spec only partially defines durations for CCs and > > > thus our look up table is already kind "flakky". In a sense that the > > > default duration is upper limit for spec defined durations. > > > > The timeouts for LONG and MEDIUM is defined by the PTP spec, we need > to maintain those as those effect the system. > > The UNDEFINED and LONG LONG is the implementation choice we driver > from empirical data we have so far. > > Where can be get this empirical data? From testing the HW. > > You are not only adding 30s delay but also turning the 2s delay to 12s delay. I'm adding 3 min, no other changes. Where is 12s? > IMHO we could very well use PTP LONG for all commands as the timeout. > Why that would not work? Empirically it doesn't go test it you have the HW. Thanks Tomas
On Tue, 2018-03-06 at 08:06 +0000, Winkler, Tomas wrote: > > > > > > On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote: > > > > > > Why you need cover letter? What are u missing in the patch > > > description > > > > If you submit a *patch set* I *require* a cover letter, yes. > > It's good but it is not must, you are inventing your own rules. As long as the Maintainer is the gatekeeper, you're not going to get very far with this argument. The fact is that a lot of subsystems have varying rules; often undocumented, some of which are even in conflict, like alphabetic vs reverse christmas tree format for includes. A cover letter is actually one of the more uniform rules. It's referred to in submitting patches, but not actually documented there. James
> > On Tue, 2018-03-06 at 08:06 +0000, Winkler, Tomas wrote: > > > > > > > > > On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote: > > > > > > > > Why you need cover letter? What are u missing in the patch > > > > description > > > > > > If you submit a *patch set* I *require* a cover letter, yes. > > > > It's good but it is not must, you are inventing your own rules. > > As long as the Maintainer is the gatekeeper, you're not going to get very far > with this argument. The fact is that a lot of subsystems have varying rules; > often undocumented, some of which are even in conflict, like alphabetic vs > reverse christmas tree format for includes. Usually I'm trying to stay in convention of the surouding code even if it's agains my personal taste. I think this particular case was a bit different. > A cover letter is actually one of the more uniform rules. It's referred to in > submitting patches, but not actually documented there. I'm all for cover letters, but for few code line fixes it's more comments then code. What is most important I think I and Jarkko had found finally a common ground. Thanks Tomas
On Tue, 2018-03-06 at 08:32 -0800, James Bottomley wrote: > On Tue, 2018-03-06 at 08:06 +0000, Winkler, Tomas wrote: > > > > > > > > > On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote: > > > > > > > > Why you need cover letter? What are u missing in the patch > > > > description > > > > > > If you submit a *patch set* I *require* a cover letter, yes. > > > > It's good but it is not must, you are inventing your own rules. > > As long as the Maintainer is the gatekeeper, you're not going to get > very far with this argument. The fact is that a lot of subsystems have > varying rules; often undocumented, some of which are even in conflict, > like alphabetic vs reverse christmas tree format for includes. > > A cover letter is actually one of the more uniform rules. It's > referred to in submitting patches, but not actually documented there. I've heard that some maintainers are moving away from cover letters, since they are not include in the git repo and are lost. I've seen Andrew Morton cut and paste the cover letter in the first patch description of the patch set. Mimi
On Tue, Mar 06, 2018 at 01:36:36PM -0500, Mimi Zohar wrote: > On Tue, 2018-03-06 at 08:32 -0800, James Bottomley wrote: > > On Tue, 2018-03-06 at 08:06 +0000, Winkler, Tomas wrote: > > > > > > > > > > > > On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote: > > > > > > > > > > Why you need cover letter? What are u missing in the patch > > > > > description > > > > > > > > If you submit a *patch set* I *require* a cover letter, yes. > > > > > > It's good but it is not must, you are inventing your own rules. > > > > As long as the Maintainer is the gatekeeper, you're not going to get > > very far with this argument. The fact is that a lot of subsystems have > > varying rules; often undocumented, some of which are even in conflict, > > like alphabetic vs reverse christmas tree format for includes. > > > > A cover letter is actually one of the more uniform rules. It's > > referred to in submitting patches, but not actually documented there. > > I've heard that some maintainers are moving away from cover letters, > since they are not include in the git repo and are lost. I've seen > Andrew Morton cut and paste the cover letter in the first patch > description of the patch set. Andrew has a workflow unlike any other I've seen.. In my view the cover letter should explain why the maintainer should apply the series, and give any guidance to the review process. Not duplicate information that belongs in the patch comments. It shouldn't explain how/why the patch(es) works, etc. Jason
On Tue, 2018-03-06 at 14:59 -0700, Jason Gunthorpe wrote: > On Tue, Mar 06, 2018 at 01:36:36PM -0500, Mimi Zohar wrote: > > On Tue, 2018-03-06 at 08:32 -0800, James Bottomley wrote: > > > On Tue, 2018-03-06 at 08:06 +0000, Winkler, Tomas wrote: > > > > > > > > > > > > > > > On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote: > > > > > > > > > > > > Why you need cover letter? What are u missing in the patch > > > > > > description > > > > > > > > > > If you submit a *patch set* I *require* a cover letter, yes. > > > > > > > > It's good but it is not must, you are inventing your own rules. > > > > > > As long as the Maintainer is the gatekeeper, you're not going to get > > > very far with this argument. The fact is that a lot of subsystems have > > > varying rules; often undocumented, some of which are even in conflict, > > > like alphabetic vs reverse christmas tree format for includes. > > > > > > A cover letter is actually one of the more uniform rules. It's > > > referred to in submitting patches, but not actually documented there. > > > > I've heard that some maintainers are moving away from cover letters, > > since they are not include in the git repo and are lost. I've seen > > Andrew Morton cut and paste the cover letter in the first patch > > description of the patch set. > > Andrew has a workflow unlike any other I've seen.. Andrew is the only one that actually cut and pasted the cover letter in the first patch description, but I've heard this from others. > In my view the cover letter should explain why the maintainer should > apply the series, and give any guidance to the review process. > Not duplicate information that belongs in the patch comments. It > shouldn't explain how/why the patch(es) works, etc. Patch descriptions should never explain how/why a particular patch works. If it is that difficult to understand, then something is wrong with the patch. The individual patch descriptions should provide the current status, the motivation for the change, and short summary of the change (eg. new features, configs, etc). The cover letter is needed for (larger) patch sets in order to explain the overall motivation, instead of having to guess where the patch set is going. I wouldn't expect to see a cover letter for a single bug fix or two. Mimi
> > On Tue, 2018-03-06 at 14:59 -0700, Jason Gunthorpe wrote: > > On Tue, Mar 06, 2018 at 01:36:36PM -0500, Mimi Zohar wrote: > > > On Tue, 2018-03-06 at 08:32 -0800, James Bottomley wrote: > > > > On Tue, 2018-03-06 at 08:06 +0000, Winkler, Tomas wrote: > > > > > > > > > > > > > > > > > > On Mon, Mar 05, 2018 at 01:09:09PM +0000, Winkler, Tomas wrote: > > > > > > > > > > > > > > Why you need cover letter? What are u missing in the patch > > > > > > > description > > > > > > > > > > > > If you submit a *patch set* I *require* a cover letter, yes. > > > > > > > > > > It's good but it is not must, you are inventing your own rules. > > > > > > > > As long as the Maintainer is the gatekeeper, you're not going to > > > > get very far with this argument. The fact is that a lot of > > > > subsystems have varying rules; often undocumented, some of which > > > > are even in conflict, like alphabetic vs reverse christmas tree format for > includes. > > > > > > > > A cover letter is actually one of the more uniform rules. It's > > > > referred to in submitting patches, but not actually documented there. > > > > > > I've heard that some maintainers are moving away from cover letters, > > > since they are not include in the git repo and are lost. I've seen > > > Andrew Morton cut and paste the cover letter in the first patch > > > description of the patch set. > > > > Andrew has a workflow unlike any other I've seen.. > > Andrew is the only one that actually cut and pasted the cover letter in the > first patch description, but I've heard this from others. > > > In my view the cover letter should explain why the maintainer should > > apply the series, and give any guidance to the review process. > > > Not duplicate information that belongs in the patch comments. It > > shouldn't explain how/why the patch(es) works, etc. > > Patch descriptions should never explain how/why a particular patch > works. If it is that difficult to understand, then something is wrong with the > patch. The individual patch descriptions should provide the current status, > the motivation for the change, and short summary of the change (eg. new > features, configs, etc). > > The cover letter is needed for (larger) patch sets in order to explain the > overall motivation, instead of having to guess where the patch set is going. I > wouldn't expect to see a cover letter for a single bug fix or two. > > Mimi I second that. Tomas.
On Tue, 06 Mar 2018 13:36:36 -0500 Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: > I've heard that some maintainers are moving away from cover letters, > since they are not include in the git repo and are lost. If I get a patch series with a cover letter that should be preserved, I apply the series in a branch then do a no-ff merge; the cover letter can then go into the merge commit. There's no reason why cover letters need to be lost. jon
> > On Tue, 06 Mar 2018 13:36:36 -0500 > Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: > > > I've heard that some maintainers are moving away from cover letters, > > since they are not include in the git repo and are lost. > > If I get a patch series with a cover letter that should be preserved, I apply the > series in a branch then do a no-ff merge; the cover letter can then go into > the merge commit. There's no reason why cover letters need to be lost. That's clever, but still it feels like a hack. I've try to keep cover letters as an empty commits for that but it has to be always forced when rebasing. Not sure git has a real user friendly support for that. Tomas
On Wed, 2018-03-07 at 08:54 -0700, Jonathan Corbet wrote: > On Tue, 06 Mar 2018 13:36:36 -0500 > Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: > > > I've heard that some maintainers are moving away from cover letters, > > since they are not include in the git repo and are lost. > > If I get a patch series with a cover letter that should be preserved, I > apply the series in a branch then do a no-ff merge; the cover letter can > then go into the merge commit. There's no reason why cover letters need to > be lost. Thanks, Jon. That sounds like a really, good idea. Some maintainers are saying to put the Changelog after the "---" so that it isn't included in the patch description. One of the reasons for including the Changelog in the patch description, is to credit people with bug fixes, important rework of the patch, etc. Do you have any thoughts on this? Mimi
On Wed, 07 Mar 2018 11:35:03 -0500 Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: > Some maintainers are saying to put the Changelog after the "---" so > that it isn't included in the patch description. > > One of the reasons for including the Changelog in the patch > description, is to credit people with bug fixes, important rework of > the patch, etc. > > Do you have any thoughts on this? Not sure I have a strong opinion there. We do have various tags meant to ensure proper credit; I would lean toward using those when possible rather than including long change histories that, in the long term, don't help readers to understand why the patch was applied. Thanks, jon
On Tue, 2018-03-06 at 13:36 -0500, Mimi Zohar wrote: > I've heard that some maintainers are moving away from cover letters, > since they are not include in the git repo and are lost. I've seen > Andrew Morton cut and paste the cover letter in the first patch > description of the patch set. When I contribute code, the cover letter helps me to do the Right Thing.. Taking the time to write a proper cover letter helps to do a "mental exercise" that 1. The changes make sense in the first place. 2. Only the necessary is done, not more or less. Even for a small patch set the time used to write the cover letter will pay off because it helps the maitainer to make a fair and educated decision. /Jarkko
On Wed, 2018-03-07 at 08:54 -0700, Jonathan Corbet wrote: > On Tue, 06 Mar 2018 13:36:36 -0500 > Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: > > > I've heard that some maintainers are moving away from cover letters, > > since they are not include in the git repo and are lost. > > If I get a patch series with a cover letter that should be preserved, I > apply the series in a branch then do a no-ff merge; the cover letter can > then go into the merge commit. There's no reason why cover letters need to > be lost. That is an awesome idea. I'll start using this approach. Thank you. /Jarkko
On Wed, 2018-03-07 at 11:35 -0500, Mimi Zohar wrote: > On Wed, 2018-03-07 at 08:54 -0700, Jonathan Corbet wrote: > > On Tue, 06 Mar 2018 13:36:36 -0500 > > Mimi Zohar <zohar@linux.vnet.ibm.com> wrote: > > > > > I've heard that some maintainers are moving away from cover letters, > > > since they are not include in the git repo and are lost. > > > > If I get a patch series with a cover letter that should be preserved, I > > apply the series in a branch then do a no-ff merge; the cover letter can > > then go into the merge commit. There's no reason why cover letters need to > > be lost. > > Thanks, Jon. That sounds like a really, good idea. > > Some maintainers are saying to put the Changelog after the "---" so > that it isn't included in the patch description. > > One of the reasons for including the Changelog in the patch > description, is to credit people with bug fixes, important rework of > the patch, etc. This is a really good point. By adding the cover letter to the GIT history helps to better track people who to thank or blame :-) /Jarkko
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c index 85bdfa8c3348..c0aa9d11ec7a 100644 --- a/drivers/char/tpm/tpm-interface.c +++ b/drivers/char/tpm/tpm-interface.c @@ -699,6 +699,10 @@ int tpm_get_timeouts(struct tpm_chip *chip) msecs_to_jiffies(TPM2_DURATION_MEDIUM); chip->duration[TPM_LONG] = msecs_to_jiffies(TPM2_DURATION_LONG); + chip->duration[TPM_LONG_LONG] = + msecs_to_jiffies(TPM2_DURATION_LONG_LONG); + chip->duration[TPM_UNDEFINED] = + msecs_to_jiffies(TPM2_DURATION_DEFAULT); chip->flags |= TPM_CHIP_FLAG_HAVE_TIMEOUTS; return 0; diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index f895fba4e20d..192ba68b39c2 100644 --- a/drivers/char/tpm/tpm.h +++ b/drivers/char/tpm/tpm.h @@ -67,7 +67,9 @@ enum tpm_duration { TPM_SHORT = 0, TPM_MEDIUM = 1, TPM_LONG = 2, - TPM_UNDEFINED, + TPM_LONG_LONG = 3, + TPM_UNDEFINED = 4, + TPM_DURATION_MAX, }; #define TPM_WARN_RETRY 0x800 @@ -79,15 +81,17 @@ enum tpm_duration { #define TPM_HEADER_SIZE 10 enum tpm2_const { - TPM2_PLATFORM_PCR = 24, - TPM2_PCR_SELECT_MIN = ((TPM2_PLATFORM_PCR + 7) / 8), - TPM2_TIMEOUT_A = 750, - TPM2_TIMEOUT_B = 2000, - TPM2_TIMEOUT_C = 200, - TPM2_TIMEOUT_D = 30, - TPM2_DURATION_SHORT = 20, - TPM2_DURATION_MEDIUM = 750, - TPM2_DURATION_LONG = 2000, + TPM2_PLATFORM_PCR = 24, + TPM2_PCR_SELECT_MIN = ((TPM2_PLATFORM_PCR + 7) / 8), + TPM2_TIMEOUT_A = 750, + TPM2_TIMEOUT_B = 2000, + TPM2_TIMEOUT_C = 200, + TPM2_TIMEOUT_D = 30, + TPM2_DURATION_SHORT = 20, + TPM2_DURATION_MEDIUM = 750, + TPM2_DURATION_LONG = 2000, + TPM2_DURATION_LONG_LONG = 300000, + TPM2_DURATION_DEFAULT = 120000, }; enum tpm2_structures { @@ -123,6 +127,7 @@ enum tpm2_algorithms { enum tpm2_command_codes { TPM2_CC_FIRST = 0x011F, + TPM2_CC_CREATE_PRIMARY = 0x0131, TPM2_CC_SELF_TEST = 0x0143, TPM2_CC_STARTUP = 0x0144, TPM2_CC_SHUTDOWN = 0x0145, @@ -227,7 +232,7 @@ struct tpm_chip { unsigned long timeout_c; /* jiffies */ unsigned long timeout_d; /* jiffies */ bool timeout_adjusted; - unsigned long duration[3]; /* jiffies */ + unsigned long duration[TPM_DURATION_MAX]; /* jiffies */ bool duration_adjusted; struct dentry *bios_dir[TPM_NUM_EVENT_LOG_FILES]; diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c index a700f8f9ead7..c1ddbbba406e 100644 --- a/drivers/char/tpm/tpm2-cmd.c +++ b/drivers/char/tpm/tpm2-cmd.c @@ -90,6 +90,8 @@ static struct tpm2_hash tpm2_hash_map[] = { * of time the chip could take to return the result. The values * of the SHORT, MEDIUM, and LONG durations are taken from the * PC Client Profile (PTP) specification. + * LONG_LONG is for commands that generates keys which empirically + * takes longer time on some systems. */ static const u8 tpm2_ordinal_duration[TPM2_CC_LAST - TPM2_CC_FIRST + 1] = { TPM_UNDEFINED, /* 11F */ @@ -110,7 +112,7 @@ static const u8 tpm2_ordinal_duration[TPM2_CC_LAST - TPM2_CC_FIRST + 1] = { TPM_UNDEFINED, /* 12e */ TPM_UNDEFINED, /* 12f */ TPM_UNDEFINED, /* 130 */ - TPM_UNDEFINED, /* 131 */ + TPM_LONG_LONG, /* 131 */ TPM_UNDEFINED, /* 132 */ TPM_UNDEFINED, /* 133 */ TPM_UNDEFINED, /* 134 */ @@ -144,7 +146,7 @@ static const u8 tpm2_ordinal_duration[TPM2_CC_LAST - TPM2_CC_FIRST + 1] = { TPM_UNDEFINED, /* 150 */ TPM_UNDEFINED, /* 151 */ TPM_UNDEFINED, /* 152 */ - TPM_UNDEFINED, /* 153 */ + TPM_LONG_LONG, /* 153 */ TPM_UNDEFINED, /* 154 */ TPM_UNDEFINED, /* 155 */ TPM_UNDEFINED, /* 156 */ @@ -821,7 +823,7 @@ unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal) duration = chip->duration[index]; if (duration <= 0) - duration = 2 * 60 * HZ; + duration = msecs_to_jiffies(TPM2_DURATION_DEFAULT); return duration; }
TPM2_CC_Create(0x153) and TPM2_CC_CreatePrimary (0x131) involve generation of crypto keys which can be a computationally intensive task. The timeout is set to 3min. Signed-off-by: Tomas Winkler <tomas.winkler@intel.com> --- drivers/char/tpm/tpm-interface.c | 4 ++++ drivers/char/tpm/tpm.h | 27 ++++++++++++++++----------- drivers/char/tpm/tpm2-cmd.c | 8 +++++--- 3 files changed, 25 insertions(+), 14 deletions(-)