Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1446474 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v3 3/5] tpm: return error code from tpm_gen_interrupt() Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-07-19 15:40 +0200
    Re: [PATCH v3 3/5] tpm: return error code from tpm_gen_interrupt() Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-07-19 22:30 +0200
      Re: [PATCH v3 3/5] tpm: return error code from tpm_gen_interrupt() Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-07-19 22:40 +0200
        Re: [tpmdd-devel] [PATCH v3 3/5] tpm: return error code from  tpm_gen_interrupt() Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-07-19 22:40 +0200
        Re: [PATCH v3 3/5] tpm: return error code from tpm_gen_interrupt() Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-07-19 22:50 +0200
          Re: [PATCH v3 3/5] tpm: return error code from tpm_gen_interrupt() Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-07-19 23:00 +0200
      Re: [PATCH v3 3/5] tpm: return error code from tpm_gen_interrupt() Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2016-07-19 22:40 +0200

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

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

[toc] | [next] | [standalone]

#1446712

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

[toc] | [prev] | [next] | [standalone]

#1446714

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1446728

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

[toc] | [prev] | [next] | [standalone]

#1446730

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

[toc] | [prev] | [next] | [standalone]

#1446721

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web