[PATCH v3 0/5] Use tpm_transmit_cmd() consistently across kernel call sites

[PATCH v3 0/5] Use tpm_transmit_cmd() consistently across kernel call sites

[Jarkko Sakkinen]

These commits update the subsystem consistently to use
tpm_transmit_cmd() throughout the subsystem the exception being
tpm_write() where it makes sense to use the raw interface because higher
level command handling is delegated to user space. The point is to make
semantics and error handling across the kernel call sites.

v3: Removed patch that renames tpm_pcr_read_dev() to tpm1_pcr_read()
because it is not mandatory for this patch set.

v2: Added commit that drops externs from all function declarations in
tpm.h as suggested by Jason Gunthorpe.

Jarkko Sakkinen (5):
  tpm: remove unnecessary externs from tpm.h
  tpm: unify tpm_gen_interrupt()
  tpm: return error code from tpm_gen_interrupt()
  tpm: use tpm_transmit_cmd() in tpm2_probe()
  tpm: use tpm_pcr_read_dev() in tpm_do_selftest()

 drivers/char/tpm/tpm-interface.c | 32 +++++++++++++++++---------------
 drivers/char/tpm/tpm.h           | 35 +++++++++++++++++------------------
 drivers/char/tpm/tpm2-cmd.c      | 21 +--------------------
 drivers/char/tpm/tpm_tis_core.c  |  7 +++----
 4 files changed, 38 insertions(+), 57 deletions(-)

-- 
2.7.4

[PATCH v3 1/5] tpm: remove unnecessary externs from tpm.h

[Jarkko Sakkinen]

Removed unnecessary externs from tpm.h.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm.h | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 3e32d5b..8b864dd 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -482,26 +482,26 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
 		     size_t bufsiz);
 ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd, int len,
 			 const char *desc);
-extern int tpm_get_timeouts(struct tpm_chip *);
-extern void tpm_gen_interrupt(struct tpm_chip *);
+int tpm_get_timeouts(struct tpm_chip *chip);
+void tpm_gen_interrupt(struct tpm_chip *chip);
 int tpm1_auto_startup(struct tpm_chip *chip);
-extern int tpm_do_selftest(struct tpm_chip *);
-extern unsigned long tpm_calc_ordinal_duration(struct tpm_chip *, u32);
-extern int tpm_pm_suspend(struct device *);
-extern int tpm_pm_resume(struct device *);
-extern int wait_for_tpm_stat(struct tpm_chip *, u8, unsigned long,
-			     wait_queue_head_t *, bool);
+int tpm_do_selftest(struct tpm_chip *chip);
+unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
+int tpm_pm_suspend(struct device *dev);
+int tpm_pm_resume(struct device *dev);
+int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
+		      wait_queue_head_t *queue, bool check_cancel);
 
 struct tpm_chip *tpm_chip_find_get(int chip_num);
 __must_check int tpm_try_get_ops(struct tpm_chip *chip);
 void tpm_put_ops(struct tpm_chip *chip);
 
-extern struct tpm_chip *tpm_chip_alloc(struct device *dev,
-				       const struct tpm_class_ops *ops);
-extern struct tpm_chip *tpmm_chip_alloc(struct device *pdev,
-				       const struct tpm_class_ops *ops);
-extern int tpm_chip_register(struct tpm_chip *chip);
-extern void tpm_chip_unregister(struct tpm_chip *chip);
+struct tpm_chip *tpm_chip_alloc(struct device *dev,
+				const struct tpm_class_ops *ops);
+struct tpm_chip *tpmm_chip_alloc(struct device *pdev,
+				 const struct tpm_class_ops *ops);
+int tpm_chip_register(struct tpm_chip *chip);
+void tpm_chip_unregister(struct tpm_chip *chip);
 
 void tpm_sysfs_add_device(struct tpm_chip *chip);
 
@@ -528,8 +528,8 @@ ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id,
 			u32 *value, const char *desc);
 
 int tpm2_auto_startup(struct tpm_chip *chip);
-extern void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type);
-extern unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *, u32);
-extern int tpm2_gen_interrupt(struct tpm_chip *chip);
-extern int tpm2_probe(struct tpm_chip *chip);
+void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type);
+unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
+int tpm2_gen_interrupt(struct tpm_chip *chip);
+int tpm2_probe(struct tpm_chip *chip);
 #endif
-- 
2.7.4

[PATCH v3 2/5] tpm: unify tpm_gen_interrupt()

[Jarkko Sakkinen]

Migrated into single tpm_gen_interrupt() function and cleaned up the
whole construction in general because it was starting to turn into a
train wreck.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm-interface.c | 23 ++++++++++++++---------
 drivers/char/tpm/tpm.h           |  1 -
 drivers/char/tpm/tpm2-cmd.c      | 17 -----------------
 drivers/char/tpm/tpm_tis_core.c  |  5 +----
 4 files changed, 15 insertions(+), 31 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 1abe2d7..88dafcd 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -459,18 +459,23 @@ ssize_t tpm_getcap(struct tpm_chip *chip, __be32 subcap_id, cap_t *cap,
 	return rc;
 }
 
+/**
+ * tpm_gen_interrupt -- generate an interrupt by issuing an idempotent command
+ * @chip: TPM chip to use
+ *
+ * Returns 0 on success, < 0 in case of fatal error or a value > 0 representing
+ * a TPM error code.
+ */
 void tpm_gen_interrupt(struct tpm_chip *chip)
 {
-	struct	tpm_cmd_t tpm_cmd;
-	ssize_t rc;
+	const char *desc = "attempting to generate an interrupt";
+	u32 cap2;
+	cap_t cap;
 
-	tpm_cmd.header.in = tpm_getcap_header;
-	tpm_cmd.params.getcap_in.cap = TPM_CAP_PROP;
-	tpm_cmd.params.getcap_in.subcap_size = cpu_to_be32(4);
-	tpm_cmd.params.getcap_in.subcap = TPM_CAP_PROP_TIS_TIMEOUT;
-
-	rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE,
-			      "attempting to determine the timeouts");
+	if (chip->flags & TPM_CHIP_FLAG_TPM2)
+		tpm2_get_tpm_pt(chip, 0x100, &cap2, desc);
+	else
+		tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc);
 }
 EXPORT_SYMBOL_GPL(tpm_gen_interrupt);
 
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 8b864dd..ec1f877 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -530,6 +530,5 @@ ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id,
 int tpm2_auto_startup(struct tpm_chip *chip);
 void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type);
 unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
-int tpm2_gen_interrupt(struct tpm_chip *chip);
 int tpm2_probe(struct tpm_chip *chip);
 #endif
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 08c7e23..1c393ba 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -895,23 +895,6 @@ static int tpm2_do_selftest(struct tpm_chip *chip)
 }
 
 /**
- * tpm2_gen_interrupt() - generate an interrupt
- * @chip: TPM chip to use
- *
- * 0 is returned when the operation is successful. If a negative number is
- * returned it remarks a POSIX error code. If a positive number is returned
- * it remarks a TPM error.
- */
-int tpm2_gen_interrupt(struct tpm_chip *chip)
-{
-	u32 dummy;
-
-	return tpm2_get_tpm_pt(chip, 0x100, &dummy,
-			       "attempting to generate an interrupt");
-}
-EXPORT_SYMBOL_GPL(tpm2_gen_interrupt);
-
-/**
  * tpm2_probe() - probe TPM 2.0
  * @chip: TPM chip to use
  *
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index d66f51b..b67d225 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -575,10 +575,7 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
 	/* Generate an interrupt by having the core call through to
 	 * tpm_tis_send
 	 */
-	if (chip->flags & TPM_CHIP_FLAG_TPM2)
-		tpm2_gen_interrupt(chip);
-	else
-		tpm_gen_interrupt(chip);
+	tpm_gen_interrupt(chip);
 
 	/* tpm_tis_send will either confirm the interrupt is working or it
 	 * will call disable_irq which undoes all of the above.
-- 
2.7.4

[PATCH v3 3/5] tpm: return error code from tpm_gen_interrupt()

[Jarkko Sakkinen]

Return error code from tpm_gen_interrupt() and fail tpm_tis family of
drivers on a system error. It doesn't make sense to continue if we
cannot even reach the TPM.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm-interface.c | 6 +++---
 drivers/char/tpm/tpm.h           | 2 +-
 drivers/char/tpm/tpm_tis_core.c  | 4 +++-
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 88dafcd..35b2722 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -466,16 +466,16 @@ ssize_t tpm_getcap(struct tpm_chip *chip, __be32 subcap_id, cap_t *cap,
  * Returns 0 on success, < 0 in case of fatal error or a value > 0 representing
  * a TPM error code.
  */
-void tpm_gen_interrupt(struct tpm_chip *chip)
+int tpm_gen_interrupt(struct tpm_chip *chip)
 {
 	const char *desc = "attempting to generate an interrupt";
 	u32 cap2;
 	cap_t cap;
 
 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
-		tpm2_get_tpm_pt(chip, 0x100, &cap2, desc);
+		return tpm2_get_tpm_pt(chip, 0x100, &cap2, desc);
 	else
-		tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc);
+		return tpm_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc);
 }
 EXPORT_SYMBOL_GPL(tpm_gen_interrupt);
 
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index ec1f877..0cbb598 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -483,7 +483,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
 ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd, int len,
 			 const char *desc);
 int tpm_get_timeouts(struct tpm_chip *chip);
-void tpm_gen_interrupt(struct tpm_chip *chip);
+int tpm_gen_interrupt(struct tpm_chip *chip);
 int tpm1_auto_startup(struct tpm_chip *chip);
 int tpm_do_selftest(struct tpm_chip *chip);
 unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index b67d225..45159e1 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -575,7 +575,9 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask,
 	/* Generate an interrupt by having the core call through to
 	 * tpm_tis_send
 	 */
-	tpm_gen_interrupt(chip);
+	rc = tpm_gen_interrupt(chip);
+	if (rc < 0)
+		return rc;
 
 	/* tpm_tis_send will either confirm the interrupt is working or it
 	 * will call disable_irq which undoes all of the above.
-- 
2.7.4

[PATCH v3 4/5] tpm: use tpm_transmit_cmd() in tpm2_probe()

[Jarkko Sakkinen]

It is better to tpm_transmit_cmd() in tpm2_probe() in order to get
consistent command handling throughout the subsystem.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm2-cmd.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 1c393ba..e1db404 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -911,11 +911,9 @@ int tpm2_probe(struct tpm_chip *chip)
 	cmd.params.get_tpm_pt_in.property_id = cpu_to_be32(0x100);
 	cmd.params.get_tpm_pt_in.property_cnt = cpu_to_be32(1);
 
-	rc = tpm_transmit(chip, (const char *) &cmd, sizeof(cmd));
+	rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), NULL);
 	if (rc <  0)
 		return rc;
-	else if (rc < TPM_HEADER_SIZE)
-		return -EFAULT;
 
 	if (be16_to_cpu(cmd.header.out.tag) == TPM2_ST_NO_SESSIONS)
 		chip->flags |= TPM_CHIP_FLAG_TPM2;
-- 
2.7.4

[PATCH v3 5/5] tpm: use tpm_pcr_read_dev() in tpm_do_selftest()

[Jarkko Sakkinen]

Remove ad-hoc protocol message construction and call instead
tpm_pcr_read_dev().

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm-interface.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 35b2722..2db1610e 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -797,7 +797,7 @@ int tpm_do_selftest(struct tpm_chip *chip)
 	unsigned int loops;
 	unsigned int delay_msec = 100;
 	unsigned long duration;
-	struct tpm_cmd_t cmd;
+	u8 dummy[TPM_DIGEST_SIZE];
 
 	duration = tpm_calc_ordinal_duration(chip, TPM_ORD_CONTINUE_SELFTEST);
 
@@ -812,9 +812,7 @@ int tpm_do_selftest(struct tpm_chip *chip)
 
 	do {
 		/* Attempt to read a PCR value */
-		cmd.header.in = pcrread_header;
-		cmd.params.pcrread_in.pcr_idx = cpu_to_be32(0);
-		rc = tpm_transmit(chip, (u8 *) &cmd, READ_PCR_RESULT_SIZE);
+		rc = tpm_pcr_read_dev(chip, 0, dummy);
 		/* Some buggy TPMs will not respond to tpm_tis_ready() for
 		 * around 300ms while the self test is ongoing, keep trying
 		 * until the self test duration expires. */
@@ -829,7 +827,6 @@ int tpm_do_selftest(struct tpm_chip *chip)
 		if (rc < TPM_HEADER_SIZE)
 			return -EFAULT;
 
-		rc = be32_to_cpu(cmd.header.out.return_code);
 		if (rc == TPM_ERR_DISABLED || rc == TPM_ERR_DEACTIVATED) {
 			dev_info(&chip->dev,
 				 "TPM is disabled/deactivated (0x%X)\n", rc);
-- 
2.7.4

Re: [PATCH v3 3/5] tpm: return error code from tpm_gen_interrupt()

[Jason Gunthorpe]

On Tue, Jul 19, 2016 at 04:32:47PM +0300, Jarkko Sakkinen wrote:
> Return error code from tpm_gen_interrupt() and fail tpm_tis family of
> drivers on a system error. It doesn't make sense to continue if we
> cannot even reach the TPM.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>  drivers/char/tpm/tpm-interface.c | 6 +++---
>  drivers/char/tpm/tpm.h           | 2 +-
>  drivers/char/tpm/tpm_tis_core.c  | 4 +++-
>  3 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 88dafcd..35b2722 100644
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -466,16 +466,16 @@ ssize_t tpm_getcap(struct tpm_chip *chip, __be32 subcap_id, cap_t *cap,
>   * Returns 0 on success, < 0 in case of fatal error or a value > 0 representing
>   * a TPM error code.
>   */
> -void tpm_gen_interrupt(struct tpm_chip *chip)
> +int tpm_gen_interrupt(struct tpm_chip *chip)

drivers/char/tpm/st33zp24/st33zp24.c needs to be updated too.

I looked at st33zp24.c and it looks broken, I don't see any logic that
de-asserts TPM_CHIP_FLAG_IRQ if the irq test triggered by
tpm_gen_interrupt, so presumably it should not be calling it at all.

IMHO, DT binding devices should never auto-probe IRQS anyhow, we only
do it on PC because PC is insane...

If we fix st33 then I suggest just moving tpm_gen_interrupt into
tpm_tis - nothing else should really be using it..

Jason

Re: [PATCH v3 0/5] Use tpm_transmit_cmd() consistently across kernel call sites

[Jason Gunthorpe]

On Tue, Jul 19, 2016 at 04:32:44PM +0300, Jarkko Sakkinen wrote:
> These commits update the subsystem consistently to use
> tpm_transmit_cmd() throughout the subsystem the exception being
> tpm_write() where it makes sense to use the raw interface because higher
> level command handling is delegated to user space. The point is to make
> semantics and error handling across the kernel call sites.
> 
> v3: Removed patch that renames tpm_pcr_read_dev() to tpm1_pcr_read()
> because it is not mandatory for this patch set.
> 
> v2: Added commit that drops externs from all function declarations in
> tpm.h as suggested by Jason Gunthorpe.

Other than my comment:

Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

Jason

Re: [PATCH v3 3/5] tpm: return error code from tpm_gen_interrupt()

[Jarkko Sakkinen]

On Tue, Jul 19, 2016 at 02:27:41PM -0600, Jason Gunthorpe wrote:
> On Tue, Jul 19, 2016 at 04:32:47PM +0300, Jarkko Sakkinen wrote:
> > Return error code from tpm_gen_interrupt() and fail tpm_tis family of
> > drivers on a system error. It doesn't make sense to continue if we
> > cannot even reach the TPM.
> > 
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> >  drivers/char/tpm/tpm-interface.c | 6 +++---
> >  drivers/char/tpm/tpm.h           | 2 +-
> >  drivers/char/tpm/tpm_tis_core.c  | 4 +++-
> >  3 files changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> > index 88dafcd..35b2722 100644
> > +++ b/drivers/char/tpm/tpm-interface.c
> > @@ -466,16 +466,16 @@ ssize_t tpm_getcap(struct tpm_chip *chip, __be32 subcap_id, cap_t *cap,
> >   * Returns 0 on success, < 0 in case of fatal error or a value > 0 representing
> >   * a TPM error code.
> >   */
> > -void tpm_gen_interrupt(struct tpm_chip *chip)
> > +int tpm_gen_interrupt(struct tpm_chip *chip)
> 
> drivers/char/tpm/st33zp24/st33zp24.c needs to be updated too.
> 
> I looked at st33zp24.c and it looks broken, I don't see any logic that
> de-asserts TPM_CHIP_FLAG_IRQ if the irq test triggered by
> tpm_gen_interrupt, so presumably it should not be calling it at all.
> 
> IMHO, DT binding devices should never auto-probe IRQS anyhow, we only
> do it on PC because PC is insane...
> 
> If we fix st33 then I suggest just moving tpm_gen_interrupt into
> tpm_tis - nothing else should really be using it..

I'm happy to take fix for st33 but not the move because it does not
matter for the release.

/Jarkko

Re: [PATCH v3 3/5] tpm: return error code from tpm_gen_interrupt()

[Jarkko Sakkinen]

On Tue, Jul 19, 2016 at 11:31:47PM +0300, Jarkko Sakkinen wrote:
> On Tue, Jul 19, 2016 at 02:27:41PM -0600, Jason Gunthorpe wrote:
> > On Tue, Jul 19, 2016 at 04:32:47PM +0300, Jarkko Sakkinen wrote:
> > > Return error code from tpm_gen_interrupt() and fail tpm_tis family of
> > > drivers on a system error. It doesn't make sense to continue if we
> > > cannot even reach the TPM.
> > > 
> > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > >  drivers/char/tpm/tpm-interface.c | 6 +++---
> > >  drivers/char/tpm/tpm.h           | 2 +-
> > >  drivers/char/tpm/tpm_tis_core.c  | 4 +++-
> > >  3 files changed, 7 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> > > index 88dafcd..35b2722 100644
> > > +++ b/drivers/char/tpm/tpm-interface.c
> > > @@ -466,16 +466,16 @@ ssize_t tpm_getcap(struct tpm_chip *chip, __be32 subcap_id, cap_t *cap,
> > >   * Returns 0 on success, < 0 in case of fatal error or a value > 0 representing
> > >   * a TPM error code.
> > >   */
> > > -void tpm_gen_interrupt(struct tpm_chip *chip)
> > > +int tpm_gen_interrupt(struct tpm_chip *chip)
> > 
> > drivers/char/tpm/st33zp24/st33zp24.c needs to be updated too.
> > 
> > I looked at st33zp24.c and it looks broken, I don't see any logic that
> > de-asserts TPM_CHIP_FLAG_IRQ if the irq test triggered by
> > tpm_gen_interrupt, so presumably it should not be calling it at all.
> > 
> > IMHO, DT binding devices should never auto-probe IRQS anyhow, we only
> > do it on PC because PC is insane...
> > 
> > If we fix st33 then I suggest just moving tpm_gen_interrupt into
> > tpm_tis - nothing else should really be using it..
> 
> I'm happy to take fix for st33 but not the move because it does not
> matter for the release.

Ignore this comment, this series is not anyway going to 4.8 release.

If Christophe could submit a fix for st33, I could include it to this
series and make one more revision. Thank you for reviewing this!

/Jarkko

Re: [tpmdd-devel] [PATCH v3 3/5] tpm: return error code from tpm_gen_interrupt()

[Jarkko Sakkinen]

On Tue, Jul 19, 2016 at 11:36:34PM +0300, Jarkko Sakkinen wrote:
> On Tue, Jul 19, 2016 at 11:31:47PM +0300, Jarkko Sakkinen wrote:
> > On Tue, Jul 19, 2016 at 02:27:41PM -0600, Jason Gunthorpe wrote:
> > > On Tue, Jul 19, 2016 at 04:32:47PM +0300, Jarkko Sakkinen wrote:
> > > > Return error code from tpm_gen_interrupt() and fail tpm_tis family of
> > > > drivers on a system error. It doesn't make sense to continue if we
> > > > cannot even reach the TPM.
> > > > 
> > > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > >  drivers/char/tpm/tpm-interface.c | 6 +++---
> > > >  drivers/char/tpm/tpm.h           | 2 +-
> > > >  drivers/char/tpm/tpm_tis_core.c  | 4 +++-
> > > >  3 files changed, 7 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> > > > index 88dafcd..35b2722 100644
> > > > +++ b/drivers/char/tpm/tpm-interface.c
> > > > @@ -466,16 +466,16 @@ ssize_t tpm_getcap(struct tpm_chip *chip, __be32 subcap_id, cap_t *cap,
> > > >   * Returns 0 on success, < 0 in case of fatal error or a value > 0 representing
> > > >   * a TPM error code.
> > > >   */
> > > > -void tpm_gen_interrupt(struct tpm_chip *chip)
> > > > +int tpm_gen_interrupt(struct tpm_chip *chip)
> > > 
> > > drivers/char/tpm/st33zp24/st33zp24.c needs to be updated too.
> > > 
> > > I looked at st33zp24.c and it looks broken, I don't see any logic that
> > > de-asserts TPM_CHIP_FLAG_IRQ if the irq test triggered by
> > > tpm_gen_interrupt, so presumably it should not be calling it at all.
> > > 
> > > IMHO, DT binding devices should never auto-probe IRQS anyhow, we only
> > > do it on PC because PC is insane...
> > > 
> > > If we fix st33 then I suggest just moving tpm_gen_interrupt into
> > > tpm_tis - nothing else should really be using it..
> > 
> > I'm happy to take fix for st33 but not the move because it does not
> > matter for the release.
> 
> Ignore this comment, this series is not anyway going to 4.8 release.
> 
> If Christophe could submit a fix for st33, I could include it to this
> series and make one more revision. Thank you for reviewing this!

Or alternatively if you can provide the fix, Christophe could test it.

/Jarkko

Re: [PATCH v3 3/5] tpm: return error code from tpm_gen_interrupt()

[Jason Gunthorpe]

On Tue, Jul 19, 2016 at 11:36:34PM +0300, Jarkko Sakkinen wrote:
> If Christophe could submit a fix for st33, I could include it to this
> series and make one more revision. Thank you for reviewing this!

Here is a commit:

>From 5e178858dcdc2bff9ac31f9851db52370cc282cb Mon Sep 17 00:00:00 2001
From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Date: Tue, 19 Jul 2016 14:38:55 -0600
Subject: [PATCH] tpm/st33zp24: Remove useless tpm_gen_interrupt

This function should only be called as part of an IRQ probing protocol
and st33 does not have any code to detect that the IRQ it tries to
generate was not generated and disable the IRQ.

Since st33 is primarily a DT binding driver it should not be doing
IRQ probing anyhow, so let us just delete this useless call.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 drivers/char/tpm/st33zp24/st33zp24.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/char/tpm/st33zp24/st33zp24.c b/drivers/char/tpm/st33zp24/st33zp24.c
index c2ee30451e41..6f060c76217b 100644
--- a/drivers/char/tpm/st33zp24/st33zp24.c
+++ b/drivers/char/tpm/st33zp24/st33zp24.c
@@ -589,8 +589,6 @@ int st33zp24_probe(void *phy_id, const struct st33zp24_phy_ops *ops,
 		chip->flags |= TPM_CHIP_FLAG_IRQ;
 
 		disable_irq_nosync(tpm_dev->irq);
-
-		tpm_gen_interrupt(chip);
 	}
 
 	return tpm_chip_register(chip);
-- 
2.1.4

Re: [PATCH v3 3/5] tpm: return error code from tpm_gen_interrupt()

[Jarkko Sakkinen]

On Tue, Jul 19, 2016 at 02:40:27PM -0600, Jason Gunthorpe wrote:
> On Tue, Jul 19, 2016 at 11:36:34PM +0300, Jarkko Sakkinen wrote:
> > If Christophe could submit a fix for st33, I could include it to this
> > series and make one more revision. Thank you for reviewing this!
> 
> Here is a commit:

Right I see. I just read through that file and now I understand the
context.

I revamp one more revision of the series with tpm_gen_interrupt()
moved as internal function for tis.

/Jarkko

> From 5e178858dcdc2bff9ac31f9851db52370cc282cb Mon Sep 17 00:00:00 2001
> From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Date: Tue, 19 Jul 2016 14:38:55 -0600
> Subject: [PATCH] tpm/st33zp24: Remove useless tpm_gen_interrupt
> 
> This function should only be called as part of an IRQ probing protocol
> and st33 does not have any code to detect that the IRQ it tries to
> generate was not generated and disable the IRQ.
> 
> Since st33 is primarily a DT binding driver it should not be doing
> IRQ probing anyhow, so let us just delete this useless call.
> 
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> ---
>  drivers/char/tpm/st33zp24/st33zp24.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/char/tpm/st33zp24/st33zp24.c b/drivers/char/tpm/st33zp24/st33zp24.c
> index c2ee30451e41..6f060c76217b 100644
> --- a/drivers/char/tpm/st33zp24/st33zp24.c
> +++ b/drivers/char/tpm/st33zp24/st33zp24.c
> @@ -589,8 +589,6 @@ int st33zp24_probe(void *phy_id, const struct st33zp24_phy_ops *ops,
>  		chip->flags |= TPM_CHIP_FLAG_IRQ;
>  
>  		disable_irq_nosync(tpm_dev->irq);
> -
> -		tpm_gen_interrupt(chip);
>  	}
>  
>  	return tpm_chip_register(chip);
> -- 
> 2.1.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html