* Re: [PATCH] tpm: WARN_ONCE() -> pr_warn_once() in tpm_tis_status()
2021-02-02 15:33 [PATCH] tpm: WARN_ONCE() -> pr_warn_once() in tpm_tis_status() jarkko
@ 2021-02-02 15:43 ` Guenter Roeck
2021-02-02 22:05 ` Jarkko Sakkinen
2021-02-02 16:05 ` Stefan Berger
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Guenter Roeck @ 2021-02-02 15:43 UTC (permalink / raw)
To: jarkko, linux-integrity
Cc: stable, Peter Huewe, Jason Gunthorpe, Stefan Berger, Wang Hai
On 2/2/21 7:33 AM, jarkko@kernel.org wrote:
> From: Jarkko Sakkinen <jarkko@kernel.org>
>
> An unexpected status from TPM chip is not irrecovable failure of the
> kernel. It's only undesirable situation. Thus, change the WARN_ONCE
> instance inside tpm_tis_status() to pr_warn_once().
>
> In addition: print the status in the log message because it is actually
> useful information lacking from the existing log message.
>
> Suggested-by: Guenter Roeck <linux@roeck-us.net>
> Cc: stable@vger.kernel.org
> Fixes: 6f4f57f0b909 ("tpm: ibmvtpm: fix error return code in tpm_ibmvtpm_probe()")
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> ---
> drivers/char/tpm/tpm_tis_core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 431919d5f48a..21f67c6366cb 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -202,7 +202,7 @@ static u8 tpm_tis_status(struct tpm_chip *chip)
> * acquired. Usually because tpm_try_get_ops() hasn't
> * been called before doing a TPM operation.
> */
> - WARN_ONCE(1, "TPM returned invalid status\n");
> + pr_warn_once("TPM returned invalid status: 0x%x\n", status);
> return 0;
> }
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] tpm: WARN_ONCE() -> pr_warn_once() in tpm_tis_status()
2021-02-02 15:43 ` Guenter Roeck
@ 2021-02-02 22:05 ` Jarkko Sakkinen
0 siblings, 0 replies; 13+ messages in thread
From: Jarkko Sakkinen @ 2021-02-02 22:05 UTC (permalink / raw)
To: Guenter Roeck
Cc: linux-integrity, stable, Peter Huewe, Jason Gunthorpe,
Stefan Berger, Wang Hai
On Tue, Feb 02, 2021 at 07:43:04AM -0800, Guenter Roeck wrote:
> On 2/2/21 7:33 AM, jarkko@kernel.org wrote:
> > From: Jarkko Sakkinen <jarkko@kernel.org>
> >
> > An unexpected status from TPM chip is not irrecovable failure of the
> > kernel. It's only undesirable situation. Thus, change the WARN_ONCE
> > instance inside tpm_tis_status() to pr_warn_once().
> >
> > In addition: print the status in the log message because it is actually
> > useful information lacking from the existing log message.
> >
> > Suggested-by: Guenter Roeck <linux@roeck-us.net>
> > Cc: stable@vger.kernel.org
> > Fixes: 6f4f57f0b909 ("tpm: ibmvtpm: fix error return code in tpm_ibmvtpm_probe()")
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Thanks.
/Jarkko
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tpm: WARN_ONCE() -> pr_warn_once() in tpm_tis_status()
2021-02-02 15:33 [PATCH] tpm: WARN_ONCE() -> pr_warn_once() in tpm_tis_status() jarkko
2021-02-02 15:43 ` Guenter Roeck
@ 2021-02-02 16:05 ` Stefan Berger
2021-02-02 22:06 ` Jarkko Sakkinen
2021-02-02 16:46 ` Jason Gunthorpe
2021-02-02 17:26 ` Serge E. Hallyn
3 siblings, 1 reply; 13+ messages in thread
From: Stefan Berger @ 2021-02-02 16:05 UTC (permalink / raw)
To: jarkko, linux-integrity
Cc: Guenter Roeck, stable, Peter Huewe, Jason Gunthorpe, Wang Hai
On 2/2/21 10:33 AM, jarkko@kernel.org wrote:
> From: Jarkko Sakkinen <jarkko@kernel.org>
>
> An unexpected status from TPM chip is not irrecovable failure of the
> kernel. It's only undesirable situation. Thus, change the WARN_ONCE
> instance inside tpm_tis_status() to pr_warn_once().
>
> In addition: print the status in the log message because it is actually
> useful information lacking from the existing log message.
>
> Suggested-by: Guenter Roeck <linux@roeck-us.net>
> Cc: stable@vger.kernel.org
> Fixes: 6f4f57f0b909 ("tpm: ibmvtpm: fix error return code in tpm_ibmvtpm_probe()")
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
> drivers/char/tpm/tpm_tis_core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 431919d5f48a..21f67c6366cb 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -202,7 +202,7 @@ static u8 tpm_tis_status(struct tpm_chip *chip)
> * acquired. Usually because tpm_try_get_ops() hasn't
> * been called before doing a TPM operation.
> */
> - WARN_ONCE(1, "TPM returned invalid status\n");
> + pr_warn_once("TPM returned invalid status: 0x%x\n", status);
> return 0;
> }
>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] tpm: WARN_ONCE() -> pr_warn_once() in tpm_tis_status()
2021-02-02 16:05 ` Stefan Berger
@ 2021-02-02 22:06 ` Jarkko Sakkinen
0 siblings, 0 replies; 13+ messages in thread
From: Jarkko Sakkinen @ 2021-02-02 22:06 UTC (permalink / raw)
To: Stefan Berger
Cc: linux-integrity, Guenter Roeck, stable, Peter Huewe,
Jason Gunthorpe, Wang Hai
On Tue, Feb 02, 2021 at 11:05:36AM -0500, Stefan Berger wrote:
> On 2/2/21 10:33 AM, jarkko@kernel.org wrote:
> > From: Jarkko Sakkinen <jarkko@kernel.org>
> >
> > An unexpected status from TPM chip is not irrecovable failure of the
> > kernel. It's only undesirable situation. Thus, change the WARN_ONCE
> > instance inside tpm_tis_status() to pr_warn_once().
> >
> > In addition: print the status in the log message because it is actually
> > useful information lacking from the existing log message.
> >
> > Suggested-by: Guenter Roeck <linux@roeck-us.net>
> > Cc: stable@vger.kernel.org
> > Fixes: 6f4f57f0b909 ("tpm: ibmvtpm: fix error return code in tpm_ibmvtpm_probe()")
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > ---
> > drivers/char/tpm/tpm_tis_core.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> > index 431919d5f48a..21f67c6366cb 100644
> > --- a/drivers/char/tpm/tpm_tis_core.c
> > +++ b/drivers/char/tpm/tpm_tis_core.c
> > @@ -202,7 +202,7 @@ static u8 tpm_tis_status(struct tpm_chip *chip)
> > * acquired. Usually because tpm_try_get_ops() hasn't
> > * been called before doing a TPM operation.
> > */
> > - WARN_ONCE(1, "TPM returned invalid status\n");
> > + pr_warn_once("TPM returned invalid status: 0x%x\n", status);
> > return 0;
> > }
>
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Thank you.
/Jarkko
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tpm: WARN_ONCE() -> pr_warn_once() in tpm_tis_status()
2021-02-02 15:33 [PATCH] tpm: WARN_ONCE() -> pr_warn_once() in tpm_tis_status() jarkko
2021-02-02 15:43 ` Guenter Roeck
2021-02-02 16:05 ` Stefan Berger
@ 2021-02-02 16:46 ` Jason Gunthorpe
2021-02-02 22:25 ` Jarkko Sakkinen
2021-02-02 17:26 ` Serge E. Hallyn
3 siblings, 1 reply; 13+ messages in thread
From: Jason Gunthorpe @ 2021-02-02 16:46 UTC (permalink / raw)
To: jarkko
Cc: linux-integrity, Guenter Roeck, stable, Peter Huewe,
Stefan Berger, Wang Hai
On Tue, Feb 02, 2021 at 05:33:17PM +0200, jarkko@kernel.org wrote:
> From: Jarkko Sakkinen <jarkko@kernel.org>
>
> An unexpected status from TPM chip is not irrecovable failure of the
> kernel. It's only undesirable situation. Thus, change the WARN_ONCE
> instance inside tpm_tis_status() to pr_warn_once().
>
> In addition: print the status in the log message because it is actually
> useful information lacking from the existing log message.
>
> Suggested-by: Guenter Roeck <linux@roeck-us.net>
> Cc: stable@vger.kernel.org
> Fixes: 6f4f57f0b909 ("tpm: ibmvtpm: fix error return code in tpm_ibmvtpm_probe()")
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> drivers/char/tpm/tpm_tis_core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 431919d5f48a..21f67c6366cb 100644
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -202,7 +202,7 @@ static u8 tpm_tis_status(struct tpm_chip *chip)
> * acquired. Usually because tpm_try_get_ops() hasn't
> * been called before doing a TPM operation.
> */
> - WARN_ONCE(1, "TPM returned invalid status\n");
> + pr_warn_once("TPM returned invalid status: 0x%x\n", status);
Use dev_warn_once since we have a chip->dev here
Jason
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] tpm: WARN_ONCE() -> pr_warn_once() in tpm_tis_status()
2021-02-02 16:46 ` Jason Gunthorpe
@ 2021-02-02 22:25 ` Jarkko Sakkinen
0 siblings, 0 replies; 13+ messages in thread
From: Jarkko Sakkinen @ 2021-02-02 22:25 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: linux-integrity, Guenter Roeck, stable, Peter Huewe,
Stefan Berger, Wang Hai
On Tue, Feb 02, 2021 at 12:46:15PM -0400, Jason Gunthorpe wrote:
> On Tue, Feb 02, 2021 at 05:33:17PM +0200, jarkko@kernel.org wrote:
> > From: Jarkko Sakkinen <jarkko@kernel.org>
> >
> > An unexpected status from TPM chip is not irrecovable failure of the
> > kernel. It's only undesirable situation. Thus, change the WARN_ONCE
> > instance inside tpm_tis_status() to pr_warn_once().
> >
> > In addition: print the status in the log message because it is actually
> > useful information lacking from the existing log message.
> >
> > Suggested-by: Guenter Roeck <linux@roeck-us.net>
> > Cc: stable@vger.kernel.org
> > Fixes: 6f4f57f0b909 ("tpm: ibmvtpm: fix error return code in tpm_ibmvtpm_probe()")
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > drivers/char/tpm/tpm_tis_core.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> > index 431919d5f48a..21f67c6366cb 100644
> > +++ b/drivers/char/tpm/tpm_tis_core.c
> > @@ -202,7 +202,7 @@ static u8 tpm_tis_status(struct tpm_chip *chip)
> > * acquired. Usually because tpm_try_get_ops() hasn't
> > * been called before doing a TPM operation.
> > */
> > - WARN_ONCE(1, "TPM returned invalid status\n");
> > + pr_warn_once("TPM returned invalid status: 0x%x\n", status);
>
> Use dev_warn_once since we have a chip->dev here
>
> Jason
Right obviously that one is best to be preferred. I'll do that. Thanks.
/Jarkko
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tpm: WARN_ONCE() -> pr_warn_once() in tpm_tis_status()
2021-02-02 15:33 [PATCH] tpm: WARN_ONCE() -> pr_warn_once() in tpm_tis_status() jarkko
` (2 preceding siblings ...)
2021-02-02 16:46 ` Jason Gunthorpe
@ 2021-02-02 17:26 ` Serge E. Hallyn
2021-02-02 17:58 ` James Bottomley
3 siblings, 1 reply; 13+ messages in thread
From: Serge E. Hallyn @ 2021-02-02 17:26 UTC (permalink / raw)
To: jarkko
Cc: linux-integrity, Guenter Roeck, stable, Peter Huewe,
Jason Gunthorpe, Stefan Berger, Wang Hai
On Tue, Feb 02, 2021 at 05:33:17PM +0200, jarkko@kernel.org wrote:
> From: Jarkko Sakkinen <jarkko@kernel.org>
>
> An unexpected status from TPM chip is not irrecovable failure of the
> kernel. It's only undesirable situation. Thus, change the WARN_ONCE
> instance inside tpm_tis_status() to pr_warn_once().
>
> In addition: print the status in the log message because it is actually
> useful information lacking from the existing log message.
>
> Suggested-by: Guenter Roeck <linux@roeck-us.net>
> Cc: stable@vger.kernel.org
> Fixes: 6f4f57f0b909 ("tpm: ibmvtpm: fix error return code in tpm_ibmvtpm_probe()")
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
> drivers/char/tpm/tpm_tis_core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 431919d5f48a..21f67c6366cb 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -202,7 +202,7 @@ static u8 tpm_tis_status(struct tpm_chip *chip)
> * acquired. Usually because tpm_try_get_ops() hasn't
> * been called before doing a TPM operation.
> */
> - WARN_ONCE(1, "TPM returned invalid status\n");
> + pr_warn_once("TPM returned invalid status: 0x%x\n", status);
> return 0;
> }
Actually in this case I don't understand why _once, especially based on
the comment. Would ratelimited not be better? So we can see if it
happens repeatedly? Even better would be if we could see when it next
gave a valid status after an invalid one.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] tpm: WARN_ONCE() -> pr_warn_once() in tpm_tis_status()
2021-02-02 17:26 ` Serge E. Hallyn
@ 2021-02-02 17:58 ` James Bottomley
2021-02-02 22:27 ` Jarkko Sakkinen
0 siblings, 1 reply; 13+ messages in thread
From: James Bottomley @ 2021-02-02 17:58 UTC (permalink / raw)
To: Serge E. Hallyn, jarkko
Cc: linux-integrity, Guenter Roeck, stable, Peter Huewe,
Jason Gunthorpe, Stefan Berger, Wang Hai
On Tue, 2021-02-02 at 11:26 -0600, Serge E. Hallyn wrote:
> On Tue, Feb 02, 2021 at 05:33:17PM +0200, jarkko@kernel.org wrote:
> > From: Jarkko Sakkinen <jarkko@kernel.org>
> >
> > An unexpected status from TPM chip is not irrecovable failure of
> > the
> > kernel. It's only undesirable situation. Thus, change the WARN_ONCE
> > instance inside tpm_tis_status() to pr_warn_once().
> >
> > In addition: print the status in the log message because it is
> > actually
> > useful information lacking from the existing log message.
> >
> > Suggested-by: Guenter Roeck <linux@roeck-us.net>
> > Cc: stable@vger.kernel.org
> > Fixes: 6f4f57f0b909 ("tpm: ibmvtpm: fix error return code in
> > tpm_ibmvtpm_probe()")
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > ---
> > drivers/char/tpm/tpm_tis_core.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/char/tpm/tpm_tis_core.c
> > b/drivers/char/tpm/tpm_tis_core.c
> > index 431919d5f48a..21f67c6366cb 100644
> > --- a/drivers/char/tpm/tpm_tis_core.c
> > +++ b/drivers/char/tpm/tpm_tis_core.c
> > @@ -202,7 +202,7 @@ static u8 tpm_tis_status(struct tpm_chip *chip)
> > * acquired. Usually because tpm_try_get_ops() hasn't
> > * been called before doing a TPM operation.
> > */
> > - WARN_ONCE(1, "TPM returned invalid status\n");
> > + pr_warn_once("TPM returned invalid status: 0x%x\n",
> > status);
> > return 0;
> > }
>
> Actually in this case I don't understand why _once, especially based
> on the comment. Would ratelimited not be better? So we can see if
> it happens repeatedly? Even better would be if we could see when it
> next gave a valid status after an invalid one.
The reason was that we're trying to catch and kill paths to the status
where the locality is incorrect. If you do some operation that finds
an incorrect path the likelihood is you'll exercise it more than once,
but all we need to identify it is the call trace from a single access.
The symptom the user space process sees is a TPM timeout, but we still
have the in-kernel trace to tell us why.
James
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] tpm: WARN_ONCE() -> pr_warn_once() in tpm_tis_status()
2021-02-02 17:58 ` James Bottomley
@ 2021-02-02 22:27 ` Jarkko Sakkinen
2021-02-02 22:29 ` Jarkko Sakkinen
2021-02-02 23:00 ` James Bottomley
0 siblings, 2 replies; 13+ messages in thread
From: Jarkko Sakkinen @ 2021-02-02 22:27 UTC (permalink / raw)
To: James Bottomley
Cc: Serge E. Hallyn, linux-integrity, Guenter Roeck, stable,
Peter Huewe, Jason Gunthorpe, Stefan Berger, Wang Hai
On Tue, Feb 02, 2021 at 09:58:24AM -0800, James Bottomley wrote:
> On Tue, 2021-02-02 at 11:26 -0600, Serge E. Hallyn wrote:
> > On Tue, Feb 02, 2021 at 05:33:17PM +0200, jarkko@kernel.org wrote:
> > > From: Jarkko Sakkinen <jarkko@kernel.org>
> > >
> > > An unexpected status from TPM chip is not irrecovable failure of
> > > the
> > > kernel. It's only undesirable situation. Thus, change the WARN_ONCE
> > > instance inside tpm_tis_status() to pr_warn_once().
> > >
> > > In addition: print the status in the log message because it is
> > > actually
> > > useful information lacking from the existing log message.
> > >
> > > Suggested-by: Guenter Roeck <linux@roeck-us.net>
> > > Cc: stable@vger.kernel.org
> > > Fixes: 6f4f57f0b909 ("tpm: ibmvtpm: fix error return code in
> > > tpm_ibmvtpm_probe()")
> > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > > ---
> > > drivers/char/tpm/tpm_tis_core.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/char/tpm/tpm_tis_core.c
> > > b/drivers/char/tpm/tpm_tis_core.c
> > > index 431919d5f48a..21f67c6366cb 100644
> > > --- a/drivers/char/tpm/tpm_tis_core.c
> > > +++ b/drivers/char/tpm/tpm_tis_core.c
> > > @@ -202,7 +202,7 @@ static u8 tpm_tis_status(struct tpm_chip *chip)
> > > * acquired. Usually because tpm_try_get_ops() hasn't
> > > * been called before doing a TPM operation.
> > > */
> > > - WARN_ONCE(1, "TPM returned invalid status\n");
> > > + pr_warn_once("TPM returned invalid status: 0x%x\n",
> > > status);
> > > return 0;
> > > }
> >
> > Actually in this case I don't understand why _once, especially based
> > on the comment. Would ratelimited not be better? So we can see if
> > it happens repeatedly? Even better would be if we could see when it
> > next gave a valid status after an invalid one.
>
> The reason was that we're trying to catch and kill paths to the status
> where the locality is incorrect. If you do some operation that finds
> an incorrect path the likelihood is you'll exercise it more than once,
> but all we need to identify it is the call trace from a single access.
> The symptom the user space process sees is a TPM timeout, but we still
> have the in-kernel trace to tell us why.
I don't agree with this reasoning. This warn could spun off also from chip
not following TCG spec. The patch does provide the status code, which is
always useful information.
/Jarkko
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] tpm: WARN_ONCE() -> pr_warn_once() in tpm_tis_status()
2021-02-02 22:27 ` Jarkko Sakkinen
@ 2021-02-02 22:29 ` Jarkko Sakkinen
2021-02-02 23:00 ` James Bottomley
1 sibling, 0 replies; 13+ messages in thread
From: Jarkko Sakkinen @ 2021-02-02 22:29 UTC (permalink / raw)
To: James Bottomley
Cc: Serge E. Hallyn, linux-integrity, Guenter Roeck, stable,
Peter Huewe, Jason Gunthorpe, Stefan Berger, Wang Hai
On Wed, Feb 03, 2021 at 12:27:40AM +0200, Jarkko Sakkinen wrote:
> On Tue, Feb 02, 2021 at 09:58:24AM -0800, James Bottomley wrote:
> > On Tue, 2021-02-02 at 11:26 -0600, Serge E. Hallyn wrote:
> > > On Tue, Feb 02, 2021 at 05:33:17PM +0200, jarkko@kernel.org wrote:
> > > > From: Jarkko Sakkinen <jarkko@kernel.org>
> > > >
> > > > An unexpected status from TPM chip is not irrecovable failure of
> > > > the
> > > > kernel. It's only undesirable situation. Thus, change the WARN_ONCE
> > > > instance inside tpm_tis_status() to pr_warn_once().
> > > >
> > > > In addition: print the status in the log message because it is
> > > > actually
> > > > useful information lacking from the existing log message.
> > > >
> > > > Suggested-by: Guenter Roeck <linux@roeck-us.net>
> > > > Cc: stable@vger.kernel.org
> > > > Fixes: 6f4f57f0b909 ("tpm: ibmvtpm: fix error return code in
> > > > tpm_ibmvtpm_probe()")
> > > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > > > ---
> > > > drivers/char/tpm/tpm_tis_core.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/char/tpm/tpm_tis_core.c
> > > > b/drivers/char/tpm/tpm_tis_core.c
> > > > index 431919d5f48a..21f67c6366cb 100644
> > > > --- a/drivers/char/tpm/tpm_tis_core.c
> > > > +++ b/drivers/char/tpm/tpm_tis_core.c
> > > > @@ -202,7 +202,7 @@ static u8 tpm_tis_status(struct tpm_chip *chip)
> > > > * acquired. Usually because tpm_try_get_ops() hasn't
> > > > * been called before doing a TPM operation.
> > > > */
> > > > - WARN_ONCE(1, "TPM returned invalid status\n");
> > > > + pr_warn_once("TPM returned invalid status: 0x%x\n",
> > > > status);
> > > > return 0;
> > > > }
> > >
> > > Actually in this case I don't understand why _once, especially based
> > > on the comment. Would ratelimited not be better? So we can see if
> > > it happens repeatedly? Even better would be if we could see when it
> > > next gave a valid status after an invalid one.
> >
> > The reason was that we're trying to catch and kill paths to the status
> > where the locality is incorrect. If you do some operation that finds
> > an incorrect path the likelihood is you'll exercise it more than once,
> > but all we need to identify it is the call trace from a single access.
> > The symptom the user space process sees is a TPM timeout, but we still
> > have the in-kernel trace to tell us why.
>
> I don't agree with this reasoning. This warn could spun off also from chip
> not following TCG spec. The patch does provide the status code, which is
> always useful information.
I.e. WARN() implies usually that there is something wrong in the kernel
state risking its stability which *is not* case here. Thus, it's best to
make the status code visible, not the stack trace, and make rest of the
conclusions from that.
/Jarkko
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH] tpm: WARN_ONCE() -> pr_warn_once() in tpm_tis_status()
2021-02-02 22:27 ` Jarkko Sakkinen
2021-02-02 22:29 ` Jarkko Sakkinen
@ 2021-02-02 23:00 ` James Bottomley
2021-02-03 0:01 ` Jarkko Sakkinen
1 sibling, 1 reply; 13+ messages in thread
From: James Bottomley @ 2021-02-02 23:00 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Serge E. Hallyn, linux-integrity, Guenter Roeck, stable,
Peter Huewe, Jason Gunthorpe, Stefan Berger, Wang Hai
On Wed, 2021-02-03 at 00:27 +0200, Jarkko Sakkinen wrote:
> On Tue, Feb 02, 2021 at 09:58:24AM -0800, James Bottomley wrote:
> > On Tue, 2021-02-02 at 11:26 -0600, Serge E. Hallyn wrote:
[...]
> > >
> > > Actually in this case I don't understand why _once, especially
> > > based on the comment. Would ratelimited not be better? So we
> > > can see if it happens repeatedly? Even better would be if we
> > > could see when it next gave a valid status after an invalid one.
> >
> > The reason was that we're trying to catch and kill paths to the
> > status where the locality is incorrect. If you do some operation
> > that finds an incorrect path the likelihood is you'll exercise it
> > more than once, but all we need to identify it is the call trace
> > from a single access. The symptom the user space process sees is a
> > TPM timeout, but we still have the in-kernel trace to tell us why.
>
> I don't agree with this reasoning. This warn could spun off also from
> chip not following TCG spec.
If it doesn't follow this basic part of the spec, the chip is unusable
by us anyway because we need the status to proceed with command
handling.
> The patch does provide the status code, which is always useful
> information.
In the wrong locality that will be bus not connected, so likely 0xff.
The most useful thing to know is what path triggered the condition
because the most likely cause is coding error.
James
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] tpm: WARN_ONCE() -> pr_warn_once() in tpm_tis_status()
2021-02-02 23:00 ` James Bottomley
@ 2021-02-03 0:01 ` Jarkko Sakkinen
0 siblings, 0 replies; 13+ messages in thread
From: Jarkko Sakkinen @ 2021-02-03 0:01 UTC (permalink / raw)
To: James Bottomley
Cc: Serge E. Hallyn, linux-integrity, Guenter Roeck, stable,
Peter Huewe, Jason Gunthorpe, Stefan Berger, Wang Hai
On Tue, Feb 02, 2021 at 03:00:34PM -0800, James Bottomley wrote:
> On Wed, 2021-02-03 at 00:27 +0200, Jarkko Sakkinen wrote:
> > On Tue, Feb 02, 2021 at 09:58:24AM -0800, James Bottomley wrote:
> > > On Tue, 2021-02-02 at 11:26 -0600, Serge E. Hallyn wrote:
> [...]
> > > >
> > > > Actually in this case I don't understand why _once, especially
> > > > based on the comment. Would ratelimited not be better? So we
> > > > can see if it happens repeatedly? Even better would be if we
> > > > could see when it next gave a valid status after an invalid one.
> > >
> > > The reason was that we're trying to catch and kill paths to the
> > > status where the locality is incorrect. If you do some operation
> > > that finds an incorrect path the likelihood is you'll exercise it
> > > more than once, but all we need to identify it is the call trace
> > > from a single access. The symptom the user space process sees is a
> > > TPM timeout, but we still have the in-kernel trace to tell us why.
> >
> > I don't agree with this reasoning. This warn could spun off also from
> > chip not following TCG spec.
>
> If it doesn't follow this basic part of the spec, the chip is unusable
> by us anyway because we need the status to proceed with command
> handling.
>
> > The patch does provide the status code, which is always useful
> > information.
>
> In the wrong locality that will be bus not connected, so likely 0xff.
> The most useful thing to know is what path triggered the condition
> because the most likely cause is coding error.
>
> James
I tend to agree for now. Let's focus on collecting the fixes. Thanks.
/Jarkko
^ permalink raw reply [flat|nested] 13+ messages in thread