| Message ID | 1446740353-15235-2-git-send-email-martin.wilck@ts.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 ------------------------------------------------------------------------------
> > 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 ------------------------------------------------------------------------------
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 ------------------------------------------------------------------------------
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;