On Fri, Jan 08, 2021 at 05:41:24PM +0100, Mark Kettenis wrote: > > Date: Fri, 8 Jan 2021 10:27:38 -0600 > > From: Scott Cheloha <[email protected]> > > > > On Wed, Jan 06, 2021 at 11:26:27PM +0100, Mark Kettenis wrote: > > > > Date: Wed, 6 Jan 2021 16:16:27 -0600 > > > > From: Scott Cheloha <[email protected]> > > > > > > > > On Wed, Jan 06, 2021 at 12:16:13PM -0600, Scott Cheloha wrote: > > > > > As mentioned in a prior mail, tpm(4) is the last user of tvtohz(9) in > > > > > the tree. > > > > > > > > > > However, we don't need to use tvtohz(9) in tpm(4) at all. Converting > > > > > from milliseconds to ticks is trivial. Using an intermediary timeval > > > > > is just pointless indirection. > > > > > > > > > > With this committed I will be able to remove both tvtohz(9) and > > > > > tstohz(9) from the tree in a subsequent patch. > > > > > > > > Whoops, made a mistake in the prior diff. > > > > > > > > We want to MAX() the result up to 1 to avoid dividing to zero and > > > > blocking indefinitely in e.g. tsleep(9). Previous diff MIN'd the > > > > result down to 1, which is very wrong. > > > > > > To be honest I'd just zap the function completely and instead simply do: > > > > > > to = TPM_ACCESS_TMO / 10; > > > > > > and > > > > > > to = TPM_BURST_TMO / 10; > > > > That won't work on custom kernels where HZ is not 100, no? > > HZ is irrelevant here. These TPM_XXX_TMO defines specify a timeout in > microseconds and DELAY(10) just delays for 10us. So you just need to > figure out how many times you need to do the 10us delay to reach the > timeout specified by TPM_XXX_TMO.
Whoops, yes, you're right. > Also, this driver is only supported on amd64/i386 at this point (but > might show up on arm64 at some point). > > > > There is no magic happening here. The code is just doing a hardware > > > register poll loop in 10us steps. > > > > > > Hmm, actually it seems the code is broken and uses steps of 10 > > > microseconds instead of milliseconds. So instead it should probably > > > use: > > > > > > to = TPM_ACCESS_TMO * 100; > > > > > > and > > > > > > to = TPM_BURST_TMO * 100; > > > > This problem came up in a different thread: > > > > https://marc.info/?l=openbsd-tech&m=160833962329381&w=2 > > > > jcs@ said, and I paraphrase, "tpm(4) sucks, we should merge NetBSD's > > latest code, which is much nicer now." > > > > That sounds like a much taller order than I can fill, so I'm just > > trying to remove the tvtohz(9) call without changing any behavior, > > even if the behavior looks wrong (like using the wrong units). > > > > I don't even have a tpm(4) device to test. Until I have one I'm > > reluctant to do things like expanding the delay time in these loops. > > As of now the driver "works" for some subset of people, which is not > > nothing. > > But having illogical code is a problem as well. So I think we should > at least attempt to fix it the right way. All it takes is to find > someone with a laptop where the driver attaches and have them > suspend/resume it. We can try that, sure. Here's the patch: - Remove tpm_tmotohz(). - tpm_waitfor() does DELAY(1), so in that case convert from milliseconds to microseconds. Note in the function argument that we expect milliseconds, not "tries". - In the other cases we do DELAY(10), so we only multiply by 100. Leave a comment explaining what we're doing. Unsure who can test. We need a suspend/resume test with this patch. Probably an older machine. Newer machines tend to have TPM 2.0 chips. Index: tpm.c =================================================================== RCS file: /cvs/src/sys/dev/acpi/tpm.c,v retrieving revision 1.10 diff -u -p -r1.10 tpm.c --- tpm.c 22 May 2020 10:16:37 -0000 1.10 +++ tpm.c 8 Jan 2021 17:43:35 -0000 @@ -158,7 +158,6 @@ int tpm_request_locality(struct tpm_soft void tpm_release_locality(struct tpm_softc *); int tpm_getburst(struct tpm_softc *); uint8_t tpm_status(struct tpm_softc *); -int tpm_tmotohz(int); struct cfattach tpm_ca = { sizeof(struct tpm_softc), @@ -385,7 +384,7 @@ tpm_request_locality(struct tpm_softc *s bus_space_write_1(sc->sc_bt, sc->sc_bh, TPM_ACCESS, TPM_ACCESS_REQUEST_USE); - to = tpm_tmotohz(TPM_ACCESS_TMO); + to = TPM_ACCESS_TMO * 100; /* steps of 10 microseconds */ while ((r = bus_space_read_1(sc->sc_bt, sc->sc_bh, TPM_ACCESS) & (TPM_ACCESS_VALID | TPM_ACCESS_ACTIVE_LOCALITY)) != @@ -420,7 +419,7 @@ tpm_getburst(struct tpm_softc *sc) { int burst, burst2, to; - to = tpm_tmotohz(TPM_BURST_TMO); + to = TPM_BURST_TMO * 100; /* steps of 10 microseconds */ burst = 0; while (burst == 0 && to--) { @@ -453,29 +452,21 @@ tpm_status(struct tpm_softc *sc) } int -tpm_tmotohz(int tmo) -{ - struct timeval tv; - - tv.tv_sec = tmo / 1000; - tv.tv_usec = 1000 * (tmo % 1000); - - return tvtohz(&tv); -} - -int -tpm_waitfor(struct tpm_softc *sc, uint8_t mask, int tries) +tpm_waitfor(struct tpm_softc *sc, uint8_t mask, int msecs) { + int usecs; uint8_t status; + usecs = msecs * 1000; + while (((status = tpm_status(sc)) & mask) != mask) { - if (tries == 0) { + if (usecs == 0) { DPRINTF(("%s: %s: timed out, status 0x%x != 0x%x\n", sc->sc_dev.dv_xname, __func__, status, mask)); return status; } - tries--; + usecs--; DELAY(1); }
