| Message ID | 1412712189-1234-12-git-send-email-christophe-h.ricard@st.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tue, Oct 07, 2014 at 10:03:04PM +0200, Christophe Ricard wrote: > Remove useless i2c read on TPM_INT_ENABLE and TPM_INT_STATUS > > Signed-off-by: Christophe Ricard <christophe-h.ricard@st.com> > drivers/char/tpm/tpm_i2c_stm_st33.c | 9 --------- > 1 file changed, 9 deletions(-) > > diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c > index e99bb78..660ff8b 100644 > +++ b/drivers/char/tpm/tpm_i2c_stm_st33.c > @@ -187,7 +187,6 @@ static u8 clear_interruption(struct tpm_stm_dev *tpm_dev) > > I2C_READ_DATA(tpm_dev, TPM_INT_STATUS, &interrupt, 1); > I2C_WRITE_DATA(tpm_dev, TPM_INT_STATUS, &interrupt, 1); > - I2C_READ_DATA(tpm_dev, TPM_INT_STATUS, &interrupt, 1); Hum, I don't have a chip datasheet here, but is this really useless? It looks like a synchronizing read to me - ie completing the read at the CPU is enough to know that the TPM itself has processed the prior write and is known to have lowered the level triggered interrupt.. A read like this is the sort of thing that you'd expect to avoid the udelay in your 'Add delay before release_locality to make sure irq are cleared' Jason ------------------------------------------------------------------------------ Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk
Hi Jason, On 08/10/2014 00:09, Jason Gunthorpe wrote: > On Tue, Oct 07, 2014 at 10:03:04PM +0200, Christophe Ricard wrote: >> Remove useless i2c read on TPM_INT_ENABLE and TPM_INT_STATUS >> >> Signed-off-by: Christophe Ricard <christophe-h.ricard@st.com> >> drivers/char/tpm/tpm_i2c_stm_st33.c | 9 --------- >> 1 file changed, 9 deletions(-) >> >> diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c >> index e99bb78..660ff8b 100644 >> +++ b/drivers/char/tpm/tpm_i2c_stm_st33.c >> @@ -187,7 +187,6 @@ static u8 clear_interruption(struct tpm_stm_dev *tpm_dev) >> >> I2C_READ_DATA(tpm_dev, TPM_INT_STATUS, &interrupt, 1); >> I2C_WRITE_DATA(tpm_dev, TPM_INT_STATUS, &interrupt, 1); >> - I2C_READ_DATA(tpm_dev, TPM_INT_STATUS, &interrupt, 1); > Hum, I don't have a chip datasheet here, but is this really useless? > > It looks like a synchronizing read to me - ie completing the read at > the CPU is enough to know that the TPM itself has processed the prior > write and is known to have lowered the level triggered interrupt.. > > A read like this is the sort of thing that you'd expect to avoid the > udelay in your 'Add delay before release_locality to make sure irq are > cleared' I believe this is completely 2 different things. The delay before the release locality is to make sure that the isr will be service before release_locality to guarantee that any pending interrupt are cleared while the locality is active. Here i just want to save 2 i2c transaction as only 1 write is enough to get the register change as effective. > > Jason Best Regards Christophe ------------------------------------------------------------------------------ Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk
On Wed, Oct 08, 2014 at 07:36:04AM +0200, Christophe RICARD wrote: > I believe this is completely 2 different things. The delay before the > release locality is to make sure that the isr will be service before > release_locality to guarantee > that any pending interrupt are cleared while the locality is active. > Here i just want to save 2 i2c transaction as only 1 write is enough > to get the register change as effective. I think I follow the interrupt changes a bit better now.. First of all, things are spread out too much, ie patch 10 makes the actual ISR handler change but patch 13 corrects the locking bug introduced in patch 10, and patch 7 switches to the threaded irq required by patch 13 - this should all be in the same patch. Second, I don't think switching to threaded IRQs and then using I2C transactions in the handler is a great idea. I think you should follow the pattern in the nuvoton driver: The IRQ handler simply records the interrupt occured, triggers a WQ and disables the IRQ: static irqreturn_t i2c_nuvoton_int_handler(int dummy, void *dev_id) { struct tpm_chip *chip = dev_id; struct priv_data *priv = chip->vendor.priv; priv->intrs++; wake_up(&chip->vendor.read_queue); disable_irq_nosync(chip->vendor.irq); return IRQ_HANDLED; } The ops explicitly enables the IRQ before sleeping waiting on the output FIFO: if (chip->vendor.irq && queue) { s32 rc; struct priv_data *priv = chip->vendor.priv; unsigned int cur_intrs = priv->intrs; enable_irq(chip->vendor.irq); rc = wait_event_interruptible_timeout(*queue, cur_intrs != priv->intrs, timeout); if (rc > 0) return 0; For your chip you might need to ack the IRQ before writing a new command to the input FIFO. 1) This gets rid of the udelay, the IRQ doesn't do anything so any acks can be sequenced properly with the request_locality 2) This gets rid of the locking, the IRQ doesn't attempt to ack, and acks can be sequenced before any enable_irq If your chip is sane like other TPMs the IRQ pin will only be asserted while there is data pending in the out command FIFO, reading the FIFO should naturally clear the IRQ and the acking process may be entirely unnecessary and can be removed. If you have to ack via that weird read/write sequence then it should always be done before submitting a new command to the input fifo. Jason ------------------------------------------------------------------------------ Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk
Hi Jason, > If your chip is sane like other TPMs the IRQ pin will *only* be asserted while there is data pending in the out command FIFO, reading the FIFO *should naturally clear the IRQ *and the acking process may be entirely unnecessary and can be removed. I believe this assessment is wrong according to existing specifications and current implementation and I will explain you why: Our TPM is managing the TPM TIS registers Interrupt Enable and Interrupt Status. The TPM register Interrupt Enable support globalIntEnable (bit 31), commandReadyEnable (bit 7), fifoAvailableEnable (bit 6), Wakeup ready enable (bit 5), loc4SoftRelease(bit 3), stsValidIntEnable(bit 1), dataAvailIntEnable(bit 0). Except fifoAvailableEnable (bit 6), Wakeup ready enable (bit 5), loc4SoftRelease(bit 3) all of them are specified in the TCG TIS specification. The TPM register Interrupt Status support commandReadyIntOccured(bit 7), fifoAvailableOccured(bit 6), WakeupReadyOccured(bit 5), loc4SoftRelease(bit 3), localityChangeIntOccured(bit 2), stsValidIntOccured(bit 1), dataAvailIntOccured(bit 0). Except fifoAvailableOccured(bit 6), WakeupReadyOccured(bit 5), loc4SoftRelease(bit 3) all of them are specified in the TCG TIS specification. When any enabled interrupt is getting active, the serirq line will get active (high state reversed from lpc implementation) but it will not tell which one. It could be any of the interrupt status register. Still according to the TCG_PCClientTPMInterfaceSpecification_TIS, the only when to clear a pending interrupt is to write a 1 to the corresponding bit in the TPM_INT_STATUS register. According to which one is triggered, the wait queue to wake up is different it could be chip->vendor.read_queue or chip->vendor.int_queue. According to other driver implementation: - chip->vendor.read_queue is used to signal data availability. - chip->vendor.int_queue is used to signal any other interrupt sts_valid_int, cmd_read_int, locality_change_int. As a possible clean up, I can see: - the locality_change_int in the handler may not be manage in my isr as it is not supposed to happen in the OS. - the interrupt TPM_INTF_LOCALITY_CHANGE_INT, TPM_INTF_FIFO_AVAILABLE_INT, TPM_INTF_WAKE_UP_READY_INT does not need to be activated. The udelay before the release_locality has be chosen small in order to reduce the impact in case the driver is used in polling mode. I would point out as well the int_queue initialization in the i2c_nuvoton_probe which is never used in the tpm_i2c_nuvoton.c nor in any tpm core file. The only point that could be raised is that there is no TPM 1.2 protocol specification for I2C. During our chip implementation, we tried to fit best to existing documentation. Therefore I believe claiming this I2C TPM implementation or this one should be taken as reference is not appropriate as per previous statement. As a conclusion to this, I believe I can add the clean-up pointed out previously and I will try to respin and rework patch 10, 13 and 7. However, I am not in favor to change to non-threaded irq unless I have a clear and convincing argument to do so. I will submit a v2 patchset including as much as possible fix/clean according to your feedback soon. Thank you for your feedback. Best Regards Christophe -----Original Message----- From: Jason Gunthorpe [mailto:jgunthorpe@obsidianresearch.com] Sent: mercredi 8 octobre 2014 19:12 To: Christophe RICARD Cc: peterhuewe@gmx.de; ashley@ashleylai.com; tpmdd@selhorst.net; devicetree@vger.kernel.org; tpmdd-devel@lists.sourceforge.net; Christophe Henri RICARD; Jean-Luc BLANC Subject: Re: [tpmdd-devel] [PATCH 11/16] tpm/tpm_i2c_stm_st33: Remove useless i2c read on interrupt registers On Wed, Oct 08, 2014 at 07:36:04AM +0200, Christophe RICARD wrote: > I believe this is completely 2 different things. The delay before the > release locality is to make sure that the isr will be service before > release_locality to guarantee that any pending interrupt are cleared > while the locality is active. > Here i just want to save 2 i2c transaction as only 1 write is enough > to get the register change as effective. I think I follow the interrupt changes a bit better now.. First of all, things are spread out too much, ie patch 10 makes the actual ISR handler change but patch 13 corrects the locking bug introduced in patch 10, and patch 7 switches to the threaded irq required by patch 13 - this should all be in the same patch. Second, I don't think switching to threaded IRQs and then using I2C transactions in the handler is a great idea. I think you should follow the pattern in the nuvoton driver: The IRQ handler simply records the interrupt occured, triggers a WQ and disables the IRQ: static irqreturn_t i2c_nuvoton_int_handler(int dummy, void *dev_id) { struct tpm_chip *chip = dev_id; struct priv_data *priv = chip->vendor.priv; priv->intrs++; wake_up(&chip->vendor.read_queue); disable_irq_nosync(chip->vendor.irq); return IRQ_HANDLED; } The ops explicitly enables the IRQ before sleeping waiting on the output FIFO: if (chip->vendor.irq && queue) { s32 rc; struct priv_data *priv = chip->vendor.priv; unsigned int cur_intrs = priv->intrs; enable_irq(chip->vendor.irq); rc = wait_event_interruptible_timeout(*queue, cur_intrs != priv->intrs, timeout); if (rc > 0) return 0; For your chip you might need to ack the IRQ before writing a new command to the input FIFO. 1) This gets rid of the udelay, the IRQ doesn't do anything so any acks can be sequenced properly with the request_locality 2) This gets rid of the locking, the IRQ doesn't attempt to ack, and acks can be sequenced before any enable_irq If your chip is sane like other TPMs the IRQ pin will only be asserted while there is data pending in the out command FIFO, reading the FIFO should naturally clear the IRQ and the acking process may be entirely unnecessary and can be removed. If you have to ack via that weird read/write sequence then it should always be done before submitting a new command to the input fifo. Jason ------------------------------------------------------------------------------ Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk
On Thu, Oct 09, 2014 at 07:35:07PM +0200, Christophe Henri RICARD wrote: > Hi Jason, > > > If your chip is sane like other TPMs the IRQ pin will *only* be > > asserted while there is data pending in the out command FIFO, > > reading the FIFO *should naturally clear the IRQ *and the acking > > process may be entirely unnecessary and can be removed. > I believe this assessment is wrong according to existing > specifications and current implementation and I will explain you > why: It does sound like it is not how the ST33 chip operates. > Our TPM is managing the TPM TIS registers Interrupt Enable and > Interrupt Status. The TIS register mapping was intended for environments where register access is not expensive, and can be done from an ISR. I2C is not such an environment, which is why other vendors are not using such a strict mapping of the TIS registers. That doesn't really change anything, it just means the driver has to do more I2C transactions to manage this extra chip state. > According to which one is triggered, the wait queue to wake up is > different it could be chip->vendor.read_queue or > chip->vendor.int_queue. These different queues are not needed, the driver knows what it is waiting for when it enables the interrupt handler, and can manipulate the interrupt mask if necessary for special cases. Multiple queues make more sense when the ISR can cheaply read the status register, which is not true for I2C. > I would point out as well the int_queue initialization in the > i2c_nuvoton_probe which is never used in the tpm_i2c_nuvoton.c nor > in any tpm core file. Chip structure members that are not used in the core code are hold overs from the ancient core design. Drivers ideally should not use them, favoring their own structures in their driver private allocation. Someday that will get cleaned up. Don't get confused that int_queue and read_queue are in the global chip, they have no special meaning, modern drivers should not use them at all. > However, I am not in favor to change to non-threaded irq unless I > have a clear and convincing argument to do so. The need for the udelay should be all the convincing required. The reason for the udelay clearly shows the driver has a synchronization problem - and sleeping to solve synchronization problem is rarely correct. This is especially true since I've already explained how to design so everything is synchronous and solve the issue. Again: In TPM the interrupt is not delivering an asynchronous notification, everything is synchronous to the driver state, ISRs happen only to indicate completion of driver initiated actions. This is why the synchronous flow I suggested is much safer and better, the driver just can't get the situation where the IRQ cannot safely run because the main driver thread has changed the TPM state. To summarize, the flow for an interrupt wait becomes very simple, ie to wait for command ready: - Do write to clear all interrupts - do read on command ready - if not ready then write to enable only the command ready occured interrupt - enable_irq - wait for irq w/ timer - do read on command ready - if not ready and no timeout, do write to clear all interrupts, enable_irq, loop. - if not ready and timeout, disable irq, return error All sleepables follow the same synchronous pattern. This makes the driver compeltely single threaded so no analysis for thread problems is need. No locking primitives are needed. The inter-locking problem with request_locality goes away. Jason ------------------------------------------------------------------------------ Meet PCI DSS 3.0 Compliance Requirements with EventLog Analyzer Achieve PCI DSS 3.0 Compliant Status with Out-of-the-box PCI DSS Reports Are you Audit-Ready for PCI DSS 3.0 Compliance? Download White paper Comply to PCI DSS 3.0 Requirement 10 and 11.5 with EventLog Analyzer http://pubads.g.doubleclick.net/gampad/clk?id=154622311&iu=/4140/ostg.clktrk
diff --git a/drivers/char/tpm/tpm_i2c_stm_st33.c b/drivers/char/tpm/tpm_i2c_stm_st33.c index e99bb78..660ff8b 100644 --- a/drivers/char/tpm/tpm_i2c_stm_st33.c +++ b/drivers/char/tpm/tpm_i2c_stm_st33.c @@ -187,7 +187,6 @@ static u8 clear_interruption(struct tpm_stm_dev *tpm_dev) I2C_READ_DATA(tpm_dev, TPM_INT_STATUS, &interrupt, 1); I2C_WRITE_DATA(tpm_dev, TPM_INT_STATUS, &interrupt, 1); - I2C_READ_DATA(tpm_dev, TPM_INT_STATUS, &interrupt, 1); return interrupt; } /* clear_interruption() */ @@ -753,10 +752,6 @@ tpm_stm_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id) goto _tpm_clean_answer; } - r = I2C_READ_DATA(tpm_dev, TPM_INT_ENABLE, &intmask, 1); - if (r < 0) - goto _tpm_clean_answer; - intmask |= TPM_INTF_CMD_READY_INT | TPM_INTF_FIFO_AVALAIBLE_INT | TPM_INTF_WAKE_UP_READY_INT @@ -773,10 +768,6 @@ tpm_stm_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id) if (r < 0) goto _tpm_clean_answer; - r = I2C_READ_DATA(tpm_dev, TPM_INT_STATUS, &intmask, 1); - if (r < 0) - goto _tpm_clean_answer; - chip->vendor.irq = client->irq; tpm_gen_interrupt(chip);
Remove useless i2c read on TPM_INT_ENABLE and TPM_INT_STATUS Signed-off-by: Christophe Ricard <christophe-h.ricard@st.com> --- drivers/char/tpm/tpm_i2c_stm_st33.c | 9 --------- 1 file changed, 9 deletions(-)