| Message ID | 20200331113207.107080-5-amirmizi6@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add tpm i2c ptp driver | expand |
On Tue, Mar 31, 2020 at 02:32:04PM +0300, amirmizi6@gmail.com wrote: > From: Amir Mizinski <amirmizi6@gmail.com> > > Today, actual implementation for send massage is not correct. We check and > loop only on TPM_STS.stsValid bit and next we single check TPM_STS.expect > bit value. > TPM_STS.expected bit shall be checked in the same time of > TPM_STS.stsValid, and should be repeated until timeout_A. > To aquire that, "wait_for_tpm_stat" function is modified to > "wait_for_tpm_stat_result". this function read regulary status register > and check bit defined by "mask" to reach value defined in "mask_result" > (that way a bit in mask can be checked if reached 1 or 0). > > Respectively, to send message as defined in > TCG_DesignPrinciples_TPM2p0Driver_vp24_pubrev.pdf, all bytes should be > sent in one shot instead of sending last byte in exception. > > This improvment was suggested by Benoit Houyere. Use suggested-by tag. Also if something is not correct, please provide a fixes tag. You are speaking now in theoretical level, which we don't really care that much. Is this causing you real issues? If the answer is yes, please report them. If the answer is no, we don't need this. /Jarkko > > Signed-off-by: Amir Mizinski <amirmizi6@gmail.com> > --- > drivers/char/tpm/tpm_tis_core.c | 72 ++++++++++++++++------------------------- > 1 file changed, 28 insertions(+), 44 deletions(-) > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c > index 18b9dc4..c8f4cf8 100644 > --- a/drivers/char/tpm/tpm_tis_core.c > +++ b/drivers/char/tpm/tpm_tis_core.c > @@ -44,9 +44,10 @@ static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask, > return false; > } > > -static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, > - unsigned long timeout, wait_queue_head_t *queue, > - bool check_cancel) > +static int wait_for_tpm_stat_result(struct tpm_chip *chip, u8 mask, > + u8 mask_result, unsigned long timeout, > + wait_queue_head_t *queue, > + bool check_cancel) > { > unsigned long stop; > long rc; > @@ -55,7 +56,7 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, > > /* check current status */ > status = chip->ops->status(chip); > - if ((status & mask) == mask) > + if ((status & mask) == mask_result) > return 0; > > stop = jiffies + timeout; > @@ -83,7 +84,7 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, > usleep_range(TPM_TIMEOUT_USECS_MIN, > TPM_TIMEOUT_USECS_MAX); > status = chip->ops->status(chip); > - if ((status & mask) == mask) > + if ((status & mask) == mask_result) > return 0; > } while (time_before(jiffies, stop)); > } > @@ -290,10 +291,11 @@ static int recv_data(struct tpm_chip *chip, u8 *buf, size_t count) > int size = 0, burstcnt, rc; > > while (size < count) { > - rc = wait_for_tpm_stat(chip, > - TPM_STS_DATA_AVAIL | TPM_STS_VALID, > - chip->timeout_c, > - &priv->read_queue, true); > + rc = wait_for_tpm_stat_result(chip, > + TPM_STS_DATA_AVAIL | TPM_STS_VALID, > + TPM_STS_DATA_AVAIL | TPM_STS_VALID, > + chip->timeout_c, > + &priv->read_queue, true); > if (rc < 0) > return rc; > burstcnt = get_burstcount(chip); > @@ -348,8 +350,9 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count) > goto out; > } > > - if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c, > - &priv->int_queue, false) < 0) { > + if (wait_for_tpm_stat_result(chip, TPM_STS_VALID, > + TPM_STS_VALID, chip->timeout_c, > + &priv->int_queue, false) < 0) { > size = -ETIME; > goto out; > } > @@ -385,61 +388,40 @@ static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len) > struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > int rc, status, burstcnt; > size_t count = 0; > - bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND; > > status = tpm_tis_status(chip); > if ((status & TPM_STS_COMMAND_READY) == 0) { > tpm_tis_ready(chip); > - if (wait_for_tpm_stat > - (chip, TPM_STS_COMMAND_READY, chip->timeout_b, > - &priv->int_queue, false) < 0) { > + if (wait_for_tpm_stat_result(chip, TPM_STS_COMMAND_READY, > + TPM_STS_COMMAND_READY, > + chip->timeout_b, > + &priv->int_queue, false) < 0) { > rc = -ETIME; > goto out_err; > } > } > > - while (count < len - 1) { > + while (count < len) { > burstcnt = get_burstcount(chip); > if (burstcnt < 0) { > dev_err(&chip->dev, "Unable to read burstcount\n"); > rc = burstcnt; > goto out_err; > } > - burstcnt = min_t(int, burstcnt, len - count - 1); > + burstcnt = min_t(int, burstcnt, len - count); > rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality), > burstcnt, buf + count); > if (rc < 0) > goto out_err; > > count += burstcnt; > - > - if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c, > - &priv->int_queue, false) < 0) { > - rc = -ETIME; > - goto out_err; > - } > - status = tpm_tis_status(chip); > - if (!itpm && (status & TPM_STS_DATA_EXPECT) == 0) { > - rc = -EIO; > - goto out_err; > - } > } > - > - /* write last byte */ > - rc = tpm_tis_write8(priv, TPM_DATA_FIFO(priv->locality), buf[count]); > - if (rc < 0) > - goto out_err; > - > - if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c, > - &priv->int_queue, false) < 0) { > + if (wait_for_tpm_stat_result(chip, TPM_STS_VALID | TPM_STS_DATA_EXPECT, > + TPM_STS_VALID, chip->timeout_c, > + &priv->int_queue, false) < 0) { > rc = -ETIME; > goto out_err; > } > - status = tpm_tis_status(chip); > - if (!itpm && (status & TPM_STS_DATA_EXPECT) != 0) { > - rc = -EIO; > - goto out_err; > - } > > return 0; > > @@ -496,9 +478,11 @@ static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, size_t len) > ordinal = be32_to_cpu(*((__be32 *) (buf + 6))); > > dur = tpm_calc_ordinal_duration(chip, ordinal); > - if (wait_for_tpm_stat > - (chip, TPM_STS_DATA_AVAIL | TPM_STS_VALID, dur, > - &priv->read_queue, false) < 0) { > + if (wait_for_tpm_stat_result(chip, > + TPM_STS_DATA_AVAIL | TPM_STS_VALID, > + TPM_STS_DATA_AVAIL | TPM_STS_VALID, > + dur, > + &priv->read_queue, false) < 0) { > rc = -ETIME; > goto out_err; > } > -- > 2.7.4 > > > > =========================================================================================== > The privileged confidential information contained in this email is intended for use only by the addressees as indicated by the original sender of this email. If you are not the addressee indicated in this email or are not responsible for delivery of the email to such a person, please kindly reply to the sender indicating this fact and delete all copies of it from your computer and network server immediately. Your cooperation is highly appreciated. It is advised that any unauthorized use of confidential information of Nuvoton is strictly prohibited; and any information in this email irrelevant to the official business of Nuvoton shall be deemed as neither given nor endorsed by Nuvoton.
> On Tue, Mar 31, 2020 at 02:32:04PM +0300, amirmizi6@gmail.com wrote: > > From: Amir Mizinski <amirmizi6@gmail.com> > > > > Today, actual implementation for send massage is not correct. We check > > and loop only on TPM_STS.stsValid bit and next we single check > > TPM_STS.expect bit value. > > TPM_STS.expected bit shall be checked in the same time of > > TPM_STS.stsValid, and should be repeated until timeout_A. > > To aquire that, "wait_for_tpm_stat" function is modified to > > "wait_for_tpm_stat_result". this function read regulary status > > register and check bit defined by "mask" to reach value defined in "mask_result" > > (that way a bit in mask can be checked if reached 1 or 0). > > > > Respectively, to send message as defined in > > TCG_DesignPrinciples_TPM2p0Driver_vp24_pubrev.pdf, all bytes should be > > sent in one shot instead of sending last byte in exception. > > > > This improvment was suggested by Benoit Houyere. >Use suggested-by tag. >Also if something is not correct, please provide a fixes tag. > You are speaking now in theoretical level, which we don't really care that much. Is this causing you real issues? If the answer is yes, please report them. If the > >answer is no, we don't need this. > /Jarkko I2C TPM specification introduce CRC calculation on TPM command bytes. CRC calculation take place from last byte acquired to TPM_STS.expected bit reset (=0) .It introduces latency and actual incorrect implementation becomes visible now under I2C on the contrary before that's all. The case where TPM keeps TPM_STS.expected bit set with TPM_STS.stsValid set after last byte reception is possible and is not an issue. It's not theoretical level, it's practical level now.
On Tue, Mar 31, 2020 at 09:34:28PM +0000, Benoit HOUYERE wrote: > > > On Tue, Mar 31, 2020 at 02:32:04PM +0300, amirmizi6@gmail.com wrote: > > > From: Amir Mizinski <amirmizi6@gmail.com> > > > > > > Today, actual implementation for send massage is not correct. We check > > > and loop only on TPM_STS.stsValid bit and next we single check > > > TPM_STS.expect bit value. > > > TPM_STS.expected bit shall be checked in the same time of > > > TPM_STS.stsValid, and should be repeated until timeout_A. > > > To aquire that, "wait_for_tpm_stat" function is modified to > > > "wait_for_tpm_stat_result". this function read regulary status > > > register and check bit defined by "mask" to reach value defined in "mask_result" > > > (that way a bit in mask can be checked if reached 1 or 0). > > > > > > Respectively, to send message as defined in > > > TCG_DesignPrinciples_TPM2p0Driver_vp24_pubrev.pdf, all bytes should be > > > sent in one shot instead of sending last byte in exception. > > > > > > This improvment was suggested by Benoit Houyere. > > >Use suggested-by tag. > > >Also if something is not correct, please provide a fixes tag. > > > You are speaking now in theoretical level, which we don't really care that much. Is this causing you real issues? If the answer is yes, please report them. If the > >answer is no, we don't need this. > > > /Jarkko > > I2C TPM specification introduce CRC calculation on TPM command bytes. > CRC calculation take place from last byte acquired to > TPM_STS.expected bit reset (=0) .It introduces latency and actual > incorrect implementation becomes visible now under I2C on the contrary > before that's all. The case where TPM keeps TPM_STS.expected bit set > with TPM_STS.stsValid set after last byte reception is possible and is > not an issue. It's not theoretical level, it's practical level now. Thank you, think I got it. This means that it does not need a fixes tag because it does not break any hardware that it currently supported. I'd suggest refining the commit message. Not only it is somewhat loosely writte peace of text but also has typos like "massage". /Jarkko
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c index 18b9dc4..c8f4cf8 100644 --- a/drivers/char/tpm/tpm_tis_core.c +++ b/drivers/char/tpm/tpm_tis_core.c @@ -44,9 +44,10 @@ static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask, return false; } -static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, - unsigned long timeout, wait_queue_head_t *queue, - bool check_cancel) +static int wait_for_tpm_stat_result(struct tpm_chip *chip, u8 mask, + u8 mask_result, unsigned long timeout, + wait_queue_head_t *queue, + bool check_cancel) { unsigned long stop; long rc; @@ -55,7 +56,7 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, /* check current status */ status = chip->ops->status(chip); - if ((status & mask) == mask) + if ((status & mask) == mask_result) return 0; stop = jiffies + timeout; @@ -83,7 +84,7 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, usleep_range(TPM_TIMEOUT_USECS_MIN, TPM_TIMEOUT_USECS_MAX); status = chip->ops->status(chip); - if ((status & mask) == mask) + if ((status & mask) == mask_result) return 0; } while (time_before(jiffies, stop)); } @@ -290,10 +291,11 @@ static int recv_data(struct tpm_chip *chip, u8 *buf, size_t count) int size = 0, burstcnt, rc; while (size < count) { - rc = wait_for_tpm_stat(chip, - TPM_STS_DATA_AVAIL | TPM_STS_VALID, - chip->timeout_c, - &priv->read_queue, true); + rc = wait_for_tpm_stat_result(chip, + TPM_STS_DATA_AVAIL | TPM_STS_VALID, + TPM_STS_DATA_AVAIL | TPM_STS_VALID, + chip->timeout_c, + &priv->read_queue, true); if (rc < 0) return rc; burstcnt = get_burstcount(chip); @@ -348,8 +350,9 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count) goto out; } - if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c, - &priv->int_queue, false) < 0) { + if (wait_for_tpm_stat_result(chip, TPM_STS_VALID, + TPM_STS_VALID, chip->timeout_c, + &priv->int_queue, false) < 0) { size = -ETIME; goto out; } @@ -385,61 +388,40 @@ static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len) struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); int rc, status, burstcnt; size_t count = 0; - bool itpm = priv->flags & TPM_TIS_ITPM_WORKAROUND; status = tpm_tis_status(chip); if ((status & TPM_STS_COMMAND_READY) == 0) { tpm_tis_ready(chip); - if (wait_for_tpm_stat - (chip, TPM_STS_COMMAND_READY, chip->timeout_b, - &priv->int_queue, false) < 0) { + if (wait_for_tpm_stat_result(chip, TPM_STS_COMMAND_READY, + TPM_STS_COMMAND_READY, + chip->timeout_b, + &priv->int_queue, false) < 0) { rc = -ETIME; goto out_err; } } - while (count < len - 1) { + while (count < len) { burstcnt = get_burstcount(chip); if (burstcnt < 0) { dev_err(&chip->dev, "Unable to read burstcount\n"); rc = burstcnt; goto out_err; } - burstcnt = min_t(int, burstcnt, len - count - 1); + burstcnt = min_t(int, burstcnt, len - count); rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality), burstcnt, buf + count); if (rc < 0) goto out_err; count += burstcnt; - - if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c, - &priv->int_queue, false) < 0) { - rc = -ETIME; - goto out_err; - } - status = tpm_tis_status(chip); - if (!itpm && (status & TPM_STS_DATA_EXPECT) == 0) { - rc = -EIO; - goto out_err; - } } - - /* write last byte */ - rc = tpm_tis_write8(priv, TPM_DATA_FIFO(priv->locality), buf[count]); - if (rc < 0) - goto out_err; - - if (wait_for_tpm_stat(chip, TPM_STS_VALID, chip->timeout_c, - &priv->int_queue, false) < 0) { + if (wait_for_tpm_stat_result(chip, TPM_STS_VALID | TPM_STS_DATA_EXPECT, + TPM_STS_VALID, chip->timeout_c, + &priv->int_queue, false) < 0) { rc = -ETIME; goto out_err; } - status = tpm_tis_status(chip); - if (!itpm && (status & TPM_STS_DATA_EXPECT) != 0) { - rc = -EIO; - goto out_err; - } return 0; @@ -496,9 +478,11 @@ static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, size_t len) ordinal = be32_to_cpu(*((__be32 *) (buf + 6))); dur = tpm_calc_ordinal_duration(chip, ordinal); - if (wait_for_tpm_stat - (chip, TPM_STS_DATA_AVAIL | TPM_STS_VALID, dur, - &priv->read_queue, false) < 0) { + if (wait_for_tpm_stat_result(chip, + TPM_STS_DATA_AVAIL | TPM_STS_VALID, + TPM_STS_DATA_AVAIL | TPM_STS_VALID, + dur, + &priv->read_queue, false) < 0) { rc = -ETIME; goto out_err; }