diff mbox

[1/3,RESEND] tpm: add longer timeouts for creation commands.

Message ID 20180304121205.16934-1-tomas.winkler@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Winkler, Tomas March 4, 2018, 12:12 p.m. UTC
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(-)

Comments

Jarkko Sakkinen March 5, 2018, 12:56 p.m. UTC | #1
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Winkler, Tomas March 5, 2018, 1:09 p.m. UTC | #2
> -----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

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarkko Sakkinen March 5, 2018, 5:59 p.m. UTC | #3
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Winkler, Tomas March 5, 2018, 6:04 p.m. UTC | #4
> 
> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarkko Sakkinen March 6, 2018, 7:49 a.m. UTC | #5
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarkko Sakkinen March 6, 2018, 8:02 a.m. UTC | #6
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Winkler, Tomas March 6, 2018, 8:06 a.m. UTC | #7
> 
> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Winkler, Tomas March 6, 2018, 8:09 a.m. UTC | #8
> 
> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
James Bottomley March 6, 2018, 4:32 p.m. UTC | #9
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

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Winkler, Tomas March 6, 2018, 4:45 p.m. UTC | #10
> 

> 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
Mimi Zohar March 6, 2018, 6:36 p.m. UTC | #11
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

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jason Gunthorpe March 6, 2018, 9:59 p.m. UTC | #12
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mimi Zohar March 7, 2018, 3:22 p.m. UTC | #13
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

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Winkler, Tomas March 7, 2018, 3:41 p.m. UTC | #14
> 

> 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.
Jonathan Corbet March 7, 2018, 3:54 p.m. UTC | #15
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mimi Zohar March 7, 2018, 4:35 p.m. UTC | #16
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

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jonathan Corbet March 7, 2018, 6:24 p.m. UTC | #17
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarkko Sakkinen March 10, 2018, 12:37 p.m. UTC | #18
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarkko Sakkinen March 10, 2018, 12:44 p.m. UTC | #19
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarkko Sakkinen March 10, 2018, 12:46 p.m. UTC | #20
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;
 }