[tpmdd-devel,1/6] tpm_tis: calculate command durations before irq probing
diff mbox

Message ID 1446740353-15235-2-git-send-email-martin.wilck@ts.fujitsu.com
State New
Headers show

Commit Message

Martin Wilck Nov. 5, 2015, 4:19 p.m. UTC
From: Martin Wilck <Martin.Wilck@ts.fujitsu.com>

For reliable IRQ probing, the expected command durations should
be known before doing the probe.

Also, in the probing algorithm, wait another ms for the IRQ to
arrive, as it is done in the IRQ testing code in tpm_tis_send().

Signed-off-by: Martin Wilck <Martin.Wilck@ts.fujitsu.com>
---
 drivers/char/tpm/tpm_tis.c | 43 ++++++++++++++++++++++++++-----------------
 1 file changed, 26 insertions(+), 17 deletions(-)

Comments

Jarkko Sakkinen Nov. 5, 2015, 9:59 p.m. UTC | #1
On Thu, Nov 05, 2015 at 05:19:08PM +0100, martin.wilck@ts.fujitsu.com wrote:
> From: Martin Wilck <Martin.Wilck@ts.fujitsu.com>
> 
> For reliable IRQ probing, the expected command durations should
> be known before doing the probe.
> 
> Also, in the probing algorithm, wait another ms for the IRQ to
> arrive, as it is done in the IRQ testing code in tpm_tis_send().

NAK from my side unless there is reproducable regression or some other
kind of harm because of the current behavior.

> Signed-off-by: Martin Wilck <Martin.Wilck@ts.fujitsu.com>

/Jarkko

> ---
>  drivers/char/tpm/tpm_tis.c | 43 ++++++++++++++++++++++++++-----------------
>  1 file changed, 26 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index 696ef1d..3ed7e6ce 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -730,6 +730,25 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
>  	if (intfcaps & TPM_INTF_DATA_AVAIL_INT)
>  		dev_dbg(dev, "\tData Avail Int Support\n");
>  
> +	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> +		chip->vendor.timeout_a = msecs_to_jiffies(TPM2_TIMEOUT_A);
> +		chip->vendor.timeout_b = msecs_to_jiffies(TPM2_TIMEOUT_B);
> +		chip->vendor.timeout_c = msecs_to_jiffies(TPM2_TIMEOUT_C);
> +		chip->vendor.timeout_d = msecs_to_jiffies(TPM2_TIMEOUT_D);
> +		chip->vendor.duration[TPM_SHORT] =
> +			msecs_to_jiffies(TPM2_DURATION_SHORT);
> +		chip->vendor.duration[TPM_MEDIUM] =
> +			msecs_to_jiffies(TPM2_DURATION_MEDIUM);
> +		chip->vendor.duration[TPM_LONG] =
> +			msecs_to_jiffies(TPM2_DURATION_LONG);
> +	} else {
> +		if (tpm_get_timeouts(chip)) {
> +			dev_err(dev, "Could not get TPM timeouts and durations\n");
> +			rc = -ENODEV;
> +			goto out_err;
> +		}
> +	}
> +
>  	/* INTERRUPT Setup */
>  	init_waitqueue_head(&chip->vendor.read_queue);
>  	init_waitqueue_head(&chip->vendor.int_queue);
> @@ -759,6 +778,7 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
>  		}
>  
>  		for (i = irq_s; i <= irq_e && chip->vendor.irq == 0; i++) {
> +			dev_dbg(dev, "Probing irq %d\n", i);
>  			iowrite8(i, chip->vendor.iobase +
>  				 TPM_INT_VECTOR(chip->vendor.locality));
>  			if (devm_request_irq
> @@ -790,8 +810,14 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
>  			else
>  				tpm_gen_interrupt(chip);
>  
> +			if (!chip->vendor.probed_irq)
> +				msleep(1);
> +
>  			chip->vendor.irq = chip->vendor.probed_irq;
>  
> +			if (chip->vendor.irq)
> +				dev_info(dev, "Probed IRQ: %d\n", i);
> +
>  			/* free_irq will call into tis_int_probe;
>  			   clear all irqs we haven't seen while doing
>  			   tpm_gen_interrupt */
> @@ -834,17 +860,6 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
>  	}
>  
>  	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> -		chip->vendor.timeout_a = msecs_to_jiffies(TPM2_TIMEOUT_A);
> -		chip->vendor.timeout_b = msecs_to_jiffies(TPM2_TIMEOUT_B);
> -		chip->vendor.timeout_c = msecs_to_jiffies(TPM2_TIMEOUT_C);
> -		chip->vendor.timeout_d = msecs_to_jiffies(TPM2_TIMEOUT_D);
> -		chip->vendor.duration[TPM_SHORT] =
> -			msecs_to_jiffies(TPM2_DURATION_SHORT);
> -		chip->vendor.duration[TPM_MEDIUM] =
> -			msecs_to_jiffies(TPM2_DURATION_MEDIUM);
> -		chip->vendor.duration[TPM_LONG] =
> -			msecs_to_jiffies(TPM2_DURATION_LONG);
> -
>  		rc = tpm2_do_selftest(chip);
>  		if (rc == TPM2_RC_INITIALIZE) {
>  			dev_warn(dev, "Firmware has not started TPM\n");
> @@ -860,12 +875,6 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
>  			goto out_err;
>  		}
>  	} else {
> -		if (tpm_get_timeouts(chip)) {
> -			dev_err(dev, "Could not get TPM timeouts and durations\n");
> -			rc = -ENODEV;
> -			goto out_err;
> -		}
> -
>  		if (tpm_do_selftest(chip)) {
>  			dev_err(dev, "TPM self test failed\n");
>  			rc = -ENODEV;
> -- 
> 1.8.3.1
> 
> 
> ------------------------------------------------------------------------------
> _______________________________________________
> tpmdd-devel mailing list
> tpmdd-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

------------------------------------------------------------------------------
Martin Wilck Nov. 6, 2015, 7:40 a.m. UTC | #2
> > For reliable IRQ probing, the expected command durations should
> > be known before doing the probe.
> > 
> > Also, in the probing algorithm, wait another ms for the IRQ to
> > arrive, as it is done in the IRQ testing code in tpm_tis_send().
> 
> NAK from my side unless there is reproducable regression or some other
> kind of harm because of the current behavior.

A few weeks ago I thought I had seen a regression but that was a mistake
of mine. So no, at this time I have no actual error to report.

But anyway:

tpm_tis_init
   tpm2_gen_interupt
      tpm2_get_tpm_pt
         tpm_transmit_cmd
            tpm_transmit
               tpm2_calc_ordinal_duration

This function accesses
    chip->vendor.duration[tpm2_ordinal_duration[ordinal]]

IMO it's a matter of correctness to initialize these values before
calling tpm2_gen_interrupt. AFAICS, the tpm_chip struct is filled with 0
at initialization time, so before the explicit initialization,
chip->vendor.duration[i] is zero == TPM_SHORT, thus there's an actual
risk that the driver won't wait long enough. Of course you can take a
simpler approach and just fill the duration array with TPM_LONG or
TPM_UNDEFINED in the beginning.

About the msleep(1), we do it in tpm_tis_send(), so why not here?
There is a chance for a race here (tpm2_gen_interrupt may
return before the IRQ handler is actually called), and the additional
wait practically eliminates that.

Regards
Martin


> 
> > Signed-off-by: Martin Wilck <Martin.Wilck@ts.fujitsu.com>
> 
> /Jarkko
> 
> > ---
> >  drivers/char/tpm/tpm_tis.c | 43 ++++++++++++++++++++++++++-----------------
> >  1 file changed, 26 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> > index 696ef1d..3ed7e6ce 100644
> > --- a/drivers/char/tpm/tpm_tis.c
> > +++ b/drivers/char/tpm/tpm_tis.c
> > @@ -730,6 +730,25 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
> >  	if (intfcaps & TPM_INTF_DATA_AVAIL_INT)
> >  		dev_dbg(dev, "\tData Avail Int Support\n");
> >  
> > +	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> > +		chip->vendor.timeout_a = msecs_to_jiffies(TPM2_TIMEOUT_A);
> > +		chip->vendor.timeout_b = msecs_to_jiffies(TPM2_TIMEOUT_B);
> > +		chip->vendor.timeout_c = msecs_to_jiffies(TPM2_TIMEOUT_C);
> > +		chip->vendor.timeout_d = msecs_to_jiffies(TPM2_TIMEOUT_D);
> > +		chip->vendor.duration[TPM_SHORT] =
> > +			msecs_to_jiffies(TPM2_DURATION_SHORT);
> > +		chip->vendor.duration[TPM_MEDIUM] =
> > +			msecs_to_jiffies(TPM2_DURATION_MEDIUM);
> > +		chip->vendor.duration[TPM_LONG] =
> > +			msecs_to_jiffies(TPM2_DURATION_LONG);
> > +	} else {
> > +		if (tpm_get_timeouts(chip)) {
> > +			dev_err(dev, "Could not get TPM timeouts and durations\n");
> > +			rc = -ENODEV;
> > +			goto out_err;
> > +		}
> > +	}
> > +
> >  	/* INTERRUPT Setup */
> >  	init_waitqueue_head(&chip->vendor.read_queue);
> >  	init_waitqueue_head(&chip->vendor.int_queue);
> > @@ -759,6 +778,7 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
> >  		}
> >  
> >  		for (i = irq_s; i <= irq_e && chip->vendor.irq == 0; i++) {
> > +			dev_dbg(dev, "Probing irq %d\n", i);
> >  			iowrite8(i, chip->vendor.iobase +
> >  				 TPM_INT_VECTOR(chip->vendor.locality));
> >  			if (devm_request_irq
> > @@ -790,8 +810,14 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
> >  			else
> >  				tpm_gen_interrupt(chip);
> >  
> > +			if (!chip->vendor.probed_irq)
> > +				msleep(1);
> > +
> >  			chip->vendor.irq = chip->vendor.probed_irq;
> >  
> > +			if (chip->vendor.irq)
> > +				dev_info(dev, "Probed IRQ: %d\n", i);
> > +
> >  			/* free_irq will call into tis_int_probe;
> >  			   clear all irqs we haven't seen while doing
> >  			   tpm_gen_interrupt */
> > @@ -834,17 +860,6 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
> >  	}
> >  
> >  	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> > -		chip->vendor.timeout_a = msecs_to_jiffies(TPM2_TIMEOUT_A);
> > -		chip->vendor.timeout_b = msecs_to_jiffies(TPM2_TIMEOUT_B);
> > -		chip->vendor.timeout_c = msecs_to_jiffies(TPM2_TIMEOUT_C);
> > -		chip->vendor.timeout_d = msecs_to_jiffies(TPM2_TIMEOUT_D);
> > -		chip->vendor.duration[TPM_SHORT] =
> > -			msecs_to_jiffies(TPM2_DURATION_SHORT);
> > -		chip->vendor.duration[TPM_MEDIUM] =
> > -			msecs_to_jiffies(TPM2_DURATION_MEDIUM);
> > -		chip->vendor.duration[TPM_LONG] =
> > -			msecs_to_jiffies(TPM2_DURATION_LONG);
> > -
> >  		rc = tpm2_do_selftest(chip);
> >  		if (rc == TPM2_RC_INITIALIZE) {
> >  			dev_warn(dev, "Firmware has not started TPM\n");
> > @@ -860,12 +875,6 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
> >  			goto out_err;
> >  		}
> >  	} else {
> > -		if (tpm_get_timeouts(chip)) {
> > -			dev_err(dev, "Could not get TPM timeouts and durations\n");
> > -			rc = -ENODEV;
> > -			goto out_err;
> > -		}
> > -
> >  		if (tpm_do_selftest(chip)) {
> >  			dev_err(dev, "TPM self test failed\n");
> >  			rc = -ENODEV;
> > -- 
> > 1.8.3.1
> > 
> > 
> > ------------------------------------------------------------------------------
> > _______________________________________________
> > tpmdd-devel mailing list
> > tpmdd-devel@lists.sourceforge.net
> > https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
------------------------------------------------------------------------------
Jarkko Sakkinen Nov. 6, 2015, 2:06 p.m. UTC | #3
On Fri, Nov 06, 2015 at 08:40:14AM +0100, Wilck, Martin wrote:
> > > For reliable IRQ probing, the expected command durations should
> > > be known before doing the probe.
> > > 
> > > Also, in the probing algorithm, wait another ms for the IRQ to
> > > arrive, as it is done in the IRQ testing code in tpm_tis_send().
> > 
> > NAK from my side unless there is reproducable regression or some other
> > kind of harm because of the current behavior.
> 
> A few weeks ago I thought I had seen a regression but that was a mistake
> of mine. So no, at this time I have no actual error to report.
> 
> But anyway:
> 
> tpm_tis_init
>    tpm2_gen_interupt
>       tpm2_get_tpm_pt
>          tpm_transmit_cmd
>             tpm_transmit
>                tpm2_calc_ordinal_duration
> 
> This function accesses
>     chip->vendor.duration[tpm2_ordinal_duration[ordinal]]
> 
> IMO it's a matter of correctness to initialize these values before
> calling tpm2_gen_interrupt. AFAICS, the tpm_chip struct is filled with 0
> at initialization time, so before the explicit initialization,
> chip->vendor.duration[i] is zero == TPM_SHORT, thus there's an actual
> risk that the driver won't wait long enough. Of course you can take a
> simpler approach and just fill the duration array with TPM_LONG or
> TPM_UNDEFINED in the beginning.

Got you, your change *does* make sense but the current commit message
does not. It should explain the regressino, which is that we end up
using TPM_SHORT for everything. That's why I didn't understand it first.

Please make a separate fix from this.

> About the msleep(1), we do it in tpm_tis_send(), so why not here?
> There is a chance for a race here (tpm2_gen_interrupt may
> return before the IRQ handler is actually called), and the additional
> wait practically eliminates that.

This should be a separate patch.

> Regards
> Martin

/Jarkko

------------------------------------------------------------------------------

Patch
diff mbox

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 696ef1d..3ed7e6ce 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -730,6 +730,25 @@  static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
 	if (intfcaps & TPM_INTF_DATA_AVAIL_INT)
 		dev_dbg(dev, "\tData Avail Int Support\n");
 
+	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
+		chip->vendor.timeout_a = msecs_to_jiffies(TPM2_TIMEOUT_A);
+		chip->vendor.timeout_b = msecs_to_jiffies(TPM2_TIMEOUT_B);
+		chip->vendor.timeout_c = msecs_to_jiffies(TPM2_TIMEOUT_C);
+		chip->vendor.timeout_d = msecs_to_jiffies(TPM2_TIMEOUT_D);
+		chip->vendor.duration[TPM_SHORT] =
+			msecs_to_jiffies(TPM2_DURATION_SHORT);
+		chip->vendor.duration[TPM_MEDIUM] =
+			msecs_to_jiffies(TPM2_DURATION_MEDIUM);
+		chip->vendor.duration[TPM_LONG] =
+			msecs_to_jiffies(TPM2_DURATION_LONG);
+	} else {
+		if (tpm_get_timeouts(chip)) {
+			dev_err(dev, "Could not get TPM timeouts and durations\n");
+			rc = -ENODEV;
+			goto out_err;
+		}
+	}
+
 	/* INTERRUPT Setup */
 	init_waitqueue_head(&chip->vendor.read_queue);
 	init_waitqueue_head(&chip->vendor.int_queue);
@@ -759,6 +778,7 @@  static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
 		}
 
 		for (i = irq_s; i <= irq_e && chip->vendor.irq == 0; i++) {
+			dev_dbg(dev, "Probing irq %d\n", i);
 			iowrite8(i, chip->vendor.iobase +
 				 TPM_INT_VECTOR(chip->vendor.locality));
 			if (devm_request_irq
@@ -790,8 +810,14 @@  static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
 			else
 				tpm_gen_interrupt(chip);
 
+			if (!chip->vendor.probed_irq)
+				msleep(1);
+
 			chip->vendor.irq = chip->vendor.probed_irq;
 
+			if (chip->vendor.irq)
+				dev_info(dev, "Probed IRQ: %d\n", i);
+
 			/* free_irq will call into tis_int_probe;
 			   clear all irqs we haven't seen while doing
 			   tpm_gen_interrupt */
@@ -834,17 +860,6 @@  static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
 	}
 
 	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
-		chip->vendor.timeout_a = msecs_to_jiffies(TPM2_TIMEOUT_A);
-		chip->vendor.timeout_b = msecs_to_jiffies(TPM2_TIMEOUT_B);
-		chip->vendor.timeout_c = msecs_to_jiffies(TPM2_TIMEOUT_C);
-		chip->vendor.timeout_d = msecs_to_jiffies(TPM2_TIMEOUT_D);
-		chip->vendor.duration[TPM_SHORT] =
-			msecs_to_jiffies(TPM2_DURATION_SHORT);
-		chip->vendor.duration[TPM_MEDIUM] =
-			msecs_to_jiffies(TPM2_DURATION_MEDIUM);
-		chip->vendor.duration[TPM_LONG] =
-			msecs_to_jiffies(TPM2_DURATION_LONG);
-
 		rc = tpm2_do_selftest(chip);
 		if (rc == TPM2_RC_INITIALIZE) {
 			dev_warn(dev, "Firmware has not started TPM\n");
@@ -860,12 +875,6 @@  static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
 			goto out_err;
 		}
 	} else {
-		if (tpm_get_timeouts(chip)) {
-			dev_err(dev, "Could not get TPM timeouts and durations\n");
-			rc = -ENODEV;
-			goto out_err;
-		}
-
 		if (tpm_do_selftest(chip)) {
 			dev_err(dev, "TPM self test failed\n");
 			rc = -ENODEV;