[PATCH] tpm: fix incorrect success returns from tpm_try_transmit()

[PATCH] tpm: fix incorrect success returns from tpm_try_transmit()

[James Bottomley]

Ever since 627448e85c766 "tpm: separate cmd_ready/go_idle from
runtime_pm" we have been returning success from tpm_try_transmit()
even if an error occurred.  The reason is that the introduction of rc
= tpm_go_idle() at the end of processing overwrites the value of rc if
it contains an error code (mostly with success).  Fix this by writing
the return to a new variable rc1 instead.

Fixes: 627448e85c766 "tpm: separate cmd_ready/go_idle from runtime_pm"
Cc: stable@vger.kernel.org
Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>

---

Note: the goto out looks fishy as well.  The only go_idle implementor
is tpm_crb and that can return a timeout as -ETIME, so it looks like it
would then loop forever

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 129f640424b7..ac7ebab6140c 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -432,7 +432,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
 				unsigned int flags)
 {
 	struct tpm_output_header *header = (void *)buf;
-	int rc;
+	int rc, rc1;
 	ssize_t len = 0;
 	u32 count, ordinal;
 	unsigned long stop;
@@ -547,8 +547,8 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
 		dev_err(&chip->dev, "tpm2_commit_space: error %d\n", rc);
 
 out:
-	rc = tpm_go_idle(chip, flags);
-	if (rc)
+	rc1 = tpm_go_idle(chip, flags);
+	if (rc1)
 		goto out;
 
 	if (need_locality)

Re: [PATCH] tpm: fix incorrect success returns from tpm_try_transmit()

[Jarkko Sakkinen]

On Mon, Dec 31, 2018 at 10:27:31AM -0800, James Bottomley wrote:
> Ever since 627448e85c766 "tpm: separate cmd_ready/go_idle from
> runtime_pm" we have been returning success from tpm_try_transmit()
> even if an error occurred.  The reason is that the introduction of rc
> = tpm_go_idle() at the end of processing overwrites the value of rc if
> it contains an error code (mostly with success).  Fix this by writing
> the return to a new variable rc1 instead.
> 
> Fixes: 627448e85c766 "tpm: separate cmd_ready/go_idle from runtime_pm"
> Cc: stable@vger.kernel.org
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> 
> ---
> 
> Note: the goto out looks fishy as well.  The only go_idle implementor
> is tpm_crb and that can return a timeout as -ETIME, so it looks like it
> would then loop forever
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 129f640424b7..ac7ebab6140c 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -432,7 +432,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
>  				unsigned int flags)
>  {
>  	struct tpm_output_header *header = (void *)buf;
> -	int rc;
> +	int rc, rc1;
>  	ssize_t len = 0;
>  	u32 count, ordinal;
>  	unsigned long stop;
> @@ -547,8 +547,8 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
>  		dev_err(&chip->dev, "tpm2_commit_space: error %d\n", rc);
>  
>  out:
> -	rc = tpm_go_idle(chip, flags);
> -	if (rc)
> +	rc1 = tpm_go_idle(chip, flags);
> +	if (rc1)
>  		goto out;
>  
>  	if (need_locality)

Thanks James and sorry for latency (holiday season). Just a small
suggestion. I would just:

if (tpm_go_idle(chip, flags))
	goto out;

What do you think?

/Jarkko

RE: [PATCH] tpm: fix incorrect success returns from tpm_try_transmit()

[Winkler, Tomas]

> 
> On Mon, Dec 31, 2018 at 10:27:31AM -0800, James Bottomley wrote:
> > Ever since 627448e85c766 "tpm: separate cmd_ready/go_idle from
> > runtime_pm" we have been returning success from tpm_try_transmit()
> > even if an error occurred.  The reason is that the introduction of rc
> > = tpm_go_idle() at the end of processing overwrites the value of rc if
> > it contains an error code (mostly with success).  Fix this by writing
> > the return to a new variable rc1 instead.
> >
> > Fixes: 627448e85c766 "tpm: separate cmd_ready/go_idle from runtime_pm"
> > Cc: stable@vger.kernel.org
> > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> >
> > ---
> >
> > Note: the goto out looks fishy as well.  The only go_idle implementor
> > is tpm_crb and that can return a timeout as -ETIME, so it looks like
> > it would then loop forever
> >
> > diff --git a/drivers/char/tpm/tpm-interface.c
> > b/drivers/char/tpm/tpm-interface.c
> > index 129f640424b7..ac7ebab6140c 100644
> > --- a/drivers/char/tpm/tpm-interface.c
> > +++ b/drivers/char/tpm/tpm-interface.c
> > @@ -432,7 +432,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip
> *chip,
> >  				unsigned int flags)
> >  {
> >  	struct tpm_output_header *header = (void *)buf;
> > -	int rc;
> > +	int rc, rc1;
> >  	ssize_t len = 0;
> >  	u32 count, ordinal;
> >  	unsigned long stop;
> > @@ -547,8 +547,8 @@ static ssize_t tpm_try_transmit(struct tpm_chip
> *chip,
> >  		dev_err(&chip->dev, "tpm2_commit_space: error %d\n", rc);
> >
> >  out:
> > -	rc = tpm_go_idle(chip, flags);
> > -	if (rc)
> > +	rc1 = tpm_go_idle(chip, flags);
> > +	if (rc1)
> >  		goto out;
> >
> >  	if (need_locality)
> 
> Thanks James and sorry for latency (holiday season). Just a small suggestion. I
> would just:
> 
> if (tpm_go_idle(chip, flags))
> 	goto out;
> 
> What do you think?


This is wrong as well because of the jump to 'out'.
I've introduced this bug in version 4 of the patch, when the wrappers were added, before the jump to 'out' was correct.
If it's okay with you I will post my version of the fix.
Thanks
Tomas

Re: [PATCH] tpm: fix incorrect success returns from tpm_try_transmit()

[Jarkko Sakkinen]

On Thu, Jan 03, 2019 at 01:37:09PM +0000, Winkler, Tomas wrote:
> > 
> > On Mon, Dec 31, 2018 at 10:27:31AM -0800, James Bottomley wrote:
> > > Ever since 627448e85c766 "tpm: separate cmd_ready/go_idle from
> > > runtime_pm" we have been returning success from tpm_try_transmit()
> > > even if an error occurred.  The reason is that the introduction of rc
> > > = tpm_go_idle() at the end of processing overwrites the value of rc if
> > > it contains an error code (mostly with success).  Fix this by writing
> > > the return to a new variable rc1 instead.
> > >
> > > Fixes: 627448e85c766 "tpm: separate cmd_ready/go_idle from runtime_pm"
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> > >
> > > ---
> > >
> > > Note: the goto out looks fishy as well.  The only go_idle implementor
> > > is tpm_crb and that can return a timeout as -ETIME, so it looks like
> > > it would then loop forever
> > >
> > > diff --git a/drivers/char/tpm/tpm-interface.c
> > > b/drivers/char/tpm/tpm-interface.c
> > > index 129f640424b7..ac7ebab6140c 100644
> > > --- a/drivers/char/tpm/tpm-interface.c
> > > +++ b/drivers/char/tpm/tpm-interface.c
> > > @@ -432,7 +432,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip
> > *chip,
> > >  				unsigned int flags)
> > >  {
> > >  	struct tpm_output_header *header = (void *)buf;
> > > -	int rc;
> > > +	int rc, rc1;
> > >  	ssize_t len = 0;
> > >  	u32 count, ordinal;
> > >  	unsigned long stop;
> > > @@ -547,8 +547,8 @@ static ssize_t tpm_try_transmit(struct tpm_chip
> > *chip,
> > >  		dev_err(&chip->dev, "tpm2_commit_space: error %d\n", rc);
> > >
> > >  out:
> > > -	rc = tpm_go_idle(chip, flags);
> > > -	if (rc)
> > > +	rc1 = tpm_go_idle(chip, flags);
> > > +	if (rc1)
> > >  		goto out;
> > >
> > >  	if (need_locality)
> > 
> > Thanks James and sorry for latency (holiday season). Just a small suggestion. I
> > would just:
> > 
> > if (tpm_go_idle(chip, flags))
> > 	goto out;
> > 
> > What do you think?
> 
> 
> This is wrong as well because of the jump to 'out'.
> I've introduced this bug in version 4 of the patch, when the wrappers were added, before the jump to 'out' was correct.
> If it's okay with you I will post my version of the fix.
> Thanks
> Tomas

Works for me.

/Jarkko

Re: [PATCH] tpm: fix incorrect success returns from tpm_try_transmit()

[James Bottomley]

On Thu, 2019-01-03 at 14:59 +0200, Jarkko Sakkinen wrote:
> On Mon, Dec 31, 2018 at 10:27:31AM -0800, James Bottomley wrote:
> > Ever since 627448e85c766 "tpm: separate cmd_ready/go_idle from
> > runtime_pm" we have been returning success from tpm_try_transmit()
> > even if an error occurred.  The reason is that the introduction of
> > rc
> > = tpm_go_idle() at the end of processing overwrites the value of rc
> > if
> > it contains an error code (mostly with success).  Fix this by
> > writing
> > the return to a new variable rc1 instead.
> > 
> > Fixes: 627448e85c766 "tpm: separate cmd_ready/go_idle from
> > runtime_pm"
> > Cc: stable@vger.kernel.org
> > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.c
> > om>
> > 
> > ---
> > 
> > Note: the goto out looks fishy as well.  The only go_idle
> > implementor
> > is tpm_crb and that can return a timeout as -ETIME, so it looks
> > like it
> > would then loop forever
> > 
> > diff --git a/drivers/char/tpm/tpm-interface.c
> > b/drivers/char/tpm/tpm-interface.c
> > index 129f640424b7..ac7ebab6140c 100644
> > --- a/drivers/char/tpm/tpm-interface.c
> > +++ b/drivers/char/tpm/tpm-interface.c
> > @@ -432,7 +432,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip
> > *chip,
> >  				unsigned int flags)
> >  {
> >  	struct tpm_output_header *header = (void *)buf;
> > -	int rc;
> > +	int rc, rc1;
> >  	ssize_t len = 0;
> >  	u32 count, ordinal;
> >  	unsigned long stop;
> > @@ -547,8 +547,8 @@ static ssize_t tpm_try_transmit(struct tpm_chip
> > *chip,
> >  		dev_err(&chip->dev, "tpm2_commit_space: error
> > %d\n", rc);
> >  
> >  out:
> > -	rc = tpm_go_idle(chip, flags);
> > -	if (rc)
> > +	rc1 = tpm_go_idle(chip, flags);
> > +	if (rc1)
> >  		goto out;
> >  
> >  	if (need_locality)
> 
> Thanks James and sorry for latency (holiday season). Just a small
> suggestion. I would just:
> 
> if (tpm_go_idle(chip, flags))
> 	goto out;
> 
> What do you think?

That it doesn't solve the loop forever with no warning problem.  If
anything, I think the correct thing is probably

	rc1 = tpm_go_idle(chip, flags);
	if (rc1)
  		dev_err(&chip->dev, "go idle failed with %d\n", rc1);

so we log the problem and move on.  If it is a timeout, it will likely
show up on the next TPM operation.  Since this is the only caller of
tpm_go_idle(), I think all looping should be done inside that function,
but we should probably wait for Tomas to comment since he wrote it.

James

RE: [PATCH] tpm: fix incorrect success returns from tpm_try_transmit()

[Winkler, Tomas]

> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com]
> Sent: Thursday, January 03, 2019 17:24
> To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Cc: Winkler, Tomas <tomas.winkler@intel.com>; linux-
> integrity@vger.kernel.org
> Subject: Re: [PATCH] tpm: fix incorrect success returns from tpm_try_transmit()
> 
> On Thu, 2019-01-03 at 14:59 +0200, Jarkko Sakkinen wrote:
> > On Mon, Dec 31, 2018 at 10:27:31AM -0800, James Bottomley wrote:
> > > Ever since 627448e85c766 "tpm: separate cmd_ready/go_idle from
> > > runtime_pm" we have been returning success from tpm_try_transmit()
> > > even if an error occurred.  The reason is that the introduction of
> > > rc = tpm_go_idle() at the end of processing overwrites the value of
> > > rc if it contains an error code (mostly with success).  Fix this by
> > > writing the return to a new variable rc1 instead.
> > >
> > > Fixes: 627448e85c766 "tpm: separate cmd_ready/go_idle from
> > > runtime_pm"
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.c
> > > om>
> > >
> > > ---
> > >
> > > Note: the goto out looks fishy as well.  The only go_idle
> > > implementor is tpm_crb and that can return a timeout as -ETIME, so
> > > it looks like it would then loop forever
> > >
> > > diff --git a/drivers/char/tpm/tpm-interface.c
> > > b/drivers/char/tpm/tpm-interface.c
> > > index 129f640424b7..ac7ebab6140c 100644
> > > --- a/drivers/char/tpm/tpm-interface.c
> > > +++ b/drivers/char/tpm/tpm-interface.c
> > > @@ -432,7 +432,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip
> > > *chip,
> > >  				unsigned int flags)
> > >  {
> > >  	struct tpm_output_header *header = (void *)buf;
> > > -	int rc;
> > > +	int rc, rc1;
> > >  	ssize_t len = 0;
> > >  	u32 count, ordinal;
> > >  	unsigned long stop;
> > > @@ -547,8 +547,8 @@ static ssize_t tpm_try_transmit(struct tpm_chip
> > > *chip,
> > >  		dev_err(&chip->dev, "tpm2_commit_space: error %d\n", rc);
> > >
> > >  out:
> > > -	rc = tpm_go_idle(chip, flags);
> > > -	if (rc)
> > > +	rc1 = tpm_go_idle(chip, flags);
> > > +	if (rc1)
> > >  		goto out;
> > >
> > >  	if (need_locality)
> >
> > Thanks James and sorry for latency (holiday season). Just a small
> > suggestion. I would just:
> >
> > if (tpm_go_idle(chip, flags))
> > 	goto out;
> >
> > What do you think?
> 
> That it doesn't solve the loop forever with no warning problem.  If anything, I
> think the correct thing is probably
> 
> 	rc1 = tpm_go_idle(chip, flags);
> 	if (rc1)
>   		dev_err(&chip->dev, "go idle failed with %d\n", rc1);
> 
> so we log the problem and move on.  If it is a timeout, it will likely show up on
> the next TPM operation.  Since this is the only caller of tpm_go_idle(), I think all
> looping should be done inside that function, but we should probably wait for
> Tomas to comment since he wrote it.
> 

We've already fixed it, I forgot myself , we were drinking too much :)
https://patchwork.kernel.org/patch/10643565/
Not sure why it was dropped.

Thanks
Tomas

Re: [PATCH] tpm: fix incorrect success returns from tpm_try_transmit()

[James Bottomley]

On Thu, 2019-01-03 at 15:34 +0000, Winkler, Tomas wrote:
> > -----Original Message-----
> > From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com
> > ]
> > Sent: Thursday, January 03, 2019 17:24
> > To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > Cc: Winkler, Tomas <tomas.winkler@intel.com>; linux-
> > integrity@vger.kernel.org
> > Subject: Re: [PATCH] tpm: fix incorrect success returns from
> > tpm_try_transmit()
> > 
> > On Thu, 2019-01-03 at 14:59 +0200, Jarkko Sakkinen wrote:
> > > On Mon, Dec 31, 2018 at 10:27:31AM -0800, James Bottomley wrote:
> > > > Ever since 627448e85c766 "tpm: separate cmd_ready/go_idle from
> > > > runtime_pm" we have been returning success from
> > > > tpm_try_transmit() even if an error occurred.  The reason is
> > > > that the introduction of rc = tpm_go_idle() at the end of
> > > > processing overwrites the value of rc if it contains an error
> > > > code (mostly with success).  Fix this by writing the return to
> > > > a new variable rc1 instead.
> > > > 
> > > > Fixes: 627448e85c766 "tpm: separate cmd_ready/go_idle from
> > > > runtime_pm"
> > > > Cc: stable@vger.kernel.org
> > > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnersh
> > > > ip.c
> > > > om>
> > > > 
> > > > ---
> > > > 
> > > > Note: the goto out looks fishy as well.  The only go_idle
> > > > implementor is tpm_crb and that can return a timeout as -ETIME,
> > > > so it looks like it would then loop forever
> > > > 
> > > > diff --git a/drivers/char/tpm/tpm-interface.c
> > > > b/drivers/char/tpm/tpm-interface.c
> > > > index 129f640424b7..ac7ebab6140c 100644
> > > > --- a/drivers/char/tpm/tpm-interface.c
> > > > +++ b/drivers/char/tpm/tpm-interface.c
> > > > @@ -432,7 +432,7 @@ static ssize_t tpm_try_transmit(struct
> > > > tpm_chip
> > > > *chip,
> > > >  				unsigned int flags)
> > > >  {
> > > >  	struct tpm_output_header *header = (void *)buf;
> > > > -	int rc;
> > > > +	int rc, rc1;
> > > >  	ssize_t len = 0;
> > > >  	u32 count, ordinal;
> > > >  	unsigned long stop;
> > > > @@ -547,8 +547,8 @@ static ssize_t tpm_try_transmit(struct
> > > > tpm_chip
> > > > *chip,
> > > >  		dev_err(&chip->dev, "tpm2_commit_space: error
> > > > %d\n", rc);
> > > > 
> > > >  out:
> > > > -	rc = tpm_go_idle(chip, flags);
> > > > -	if (rc)
> > > > +	rc1 = tpm_go_idle(chip, flags);
> > > > +	if (rc1)
> > > >  		goto out;
> > > > 
> > > >  	if (need_locality)
> > > 
> > > Thanks James and sorry for latency (holiday season). Just a small
> > > suggestion. I would just:
> > > 
> > > if (tpm_go_idle(chip, flags))
> > > 	goto out;
> > > 
> > > What do you think?
> > 
> > That it doesn't solve the loop forever with no warning problem.  If
> > anything, I think the correct thing is probably
> > 
> > 	rc1 = tpm_go_idle(chip, flags);
> > 	if (rc1)
> >   		dev_err(&chip->dev, "go idle failed with %d\n",
> > rc1);
> > 
> > so we log the problem and move on.  If it is a timeout, it will
> > likely show up on the next TPM operation.  Since this is the only
> > caller of tpm_go_idle(), I think all looping should be done inside
> > that function, but we should probably wait for Tomas to comment
> > since he wrote it.
> > 
> 
> We've already fixed it, I forgot myself , we were drinking too much
> :)
> https://patchwork.kernel.org/patch/10643565/
> Not sure why it was dropped.

Taking the trouble to gather error returns and then ignoring them is
not a good practice (it's actually been the bane of filesystems for a
while).  If you want to do it this way, tpm_go_idle() needs to be a
void function that emits an error message for every problem condition.

James

Re: [PATCH] tpm: fix incorrect success returns from tpm_try_transmit()

[Jarkko Sakkinen]

On Thu, Jan 03, 2019 at 03:34:24PM +0000, Winkler, Tomas wrote:
> We've already fixed it, I forgot myself , we were drinking too much :)
> https://patchwork.kernel.org/patch/10643565/
> Not sure why it was dropped.
> 
> Thanks
> Tomas

LOL what. Clearly I'm coming from holidays. This fix is in my tree
and it was part of my last PR to James (for 5.0).

And James' patch applies neither my master and next braches so I
suppose he is using some old tree.

/Jarkko

Re: [PATCH] tpm: fix incorrect success returns from tpm_try_transmit()

[Jarkko Sakkinen]

On Thu, Jan 03, 2019 at 08:17:18AM -0800, James Bottomley wrote:
> On Thu, 2019-01-03 at 15:34 +0000, Winkler, Tomas wrote:
> > > -----Original Message-----
> > > From: James Bottomley [mailto:James.Bottomley@HansenPartnership.com
> > > ]
> > > Sent: Thursday, January 03, 2019 17:24
> > > To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > Cc: Winkler, Tomas <tomas.winkler@intel.com>; linux-
> > > integrity@vger.kernel.org
> > > Subject: Re: [PATCH] tpm: fix incorrect success returns from
> > > tpm_try_transmit()
> > > 
> > > On Thu, 2019-01-03 at 14:59 +0200, Jarkko Sakkinen wrote:
> > > > On Mon, Dec 31, 2018 at 10:27:31AM -0800, James Bottomley wrote:
> > > > > Ever since 627448e85c766 "tpm: separate cmd_ready/go_idle from
> > > > > runtime_pm" we have been returning success from
> > > > > tpm_try_transmit() even if an error occurred.  The reason is
> > > > > that the introduction of rc = tpm_go_idle() at the end of
> > > > > processing overwrites the value of rc if it contains an error
> > > > > code (mostly with success).  Fix this by writing the return to
> > > > > a new variable rc1 instead.
> > > > > 
> > > > > Fixes: 627448e85c766 "tpm: separate cmd_ready/go_idle from
> > > > > runtime_pm"
> > > > > Cc: stable@vger.kernel.org
> > > > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnersh
> > > > > ip.c
> > > > > om>
> > > > > 
> > > > > ---
> > > > > 
> > > > > Note: the goto out looks fishy as well.  The only go_idle
> > > > > implementor is tpm_crb and that can return a timeout as -ETIME,
> > > > > so it looks like it would then loop forever
> > > > > 
> > > > > diff --git a/drivers/char/tpm/tpm-interface.c
> > > > > b/drivers/char/tpm/tpm-interface.c
> > > > > index 129f640424b7..ac7ebab6140c 100644
> > > > > --- a/drivers/char/tpm/tpm-interface.c
> > > > > +++ b/drivers/char/tpm/tpm-interface.c
> > > > > @@ -432,7 +432,7 @@ static ssize_t tpm_try_transmit(struct
> > > > > tpm_chip
> > > > > *chip,
> > > > >  				unsigned int flags)
> > > > >  {
> > > > >  	struct tpm_output_header *header = (void *)buf;
> > > > > -	int rc;
> > > > > +	int rc, rc1;
> > > > >  	ssize_t len = 0;
> > > > >  	u32 count, ordinal;
> > > > >  	unsigned long stop;
> > > > > @@ -547,8 +547,8 @@ static ssize_t tpm_try_transmit(struct
> > > > > tpm_chip
> > > > > *chip,
> > > > >  		dev_err(&chip->dev, "tpm2_commit_space: error
> > > > > %d\n", rc);
> > > > > 
> > > > >  out:
> > > > > -	rc = tpm_go_idle(chip, flags);
> > > > > -	if (rc)
> > > > > +	rc1 = tpm_go_idle(chip, flags);
> > > > > +	if (rc1)
> > > > >  		goto out;
> > > > > 
> > > > >  	if (need_locality)
> > > > 
> > > > Thanks James and sorry for latency (holiday season). Just a small
> > > > suggestion. I would just:
> > > > 
> > > > if (tpm_go_idle(chip, flags))
> > > > 	goto out;
> > > > 
> > > > What do you think?
> > > 
> > > That it doesn't solve the loop forever with no warning problem.  If
> > > anything, I think the correct thing is probably
> > > 
> > > 	rc1 = tpm_go_idle(chip, flags);
> > > 	if (rc1)
> > >   		dev_err(&chip->dev, "go idle failed with %d\n",
> > > rc1);
> > > 
> > > so we log the problem and move on.  If it is a timeout, it will
> > > likely show up on the next TPM operation.  Since this is the only
> > > caller of tpm_go_idle(), I think all looping should be done inside
> > > that function, but we should probably wait for Tomas to comment
> > > since he wrote it.
> > > 
> > 
> > We've already fixed it, I forgot myself , we were drinking too much
> > :)
> > https://patchwork.kernel.org/patch/10643565/
> > Not sure why it was dropped.
> 
> Taking the trouble to gather error returns and then ignoring them is
> not a good practice (it's actually been the bane of filesystems for a
> while).  If you want to do it this way, tpm_go_idle() needs to be a
> void function that emits an error message for every problem condition.

I'm happy to take a patch that adds logging in.

/Jarkko