| Message ID | 20170807114632.1339-1-nayna@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 7. August 2017 13:46:32 MESZ schrieb Nayna Jain <nayna@linux.vnet.ibm.com>: >The TPM burstcount status indicates the number of bytes that can >be sent to the TPM without causing bus wait states. Effectively, >it is the number of empty bytes in the command FIFO. Further, >some TPMs have a static burstcount, when the value remains zero >until the entire FIFO is empty. > >This patch ignores burstcount, permitting wait states, and thus >writes the command as fast as the TPM can accept the bytes. >The performance of a 34 byte extend on a TPM 1.2 improved from >52 msec to 11 msec. > >Suggested-by: Ken Goldman <kgold@linux.vnet.ibm.com> in >conjunction with the TPM Device Driver work group. >Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com> >Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com> Are you sure this is a good idea? On lpc systems this more or less stalls the bus, including keyboard/mouse (if connected via superio lpc). On which systems have you tested this? Spi/Lpc? Architecture? This might not be noticable for small transfers, but think about much larger transfers.... Imho: NACK from my side. Thanks, Peter >--- >drivers/char/tpm/tpm_tis_core.c | 45 >++--------------------------------------- > 1 file changed, 2 insertions(+), 43 deletions(-) > >diff --git a/drivers/char/tpm/tpm_tis_core.c >b/drivers/char/tpm/tpm_tis_core.c >index b617b2eeb080..478cbc0f61c3 100644 >--- a/drivers/char/tpm/tpm_tis_core.c >+++ b/drivers/char/tpm/tpm_tis_core.c >@@ -255,9 +255,7 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 >*buf, size_t count) >static int tpm_tis_send_data(struct tpm_chip *chip, 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; >+ int rc, status; > > status = tpm_tis_status(chip); > if ((status & TPM_STS_COMMAND_READY) == 0) { >@@ -270,49 +268,10 @@ static int tpm_tis_send_data(struct tpm_chip >*chip, u8 *buf, size_t len) > } > } > >- while (count < len - 1) { >- 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); >- 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]); >+ rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality), len, >buf); > if (rc < 0) > goto out_err; > >- 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; >- } >- > return 0; > > out_err:
On 08/07/2017 05:22 PM, Peter Huewe wrote: > > > Am 7. August 2017 13:46:32 MESZ schrieb Nayna Jain <nayna@linux.vnet.ibm.com>: >> The TPM burstcount status indicates the number of bytes that can >> be sent to the TPM without causing bus wait states. Effectively, >> it is the number of empty bytes in the command FIFO. Further, >> some TPMs have a static burstcount, when the value remains zero >> until the entire FIFO is empty. >> >> This patch ignores burstcount, permitting wait states, and thus >> writes the command as fast as the TPM can accept the bytes. >> The performance of a 34 byte extend on a TPM 1.2 improved from >> 52 msec to 11 msec. >> >> Suggested-by: Ken Goldman <kgold@linux.vnet.ibm.com> in >> conjunction with the TPM Device Driver work group. >> Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com> >> Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com> > > Are you sure this is a good idea? > On lpc systems this more or less stalls the bus, including keyboard/mouse (if connected via superio lpc). Thanks Peter for quick response. I actually meant to post this patch as RFC. Sorry, missed that. It was meant to be a starting place for the discussion related to burst_count. > > On which systems have you tested this? > Spi/Lpc? Architecture? Tested it with LPC on x86. > > This might not be noticable for small transfers, but think about much larger transfers.... I did the following testing: * Ran a script with 1000 extends. This was to test continuous extends which are generally in large numbers when IMA is enabled. * Ran a command to ask TPM to hash big size file like 1MB. This was to test the long command. In both of the above cases, I didn't face any tpm specific errors. Is there any test-script or test-cases which I can try to test the scenario(stalling the bus, including keyboard/mouse) with the patch ? Thanks & Regards, - Nayna > > Imho: NACK from my side. > > Thanks, > Peter > >> --- >> drivers/char/tpm/tpm_tis_core.c | 45 >> ++--------------------------------------- >> 1 file changed, 2 insertions(+), 43 deletions(-) >> >> diff --git a/drivers/char/tpm/tpm_tis_core.c >> b/drivers/char/tpm/tpm_tis_core.c >> index b617b2eeb080..478cbc0f61c3 100644 >> --- a/drivers/char/tpm/tpm_tis_core.c >> +++ b/drivers/char/tpm/tpm_tis_core.c >> @@ -255,9 +255,7 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 >> *buf, size_t count) >> static int tpm_tis_send_data(struct tpm_chip *chip, 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; >> + int rc, status; >> >> status = tpm_tis_status(chip); >> if ((status & TPM_STS_COMMAND_READY) == 0) { >> @@ -270,49 +268,10 @@ static int tpm_tis_send_data(struct tpm_chip >> *chip, u8 *buf, size_t len) >> } >> } >> >> - while (count < len - 1) { >> - 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); >> - 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]); >> + rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality), len, >> buf); >> if (rc < 0) >> goto out_err; >> >> - 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; >> - } >> - >> return 0; >> >> out_err: > -- 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
On Mon, Aug 07, 2017 at 07:46:32AM -0400, Nayna Jain wrote: > The TPM burstcount status indicates the number of bytes that can > be sent to the TPM without causing bus wait states. Effectively, > it is the number of empty bytes in the command FIFO. Further, > some TPMs have a static burstcount, when the value remains zero > until the entire FIFO is empty. > > This patch ignores burstcount, permitting wait states, and thus > writes the command as fast as the TPM can accept the bytes. > The performance of a 34 byte extend on a TPM 1.2 improved from > 52 msec to 11 msec. > > Suggested-by: Ken Goldman <kgold@linux.vnet.ibm.com> in > conjunction with the TPM Device Driver work group. > Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com> > Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com> > --- > drivers/char/tpm/tpm_tis_core.c | 45 ++--------------------------------------- > 1 file changed, 2 insertions(+), 43 deletions(-) > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c > index b617b2eeb080..478cbc0f61c3 100644 > --- a/drivers/char/tpm/tpm_tis_core.c > +++ b/drivers/char/tpm/tpm_tis_core.c > @@ -255,9 +255,7 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count) > static int tpm_tis_send_data(struct tpm_chip *chip, 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; > + int rc, status; As you anyway edit that line you could turn this as: int rc; int status; > status = tpm_tis_status(chip); > if ((status & TPM_STS_COMMAND_READY) == 0) { > @@ -270,49 +268,10 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len) > } > } > > - while (count < len - 1) { > - 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); > - 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]); > + rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality), len, buf); > if (rc < 0) > goto out_err; > > - 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; > - } > - > return 0; > > out_err: > -- > 2.13.3 Here's an open question that I do not know the answer: can ignoring burst count cause hardware issues in the field? The commit message does not sort it out so I don't really feel safe merging this commit. /Jarkko -- 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
On Mon, Aug 07, 2017 at 01:52:34PM +0200, Peter Huewe wrote: > > > Am 7. August 2017 13:46:32 MESZ schrieb Nayna Jain <nayna@linux.vnet.ibm.com>: > >The TPM burstcount status indicates the number of bytes that can > >be sent to the TPM without causing bus wait states. Effectively, > >it is the number of empty bytes in the command FIFO. Further, > >some TPMs have a static burstcount, when the value remains zero > >until the entire FIFO is empty. > > > >This patch ignores burstcount, permitting wait states, and thus > >writes the command as fast as the TPM can accept the bytes. > >The performance of a 34 byte extend on a TPM 1.2 improved from > >52 msec to 11 msec. > > > >Suggested-by: Ken Goldman <kgold@linux.vnet.ibm.com> in > >conjunction with the TPM Device Driver work group. > >Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com> > >Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com> > > Are you sure this is a good idea? > On lpc systems this more or less stalls the bus, including keyboard/mouse (if connected via superio lpc). > > On which systems have you tested this? > Spi/Lpc? Architecture? > > This might not be noticable for small transfers, but think about much larger transfers.... > > Imho: NACK from my side. > > Thanks, > Peter Thanks Peter, a great insight. TPM could share the bus with other devices. Even if this optimizes the performace for TPM it might cause performance issues elsewhere. One more viewpoint: TCG must added the burst count for a reason (might be very well related what Peter said). Is ignoring it something that TCG recommends? Not following standard exactly in the driver code sometimes makes sense on *small details* but I would not say that this a small detail... After these viewpoints definitive NACK from my side too... /Jarkko /Jarkko -- 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
On Mon, Aug 07, 2017 at 07:55:49PM +0530, Nayna wrote: > > > On 08/07/2017 05:22 PM, Peter Huewe wrote: > > > > > > Am 7. August 2017 13:46:32 MESZ schrieb Nayna Jain <nayna@linux.vnet.ibm.com>: > > > The TPM burstcount status indicates the number of bytes that can > > > be sent to the TPM without causing bus wait states. Effectively, > > > it is the number of empty bytes in the command FIFO. Further, > > > some TPMs have a static burstcount, when the value remains zero > > > until the entire FIFO is empty. > > > > > > This patch ignores burstcount, permitting wait states, and thus > > > writes the command as fast as the TPM can accept the bytes. > > > The performance of a 34 byte extend on a TPM 1.2 improved from > > > 52 msec to 11 msec. > > > > > > Suggested-by: Ken Goldman <kgold@linux.vnet.ibm.com> in > > > conjunction with the TPM Device Driver work group. > > > Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com> > > > Acked-by: Mimi Zohar <zohar@linux.vnet.ibm.com> > > > > Are you sure this is a good idea? > > On lpc systems this more or less stalls the bus, including keyboard/mouse (if connected via superio lpc). > > Thanks Peter for quick response. > > I actually meant to post this patch as RFC. Sorry, missed that. > It was meant to be a starting place for the discussion related to > burst_count. > > > > > On which systems have you tested this? > > Spi/Lpc? Architecture? > > Tested it with LPC on x86. > > > > > This might not be noticable for small transfers, but think about much larger transfers.... > > I did the following testing: > > * Ran a script with 1000 extends. This was to test continuous extends > which are generally in large numbers when IMA is enabled. > > * Ran a command to ask TPM to hash big size file like 1MB. This was to > test the long command. > > In both of the above cases, I didn't face any tpm specific errors. > > Is there any test-script or test-cases which I can try to test the > scenario(stalling the bus, including keyboard/mouse) with the patch ? > > Thanks & Regards, > - Nayna My stand here is that if you want to such patch included there should be outstanding evidence that: - Burst count could be always safely ignored. - There's no hardware platform including TPM that assumes that driver takes the burst count into account. My main concern is stability and only if the stability is not at risk we can consider this. There's no test script to take care all of this. /Jarkko -- 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
On 8/8/2017 3:11 PM, Jarkko Sakkinen wrote: > On Mon, Aug 07, 2017 at 01:52:34PM +0200, Peter Huewe wrote: >> Imho: NACK from my side. > After these viewpoints definitive NACK from my side too... I responded to the thread comments separately. However, assuming NACK is the final response, I have a question. The problem is the 5 msec sleep between polls of burst count. In the case of one TPM with an 8 byte FIFO, a 32 byte transfer incurs 4 of these sleeps. Would another solution be to reduce the burst count poll and sleep to, e.g., 100 usec or even 10 usec? This would probably help greatly, but still not incur the wait states that triggered the NACK. My worry is that the scheduler would not be able to context switch that fast, and so we wouldn't actually see usec speed polling. Can a kernel expert offer an opinion? -- 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
On 8/8/2017 3:11 PM, Jarkko Sakkinen wrote: > On Mon, Aug 07, 2017 at 01:52:34PM +0200, Peter Huewe wrote: >> Are you sure this is a good idea? >> On lpc systems this more or less stalls the bus, including keyboard/mouse (if connected via superio lpc). >> >> On which systems have you tested this? >> Spi/Lpc? Architecture? >> >> This might not be noticable for small transfers, but think about much larger transfers.... >> >> Imho: NACK from my side. >> >> Thanks, >> Peter > > Thanks Peter, a great insight. TPM could share the bus with other > devices. Even if this optimizes the performance for TPM it might cause > performance issues elsewhere. Does anyone know of platforms where this occurs? I suspect (but not sure) that the days of SuperIO connecting floppy drives, printer ports, and PS/2 mouse ports on the LPC bus are over, and such legacy systems will not have a TPM. Would SuperIO even support the special TPM LPC bus cycles? Even then, the wait states of a mhz speed LPC are likely to be usec, not noticeable for even a mouse. Is this a reasonable assumption? If so, to we affect TPM performance to the point where it's unusable to help a case that is unlikely to appear in current platforms? > > One more viewpoint: TCG must added the burst count for a reason (might > be very well related what Peter said). Is ignoring it something that TCG > recommends? Not following standard exactly in the driver code sometimes > makes sense on *small details* but I would not say that this a small > detail... I checked with the TCG's device driver work group (DDWG). Both the spec editor and 3 TPM vendors - Infineon, Nuvoton, and ST Micro - agreed that ignoring burst count may incur wait states but nothing more. Operations will still be successful. -- 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
Hi Ken, (speaking on behalf of myself here, not my employer :) ) > On Mon, Aug 07, 2017 at 01:52:34PM +0200, Peter Huewe wrote: >> Imho: NACK from my side. > After these viewpoints definitive NACK from my side too... > I responded to the thread comments separately. However, assuming NACK > is the final response, I have a question. Nothing is ever final :) > The problem is the 5 msec sleep between polls of burst count. In the > case of one TPM with an 8 byte FIFO, a 32 byte transfer incurs 4 of > these sleeps. Yes that's bad, especially with current msleep(5) is actually msleep(20). However, please also keep in mind SPI tpms show a much higher burst count value, (255) our I2C TPM SLB9645 even shows something in the range of 1k. :) > Would another solution be to reduce the burst count poll and sleep to, > e.g., 100 usec or even 10 usec? This would probably help greatly, but > till not incur the wait states that triggered the NACK. Imho the 5ms were an arbitrary chosen value for old old tpms. Back then also only msleep was available which was msleep(20) anyway. > My worry is that the scheduler would not be able to context switch that > fast, and so we wouldn't actually see usec speed polling. > Can a kernel expert offer an opinion? If you use sleep it's not guaranteed that you wakeup after exactly xx specified amount of time. Just that you sleep at least xx amount of time. Otherwise you would have to do delay/busywaiting. https://www.kernel.org/doc/Documentation/timers/timers-howto.txt Imho the best option is to figure out whether any vendor can determine the "FIFO flush time" i.e. how much time does it take to empty the fifo and fillup the burstcount and use this on the lower bound of an usleep range. I don't think tpms are in the range of < 10 us... @Ken: Maybe can you check in DDWG? What we often see/saw during some performance optimization tests is something like burstcount 7, burstcount 1, burstcount 7, burstcount 1... which is the oposite of what you want to achieve. You'd like to have something like burstcount 8, burstcount 8, burstcount 8.... Unfortunately TPMS don't report their max burstcount (afaik), otherwise it would be easy to write a calibration routine for the poll times. Thanks, Peter -- 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
Hi Ken, (again speaking only on my behalf, not my employer) > Does anyone know of platforms where this occurs? > I suspect (but not sure) that the days of SuperIO connecting floppy > drives, printer ports, and PS/2 mouse ports on the LPC bus are over, and > such legacy systems will not have a TPM. Would SuperIO even support the > special TPM LPC bus cycles? Since we are the linux kernel, we do have to care for legacy devices. And a system with LPC, PS2Mouse on SuperIO and a TPM are not that uncommon. And heck, we even have support for 1.1b TPM devices.... >> One more viewpoint: TCG must added the burst count for a reason (might >> be very well related what Peter said). Is ignoring it something that TCG >> recommends? Not following standard exactly in the driver code sometimes >> makes sense on *small details* but I would not say that this a small >> detail... > I checked with the TCG's device driver work group (DDWG). Both the spec > editor and 3 TPM vendors - Infineon, Nuvoton, and ST Micro - agreed that > ignoring burst count may incur wait states but nothing more. Operations > will still be successful. Interesting - let me check with Georg tomorrow. Unfortunately I do not have access to my tcg mails from home (since I'm not working :), but did you _explicitly_ talk about LPC and the system? I'm sure the TPM does not care about the waitstates... If my memory does not betray me, it is actually possible to "freeze up" a system completly by flooding the lpc bus. Let me double check tomorrow... In anycase - I really would like to see a much more performant tpm subsystem - however it will be quite an effort with a lot of legacy testing. (which I unfortunately cannot spend on my private time ... and also of course lacking test systems). Thanks, Peter -- 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
On Wed, Aug 09, 2017 at 11:00:36PM +0200, Peter Huewe wrote: > Hi Ken, > (again speaking only on my behalf, not my employer) > > > Does anyone know of platforms where this occurs? > > I suspect (but not sure) that the days of SuperIO connecting floppy > > drives, printer ports, and PS/2 mouse ports on the LPC bus are over, and > > such legacy systems will not have a TPM. Would SuperIO even support the > > special TPM LPC bus cycles? > > Since we are the linux kernel, we do have to care for legacy devices. > And a system with LPC, PS2Mouse on SuperIO and a TPM are not that uncommon. > > And heck, we even have support for 1.1b TPM devices.... > > > >> One more viewpoint: TCG must added the burst count for a reason (might > >> be very well related what Peter said). Is ignoring it something that TCG > >> recommends? Not following standard exactly in the driver code sometimes > >> makes sense on *small details* but I would not say that this a small > >> detail... > > > I checked with the TCG's device driver work group (DDWG). Both the spec > > editor and 3 TPM vendors - Infineon, Nuvoton, and ST Micro - agreed that > > ignoring burst count may incur wait states but nothing more. Operations > > will still be successful. > > Interesting - let me check with Georg tomorrow. > Unfortunately I do not have access to my tcg mails from home (since I'm not working :), > but did you _explicitly_ talk about LPC and the system? > I'm sure the TPM does not care about the waitstates... > > If my memory does not betray me, > it is actually possible to "freeze up" a system completly by flooding the lpc bus. > Let me double check tomorrow... > > > In anycase - I really would like to see a much more performant tpm subsystem - > however it will be quite an effort with a lot of legacy testing. > (which I unfortunately cannot spend on my private time ... and also of course lacking test systems). > > Thanks, > Peter I would like to see tpm_msleep() wrapper to replace current msleep() usage across the subsystem before considering this. I.e. wrapper that internally uses usleep_range(). This way we can mechanically convert everything to a more low latency option. This should have been done already for patch that Mini and Nayna provided instead of open coding stuff. That change is something that can be applied right now. On the other hand, this is a very controversial change. /Jarkko -- 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
On Fri, 2017-08-11 at 14:14 +0300, Jarkko Sakkinen wrote: > On Wed, Aug 09, 2017 at 11:00:36PM +0200, Peter Huewe wrote: > > Hi Ken, > > (again speaking only on my behalf, not my employer) > > > > > Does anyone know of platforms where this occurs? > > > I suspect (but not sure) that the days of SuperIO connecting floppy > > > drives, printer ports, and PS/2 mouse ports on the LPC bus are over, and > > > such legacy systems will not have a TPM. Would SuperIO even support the > > > special TPM LPC bus cycles? > > > > Since we are the linux kernel, we do have to care for legacy devices. > > And a system with LPC, PS2Mouse on SuperIO and a TPM are not that uncommon. > > > > And heck, we even have support for 1.1b TPM devices.... > > > > > > >> One more viewpoint: TCG must added the burst count for a reason (might > > >> be very well related what Peter said). Is ignoring it something that TCG > > >> recommends? Not following standard exactly in the driver code sometimes > > >> makes sense on *small details* but I would not say that this a small > > >> detail... > > > > > I checked with the TCG's device driver work group (DDWG). Both the spec > > > editor and 3 TPM vendors - Infineon, Nuvoton, and ST Micro - agreed that > > > ignoring burst count may incur wait states but nothing more. Operations > > > will still be successful. > > > > Interesting - let me check with Georg tomorrow. > > Unfortunately I do not have access to my tcg mails from home (since I'm not working :), > > but did you _explicitly_ talk about LPC and the system? > > I'm sure the TPM does not care about the waitstates... > > > > If my memory does not betray me, > > it is actually possible to "freeze up" a system completly by flooding the lpc bus. > > Let me double check tomorrow... > > > > > > In anycase - I really would like to see a much more performant tpm subsystem - > > however it will be quite an effort with a lot of legacy testing. > > (which I unfortunately cannot spend on my private time ... and also of course lacking test systems). > > > > Thanks, > > Peter > > I would like to see tpm_msleep() wrapper to replace current msleep() > usage across the subsystem before considering this. I.e. wrapper that > internally uses usleep_range(). This way we can mechanically convert > everything to a more low latency option. Fine. I assume you meant tpm_sleep(), not tpm_msleep(). > This should have been done already for patch that Mini and Nayna > provided instead of open coding stuff. At that time, we had no idea what caused the major change in TPM performance. We only knew that the change occurred somewhere between linux-4.7 and linux-4.8. Even after figuring out it was the change to msleep(), we were hoping that msleep() would be fixed. So your comment, that we should have done it differently back then, is unwarranted. > That change is something that can be applied right now. On the other > hand, this is a very controversial change. Since the main concern about this change is breaking old systems that might potentially have other peripherals hanging off the LPC bus, can we define a new Kconfig option, with the default as 'N'? Mimi -- 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
On 8/9/2017 5:00 PM, Peter Huewe wrote: > > Since we are the linux kernel, we do have to care for legacy > devices. And a system with LPC, PS2Mouse on SuperIO and a TPM are not > that uncommon. > > And heck, we even have support for 1.1b TPM devices.... Understood. However, remember that SuperIO is a 1980's device that predates the TPM. Since the TPM requires special LPC bus cycles, it's even less likely that an old chipset has an attached TPM. > Interesting - let me check with Georg tomorrow. Unfortunately I do > not have access to my tcg mails from home (since I'm not working :), > but did you _explicitly_ talk about LPC and the system? I'm sure the > TPM does not care about the waitstates... Yes, Infineon was one of the 3 TPM vendors who confirmed this. -- 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
Following up on this thread based on this week's TCG call ... 1 - burstCount can safely be ignored on writes. This is explicit in most places in the TCG spec. In places where it is not explicit, it was simply an editorial omission. We are going through the spec and adding "without incurring wait states." TCG is willing to publish an errata if that makes developers more comfortable. 2 - These are multi-mhz buses. The TPM vendors conformed that wait states, even if incurred, will be sub-usec. I.e., less that a microsecond. Essentially, the DD is loading the FIFO, and the TPM is unloading the FIFO at processor speeds. Thus, even if one were worried about an odd system new enough to have a TPM, but old enough to have an LPC attached printer, keyboard, mouse or floppy, the delay in printing or typing will be insignificant. 3 - I asked several platform vendors with long TCG experience, and they said that they know of no motherboards that share the LPC bus with a TPM plus another device. -- 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
On 8/9/2017 4:43 PM, Peter Huewe wrote: > > Yes that's bad, especially with current msleep(5) is actually > msleep(20). However, please also keep in mind SPI tpms show a much > higher burst count value, (255) our I2C TPM SLB9645 even shows > something in the range of 1k. :) One of our platforms has a TPM 1.2 with an 8 byte FIFO and a static burst count. This is where we're debugging. >> Would another solution be to reduce the burst count poll and sleep >> to, e.g., 100 usec or even 10 usec? This would probably help >> greatly, but till not incur the wait states that triggered the >> NACK. > > If you use sleep it's not guaranteed that you wakeup after exactly xx > specified amount of time. Just that you sleep at least xx amount of > time. Otherwise you would have to do delay/busywaiting. Understood. However, if the DD maintainers will not accept ignoring burstCount, would a short sleep (e.g., 10 usec) be acceptable. If so, we can benchmark it. > Imho the best option is to figure out whether any vendor can > determine the "FIFO flush time" i.e. how much time does it take to > empty the fifo and fillup the burstcount and use this on the lower > bound of an usleep range. I don't think tpms are in the range of < 10 > us... > > @Ken: Maybe can you check in DDWG? I asked this week. Nuvoton, ST Micro, and Infineon confirmed that the TPM empties a byte from the FIFO in under 1 usec. So, even with a static burst count, the entire FIFO would empty in under 10 usec. -- 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
Hello, On Fri, 11 Aug 2017 17:32:12 -0400 Ken Goldman <kgold@linux.vnet.ibm.com> wrote: > On 8/9/2017 5:00 PM, Peter Huewe wrote: > > > > Since we are the linux kernel, we do have to care for legacy > > devices. And a system with LPC, PS2Mouse on SuperIO and a TPM are > > not that uncommon. > > > > And heck, we even have support for 1.1b TPM devices.... > > Understood. However, remember that SuperIO is a 1980's device that > predates the TPM. Since the TPM requires special LPC bus cycles, it's > even less likely that an old chipset has an attached TPM. Where are the PS/2 ports attached today? About 500 out of 700 mainboards sold today has a PS/2 port which is probably due to prevalence of legacy devices and usbhid limitations. Similarily many boards have serial and parallel hardware ports. In all diagrams detailed enough to show these ports I have seen them attached to the LPC bus. Thanks Michal -- 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
On Fri, Aug 11, 2017 at 11:30:19AM -0400, Mimi Zohar wrote: > On Fri, 2017-08-11 at 14:14 +0300, Jarkko Sakkinen wrote: > > On Wed, Aug 09, 2017 at 11:00:36PM +0200, Peter Huewe wrote: > > > Hi Ken, > > > (again speaking only on my behalf, not my employer) > > > > > > > Does anyone know of platforms where this occurs? > > > > I suspect (but not sure) that the days of SuperIO connecting floppy > > > > drives, printer ports, and PS/2 mouse ports on the LPC bus are over, and > > > > such legacy systems will not have a TPM. Would SuperIO even support the > > > > special TPM LPC bus cycles? > > > > > > Since we are the linux kernel, we do have to care for legacy devices. > > > And a system with LPC, PS2Mouse on SuperIO and a TPM are not that uncommon. > > > > > > And heck, we even have support for 1.1b TPM devices.... > > > > > > > > > >> One more viewpoint: TCG must added the burst count for a reason (might > > > >> be very well related what Peter said). Is ignoring it something that TCG > > > >> recommends? Not following standard exactly in the driver code sometimes > > > >> makes sense on *small details* but I would not say that this a small > > > >> detail... > > > > > > > I checked with the TCG's device driver work group (DDWG). Both the spec > > > > editor and 3 TPM vendors - Infineon, Nuvoton, and ST Micro - agreed that > > > > ignoring burst count may incur wait states but nothing more. Operations > > > > will still be successful. > > > > > > Interesting - let me check with Georg tomorrow. > > > Unfortunately I do not have access to my tcg mails from home (since I'm not working :), > > > but did you _explicitly_ talk about LPC and the system? > > > I'm sure the TPM does not care about the waitstates... > > > > > > If my memory does not betray me, > > > it is actually possible to "freeze up" a system completly by flooding the lpc bus. > > > Let me double check tomorrow... > > > > > > > > > In anycase - I really would like to see a much more performant tpm subsystem - > > > however it will be quite an effort with a lot of legacy testing. > > > (which I unfortunately cannot spend on my private time ... and also of course lacking test systems). > > > > > > Thanks, > > > Peter > > > > I would like to see tpm_msleep() wrapper to replace current msleep() > > usage across the subsystem before considering this. I.e. wrapper that > > internally uses usleep_range(). This way we can mechanically convert > > everything to a more low latency option. > > Fine. I assume you meant tpm_sleep(), not tpm_msleep(). I think it would sense to have a function that takes msecs because msecs are mostly used everywhere in the subsystem. This way we don't have to change any of the existing constants. > > This should have been done already for patch that Mini and Nayna > > provided instead of open coding stuff. > > At that time, we had no idea what caused the major change in TPM > performance. We only knew that the change occurred somewhere between > linux-4.7 and linux-4.8. Even after figuring out it was the change to > msleep(), we were hoping that msleep() would be fixed. So your > comment, that we should have done it differently back then, is > unwarranted. I wasn't trying to point the blame to you at all. I didn't bring this to table back then myself. I agree what you are saying. I was mainly trying to explain why I think it should be done this way now while I didn't suggest it back then :-) > > That change is something that can be applied right now. On the other > > hand, this is a very controversial change. > > Since the main concern about this change is breaking old systems that > might potentially have other peripherals hanging off the LPC bus, can > we define a new Kconfig option, with the default as 'N'? > > Mimi I guess that could make sense but I would like to hear feedback first. /Jarkko -- 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
On Mon, Aug 14, 2017 at 01:51:30PM +0300, Jarkko Sakkinen wrote: > On Fri, Aug 11, 2017 at 11:30:19AM -0400, Mimi Zohar wrote: > > On Fri, 2017-08-11 at 14:14 +0300, Jarkko Sakkinen wrote: > > > On Wed, Aug 09, 2017 at 11:00:36PM +0200, Peter Huewe wrote: > > > > Hi Ken, > > > > (again speaking only on my behalf, not my employer) > > > > > > > > > Does anyone know of platforms where this occurs? > > > > > I suspect (but not sure) that the days of SuperIO connecting floppy > > > > > drives, printer ports, and PS/2 mouse ports on the LPC bus are over, and > > > > > such legacy systems will not have a TPM. Would SuperIO even support the > > > > > special TPM LPC bus cycles? > > > > > > > > Since we are the linux kernel, we do have to care for legacy devices. > > > > And a system with LPC, PS2Mouse on SuperIO and a TPM are not that uncommon. > > > > > > > > And heck, we even have support for 1.1b TPM devices.... > > > > > > > > > > > > >> One more viewpoint: TCG must added the burst count for a reason (might > > > > >> be very well related what Peter said). Is ignoring it something that TCG > > > > >> recommends? Not following standard exactly in the driver code sometimes > > > > >> makes sense on *small details* but I would not say that this a small > > > > >> detail... > > > > > > > > > I checked with the TCG's device driver work group (DDWG). Both the spec > > > > > editor and 3 TPM vendors - Infineon, Nuvoton, and ST Micro - agreed that > > > > > ignoring burst count may incur wait states but nothing more. Operations > > > > > will still be successful. > > > > > > > > Interesting - let me check with Georg tomorrow. > > > > Unfortunately I do not have access to my tcg mails from home (since I'm not working :), > > > > but did you _explicitly_ talk about LPC and the system? > > > > I'm sure the TPM does not care about the waitstates... > > > > > > > > If my memory does not betray me, > > > > it is actually possible to "freeze up" a system completly by flooding the lpc bus. > > > > Let me double check tomorrow... > > > > > > > > > > > > In anycase - I really would like to see a much more performant tpm subsystem - > > > > however it will be quite an effort with a lot of legacy testing. > > > > (which I unfortunately cannot spend on my private time ... and also of course lacking test systems). > > > > > > > > Thanks, > > > > Peter > > > > > > I would like to see tpm_msleep() wrapper to replace current msleep() > > > usage across the subsystem before considering this. I.e. wrapper that > > > internally uses usleep_range(). This way we can mechanically convert > > > everything to a more low latency option. > > > > Fine. I assume you meant tpm_sleep(), not tpm_msleep(). > > I think it would sense to have a function that takes msecs because msecs > are mostly used everywhere in the subsystem. This way we don't have to > change any of the existing constants. > > > > This should have been done already for patch that Mini and Nayna > > > provided instead of open coding stuff. > > > > At that time, we had no idea what caused the major change in TPM > > performance. We only knew that the change occurred somewhere between > > linux-4.7 and linux-4.8. Even after figuring out it was the change to > > msleep(), we were hoping that msleep() would be fixed. So your > > comment, that we should have done it differently back then, is > > unwarranted. > > I wasn't trying to point the blame to you at all. I didn't bring this to > table back then myself. I agree what you are saying. > > I was mainly trying to explain why I think it should be done this way > now while I didn't suggest it back then :-) > > > > That change is something that can be applied right now. On the other > > > hand, this is a very controversial change. > > > > Since the main concern about this change is breaking old systems that > > might potentially have other peripherals hanging off the LPC bus, can > > we define a new Kconfig option, with the default as 'N'? > > > > Mimi > > I guess that could make sense but I would like to hear feedback first. > > /Jarkko And I'm worried would that it'd be left for many years to come as an option. I do not have any metrics what portion of hardware in the field would break if this is turned on. It would slow down kernel testing as I would have to run tests for the driver with that option turned on and off because it is a major shift from how driver functions. And I have zero idea how long I would go on doing this. One maybe a little bit better option would be to have a sysfs attribute for this functionality (disable_burst_count). What do you think about that? /Jarkko -- 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
On Mon, 2017-08-14 at 13:56 +0300, Jarkko Sakkinen wrote: > > > > I would like to see tpm_msleep() wrapper to replace current msleep() > > > > usage across the subsystem before considering this. I.e. wrapper that > > > > internally uses usleep_range(). This way we can mechanically convert > > > > everything to a more low latency option. > > > > > > Fine. I assume you meant tpm_sleep(), not tpm_msleep(). > > > > I think it would sense to have a function that takes msecs because msecs > > are mostly used everywhere in the subsystem. This way we don't have to > > change any of the existing constants. For now converting from msleep() to tpm_msleep() will be straight forward. Internally we would just use usleep_range(). Going forward, my concern is that even 1 msec might be too long for some of these sleeps. Mimi -- 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
On Mon, 2017-08-14 at 13:56 +0300, Jarkko Sakkinen wrote: > > > Since the main concern about this change is breaking old systems that > > > might potentially have other peripherals hanging off the LPC bus, can > > > we define a new Kconfig option, with the default as 'N'? > > > > > > Mimi > > > > I guess that could make sense but I would like to hear feedback first. > > > > /Jarkko > > And I'm worried would that it'd be left for many years to come as an > option. I do not have any metrics what portion of hardware in the field > would break if this is turned on. > > It would slow down kernel testing as I would have to run tests for the > driver with that option turned on and off because it is a major shift > from how driver functions. And I have zero idea how long I would go on > doing this. > > One maybe a little bit better option would be to have a sysfs attribute > for this functionality (disable_burst_count). What do you think about > that? That works! So we'll define a module_param named disable_burst_count, which can be specified on the boot command line. Mimi -- 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
On Mon, Aug 14, 2017 at 08:03:05AM -0400, Mimi Zohar wrote: > On Mon, 2017-08-14 at 13:56 +0300, Jarkko Sakkinen wrote: > > > > > > I would like to see tpm_msleep() wrapper to replace current msleep() > > > > > usage across the subsystem before considering this. I.e. wrapper that > > > > > internally uses usleep_range(). This way we can mechanically convert > > > > > everything to a more low latency option. > > > > > > > > Fine. I assume you meant tpm_sleep(), not tpm_msleep(). > > > > > > I think it would sense to have a function that takes msecs because msecs > > > are mostly used everywhere in the subsystem. This way we don't have to > > > change any of the existing constants. > > For now converting from msleep() to tpm_msleep() will be straight > forward. Internally we would just use usleep_range(). > > Going forward, my concern is that even 1 msec might be too long for > some of these sleeps. > > Mimi We can revisit this. I would take the simple route right now. /Jarkko -- 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
On Mon, Aug 14, 2017 at 08:12:53AM -0400, Mimi Zohar wrote: > On Mon, 2017-08-14 at 13:56 +0300, Jarkko Sakkinen wrote: > > > > > Since the main concern about this change is breaking old systems that > > > > might potentially have other peripherals hanging off the LPC bus, can > > > > we define a new Kconfig option, with the default as 'N'? > > > > > > > > Mimi > > > > > > I guess that could make sense but I would like to hear feedback first. > > > > > > /Jarkko > > > > And I'm worried would that it'd be left for many years to come as an > > option. I do not have any metrics what portion of hardware in the field > > would break if this is turned on. > > > > It would slow down kernel testing as I would have to run tests for the > > driver with that option turned on and off because it is a major shift > > from how driver functions. And I have zero idea how long I would go on > > doing this. > > > > One maybe a little bit better option would be to have a sysfs attribute > > for this functionality (disable_burst_count). What do you think about > > that? > > That works! So we'll define a module_param named disable_burst_count, > which can be specified on the boot command line. > > Mimi That would work for me. /Jarkko -- 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
On 8/13/2017 7:53 PM, msuchanek wrote: > About 500 out of 700 mainboards sold today has a PS/2 port which is > probably due to prevalence of legacy devices and usbhid limitations. > > Similarily many boards have serial and parallel hardware ports. > > In all diagrams detailed enough to show these ports I have seen them > attached to the LPC bus. Do these boards have a TPM? Remember that the TPM requires special LPC bus cycles. Even if so, the TPM LPC bus wait states are less than a usec. My thought is that it's unlikely that any device (serial port, mouse, keyboard, printer) will be adversely affected. -- 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
On Tue, 15 Aug 2017 18:02:57 -0400 Ken Goldman <kgold@linux.vnet.ibm.com> wrote: > On 8/13/2017 7:53 PM, msuchanek wrote: > > About 500 out of 700 mainboards sold today has a PS/2 port which is > > probably due to prevalence of legacy devices and usbhid limitations. > > > > Similarily many boards have serial and parallel hardware ports. > > > > In all diagrams detailed enough to show these ports I have seen them > > attached to the LPC bus. > > Do these boards have a TPM? Remember that the TPM requires special > LPC bus cycles. Out of nearly 700 boards over 500 have PS/2 connector and over 400 have TPM slot (which is subset of the PS/2 enabled boards). Some more possibly have on-board TPM chip. > > Even if so, the TPM LPC bus wait states are less than a usec. My > thought is that it's unlikely that any device (serial port, mouse, > keyboard, printer) will be adversely affected. Yes, in theory this is negligible. So unless there is a possibility these wait states chain or the device otherwise takes over the bus for extended period of time this should be fine. Thanks Michal -- 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
On 8/14/2017 6:10 AM, Jarkko Sakkinen wrote: >> Nuvoton, ST Micro, and Infineon confirmed that the TPM empties a byte from >> the FIFO in under 1 usec. So, even with a static burst count, the entire >> FIFO would empty in under 10 usec. > > Does this break anything lets say in a decade time frame? If it does, I > will not even consider this. Can you give a definitive answer for that? My attempt at predicting the future ... 1 - Clock speed should get faster, not slower, so the 1 usec wait state should get shorter. 2 - TPM buffers should get larger. I'm already reading of 256 byte buffers. So there should be fewer wait states. 3 - There is a trend toward using the CRB, making the FIFO less applicable. 4 - There is a trend away from LPC connected serial port devices, printers, and PS/2 mouse and keyboard, and toward USB connected devices. I assume this will continue, making the already insignificant wait states irrelevant. -- 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
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c index b617b2eeb080..478cbc0f61c3 100644 --- a/drivers/char/tpm/tpm_tis_core.c +++ b/drivers/char/tpm/tpm_tis_core.c @@ -255,9 +255,7 @@ static int tpm_tis_recv(struct tpm_chip *chip, u8 *buf, size_t count) static int tpm_tis_send_data(struct tpm_chip *chip, 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; + int rc, status; status = tpm_tis_status(chip); if ((status & TPM_STS_COMMAND_READY) == 0) { @@ -270,49 +268,10 @@ static int tpm_tis_send_data(struct tpm_chip *chip, u8 *buf, size_t len) } } - while (count < len - 1) { - 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); - 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]); + rc = tpm_tis_write_bytes(priv, TPM_DATA_FIFO(priv->locality), len, buf); if (rc < 0) goto out_err; - 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; - } - return 0; out_err: