Hi James, > Upping that to 15ms introduces a 100x delay in > our status wait for the TPM to become ready, potentially slowing down > all TIS TPM operations by 100x, which will hit us most with the PCR > writes we do for IMA logging ... that seems like a bad bargain for a > single TPM family manufacturer. 1. It is unlike to be 100x delay According to https://github.com/torvalds/linux/commit/59f5a6b07f6434efac0057dc2f303a96b871811b https://github.com/torvalds/linux/commit/424eaf910c329ab06ad03a527ef45dcf6a328f00 It only optimize from 14sec to 7sec. Which is only a 2x speed up by using sleep time from 5ms to >1ms. Here we change it back to 15 ms is very unlikely to have 100x delay. The optimization does not worth to have a breakage on chip from one manufacturer. 2. In my opinion, the kernel should support all manufacturers which were supported before. Not supporting any of them would lead to kernel divergence, because those chip users have to Use it anyway. Maybe we can see the maintainers’ opinion on this. 3. I am kind of opposing to coming up smaller values without doing comprehensive qualification on all supported manufacturers. Stable is probably more important for such software. Looking back to these commits that introduced the breakages, only one or two chips are tested. If that is a common case, probably we should refactor the TPM driver to better support per-manufacturer configuration? > However, there is another possibility: it's something to do with the > byte read; I notice you don't require the same slowdown for the burst > count read, which actually reads the status register and burst count as > a read32. If that really is the case, for the atmel would substituting > a read32 and just throwing the upper bytes away in tpm_tis_status() > allow us to keep the current timings? I can actually try doing this > and see if it fixes my nuvoton. If would be helpful if you can find the solution without reducing performance. I think it is a separate problem to address though. Maybe not worth to mix them in the same fix. Thanks Hao > On Sep 27, 2020, at 6:22 PM, James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote: > > On Sun, 2020-09-27 at 17:11 -0700, Hao Wu wrote: >> Hi James, >> >> Maybe there is a misunderstanding. Here I am using tpm_msleep, not >> msleep. tpm_msleep is using usleep underlaying. See >> https://github.com/torvalds/linux/blob/master/drivers/char/tpm/tpm.h#L188 > > Right, I had missed that. > >> The reasons I choose 15ms, is because before >> https://github.com/torvalds/linux/commit/9f3fc7bcddcb51234e23494531f93ab60475e1c3 >> (Where msleep is changed to tpm_msleep (which is essentially >> usleep)), The actual sleep time is 15ms, thus here we change this >> back to 15ms to fix regression. > > Right now most TIS TPMs operate successfully with a sleep in there of > the range 0.1-0.5ms. Upping that to 15ms introduces a 100x delay in > our status wait for the TPM to become ready, potentially slowing down > all TIS TPM operations by 100x, which will hit us most with the PCR > writes we do for IMA logging ... that seems like a bad bargain for a > single TPM family manufacturer. > > However, there is another possibility: it's something to do with the > byte read; I notice you don't require the same slowdown for the burst > count read, which actually reads the status register and burst count as > a read32. If that really is the case, for the atmel would substituting > a read32 and just throwing the upper bytes away in tpm_tis_status() > allow us to keep the current timings? I can actually try doing this > and see if it fixes my nuvoton. > > James > >
