mbox series

[v11,00/16] Remove nested TPM operations

Message ID 20190205224723.19671-1-jarkko.sakkinen@linux.intel.com (mailing list archive)
Headers show
Series Remove nested TPM operations | expand

Message

Jarkko Sakkinen Feb. 5, 2019, 10:47 p.m. UTC 
Make the changes necessary to detach TPM space code and TPM activation
code out of the tpm_transmit() flow because of both of these can cause
nested tpm_transmit() calls. The nesteds calls make the whole flow hard
to maintain, and thus, it is better to just fix things now before this
turns into a bigger mess.

v11:
* Drop the patch that tries to flush TPM space on system. Not a proper
  fallback + out of scope for this patch set.

v10:
* Use void pointers to avoid unnecessary casts in functions paramaters
  where it makes sense.

v9:
* Fixed again tpm_try_get_ops().
* Added missing reviewed-by's.

v8:
* Re-add the check for ret < 0 after calling tpm_try_transmit() that
  was dropped by mistake while moving code.
* Fix error fallback for tpm_try_get_ops() when tpm_chip_start()
  fails.

v7:
*  Reorganize series so that more trivial and self-contained changes are
   in the head.

v6:
* When tpm_validate_commmand() was moved to tpm2-space.c, the struct for
  the TPM header was incorrectly declared as struct tpm_input_header.
* Fix return value in tpm_validate_command().

v5:
* Add the missing rev's from Stefan Berger.

v4:
* Return 0 from pcrs_show() when tpm1_pcr_read() fails.
* Fix error handling flow in tpm_try_transmit().
* Replace struct tpm_input_header and struct tpm_output_header with
  struct tpm_header.

v3:
* Encapsulate power gating code to tpm_chip_start() and tpm_chip_stop().
* Move TPM power gating code and locking to tpm_try_get_ops() and
  tpm_put_ops().
* Call power gating code directly in tpm_chip_register() and
  tpm2_del_space().

v2:
* Print tpm2_commit_space() error inside tpm2_commit_space()
* Error code was not printed when recv() callback failed. It is
  fixed in this version.
* Added a patch that removes @space from tpm_transmit().
* Fixed a regression in earlier series. Forgot to amend the change
  from the staging area that renames NESTED to UNLOCKED in tpm2-space.c.

Jarkko Sakkinen (16):
  tpm: use tpm_buf in tpm_transmit_cmd() as the IO parameter
  tpm: fix invalid return value in pubek_show()
  tpm: return 0 from pcrs_show() when tpm1_pcr_read() fails
  tpm: print tpm2_commit_space() error inside tpm2_commit_space()
  tpm: declare struct tpm_header
  tpm: access command header through struct in tpm_try_transmit()
  tpm: encapsulate tpm_dev_transmit()
  tpm: clean up tpm_try_transmit() error handling flow
  tpm: move tpm_validate_commmand() to tpm2-space.c
  tpm: move TPM space code out of tpm_transmit()
  tpm: remove @space from tpm_transmit()
  tpm: use tpm_try_get_ops() in tpm-sysfs.c.
  tpm: remove TPM_TRANSMIT_UNLOCKED flag
  tpm: introduce tpm_chip_start() and tpm_chip_stop()
  tpm: take TPM chip power gating out of tpm_transmit()
  tpm: remove @flags from tpm_transmit()

 drivers/char/tpm/tpm-chip.c       | 109 ++++++++++++
 drivers/char/tpm/tpm-dev-common.c |  44 ++++-
 drivers/char/tpm/tpm-interface.c  | 264 ++++++------------------------
 drivers/char/tpm/tpm-sysfs.c      | 138 ++++++++++------
 drivers/char/tpm/tpm.h            |  64 +++-----
 drivers/char/tpm/tpm1-cmd.c       |  28 +---
 drivers/char/tpm/tpm2-cmd.c       |  72 +++-----
 drivers/char/tpm/tpm2-space.c     |  91 +++++++---
 drivers/char/tpm/tpm_i2c_atmel.c  |   5 +-
 drivers/char/tpm/tpm_vtpm_proxy.c |  12 +-
 drivers/char/tpm/xen-tpmfront.c   |   2 +-
 11 files changed, 408 insertions(+), 421 deletions(-)

Comments

Jarkko Sakkinen Feb. 6, 2019, 12:06 p.m. UTC | #1 
On Wed, Feb 06, 2019 at 12:47:07AM +0200, Jarkko Sakkinen wrote:
> Make the changes necessary to detach TPM space code and TPM activation
> code out of the tpm_transmit() flow because of both of these can cause
> nested tpm_transmit() calls. The nesteds calls make the whole flow hard
> to maintain, and thus, it is better to just fix things now before this
> turns into a bigger mess.
> 
> v11:
> * Drop the patch that tries to flush TPM space on system. Not a proper
>   fallback + out of scope for this patch set.
> 
> v10:
> * Use void pointers to avoid unnecessary casts in functions paramaters
>   where it makes sense.
> 
> v9:
> * Fixed again tpm_try_get_ops().
> * Added missing reviewed-by's.
> 
> v8:
> * Re-add the check for ret < 0 after calling tpm_try_transmit() that
>   was dropped by mistake while moving code.
> * Fix error fallback for tpm_try_get_ops() when tpm_chip_start()
>   fails.
> 
> v7:
> *  Reorganize series so that more trivial and self-contained changes are
>    in the head.
> 
> v6:
> * When tpm_validate_commmand() was moved to tpm2-space.c, the struct for
>   the TPM header was incorrectly declared as struct tpm_input_header.
> * Fix return value in tpm_validate_command().
> 
> v5:
> * Add the missing rev's from Stefan Berger.
> 
> v4:
> * Return 0 from pcrs_show() when tpm1_pcr_read() fails.
> * Fix error handling flow in tpm_try_transmit().
> * Replace struct tpm_input_header and struct tpm_output_header with
>   struct tpm_header.
> 
> v3:
> * Encapsulate power gating code to tpm_chip_start() and tpm_chip_stop().
> * Move TPM power gating code and locking to tpm_try_get_ops() and
>   tpm_put_ops().
> * Call power gating code directly in tpm_chip_register() and
>   tpm2_del_space().
> 
> v2:
> * Print tpm2_commit_space() error inside tpm2_commit_space()
> * Error code was not printed when recv() callback failed. It is
>   fixed in this version.
> * Added a patch that removes @space from tpm_transmit().
> * Fixed a regression in earlier series. Forgot to amend the change
>   from the staging area that renames NESTED to UNLOCKED in tpm2-space.c.
> 
> Jarkko Sakkinen (16):
>   tpm: use tpm_buf in tpm_transmit_cmd() as the IO parameter
>   tpm: fix invalid return value in pubek_show()
>   tpm: return 0 from pcrs_show() when tpm1_pcr_read() fails
>   tpm: print tpm2_commit_space() error inside tpm2_commit_space()
>   tpm: declare struct tpm_header
>   tpm: access command header through struct in tpm_try_transmit()
>   tpm: encapsulate tpm_dev_transmit()
>   tpm: clean up tpm_try_transmit() error handling flow
>   tpm: move tpm_validate_commmand() to tpm2-space.c
>   tpm: move TPM space code out of tpm_transmit()
>   tpm: remove @space from tpm_transmit()
>   tpm: use tpm_try_get_ops() in tpm-sysfs.c.
>   tpm: remove TPM_TRANSMIT_UNLOCKED flag
>   tpm: introduce tpm_chip_start() and tpm_chip_stop()
>   tpm: take TPM chip power gating out of tpm_transmit()
>   tpm: remove @flags from tpm_transmit()
> 
>  drivers/char/tpm/tpm-chip.c       | 109 ++++++++++++
>  drivers/char/tpm/tpm-dev-common.c |  44 ++++-
>  drivers/char/tpm/tpm-interface.c  | 264 ++++++------------------------
>  drivers/char/tpm/tpm-sysfs.c      | 138 ++++++++++------
>  drivers/char/tpm/tpm.h            |  64 +++-----
>  drivers/char/tpm/tpm1-cmd.c       |  28 +---
>  drivers/char/tpm/tpm2-cmd.c       |  72 +++-----
>  drivers/char/tpm/tpm2-space.c     |  91 +++++++---
>  drivers/char/tpm/tpm_i2c_atmel.c  |   5 +-
>  drivers/char/tpm/tpm_vtpm_proxy.c |  12 +-
>  drivers/char/tpm/xen-tpmfront.c   |   2 +-
>  11 files changed, 408 insertions(+), 421 deletions(-)
> 
> -- 
> 2.19.1
> 

Applied to master and next.

/Jarkko
Alexander Steffen Feb. 7, 2019, 6:41 p.m. UTC | #2 
On 06.02.2019 13:06, Jarkko Sakkinen wrote:
> On Wed, Feb 06, 2019 at 12:47:07AM +0200, Jarkko Sakkinen wrote:
>> Make the changes necessary to detach TPM space code and TPM activation
>> code out of the tpm_transmit() flow because of both of these can cause
>> nested tpm_transmit() calls. The nesteds calls make the whole flow hard
>> to maintain, and thus, it is better to just fix things now before this
>> turns into a bigger mess.
>>
>> v11:
>> * Drop the patch that tries to flush TPM space on system. Not a proper
>>    fallback + out of scope for this patch set.
>>
>> v10:
>> * Use void pointers to avoid unnecessary casts in functions paramaters
>>    where it makes sense.
>>
>> v9:
>> * Fixed again tpm_try_get_ops().
>> * Added missing reviewed-by's.
>>
>> v8:
>> * Re-add the check for ret < 0 after calling tpm_try_transmit() that
>>    was dropped by mistake while moving code.
>> * Fix error fallback for tpm_try_get_ops() when tpm_chip_start()
>>    fails.
>>
>> v7:
>> *  Reorganize series so that more trivial and self-contained changes are
>>     in the head.
>>
>> v6:
>> * When tpm_validate_commmand() was moved to tpm2-space.c, the struct for
>>    the TPM header was incorrectly declared as struct tpm_input_header.
>> * Fix return value in tpm_validate_command().
>>
>> v5:
>> * Add the missing rev's from Stefan Berger.
>>
>> v4:
>> * Return 0 from pcrs_show() when tpm1_pcr_read() fails.
>> * Fix error handling flow in tpm_try_transmit().
>> * Replace struct tpm_input_header and struct tpm_output_header with
>>    struct tpm_header.
>>
>> v3:
>> * Encapsulate power gating code to tpm_chip_start() and tpm_chip_stop().
>> * Move TPM power gating code and locking to tpm_try_get_ops() and
>>    tpm_put_ops().
>> * Call power gating code directly in tpm_chip_register() and
>>    tpm2_del_space().
>>
>> v2:
>> * Print tpm2_commit_space() error inside tpm2_commit_space()
>> * Error code was not printed when recv() callback failed. It is
>>    fixed in this version.
>> * Added a patch that removes @space from tpm_transmit().
>> * Fixed a regression in earlier series. Forgot to amend the change
>>    from the staging area that renames NESTED to UNLOCKED in tpm2-space.c.
>>
>> Jarkko Sakkinen (16):
>>    tpm: use tpm_buf in tpm_transmit_cmd() as the IO parameter
>>    tpm: fix invalid return value in pubek_show()
>>    tpm: return 0 from pcrs_show() when tpm1_pcr_read() fails
>>    tpm: print tpm2_commit_space() error inside tpm2_commit_space()
>>    tpm: declare struct tpm_header
>>    tpm: access command header through struct in tpm_try_transmit()
>>    tpm: encapsulate tpm_dev_transmit()
>>    tpm: clean up tpm_try_transmit() error handling flow
>>    tpm: move tpm_validate_commmand() to tpm2-space.c
>>    tpm: move TPM space code out of tpm_transmit()
>>    tpm: remove @space from tpm_transmit()
>>    tpm: use tpm_try_get_ops() in tpm-sysfs.c.
>>    tpm: remove TPM_TRANSMIT_UNLOCKED flag
>>    tpm: introduce tpm_chip_start() and tpm_chip_stop()
>>    tpm: take TPM chip power gating out of tpm_transmit()
>>    tpm: remove @flags from tpm_transmit()
>>
>>   drivers/char/tpm/tpm-chip.c       | 109 ++++++++++++
>>   drivers/char/tpm/tpm-dev-common.c |  44 ++++-
>>   drivers/char/tpm/tpm-interface.c  | 264 ++++++------------------------
>>   drivers/char/tpm/tpm-sysfs.c      | 138 ++++++++++------
>>   drivers/char/tpm/tpm.h            |  64 +++-----
>>   drivers/char/tpm/tpm1-cmd.c       |  28 +---
>>   drivers/char/tpm/tpm2-cmd.c       |  72 +++-----
>>   drivers/char/tpm/tpm2-space.c     |  91 +++++++---
>>   drivers/char/tpm/tpm_i2c_atmel.c  |   5 +-
>>   drivers/char/tpm/tpm_vtpm_proxy.c |  12 +-
>>   drivers/char/tpm/xen-tpmfront.c   |   2 +-
>>   11 files changed, 408 insertions(+), 421 deletions(-)
>>
>> -- 
>> 2.19.1
>>
> 
> Applied to master and next.

Something in this series seems to break basic TPM communication for me.

For TPM2.0s the probe command fails, causing them to be misdetected as 
TPM1.2s:

---
tpm tpm0: tpm_try_transmit: tpm_send: error -5
tpm_tis MSFT0101:00: 1.2 TPM (device-id 0x1A, rev-id 22)
tpm tpm0: A TPM error (30) occurred attempting to determine the timeouts
---
tpm tpm0: tpm_try_transmit: tpm_send: error -5
tpm_tis_spi spi0.1: 1.2 TPM (device-id 0x1B, rev-id 22)
tpm tpm0: A TPM error (30) occurred attempting to determine the timeouts
---

And for something that actually is a TPM1.2 it fails in a similar way:

---
tpm_i2c_infineon 1-0020: 1.2 TPM (device-id 0x1A)
tpm tpm0: A TPM error (-14) occurred attempting to determine the timeouts
---
tpm tpm0: tpm_try_transmit: tpm_send: error -5
tpm_tis_spi spi0.1: 1.2 TPM (device-id 0x1B, rev-id 16)
tpm tpm0: A TPM error (-14) occurred attempting to determine the timeouts
tpm_tis_spi: probe of spi0.1 failed with error -14
---

I see this problem across my entire range of TPM devices and test 
platforms. Any idea what could be wrong here?

Alexander
Stefan Berger Feb. 7, 2019, 9:14 p.m. UTC | #3 
On 2/7/19 1:41 PM, Alexander Steffen wrote:
> On 06.02.2019 13:06, Jarkko Sakkinen wrote:
>> On Wed, Feb 06, 2019 at 12:47:07AM +0200, Jarkko Sakkinen wrote:
>>> Make the changes necessary to detach TPM space code and TPM activation
>>> code out of the tpm_transmit() flow because of both of these can cause
>>> nested tpm_transmit() calls. The nesteds calls make the whole flow hard
>>> to maintain, and thus, it is better to just fix things now before this
>>> turns into a bigger mess.
>>>
>>> v11:
>>> * Drop the patch that tries to flush TPM space on system. Not a proper
>>>    fallback + out of scope for this patch set.
>>>
>>> v10:
>>> * Use void pointers to avoid unnecessary casts in functions paramaters
>>>    where it makes sense.
>>>
>>> v9:
>>> * Fixed again tpm_try_get_ops().
>>> * Added missing reviewed-by's.
>>>
>>> v8:
>>> * Re-add the check for ret < 0 after calling tpm_try_transmit() that
>>>    was dropped by mistake while moving code.
>>> * Fix error fallback for tpm_try_get_ops() when tpm_chip_start()
>>>    fails.
>>>
>>> v7:
>>> *  Reorganize series so that more trivial and self-contained changes 
>>> are
>>>     in the head.
>>>
>>> v6:
>>> * When tpm_validate_commmand() was moved to tpm2-space.c, the struct 
>>> for
>>>    the TPM header was incorrectly declared as struct tpm_input_header.
>>> * Fix return value in tpm_validate_command().
>>>
>>> v5:
>>> * Add the missing rev's from Stefan Berger.
>>>
>>> v4:
>>> * Return 0 from pcrs_show() when tpm1_pcr_read() fails.
>>> * Fix error handling flow in tpm_try_transmit().
>>> * Replace struct tpm_input_header and struct tpm_output_header with
>>>    struct tpm_header.
>>>
>>> v3:
>>> * Encapsulate power gating code to tpm_chip_start() and 
>>> tpm_chip_stop().
>>> * Move TPM power gating code and locking to tpm_try_get_ops() and
>>>    tpm_put_ops().
>>> * Call power gating code directly in tpm_chip_register() and
>>>    tpm2_del_space().
>>>
>>> v2:
>>> * Print tpm2_commit_space() error inside tpm2_commit_space()
>>> * Error code was not printed when recv() callback failed. It is
>>>    fixed in this version.
>>> * Added a patch that removes @space from tpm_transmit().
>>> * Fixed a regression in earlier series. Forgot to amend the change
>>>    from the staging area that renames NESTED to UNLOCKED in 
>>> tpm2-space.c.
>>>
>>> Jarkko Sakkinen (16):
>>>    tpm: use tpm_buf in tpm_transmit_cmd() as the IO parameter
>>>    tpm: fix invalid return value in pubek_show()
>>>    tpm: return 0 from pcrs_show() when tpm1_pcr_read() fails
>>>    tpm: print tpm2_commit_space() error inside tpm2_commit_space()
>>>    tpm: declare struct tpm_header
>>>    tpm: access command header through struct in tpm_try_transmit()
>>>    tpm: encapsulate tpm_dev_transmit()
>>>    tpm: clean up tpm_try_transmit() error handling flow
>>>    tpm: move tpm_validate_commmand() to tpm2-space.c
>>>    tpm: move TPM space code out of tpm_transmit()
>>>    tpm: remove @space from tpm_transmit()
>>>    tpm: use tpm_try_get_ops() in tpm-sysfs.c.
>>>    tpm: remove TPM_TRANSMIT_UNLOCKED flag
>>>    tpm: introduce tpm_chip_start() and tpm_chip_stop()
>>>    tpm: take TPM chip power gating out of tpm_transmit()
>>>    tpm: remove @flags from tpm_transmit()
>>>
>>>   drivers/char/tpm/tpm-chip.c       | 109 ++++++++++++
>>>   drivers/char/tpm/tpm-dev-common.c |  44 ++++-
>>>   drivers/char/tpm/tpm-interface.c  | 264 
>>> ++++++------------------------
>>>   drivers/char/tpm/tpm-sysfs.c      | 138 ++++++++++------
>>>   drivers/char/tpm/tpm.h            |  64 +++-----
>>>   drivers/char/tpm/tpm1-cmd.c       |  28 +---
>>>   drivers/char/tpm/tpm2-cmd.c       |  72 +++-----
>>>   drivers/char/tpm/tpm2-space.c     |  91 +++++++---
>>>   drivers/char/tpm/tpm_i2c_atmel.c  |   5 +-
>>>   drivers/char/tpm/tpm_vtpm_proxy.c |  12 +-
>>>   drivers/char/tpm/xen-tpmfront.c   |   2 +-
>>>   11 files changed, 408 insertions(+), 421 deletions(-)
>>>
>>> -- 
>>> 2.19.1
>>>
>>
>> Applied to master and next.
>
> Something in this series seems to break basic TPM communication for me.
>
> For TPM2.0s the probe command fails, causing them to be misdetected as 
> TPM1.2s:
>
> ---
> tpm tpm0: tpm_try_transmit: tpm_send: error -5
> tpm_tis MSFT0101:00: 1.2 TPM (device-id 0x1A, rev-id 22)
> tpm tpm0: A TPM error (30) occurred attempting to determine the timeouts
> ---
> tpm tpm0: tpm_try_transmit: tpm_send: error -5
> tpm_tis_spi spi0.1: 1.2 TPM (device-id 0x1B, rev-id 22)
> tpm tpm0: A TPM error (30) occurred attempting to determine the timeouts
> ---
>
> And for something that actually is a TPM1.2 it fails in a similar way:
>
> ---
> tpm_i2c_infineon 1-0020: 1.2 TPM (device-id 0x1A)
> tpm tpm0: A TPM error (-14) occurred attempting to determine the timeouts
> ---
> tpm tpm0: tpm_try_transmit: tpm_send: error -5
> tpm_tis_spi spi0.1: 1.2 TPM (device-id 0x1B, rev-id 16)
> tpm tpm0: A TPM error (-14) occurred attempting to determine the timeouts
> tpm_tis_spi: probe of spi0.1 failed with error -14
> ---
>
> I see this problem across my entire range of TPM devices and test 
> platforms. Any idea what could be wrong here?


Can you bisect the series ?

    Stefan



>
>
> Alexander
>
Jarkko Sakkinen Feb. 7, 2019, 9:29 p.m. UTC | #4 
On Thu, Feb 07, 2019 at 07:41:56PM +0100, Alexander Steffen wrote:
> On 06.02.2019 13:06, Jarkko Sakkinen wrote:
> > On Wed, Feb 06, 2019 at 12:47:07AM +0200, Jarkko Sakkinen wrote:
> > > Make the changes necessary to detach TPM space code and TPM activation
> > > code out of the tpm_transmit() flow because of both of these can cause
> > > nested tpm_transmit() calls. The nesteds calls make the whole flow hard
> > > to maintain, and thus, it is better to just fix things now before this
> > > turns into a bigger mess.
> > > 
> > > v11:
> > > * Drop the patch that tries to flush TPM space on system. Not a proper
> > >    fallback + out of scope for this patch set.
> > > 
> > > v10:
> > > * Use void pointers to avoid unnecessary casts in functions paramaters
> > >    where it makes sense.
> > > 
> > > v9:
> > > * Fixed again tpm_try_get_ops().
> > > * Added missing reviewed-by's.
> > > 
> > > v8:
> > > * Re-add the check for ret < 0 after calling tpm_try_transmit() that
> > >    was dropped by mistake while moving code.
> > > * Fix error fallback for tpm_try_get_ops() when tpm_chip_start()
> > >    fails.
> > > 
> > > v7:
> > > *  Reorganize series so that more trivial and self-contained changes are
> > >     in the head.
> > > 
> > > v6:
> > > * When tpm_validate_commmand() was moved to tpm2-space.c, the struct for
> > >    the TPM header was incorrectly declared as struct tpm_input_header.
> > > * Fix return value in tpm_validate_command().
> > > 
> > > v5:
> > > * Add the missing rev's from Stefan Berger.
> > > 
> > > v4:
> > > * Return 0 from pcrs_show() when tpm1_pcr_read() fails.
> > > * Fix error handling flow in tpm_try_transmit().
> > > * Replace struct tpm_input_header and struct tpm_output_header with
> > >    struct tpm_header.
> > > 
> > > v3:
> > > * Encapsulate power gating code to tpm_chip_start() and tpm_chip_stop().
> > > * Move TPM power gating code and locking to tpm_try_get_ops() and
> > >    tpm_put_ops().
> > > * Call power gating code directly in tpm_chip_register() and
> > >    tpm2_del_space().
> > > 
> > > v2:
> > > * Print tpm2_commit_space() error inside tpm2_commit_space()
> > > * Error code was not printed when recv() callback failed. It is
> > >    fixed in this version.
> > > * Added a patch that removes @space from tpm_transmit().
> > > * Fixed a regression in earlier series. Forgot to amend the change
> > >    from the staging area that renames NESTED to UNLOCKED in tpm2-space.c.
> > > 
> > > Jarkko Sakkinen (16):
> > >    tpm: use tpm_buf in tpm_transmit_cmd() as the IO parameter
> > >    tpm: fix invalid return value in pubek_show()
> > >    tpm: return 0 from pcrs_show() when tpm1_pcr_read() fails
> > >    tpm: print tpm2_commit_space() error inside tpm2_commit_space()
> > >    tpm: declare struct tpm_header
> > >    tpm: access command header through struct in tpm_try_transmit()
> > >    tpm: encapsulate tpm_dev_transmit()
> > >    tpm: clean up tpm_try_transmit() error handling flow
> > >    tpm: move tpm_validate_commmand() to tpm2-space.c
> > >    tpm: move TPM space code out of tpm_transmit()
> > >    tpm: remove @space from tpm_transmit()
> > >    tpm: use tpm_try_get_ops() in tpm-sysfs.c.
> > >    tpm: remove TPM_TRANSMIT_UNLOCKED flag
> > >    tpm: introduce tpm_chip_start() and tpm_chip_stop()
> > >    tpm: take TPM chip power gating out of tpm_transmit()
> > >    tpm: remove @flags from tpm_transmit()
> > > 
> > >   drivers/char/tpm/tpm-chip.c       | 109 ++++++++++++
> > >   drivers/char/tpm/tpm-dev-common.c |  44 ++++-
> > >   drivers/char/tpm/tpm-interface.c  | 264 ++++++------------------------
> > >   drivers/char/tpm/tpm-sysfs.c      | 138 ++++++++++------
> > >   drivers/char/tpm/tpm.h            |  64 +++-----
> > >   drivers/char/tpm/tpm1-cmd.c       |  28 +---
> > >   drivers/char/tpm/tpm2-cmd.c       |  72 +++-----
> > >   drivers/char/tpm/tpm2-space.c     |  91 +++++++---
> > >   drivers/char/tpm/tpm_i2c_atmel.c  |   5 +-
> > >   drivers/char/tpm/tpm_vtpm_proxy.c |  12 +-
> > >   drivers/char/tpm/xen-tpmfront.c   |   2 +-
> > >   11 files changed, 408 insertions(+), 421 deletions(-)
> > > 
> > > -- 
> > > 2.19.1
> > > 
> > 
> > Applied to master and next.
> 
> Something in this series seems to break basic TPM communication for me.
> 
> For TPM2.0s the probe command fails, causing them to be misdetected as
> TPM1.2s:
> 
> ---
> tpm tpm0: tpm_try_transmit: tpm_send: error -5
> tpm_tis MSFT0101:00: 1.2 TPM (device-id 0x1A, rev-id 22)
> tpm tpm0: A TPM error (30) occurred attempting to determine the timeouts
> ---
> tpm tpm0: tpm_try_transmit: tpm_send: error -5
> tpm_tis_spi spi0.1: 1.2 TPM (device-id 0x1B, rev-id 22)
> tpm tpm0: A TPM error (30) occurred attempting to determine the timeouts
> ---
> 
> And for something that actually is a TPM1.2 it fails in a similar way:
> 
> ---
> tpm_i2c_infineon 1-0020: 1.2 TPM (device-id 0x1A)
> tpm tpm0: A TPM error (-14) occurred attempting to determine the timeouts
> ---
> tpm tpm0: tpm_try_transmit: tpm_send: error -5
> tpm_tis_spi spi0.1: 1.2 TPM (device-id 0x1B, rev-id 16)
> tpm tpm0: A TPM error (-14) occurred attempting to determine the timeouts
> tpm_tis_spi: probe of spi0.1 failed with error -14
> ---
> 
> I see this problem across my entire range of TPM devices and test platforms.
> Any idea what could be wrong here?

Weird.

Can you run a bisect?

/Jarkko
Stefan Berger Feb. 7, 2019, 11:29 p.m. UTC | #5 
On 2/7/19 4:29 PM, Jarkko Sakkinen wrote:
> On Thu, Feb 07, 2019 at 07:41:56PM +0100, Alexander Steffen wrote:
>> On 06.02.2019 13:06, Jarkko Sakkinen wrote:
>>> On Wed, Feb 06, 2019 at 12:47:07AM +0200, Jarkko Sakkinen wrote:
>>>> Make the changes necessary to detach TPM space code and TPM activation
>>>> code out of the tpm_transmit() flow because of both of these can cause
>>>> nested tpm_transmit() calls. The nesteds calls make the whole flow hard
>>>> to maintain, and thus, it is better to just fix things now before this
>>>> turns into a bigger mess.
>>>>
>>>> v11:
>>>> * Drop the patch that tries to flush TPM space on system. Not a proper
>>>>     fallback + out of scope for this patch set.
>>>>
>>>> v10:
>>>> * Use void pointers to avoid unnecessary casts in functions paramaters
>>>>     where it makes sense.
>>>>
>>>> v9:
>>>> * Fixed again tpm_try_get_ops().
>>>> * Added missing reviewed-by's.
>>>>
>>>> v8:
>>>> * Re-add the check for ret < 0 after calling tpm_try_transmit() that
>>>>     was dropped by mistake while moving code.
>>>> * Fix error fallback for tpm_try_get_ops() when tpm_chip_start()
>>>>     fails.
>>>>
>>>> v7:
>>>> *  Reorganize series so that more trivial and self-contained changes are
>>>>      in the head.
>>>>
>>>> v6:
>>>> * When tpm_validate_commmand() was moved to tpm2-space.c, the struct for
>>>>     the TPM header was incorrectly declared as struct tpm_input_header.
>>>> * Fix return value in tpm_validate_command().
>>>>
>>>> v5:
>>>> * Add the missing rev's from Stefan Berger.
>>>>
>>>> v4:
>>>> * Return 0 from pcrs_show() when tpm1_pcr_read() fails.
>>>> * Fix error handling flow in tpm_try_transmit().
>>>> * Replace struct tpm_input_header and struct tpm_output_header with
>>>>     struct tpm_header.
>>>>
>>>> v3:
>>>> * Encapsulate power gating code to tpm_chip_start() and tpm_chip_stop().
>>>> * Move TPM power gating code and locking to tpm_try_get_ops() and
>>>>     tpm_put_ops().
>>>> * Call power gating code directly in tpm_chip_register() and
>>>>     tpm2_del_space().
>>>>
>>>> v2:
>>>> * Print tpm2_commit_space() error inside tpm2_commit_space()
>>>> * Error code was not printed when recv() callback failed. It is
>>>>     fixed in this version.
>>>> * Added a patch that removes @space from tpm_transmit().
>>>> * Fixed a regression in earlier series. Forgot to amend the change
>>>>     from the staging area that renames NESTED to UNLOCKED in tpm2-space.c.
>>>>
>>>> Jarkko Sakkinen (16):
>>>>     tpm: use tpm_buf in tpm_transmit_cmd() as the IO parameter
>>>>     tpm: fix invalid return value in pubek_show()
>>>>     tpm: return 0 from pcrs_show() when tpm1_pcr_read() fails
>>>>     tpm: print tpm2_commit_space() error inside tpm2_commit_space()
>>>>     tpm: declare struct tpm_header
>>>>     tpm: access command header through struct in tpm_try_transmit()
>>>>     tpm: encapsulate tpm_dev_transmit()
>>>>     tpm: clean up tpm_try_transmit() error handling flow
>>>>     tpm: move tpm_validate_commmand() to tpm2-space.c
>>>>     tpm: move TPM space code out of tpm_transmit()
>>>>     tpm: remove @space from tpm_transmit()
>>>>     tpm: use tpm_try_get_ops() in tpm-sysfs.c.
>>>>     tpm: remove TPM_TRANSMIT_UNLOCKED flag
>>>>     tpm: introduce tpm_chip_start() and tpm_chip_stop()
>>>>     tpm: take TPM chip power gating out of tpm_transmit()
>>>>     tpm: remove @flags from tpm_transmit()
>>>>
>>>>    drivers/char/tpm/tpm-chip.c       | 109 ++++++++++++
>>>>    drivers/char/tpm/tpm-dev-common.c |  44 ++++-
>>>>    drivers/char/tpm/tpm-interface.c  | 264 ++++++------------------------
>>>>    drivers/char/tpm/tpm-sysfs.c      | 138 ++++++++++------
>>>>    drivers/char/tpm/tpm.h            |  64 +++-----
>>>>    drivers/char/tpm/tpm1-cmd.c       |  28 +---
>>>>    drivers/char/tpm/tpm2-cmd.c       |  72 +++-----
>>>>    drivers/char/tpm/tpm2-space.c     |  91 +++++++---
>>>>    drivers/char/tpm/tpm_i2c_atmel.c  |   5 +-
>>>>    drivers/char/tpm/tpm_vtpm_proxy.c |  12 +-
>>>>    drivers/char/tpm/xen-tpmfront.c   |   2 +-
>>>>    11 files changed, 408 insertions(+), 421 deletions(-)
>>>>
>>>> -- 
>>>> 2.19.1
>>>>
>>> Applied to master and next.
>> Something in this series seems to break basic TPM communication for me.
>>
>> For TPM2.0s the probe command fails, causing them to be misdetected as
>> TPM1.2s:
>>
>> ---
>> tpm tpm0: tpm_try_transmit: tpm_send: error -5
>> tpm_tis MSFT0101:00: 1.2 TPM (device-id 0x1A, rev-id 22)
>> tpm tpm0: A TPM error (30) occurred attempting to determine the timeouts
>> ---
>> tpm tpm0: tpm_try_transmit: tpm_send: error -5
>> tpm_tis_spi spi0.1: 1.2 TPM (device-id 0x1B, rev-id 22)
>> tpm tpm0: A TPM error (30) occurred attempting to determine the timeouts
>> ---
>>
>> And for something that actually is a TPM1.2 it fails in a similar way:
>>
>> ---
>> tpm_i2c_infineon 1-0020: 1.2 TPM (device-id 0x1A)
>> tpm tpm0: A TPM error (-14) occurred attempting to determine the timeouts
>> ---
>> tpm tpm0: tpm_try_transmit: tpm_send: error -5
>> tpm_tis_spi spi0.1: 1.2 TPM (device-id 0x1B, rev-id 16)
>> tpm tpm0: A TPM error (-14) occurred attempting to determine the timeouts
>> tpm_tis_spi: probe of spi0.1 failed with error -14
>> ---
>>
>> I see this problem across my entire range of TPM devices and test platforms.
>> Any idea what could be wrong here?
> Weird.
>
> Can you run a bisect?

There are 2 bugs and the following overall patch against your tree fixes 
them. Let me comment on the individual patches in v17. I missed those 
obviously when testing with the tpm_vtpm_proxy...



diff --git a/drivers/char/tpm/tpm-interface.c 
b/drivers/char/tpm/tpm-interface.c
index 02e8cffd1163..34c0da55d885 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -94,6 +94,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, 
void *buf, size_t bufsiz)
          return rc;
      }

+    rc = 0;
      if (chip->flags & TPM_CHIP_FLAG_IRQ)
          goto out_recv;

diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index e74c5b7b64bf..52afe20cc8a1 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -799,7 +799,9 @@ int tpm2_probe(struct tpm_chip *chip)
      tpm_buf_append_u32(&buf, TPM2_CAP_TPM_PROPERTIES);
      tpm_buf_append_u32(&buf, TPM_PT_TOTAL_COMMANDS);
      tpm_buf_append_u32(&buf, 1);
+    tpm_chip_start(chip);
      rc = tpm_transmit_cmd(chip, &buf, 0, NULL);
+    tpm_chip_stop(chip);
      /* We ignore TPM return codes on purpose. */
      if (rc >=  0) {
          out = (struct tpm_header *)buf.data;

>
> /Jarkko
>
Jarkko Sakkinen Feb. 8, 2019, 12:33 a.m. UTC | #6 
On Thu, Feb 07, 2019 at 06:29:43PM -0500, Stefan Berger wrote:
> On 2/7/19 4:29 PM, Jarkko Sakkinen wrote:
> > On Thu, Feb 07, 2019 at 07:41:56PM +0100, Alexander Steffen wrote:
> > > On 06.02.2019 13:06, Jarkko Sakkinen wrote:
> > > > On Wed, Feb 06, 2019 at 12:47:07AM +0200, Jarkko Sakkinen wrote:
> > > > > Make the changes necessary to detach TPM space code and TPM activation
> > > > > code out of the tpm_transmit() flow because of both of these can cause
> > > > > nested tpm_transmit() calls. The nesteds calls make the whole flow hard
> > > > > to maintain, and thus, it is better to just fix things now before this
> > > > > turns into a bigger mess.
> > > > > 
> > > > > v11:
> > > > > * Drop the patch that tries to flush TPM space on system. Not a proper
> > > > >     fallback + out of scope for this patch set.
> > > > > 
> > > > > v10:
> > > > > * Use void pointers to avoid unnecessary casts in functions paramaters
> > > > >     where it makes sense.
> > > > > 
> > > > > v9:
> > > > > * Fixed again tpm_try_get_ops().
> > > > > * Added missing reviewed-by's.
> > > > > 
> > > > > v8:
> > > > > * Re-add the check for ret < 0 after calling tpm_try_transmit() that
> > > > >     was dropped by mistake while moving code.
> > > > > * Fix error fallback for tpm_try_get_ops() when tpm_chip_start()
> > > > >     fails.
> > > > > 
> > > > > v7:
> > > > > *  Reorganize series so that more trivial and self-contained changes are
> > > > >      in the head.
> > > > > 
> > > > > v6:
> > > > > * When tpm_validate_commmand() was moved to tpm2-space.c, the struct for
> > > > >     the TPM header was incorrectly declared as struct tpm_input_header.
> > > > > * Fix return value in tpm_validate_command().
> > > > > 
> > > > > v5:
> > > > > * Add the missing rev's from Stefan Berger.
> > > > > 
> > > > > v4:
> > > > > * Return 0 from pcrs_show() when tpm1_pcr_read() fails.
> > > > > * Fix error handling flow in tpm_try_transmit().
> > > > > * Replace struct tpm_input_header and struct tpm_output_header with
> > > > >     struct tpm_header.
> > > > > 
> > > > > v3:
> > > > > * Encapsulate power gating code to tpm_chip_start() and tpm_chip_stop().
> > > > > * Move TPM power gating code and locking to tpm_try_get_ops() and
> > > > >     tpm_put_ops().
> > > > > * Call power gating code directly in tpm_chip_register() and
> > > > >     tpm2_del_space().
> > > > > 
> > > > > v2:
> > > > > * Print tpm2_commit_space() error inside tpm2_commit_space()
> > > > > * Error code was not printed when recv() callback failed. It is
> > > > >     fixed in this version.
> > > > > * Added a patch that removes @space from tpm_transmit().
> > > > > * Fixed a regression in earlier series. Forgot to amend the change
> > > > >     from the staging area that renames NESTED to UNLOCKED in tpm2-space.c.
> > > > > 
> > > > > Jarkko Sakkinen (16):
> > > > >     tpm: use tpm_buf in tpm_transmit_cmd() as the IO parameter
> > > > >     tpm: fix invalid return value in pubek_show()
> > > > >     tpm: return 0 from pcrs_show() when tpm1_pcr_read() fails
> > > > >     tpm: print tpm2_commit_space() error inside tpm2_commit_space()
> > > > >     tpm: declare struct tpm_header
> > > > >     tpm: access command header through struct in tpm_try_transmit()
> > > > >     tpm: encapsulate tpm_dev_transmit()
> > > > >     tpm: clean up tpm_try_transmit() error handling flow
> > > > >     tpm: move tpm_validate_commmand() to tpm2-space.c
> > > > >     tpm: move TPM space code out of tpm_transmit()
> > > > >     tpm: remove @space from tpm_transmit()
> > > > >     tpm: use tpm_try_get_ops() in tpm-sysfs.c.
> > > > >     tpm: remove TPM_TRANSMIT_UNLOCKED flag
> > > > >     tpm: introduce tpm_chip_start() and tpm_chip_stop()
> > > > >     tpm: take TPM chip power gating out of tpm_transmit()
> > > > >     tpm: remove @flags from tpm_transmit()
> > > > > 
> > > > >    drivers/char/tpm/tpm-chip.c       | 109 ++++++++++++
> > > > >    drivers/char/tpm/tpm-dev-common.c |  44 ++++-
> > > > >    drivers/char/tpm/tpm-interface.c  | 264 ++++++------------------------
> > > > >    drivers/char/tpm/tpm-sysfs.c      | 138 ++++++++++------
> > > > >    drivers/char/tpm/tpm.h            |  64 +++-----
> > > > >    drivers/char/tpm/tpm1-cmd.c       |  28 +---
> > > > >    drivers/char/tpm/tpm2-cmd.c       |  72 +++-----
> > > > >    drivers/char/tpm/tpm2-space.c     |  91 +++++++---
> > > > >    drivers/char/tpm/tpm_i2c_atmel.c  |   5 +-
> > > > >    drivers/char/tpm/tpm_vtpm_proxy.c |  12 +-
> > > > >    drivers/char/tpm/xen-tpmfront.c   |   2 +-
> > > > >    11 files changed, 408 insertions(+), 421 deletions(-)
> > > > > 
> > > > > -- 
> > > > > 2.19.1
> > > > > 
> > > > Applied to master and next.
> > > Something in this series seems to break basic TPM communication for me.
> > > 
> > > For TPM2.0s the probe command fails, causing them to be misdetected as
> > > TPM1.2s:
> > > 
> > > ---
> > > tpm tpm0: tpm_try_transmit: tpm_send: error -5
> > > tpm_tis MSFT0101:00: 1.2 TPM (device-id 0x1A, rev-id 22)
> > > tpm tpm0: A TPM error (30) occurred attempting to determine the timeouts
> > > ---
> > > tpm tpm0: tpm_try_transmit: tpm_send: error -5
> > > tpm_tis_spi spi0.1: 1.2 TPM (device-id 0x1B, rev-id 22)
> > > tpm tpm0: A TPM error (30) occurred attempting to determine the timeouts
> > > ---
> > > 
> > > And for something that actually is a TPM1.2 it fails in a similar way:
> > > 
> > > ---
> > > tpm_i2c_infineon 1-0020: 1.2 TPM (device-id 0x1A)
> > > tpm tpm0: A TPM error (-14) occurred attempting to determine the timeouts
> > > ---
> > > tpm tpm0: tpm_try_transmit: tpm_send: error -5
> > > tpm_tis_spi spi0.1: 1.2 TPM (device-id 0x1B, rev-id 16)
> > > tpm tpm0: A TPM error (-14) occurred attempting to determine the timeouts
> > > tpm_tis_spi: probe of spi0.1 failed with error -14
> > > ---
> > > 
> > > I see this problem across my entire range of TPM devices and test platforms.
> > > Any idea what could be wrong here?
> > Weird.
> > 
> > Can you run a bisect?
> 
> There are 2 bugs and the following overall patch against your tree fixes
> them. Let me comment on the individual patches in v17. I missed those
> obviously when testing with the tpm_vtpm_proxy...
> 
> 
> 
> diff --git a/drivers/char/tpm/tpm-interface.c
> b/drivers/char/tpm/tpm-interface.c
> index 02e8cffd1163..34c0da55d885 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -94,6 +94,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
> void *buf, size_t bufsiz)
>          return rc;
>      }
> 
> +    rc = 0;
>      if (chip->flags & TPM_CHIP_FLAG_IRQ)
>          goto out_recv;

What why?

> 
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index e74c5b7b64bf..52afe20cc8a1 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -799,7 +799,9 @@ int tpm2_probe(struct tpm_chip *chip)
>      tpm_buf_append_u32(&buf, TPM2_CAP_TPM_PROPERTIES);
>      tpm_buf_append_u32(&buf, TPM_PT_TOTAL_COMMANDS);
>      tpm_buf_append_u32(&buf, 1);
> +    tpm_chip_start(chip);
>      rc = tpm_transmit_cmd(chip, &buf, 0, NULL);
> +    tpm_chip_stop(chip);

Thanks Stefan! I added call to tpm_tis_core as tpm2-cmd.c is to be kept
out of chip management common case being that you call tpm_try_get_ops(),
do 1-N TPM commands and release with tpm_put_ops(). These functions take
care starting and stopping the chip.

I fixed the 2nd issue in the master.

 /Jarkko
Stefan Berger Feb. 8, 2019, 1:51 a.m. UTC | #7 
On 2/7/19 7:33 PM, Jarkko Sakkinen wrote:
> On Thu, Feb 07, 2019 at 06:29:43PM -0500, Stefan Berger wrote:
>> On 2/7/19 4:29 PM, Jarkko Sakkinen wrote:
>>> On Thu, Feb 07, 2019 at 07:41:56PM +0100, Alexander Steffen wrote:
>>>> On 06.02.2019 13:06, Jarkko Sakkinen wrote:
>>>>> On Wed, Feb 06, 2019 at 12:47:07AM +0200, Jarkko Sakkinen wrote:
>>>>>> Make the changes necessary to detach TPM space code and TPM activation
>>>>>> code out of the tpm_transmit() flow because of both of these can cause
>>>>>> nested tpm_transmit() calls. The nesteds calls make the whole flow hard
>>>>>> to maintain, and thus, it is better to just fix things now before this
>>>>>> turns into a bigger mess.
>>>>>>
>>>>>> v11:
>>>>>> * Drop the patch that tries to flush TPM space on system. Not a proper
>>>>>>      fallback + out of scope for this patch set.
>>>>>>
>>>>>> v10:
>>>>>> * Use void pointers to avoid unnecessary casts in functions paramaters
>>>>>>      where it makes sense.
>>>>>>
>>>>>> v9:
>>>>>> * Fixed again tpm_try_get_ops().
>>>>>> * Added missing reviewed-by's.
>>>>>>
>>>>>> v8:
>>>>>> * Re-add the check for ret < 0 after calling tpm_try_transmit() that
>>>>>>      was dropped by mistake while moving code.
>>>>>> * Fix error fallback for tpm_try_get_ops() when tpm_chip_start()
>>>>>>      fails.
>>>>>>
>>>>>> v7:
>>>>>> *  Reorganize series so that more trivial and self-contained changes are
>>>>>>       in the head.
>>>>>>
>>>>>> v6:
>>>>>> * When tpm_validate_commmand() was moved to tpm2-space.c, the struct for
>>>>>>      the TPM header was incorrectly declared as struct tpm_input_header.
>>>>>> * Fix return value in tpm_validate_command().
>>>>>>
>>>>>> v5:
>>>>>> * Add the missing rev's from Stefan Berger.
>>>>>>
>>>>>> v4:
>>>>>> * Return 0 from pcrs_show() when tpm1_pcr_read() fails.
>>>>>> * Fix error handling flow in tpm_try_transmit().
>>>>>> * Replace struct tpm_input_header and struct tpm_output_header with
>>>>>>      struct tpm_header.
>>>>>>
>>>>>> v3:
>>>>>> * Encapsulate power gating code to tpm_chip_start() and tpm_chip_stop().
>>>>>> * Move TPM power gating code and locking to tpm_try_get_ops() and
>>>>>>      tpm_put_ops().
>>>>>> * Call power gating code directly in tpm_chip_register() and
>>>>>>      tpm2_del_space().
>>>>>>
>>>>>> v2:
>>>>>> * Print tpm2_commit_space() error inside tpm2_commit_space()
>>>>>> * Error code was not printed when recv() callback failed. It is
>>>>>>      fixed in this version.
>>>>>> * Added a patch that removes @space from tpm_transmit().
>>>>>> * Fixed a regression in earlier series. Forgot to amend the change
>>>>>>      from the staging area that renames NESTED to UNLOCKED in tpm2-space.c.
>>>>>>
>>>>>> Jarkko Sakkinen (16):
>>>>>>      tpm: use tpm_buf in tpm_transmit_cmd() as the IO parameter
>>>>>>      tpm: fix invalid return value in pubek_show()
>>>>>>      tpm: return 0 from pcrs_show() when tpm1_pcr_read() fails
>>>>>>      tpm: print tpm2_commit_space() error inside tpm2_commit_space()
>>>>>>      tpm: declare struct tpm_header
>>>>>>      tpm: access command header through struct in tpm_try_transmit()
>>>>>>      tpm: encapsulate tpm_dev_transmit()
>>>>>>      tpm: clean up tpm_try_transmit() error handling flow
>>>>>>      tpm: move tpm_validate_commmand() to tpm2-space.c
>>>>>>      tpm: move TPM space code out of tpm_transmit()
>>>>>>      tpm: remove @space from tpm_transmit()
>>>>>>      tpm: use tpm_try_get_ops() in tpm-sysfs.c.
>>>>>>      tpm: remove TPM_TRANSMIT_UNLOCKED flag
>>>>>>      tpm: introduce tpm_chip_start() and tpm_chip_stop()
>>>>>>      tpm: take TPM chip power gating out of tpm_transmit()
>>>>>>      tpm: remove @flags from tpm_transmit()
>>>>>>
>>>>>>     drivers/char/tpm/tpm-chip.c       | 109 ++++++++++++
>>>>>>     drivers/char/tpm/tpm-dev-common.c |  44 ++++-
>>>>>>     drivers/char/tpm/tpm-interface.c  | 264 ++++++------------------------
>>>>>>     drivers/char/tpm/tpm-sysfs.c      | 138 ++++++++++------
>>>>>>     drivers/char/tpm/tpm.h            |  64 +++-----
>>>>>>     drivers/char/tpm/tpm1-cmd.c       |  28 +---
>>>>>>     drivers/char/tpm/tpm2-cmd.c       |  72 +++-----
>>>>>>     drivers/char/tpm/tpm2-space.c     |  91 +++++++---
>>>>>>     drivers/char/tpm/tpm_i2c_atmel.c  |   5 +-
>>>>>>     drivers/char/tpm/tpm_vtpm_proxy.c |  12 +-
>>>>>>     drivers/char/tpm/xen-tpmfront.c   |   2 +-
>>>>>>     11 files changed, 408 insertions(+), 421 deletions(-)
>>>>>>
>>>>>> -- 
>>>>>> 2.19.1
>>>>>>
>>>>> Applied to master and next.
>>>> Something in this series seems to break basic TPM communication for me.
>>>>
>>>> For TPM2.0s the probe command fails, causing them to be misdetected as
>>>> TPM1.2s:
>>>>
>>>> ---
>>>> tpm tpm0: tpm_try_transmit: tpm_send: error -5
>>>> tpm_tis MSFT0101:00: 1.2 TPM (device-id 0x1A, rev-id 22)
>>>> tpm tpm0: A TPM error (30) occurred attempting to determine the timeouts
>>>> ---
>>>> tpm tpm0: tpm_try_transmit: tpm_send: error -5
>>>> tpm_tis_spi spi0.1: 1.2 TPM (device-id 0x1B, rev-id 22)
>>>> tpm tpm0: A TPM error (30) occurred attempting to determine the timeouts
>>>> ---
>>>>
>>>> And for something that actually is a TPM1.2 it fails in a similar way:
>>>>
>>>> ---
>>>> tpm_i2c_infineon 1-0020: 1.2 TPM (device-id 0x1A)
>>>> tpm tpm0: A TPM error (-14) occurred attempting to determine the timeouts
>>>> ---
>>>> tpm tpm0: tpm_try_transmit: tpm_send: error -5
>>>> tpm_tis_spi spi0.1: 1.2 TPM (device-id 0x1B, rev-id 16)
>>>> tpm tpm0: A TPM error (-14) occurred attempting to determine the timeouts
>>>> tpm_tis_spi: probe of spi0.1 failed with error -14
>>>> ---
>>>>
>>>> I see this problem across my entire range of TPM devices and test platforms.
>>>> Any idea what could be wrong here?
>>> Weird.
>>>
>>> Can you run a bisect?
>> There are 2 bugs and the following overall patch against your tree fixes
>> them. Let me comment on the individual patches in v17. I missed those
>> obviously when testing with the tpm_vtpm_proxy...
>>
>>
>>
>> diff --git a/drivers/char/tpm/tpm-interface.c
>> b/drivers/char/tpm/tpm-interface.c
>> index 02e8cffd1163..34c0da55d885 100644
>> --- a/drivers/char/tpm/tpm-interface.c
>> +++ b/drivers/char/tpm/tpm-interface.c
>> @@ -94,6 +94,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
>> void *buf, size_t bufsiz)
>>           return rc;
>>       }
>>
>> +    rc = 0;
>>       if (chip->flags & TPM_CHIP_FLAG_IRQ)
>>           goto out_recv;
> What why?


This fix seems to only be necessary when bisecting. You may want to 
apply it!


>
>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>> index e74c5b7b64bf..52afe20cc8a1 100644
>> --- a/drivers/char/tpm/tpm2-cmd.c
>> +++ b/drivers/char/tpm/tpm2-cmd.c
>> @@ -799,7 +799,9 @@ int tpm2_probe(struct tpm_chip *chip)
>>       tpm_buf_append_u32(&buf, TPM2_CAP_TPM_PROPERTIES);
>>       tpm_buf_append_u32(&buf, TPM_PT_TOTAL_COMMANDS);
>>       tpm_buf_append_u32(&buf, 1);
>> +    tpm_chip_start(chip);
>>       rc = tpm_transmit_cmd(chip, &buf, 0, NULL);
>> +    tpm_chip_stop(chip);
> Thanks Stefan! I added call to tpm_tis_core as tpm2-cmd.c is to be kept
> out of chip management common case being that you call tpm_try_get_ops(),
> do 1-N TPM commands and release with tpm_put_ops(). These functions take
> care starting and stopping the chip.
>
> I fixed the 2nd issue in the master.

You also need to export the tpm_chip_start/stop symbols.


Master now seems to (still) have a problem when rmmod'ing:

A TPM error (325) occurred stopping the TPM. This happens at the same 
patch where I had to add the tpm_chip_start/stop above.




>
>   /Jarkko
>
Stefan Berger Feb. 8, 2019, 2:14 a.m. UTC | #8 
On 2/7/19 8:51 PM, Stefan Berger wrote:
> On 2/7/19 7:33 PM, Jarkko Sakkinen wrote:
>> On Thu, Feb 07, 2019 at 06:29:43PM -0500, Stefan Berger wrote:
>>> On 2/7/19 4:29 PM, Jarkko Sakkinen wrote:
>
>
>>
>>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>>> index e74c5b7b64bf..52afe20cc8a1 100644
>>> --- a/drivers/char/tpm/tpm2-cmd.c
>>> +++ b/drivers/char/tpm/tpm2-cmd.c
>>> @@ -799,7 +799,9 @@ int tpm2_probe(struct tpm_chip *chip)
>>>       tpm_buf_append_u32(&buf, TPM2_CAP_TPM_PROPERTIES);
>>>       tpm_buf_append_u32(&buf, TPM_PT_TOTAL_COMMANDS);
>>>       tpm_buf_append_u32(&buf, 1);
>>> +    tpm_chip_start(chip);
>>>       rc = tpm_transmit_cmd(chip, &buf, 0, NULL);
>>> +    tpm_chip_stop(chip);
>> Thanks Stefan! I added call to tpm_tis_core as tpm2-cmd.c is to be kept
>> out of chip management common case being that you call 
>> tpm_try_get_ops(),
>> do 1-N TPM commands and release with tpm_put_ops(). These functions take
>> care starting and stopping the chip.
>>
>> I fixed the 2nd issue in the master.
>
> You also need to export the tpm_chip_start/stop symbols.
>
>
> Master now seems to (still) have a problem when rmmod'ing:
>
> A TPM error (325) occurred stopping the TPM. This happens at the same 
> patch where I had to add the tpm_chip_start/stop above.
>
>

Here's my current overall patch against your master. I suppose the other 
tpm2_shutdown() for power management doesn't need the chip start ?

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 9865776ee2cd..f7147706a9c3 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -115,6 +115,7 @@ int tpm_chip_start(struct tpm_chip *chip)

      return 0;
  }
+EXPORT_SYMBOL_GPL(tpm_chip_start);

  /**
   * tpm_chip_stop() - power off the TPM
@@ -131,7 +132,7 @@ void tpm_chip_stop(struct tpm_chip *chip)
      if (chip->ops->clk_enable)
          chip->ops->clk_enable(chip, false);
  }
-
+EXPORT_SYMBOL_GPL(tpm_chip_stop);

  /**
   * tpm_try_get_ops() - Get a ref to the tpm_chip
@@ -474,8 +475,11 @@ static void tpm_del_char_device(struct tpm_chip *chip)

      /* Make the driver uncallable. */
      down_write(&chip->ops_sem);
-    if (chip->flags & TPM_CHIP_FLAG_TPM2)
+    if (chip->flags & TPM_CHIP_FLAG_TPM2) {
+        tpm_chip_start(chip, 0);
          tpm2_shutdown(chip, TPM2_SU_CLEAR);
+        tpm_chip_stop(chip, 0);
+    }
      chip->ops = NULL;
      up_write(&chip->ops_sem);
  }
diff --git a/drivers/char/tpm/tpm-interface.c 
b/drivers/char/tpm/tpm-interface.c
index 02e8cffd1163..fcd845ad8c3c 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -124,6 +124,8 @@ static ssize_t tpm_try_transmit(struct tpm_chip 
*chip, void *buf, size_t bufsiz)
          dev_err(&chip->dev, "tpm_transmit: tpm_recv: error %d\n", rc);
      } else if (len < TPM_HEADER_SIZE || len != 
be32_to_cpu(header->length))
          rc = -EFAULT;
+    else
+        rc = 0;

      return rc ? rc : len;
  }




>
>
>>
>>   /Jarkko
>>
>
Jarkko Sakkinen Feb. 8, 2019, 11:14 a.m. UTC | #9 
On Thu, Feb 07, 2019 at 08:51:15PM -0500, Stefan Berger wrote:
> > > +    rc = 0;
> > >       if (chip->flags & TPM_CHIP_FLAG_IRQ)
> > >           goto out_recv;
> > What why?
> 
> 
> This fix seems to only be necessary when bisecting. You may want to apply
> it!

I don't want to apply it because I don't understand why it would
be needed. Can you give a short explanation?

/Jarkko
Jarkko Sakkinen Feb. 8, 2019, 11:50 a.m. UTC | #10 
On Thu, Feb 07, 2019 at 09:14:54PM -0500, Stefan Berger wrote:
> +EXPORT_SYMBOL_GPL(tpm_chip_start);
> 
>  /**
>   * tpm_chip_stop() - power off the TPM
> @@ -131,7 +132,7 @@ void tpm_chip_stop(struct tpm_chip *chip)
>      if (chip->ops->clk_enable)
>          chip->ops->clk_enable(chip, false);
>  }
> -
> +EXPORT_SYMBOL_GPL(tpm_chip_stop);

These are already fixed.

>  /**
>   * tpm_try_get_ops() - Get a ref to the tpm_chip
> @@ -474,8 +475,11 @@ static void tpm_del_char_device(struct tpm_chip *chip)
> 
>      /* Make the driver uncallable. */
>      down_write(&chip->ops_sem);
> -    if (chip->flags & TPM_CHIP_FLAG_TPM2)
> +    if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> +        tpm_chip_start(chip, 0);
>          tpm2_shutdown(chip, TPM2_SU_CLEAR);
> +        tpm_chip_stop(chip, 0);
> +    }

Thanks!

Needs to be also done in tpm_pm_suspend() but there it is also better to
take locks because in-kernel subsystem might be using TPM even user
space has been freezed (e.g. hibernate possibly in future).

mutex_lock(&chip->tpm_mutex);
if (!tpm_chip_start(chip, 0)) {
	tpm2_shutdown(chip, TPM2_SU_CLEAR);
	tpm_chip_stop(chip, 0);
}
mutex_unlock(&chip->tpm_mutex);

Fixed now.

>      chip->ops = NULL;
>      up_write(&chip->ops_sem);
>  }
> diff --git a/drivers/char/tpm/tpm-interface.c
> b/drivers/char/tpm/tpm-interface.c
> index 02e8cffd1163..fcd845ad8c3c 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -124,6 +124,8 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
> void *buf, size_t bufsiz)
>          dev_err(&chip->dev, "tpm_transmit: tpm_recv: error %d\n", rc);
>      } else if (len < TPM_HEADER_SIZE || len != be32_to_cpu(header->length))
>          rc = -EFAULT;
> +    else
> +        rc = 0;

Why is this needed?

/Jarkko
Stefan Berger Feb. 8, 2019, 12:05 p.m. UTC | #11 
On 2/8/19 6:14 AM, Jarkko Sakkinen wrote:
> On Thu, Feb 07, 2019 at 08:51:15PM -0500, Stefan Berger wrote:
>>>> +    rc = 0;
>>>>        if (chip->flags & TPM_CHIP_FLAG_IRQ)
>>>>            goto out_recv;
>>> What why?
>>
>> This fix seems to only be necessary when bisecting. You may want to apply
>> it!
> I don't want to apply it because I don't understand why it would
> be needed. Can you give a short explanation?


See my comment on [PATCH v11 08/16]. It needs to be added in that patch 
since otherwise rc holds a non-zero value on function exit, which is 
wrong at that point.


>
> /Jarkko
>
Stefan Berger Feb. 8, 2019, 12:22 p.m. UTC | #12 
On 2/8/19 6:50 AM, Jarkko Sakkinen wrote:
> On Thu, Feb 07, 2019 at 09:14:54PM -0500, Stefan Berger wrote:
>
>>       chip->ops = NULL;
>>       up_write(&chip->ops_sem);
>>   }
>> diff --git a/drivers/char/tpm/tpm-interface.c
>> b/drivers/char/tpm/tpm-interface.c
>> index 02e8cffd1163..fcd845ad8c3c 100644
>> --- a/drivers/char/tpm/tpm-interface.c
>> +++ b/drivers/char/tpm/tpm-interface.c
>> @@ -124,6 +124,8 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
>> void *buf, size_t bufsiz)
>>           dev_err(&chip->dev, "tpm_transmit: tpm_recv: error %d\n", rc);
>>       } else if (len < TPM_HEADER_SIZE || len != be32_to_cpu(header->length))
>>           rc = -EFAULT;
>> +    else
>> +        rc = 0;
> Why is this needed?

Because it holds a non-zero value, which is wrong at this point. Below 
it is:

return rc ? rc : len;

It will always return that rc and never 'len'.

It's not just needed for bisecting. I still need it with your latest 
tree. That's the only change I need with my current testing of 
tpm_vtpm_proxy, TIS + TPM 1.2 , TIS + TPM 2.0 , and CRB + TPM 2.0 (with 
QEMU :-) ).

    Stefan
Jarkko Sakkinen Feb. 8, 2019, 1:02 p.m. UTC | #13 
On Fri, Feb 08, 2019 at 07:05:26AM -0500, Stefan Berger wrote:
> See my comment on [PATCH v11 08/16]. It needs to be added in that patch
> since otherwise rc holds a non-zero value on function exit, which is wrong
> at that point.

The snippet in question:

rc = chip->ops->send(chip, buf, count);
if (rc < 0) {
	if (rc != -EPIPE)
		dev_err(&chip->dev,
			"%s: tpm_send: error %d\n", __func__, rc);
	return rc;
}

if (chip->flags & TPM_CHIP_FLAG_IRQ)
	goto out_recv;

'send()' ought to return zero on success case.

This is how the snippet was before applying any patches scheduled for
v5.1:

rc = chip->ops->send(chip, buf, count);
if (rc < 0) {
	if (rc != -EPIPE)
		dev_err(&chip->dev,
			"%s: tpm_send: error %d\n", __func__, rc);
	return rc;
}

if (chip->flags & TPM_CHIP_FLAG_IRQ)
	goto out_recv;

Does not compute.

/Jarkko
Stefan Berger Feb. 8, 2019, 1:10 p.m. UTC | #14 
On 2/8/19 8:02 AM, Jarkko Sakkinen wrote:
> On Fri, Feb 08, 2019 at 07:05:26AM -0500, Stefan Berger wrote:
>> See my comment on [PATCH v11 08/16]. It needs to be added in that patch
>> since otherwise rc holds a non-zero value on function exit, which is wrong
>> at that point.
> The snippet in question:
>
> rc = chip->ops->send(chip, buf, count);
> if (rc < 0) {
> 	if (rc != -EPIPE)
> 		dev_err(&chip->dev,
> 			"%s: tpm_send: error %d\n", __func__, rc);
> 	return rc;
> }
>
> if (chip->flags & TPM_CHIP_FLAG_IRQ)
> 	goto out_recv;
>
> 'send()' ought to return zero on success case.
>
> This is how the snippet was before applying any patches scheduled for
> v5.1:
>
> rc = chip->ops->send(chip, buf, count);
> if (rc < 0) {
> 	if (rc != -EPIPE)
> 		dev_err(&chip->dev,
> 			"%s: tpm_send: error %d\n", __func__, rc);
> 	return rc;
> }
>
> if (chip->flags & TPM_CHIP_FLAG_IRQ)
> 	goto out_recv;
>
> Does not compute.

tpm_tis_send_main returns 'len' and that's what we have here.



>
> /Jarkko
>
Jarkko Sakkinen Feb. 8, 2019, 1:12 p.m. UTC | #15 
On Fri, Feb 08, 2019 at 07:22:48AM -0500, Stefan Berger wrote:
> On 2/8/19 6:50 AM, Jarkko Sakkinen wrote:
> > On Thu, Feb 07, 2019 at 09:14:54PM -0500, Stefan Berger wrote:
> > 
> > >       chip->ops = NULL;
> > >       up_write(&chip->ops_sem);
> > >   }
> > > diff --git a/drivers/char/tpm/tpm-interface.c
> > > b/drivers/char/tpm/tpm-interface.c
> > > index 02e8cffd1163..fcd845ad8c3c 100644
> > > --- a/drivers/char/tpm/tpm-interface.c
> > > +++ b/drivers/char/tpm/tpm-interface.c
> > > @@ -124,6 +124,8 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
> > > void *buf, size_t bufsiz)
> > >           dev_err(&chip->dev, "tpm_transmit: tpm_recv: error %d\n", rc);
> > >       } else if (len < TPM_HEADER_SIZE || len != be32_to_cpu(header->length))
> > >           rc = -EFAULT;
> > > +    else
> > > +        rc = 0;
> > Why is this needed?
> 
> Because it holds a non-zero value, which is wrong at this point. Below it
> is:
> 
> return rc ? rc : len;
> 
> It will always return that rc and never 'len'.
> 
> It's not just needed for bisecting. I still need it with your latest tree.
> That's the only change I need with my current testing of tpm_vtpm_proxy, TIS
> + TPM 1.2 , TIS + TPM 2.0 , and CRB + TPM 2.0 (with QEMU :-) ).

The code is unchaged. If there was a regression that would have been
ages.

/Jarkko
Jarkko Sakkinen Feb. 8, 2019, 1:17 p.m. UTC | #16 
On Fri, Feb 08, 2019 at 08:10:32AM -0500, Stefan Berger wrote:
> On 2/8/19 8:02 AM, Jarkko Sakkinen wrote:
> > On Fri, Feb 08, 2019 at 07:05:26AM -0500, Stefan Berger wrote:
> > > See my comment on [PATCH v11 08/16]. It needs to be added in that patch
> > > since otherwise rc holds a non-zero value on function exit, which is wrong
> > > at that point.
> > The snippet in question:
> > 
> > rc = chip->ops->send(chip, buf, count);
> > if (rc < 0) {
> > 	if (rc != -EPIPE)
> > 		dev_err(&chip->dev,
> > 			"%s: tpm_send: error %d\n", __func__, rc);
> > 	return rc;
> > }
> > 
> > if (chip->flags & TPM_CHIP_FLAG_IRQ)
> > 	goto out_recv;
> > 
> > 'send()' ought to return zero on success case.
> > 
> > This is how the snippet was before applying any patches scheduled for
> > v5.1:
> > 
> > rc = chip->ops->send(chip, buf, count);
> > if (rc < 0) {
> > 	if (rc != -EPIPE)
> > 		dev_err(&chip->dev,
> > 			"%s: tpm_send: error %d\n", __func__, rc);
> > 	return rc;
> > }
> > 
> > if (chip->flags & TPM_CHIP_FLAG_IRQ)
> > 	goto out_recv;
> > 
> > Does not compute.
> 
> tpm_tis_send_main returns 'len' and that's what we have here.

Before doing any kind of code change, we should at least know what
has caused this that it has worked before.

And also which commit caused the regression to happen, because it
looks like a bug in tpm_tis_core, not in the main TPM driver. It
would need the fixes tag and cc to stable.

/Jarkko
Alexander Steffen Feb. 8, 2019, 1:28 p.m. UTC | #17 
On 08.02.2019 03:14, Stefan Berger wrote:
> On 2/7/19 8:51 PM, Stefan Berger wrote:
>> On 2/7/19 7:33 PM, Jarkko Sakkinen wrote:
>>> On Thu, Feb 07, 2019 at 06:29:43PM -0500, Stefan Berger wrote:
>>>> On 2/7/19 4:29 PM, Jarkko Sakkinen wrote:
>>
>>
>>>
>>>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>>>> index e74c5b7b64bf..52afe20cc8a1 100644
>>>> --- a/drivers/char/tpm/tpm2-cmd.c
>>>> +++ b/drivers/char/tpm/tpm2-cmd.c
>>>> @@ -799,7 +799,9 @@ int tpm2_probe(struct tpm_chip *chip)
>>>>       tpm_buf_append_u32(&buf, TPM2_CAP_TPM_PROPERTIES);
>>>>       tpm_buf_append_u32(&buf, TPM_PT_TOTAL_COMMANDS);
>>>>       tpm_buf_append_u32(&buf, 1);
>>>> +    tpm_chip_start(chip);
>>>>       rc = tpm_transmit_cmd(chip, &buf, 0, NULL);
>>>> +    tpm_chip_stop(chip);
>>> Thanks Stefan! I added call to tpm_tis_core as tpm2-cmd.c is to be kept
>>> out of chip management common case being that you call 
>>> tpm_try_get_ops(),
>>> do 1-N TPM commands and release with tpm_put_ops(). These functions take
>>> care starting and stopping the chip.
>>>
>>> I fixed the 2nd issue in the master.
>>
>> You also need to export the tpm_chip_start/stop symbols.
>>
>>
>> Master now seems to (still) have a problem when rmmod'ing:
>>
>> A TPM error (325) occurred stopping the TPM. This happens at the same 
>> patch where I had to add the tpm_chip_start/stop above.
>>
>>
> 
> Here's my current overall patch against your master. I suppose the other 
> tpm2_shutdown() for power management doesn't need the chip start ?
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 9865776ee2cd..f7147706a9c3 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -115,6 +115,7 @@ int tpm_chip_start(struct tpm_chip *chip)
> 
>       return 0;
>   }
> +EXPORT_SYMBOL_GPL(tpm_chip_start);
> 
>   /**
>    * tpm_chip_stop() - power off the TPM
> @@ -131,7 +132,7 @@ void tpm_chip_stop(struct tpm_chip *chip)
>       if (chip->ops->clk_enable)
>           chip->ops->clk_enable(chip, false);
>   }
> -
> +EXPORT_SYMBOL_GPL(tpm_chip_stop);
> 
>   /**
>    * tpm_try_get_ops() - Get a ref to the tpm_chip
> @@ -474,8 +475,11 @@ static void tpm_del_char_device(struct tpm_chip *chip)
> 
>       /* Make the driver uncallable. */
>       down_write(&chip->ops_sem);
> -    if (chip->flags & TPM_CHIP_FLAG_TPM2)
> +    if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> +        tpm_chip_start(chip, 0);
>           tpm2_shutdown(chip, TPM2_SU_CLEAR);
> +        tpm_chip_stop(chip, 0);
> +    }
>       chip->ops = NULL;
>       up_write(&chip->ops_sem);
>   }
> diff --git a/drivers/char/tpm/tpm-interface.c 
> b/drivers/char/tpm/tpm-interface.c
> index 02e8cffd1163..fcd845ad8c3c 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -124,6 +124,8 @@ static ssize_t tpm_try_transmit(struct tpm_chip 
> *chip, void *buf, size_t bufsiz)
>           dev_err(&chip->dev, "tpm_transmit: tpm_recv: error %d\n", rc);
>       } else if (len < TPM_HEADER_SIZE || len != 
> be32_to_cpu(header->length))
>           rc = -EFAULT;
> +    else
> +        rc = 0;
> 
>       return rc ? rc : len;
>   }

Applying those changes fixes all the issues I saw, thanks. I'll retest 
Jarkko's tree without custom patches, once you've agreed on the rc question.

Alexander
Jarkko Sakkinen Feb. 8, 2019, 1:33 p.m. UTC | #18 
On Fri, Feb 08, 2019 at 03:17:54PM +0200, Jarkko Sakkinen wrote:
> On Fri, Feb 08, 2019 at 08:10:32AM -0500, Stefan Berger wrote:
> > On 2/8/19 8:02 AM, Jarkko Sakkinen wrote:
> > > On Fri, Feb 08, 2019 at 07:05:26AM -0500, Stefan Berger wrote:
> > > > See my comment on [PATCH v11 08/16]. It needs to be added in that patch
> > > > since otherwise rc holds a non-zero value on function exit, which is wrong
> > > > at that point.
> > > The snippet in question:
> > > 
> > > rc = chip->ops->send(chip, buf, count);
> > > if (rc < 0) {
> > > 	if (rc != -EPIPE)
> > > 		dev_err(&chip->dev,
> > > 			"%s: tpm_send: error %d\n", __func__, rc);
> > > 	return rc;
> > > }
> > > 
> > > if (chip->flags & TPM_CHIP_FLAG_IRQ)
> > > 	goto out_recv;
> > > 
> > > 'send()' ought to return zero on success case.
> > > 
> > > This is how the snippet was before applying any patches scheduled for
> > > v5.1:
> > > 
> > > rc = chip->ops->send(chip, buf, count);
> > > if (rc < 0) {
> > > 	if (rc != -EPIPE)
> > > 		dev_err(&chip->dev,
> > > 			"%s: tpm_send: error %d\n", __func__, rc);
> > > 	return rc;
> > > }
> > > 
> > > if (chip->flags & TPM_CHIP_FLAG_IRQ)
> > > 	goto out_recv;
> > > 
> > > Does not compute.
> > 
> > tpm_tis_send_main returns 'len' and that's what we have here.
> 
> Before doing any kind of code change, we should at least know what
> has caused this that it has worked before.
> 
> And also which commit caused the regression to happen, because it
> looks like a bug in tpm_tis_core, not in the main TPM driver. It
> would need the fixes tag and cc to stable.

Found the root cause.

len = chip->ops->recv(chip, buf, bufsiz);
if (len < 0) {
	rc = len;
	dev_err(&chip->dev,
		"tpm_transmit: tpm_recv: error %d\n", rc);
	goto out;
} else if (len < TPM_HEADER_SIZE) {
	rc = -EFAULT;
	goto out;
}

if (len != be32_to_cpu(header->length)) {
	rc = -EFAULT;
	goto out;
}

rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
if (rc)
	dev_err(&chip->dev, "tpm2_commit_space: error %d\n", rc);

That unconditional call to commit space masked the bugs in the device
drivers. I'll provide fixes shortly.

/Jarkko
Stefan Berger Feb. 8, 2019, 2:02 p.m. UTC | #19 
On 2/8/19 8:33 AM, Jarkko Sakkinen wrote:
>
> if (len != be32_to_cpu(header->length)) {
> 	rc = -EFAULT;
> 	goto out;
> }
>
> rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
> if (rc)
> 	dev_err(&chip->dev, "tpm2_commit_space: error %d\n", rc);
>
> That unconditional call to commit space masked the bugs in the device
> drivers. I'll provide fixes shortly.

You got it! :-)


>
> /Jarkko
>
Jarkko Sakkinen Feb. 8, 2019, 2:08 p.m. UTC | #20 
On Fri, Feb 08, 2019 at 09:02:22AM -0500, Stefan Berger wrote:
> On 2/8/19 8:33 AM, Jarkko Sakkinen wrote:
> > 
> > if (len != be32_to_cpu(header->length)) {
> > 	rc = -EFAULT;
> > 	goto out;
> > }
> > 
> > rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
> > if (rc)
> > 	dev_err(&chip->dev, "tpm2_commit_space: error %d\n", rc);
> > 
> > That unconditional call to commit space masked the bugs in the device
> > drivers. I'll provide fixes shortly.
> 
> You got it! :-)

Yes, and it should not be fixes in tpm-interface.c. Sent a patch set
for review.

Thanks for spotting this out!

/Jarkko
Jarkko Sakkinen Feb. 8, 2019, 2:09 p.m. UTC | #21 
On Fri, Feb 08, 2019 at 02:28:50PM +0100, Alexander Steffen wrote:
> Applying those changes fixes all the issues I saw, thanks. I'll retest
> Jarkko's tree without custom patches, once you've agreed on the rc question.

Just send a two patch patch set how I believe the regression should
be properly fixed.

Do not have right now dTPM at hand (the NUC that has one does not
respond SSH for some reason) so I need help with testing.

/Jarkko
Alexander Steffen Feb. 8, 2019, 6:02 p.m. UTC | #22 
On 08.02.2019 15:09, Jarkko Sakkinen wrote:
> On Fri, Feb 08, 2019 at 02:28:50PM +0100, Alexander Steffen wrote:
>> Applying those changes fixes all the issues I saw, thanks. I'll retest
>> Jarkko's tree without custom patches, once you've agreed on the rc question.
> 
> Just send a two patch patch set how I believe the regression should
> be properly fixed.
> 
> Do not have right now dTPM at hand (the NUC that has one does not
> respond SSH for some reason) so I need help with testing.

Those patches seem to be changing fast :) I'll test them next week.

Alexander