All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
To: James Bottomley <James.Bottomley@HansenPartnership.com>
Cc: linux-integrity@vger.kernel.org,
	linux-security-module@vger.kernel.org, keyrings@vger.kernel.org,
	Alexander Steffen <Alexander.Steffen@infineon.com>,
	stable@vger.kernel.org,
	Jarkko Sakkinen <jarkko.sakkine@linux.intel.com>,
	Peter Huewe <peterhuewe@gmx.de>, Jason Gunthorpe <jgg@ziepe.ca>,
	Arnd Bergmann <arnd@arndb.de>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/5] tpm: Trigger only missing TPM 2.0 self tests
Date: Mon, 05 Mar 2018 11:23:34 +0000	[thread overview]
Message-ID: <20180305112334.GI25377@linux.intel.com> (raw)
In-Reply-To: <1520107919.3105.9.camel@HansenPartnership.com>

On Sat, Mar 03, 2018 at 12:11:59PM -0800, James Bottomley wrote:
> I still think removing all logging traces is a mistake for something
> that can consume a significant amount of time in the boot sequence.
>  It's going to cause lost of people doing boot timings to waste lots of
> effort.
> 
> However, removing the log messages makes the above statement a lie, so
> one of the two needs fixing.

The commit itself makes sense but the implementation was sloppy to say
the least in v3. It was like prototype/PoC version of something that
could be merged to mainline, not something that can be merged to
mainline.

For example:

* You could have implemented it more cleanly without that new
  'tpm_transmit_check' helper function as can be seen.
* Many log messages contained a redundant "TPM:" prefix.
* There duplicate logs to tpm_transmit_cmd(). Pass a NULL as
  desc tpm_transmit_cmd() if you want to take care of logging
  yourself.
* The commit has the same short summary as the commit it fixes.
  This issue still persists. Open for suggestions.

Please state if v3 contains a log message that has been removed and is
still mandatory and I can add it if it makes sense.

> > +		rc = tpm_transmit_cmd(chip, NULL, buf.data,
> > PAGE_SIZE, 0, 0,
> > +				      "attempting the self test\n");
> 
> There shouldn't be a \n in the string: the failure message already
> appends one.

Oops, my bad, will fix it.

/Jarkko

WARNING: multiple messages have this Message-ID (diff)
From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
To: James Bottomley <James.Bottomley@HansenPartnership.com>
Cc: linux-integrity@vger.kernel.org,
	linux-security-module@vger.kernel.org, keyrings@vger.kernel.org,
	Alexander Steffen <Alexander.Steffen@infineon.com>,
	stable@vger.kernel.org,
	Jarkko Sakkinen <jarkko.sakkine@linux.intel.com>,
	Peter Huewe <peterhuewe@gmx.de>, Jason Gunthorpe <jgg@ziepe.ca>,
	Arnd Bergmann <arnd@arndb.de>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/5] tpm: Trigger only missing TPM 2.0 self tests
Date: Mon, 5 Mar 2018 13:23:34 +0200	[thread overview]
Message-ID: <20180305112334.GI25377@linux.intel.com> (raw)
In-Reply-To: <1520107919.3105.9.camel@HansenPartnership.com>

On Sat, Mar 03, 2018 at 12:11:59PM -0800, James Bottomley wrote:
> I still think removing all logging traces is a mistake for something
> that can consume a significant amount of time in the boot sequence.
>  It's going to cause lost of people doing boot timings to waste lots of
> effort.
> 
> However, removing the log messages makes the above statement a lie, so
> one of the two needs fixing.

The commit itself makes sense but the implementation was sloppy to say
the least in v3. It was like prototype/PoC version of something that
could be merged to mainline, not something that can be merged to
mainline.

For example:

* You could have implemented it more cleanly without that new
  'tpm_transmit_check' helper function as can be seen.
* Many log messages contained a redundant "TPM:" prefix.
* There duplicate logs to tpm_transmit_cmd(). Pass a NULL as
  desc tpm_transmit_cmd() if you want to take care of logging
  yourself.
* The commit has the same short summary as the commit it fixes.
  This issue still persists. Open for suggestions.

Please state if v3 contains a log message that has been removed and is
still mandatory and I can add it if it makes sense.

> > +		rc = tpm_transmit_cmd(chip, NULL, buf.data,
> > PAGE_SIZE, 0, 0,
> > +				      "attempting the self test\n");
> 
> There shouldn't be a \n in the string: the failure message already
> appends one.

Oops, my bad, will fix it.

/Jarkko

WARNING: multiple messages have this Message-ID (diff)
From: jarkko.sakkinen@linux.intel.com (Jarkko Sakkinen)
To: linux-security-module@vger.kernel.org
Subject: [PATCH 1/5] tpm: Trigger only missing TPM 2.0 self tests
Date: Mon, 5 Mar 2018 13:23:34 +0200	[thread overview]
Message-ID: <20180305112334.GI25377@linux.intel.com> (raw)
In-Reply-To: <1520107919.3105.9.camel@HansenPartnership.com>

On Sat, Mar 03, 2018 at 12:11:59PM -0800, James Bottomley wrote:
> I still think removing all logging traces is a mistake for something
> that can consume a significant amount of time in the boot sequence.
> ?It's going to cause lost of people doing boot timings to waste lots of
> effort.
> 
> However, removing the log messages makes the above statement a lie, so
> one of the two needs fixing.

The commit itself makes sense but the implementation was sloppy to say
the least in v3. It was like prototype/PoC version of something that
could be merged to mainline, not something that can be merged to
mainline.

For example:

* You could have implemented it more cleanly without that new
  'tpm_transmit_check' helper function as can be seen.
* Many log messages contained a redundant "TPM:" prefix.
* There duplicate logs to tpm_transmit_cmd(). Pass a NULL as
  desc tpm_transmit_cmd() if you want to take care of logging
  yourself.
* The commit has the same short summary as the commit it fixes.
  This issue still persists. Open for suggestions.

Please state if v3 contains a log message that has been removed and is
still mandatory and I can add it if it makes sense.

> > +		rc = tpm_transmit_cmd(chip, NULL, buf.data,
> > PAGE_SIZE, 0, 0,
> > +				??????"attempting the self test\n");
> 
> There shouldn't be a \n in the string: the failure message already
> appends one.

Oops, my bad, will fix it.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

WARNING: multiple messages have this Message-ID (diff)
From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
To: James Bottomley <James.Bottomley@HansenPartnership.com>
Cc: linux-integrity@vger.kernel.org,
	linux-security-module@vger.kernel.org, keyrings@vger.kernel.org,
	Alexander Steffen <Alexander.Steffen@infineon.com>,
	stable@vger.kernel.org,
	Jarkko Sakkinen <jarkko.sakkine@linux.intel.com>,
	Peter Huewe <peterhuewe@gmx.de>, Jason Gunthorpe <jgg@ziepe.ca>,
	Arnd Bergmann <arnd@arndb.de>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/5] tpm: Trigger only missing TPM 2.0 self tests
Date: Mon, 5 Mar 2018 13:23:34 +0200	[thread overview]
Message-ID: <20180305112334.GI25377@linux.intel.com> (raw)
In-Reply-To: <1520107919.3105.9.camel@HansenPartnership.com>

On Sat, Mar 03, 2018 at 12:11:59PM -0800, James Bottomley wrote:
> I still think removing all logging traces is a mistake for something
> that can consume a significant amount of time in the boot sequence.
>  It's going to cause lost of people doing boot timings to waste lots of
> effort.
> 
> However, removing the log messages makes the above statement a lie, so
> one of the two needs fixing.

The commit itself makes sense but the implementation was sloppy to say
the least in v3. It was like prototype/PoC version of something that
could be merged to mainline, not something that can be merged to
mainline.

For example:

* You could have implemented it more cleanly without that new
  'tpm_transmit_check' helper function as can be seen.
* Many log messages contained a redundant "TPM:" prefix.
* There duplicate logs to tpm_transmit_cmd(). Pass a NULL as
  desc tpm_transmit_cmd() if you want to take care of logging
  yourself.
* The commit has the same short summary as the commit it fixes.
  This issue still persists. Open for suggestions.

Please state if v3 contains a log message that has been removed and is
still mandatory and I can add it if it makes sense.

> > +		rc = tpm_transmit_cmd(chip, NULL, buf.data,
> > PAGE_SIZE, 0, 0,
> > +				      "attempting the self test\n");
> 
> There shouldn't be a \n in the string: the failure message already
> appends one.

Oops, my bad, will fix it.

/Jarkko

WARNING: multiple messages have this Message-ID (diff)
From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
To: James Bottomley <James.Bottomley@HansenPartnership.com>
Cc: linux-integrity@vger.kernel.org,
	linux-security-module@vger.kernel.org, keyrings@vger.kernel.org,
	Alexander Steffen <Alexander.Steffen@infineon.com>,
	stable@vger.kernel.org,
	Jarkko Sakkinen <jarkko.sakkine@linux.intel.com>,
	Peter Huewe <peterhuewe@gmx.de>, Jason Gunthorpe <jgg@ziepe.ca>,
	Arnd Bergmann <arnd@arndb.de>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/5] tpm: Trigger only missing TPM 2.0 self tests
Date: Mon, 5 Mar 2018 13:23:34 +0200	[thread overview]
Message-ID: <20180305112334.GI25377@linux.intel.com> (raw)
In-Reply-To: <1520107919.3105.9.camel@HansenPartnership.com>

On Sat, Mar 03, 2018 at 12:11:59PM -0800, James Bottomley wrote:
> I still think removing all logging traces is a mistake for something
> that can consume a significant amount of time in the boot sequence.
> �It's going to cause lost of people doing boot timings to waste lots of
> effort.
> 
> However, removing the log messages makes the above statement a lie, so
> one of the two needs fixing.

The commit itself makes sense but the implementation was sloppy to say
the least in v3. It was like prototype/PoC version of something that
could be merged to mainline, not something that can be merged to
mainline.

For example:

* You could have implemented it more cleanly without that new
  'tpm_transmit_check' helper function as can be seen.
* Many log messages contained a redundant "TPM:" prefix.
* There duplicate logs to tpm_transmit_cmd(). Pass a NULL as
  desc tpm_transmit_cmd() if you want to take care of logging
  yourself.
* The commit has the same short summary as the commit it fixes.
  This issue still persists. Open for suggestions.

Please state if v3 contains a log message that has been removed and is
still mandatory and I can add it if it makes sense.

> > +		rc = tpm_transmit_cmd(chip, NULL, buf.data,
> > PAGE_SIZE, 0, 0,
> > +				������"attempting the self test\n");
> 
> There shouldn't be a \n in the string: the failure message already
> appends one.

Oops, my bad, will fix it.

/Jarkko

  reply	other threads:[~2018-03-05 11:23 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-02-28 19:58 [PATCH 0/5] Migrate all TPM 2.0 commands to use struct tpm_buf Jarkko Sakkinen
2018-02-28 19:58 ` Jarkko Sakkinen
2018-02-28 19:58 ` Jarkko Sakkinen
2018-02-28 19:58 ` [PATCH 1/5] tpm: Trigger only missing TPM 2.0 self tests Jarkko Sakkinen
2018-02-28 19:58   ` Jarkko Sakkinen
2018-02-28 19:58   ` Jarkko Sakkinen
2018-03-03 20:11   ` James Bottomley
2018-03-03 20:11     ` James Bottomley
2018-03-03 20:11     ` James Bottomley
2018-03-03 20:11     ` James Bottomley
2018-03-05 11:23     ` Jarkko Sakkinen [this message]
2018-03-05 11:23       ` Jarkko Sakkinen
2018-03-05 11:23       ` Jarkko Sakkinen
2018-03-05 11:23       ` Jarkko Sakkinen
2018-03-05 11:23       ` Jarkko Sakkinen
2018-02-28 19:58 ` [PATCH 2/5] tpm: migrate tpm2_shutdown() to use struct tpm_buf Jarkko Sakkinen
2018-02-28 19:58   ` Jarkko Sakkinen
2018-02-28 19:58   ` Jarkko Sakkinen
2018-02-28 20:25   ` Jason Gunthorpe
2018-02-28 20:25     ` Jason Gunthorpe
2018-02-28 20:25     ` Jason Gunthorpe
2018-03-01 10:11     ` Jarkko Sakkinen
2018-03-01 10:11       ` Jarkko Sakkinen
2018-03-01 10:11       ` Jarkko Sakkinen
2018-02-28 19:58 ` [PATCH 3/5] tpm: migrate tpm2_probe() " Jarkko Sakkinen
2018-02-28 19:58   ` Jarkko Sakkinen
2018-02-28 19:58   ` Jarkko Sakkinen
2018-03-01 22:10   ` J Freyensee
2018-03-01 22:10     ` J Freyensee
2018-03-01 22:10     ` J Freyensee
2018-03-01 22:10     ` J Freyensee
2018-03-05 11:11     ` Jarkko Sakkinen
2018-03-05 11:11       ` Jarkko Sakkinen
2018-03-05 11:11       ` Jarkko Sakkinen
2018-03-05 11:11       ` Jarkko Sakkinen
2018-02-28 19:58 ` [PATCH 4/5] tpm: migrate tpm2_get_tpm_pt() " Jarkko Sakkinen
2018-02-28 19:58   ` Jarkko Sakkinen
2018-02-28 19:58   ` Jarkko Sakkinen
2018-02-28 19:58 ` [PATCH 5/5] tpm: migrate tpm2_get_random() " Jarkko Sakkinen
2018-02-28 19:58   ` Jarkko Sakkinen
2018-02-28 19:58   ` Jarkko Sakkinen
2018-03-01 10:21   ` Jarkko Sakkinen
2018-03-01 10:21     ` Jarkko Sakkinen
2018-03-01 10:21     ` Jarkko Sakkinen

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=20180305112334.GI25377@linux.intel.com \
    --to=jarkko.sakkinen@linux.intel.com \
    --cc=Alexander.Steffen@infineon.com \
    --cc=James.Bottomley@HansenPartnership.com \
    --cc=arnd@arndb.de \
    --cc=gregkh@linuxfoundation.org \
    --cc=jarkko.sakkine@linux.intel.com \
    --cc=jgg@ziepe.ca \
    --cc=keyrings@vger.kernel.org \
    --cc=linux-integrity@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=peterhuewe@gmx.de \
    --cc=stable@vger.kernel.org \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.