On Fri, Jan 08, 2021 at 08:21:14PM +0100, Florian Obser wrote: > On Fri, Jan 08, 2021 at 11:48:33AM -0600, Scott Cheloha wrote: > > 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. > > My x1 gen 2 seems to have a TPM 1.2 chip. Or can emulate one? > The bios is confusing. I had it disabled until now. > It seems to be able to suspend/resume just fine with this. > > It shows up like this: > > tpm0 at acpi0 TPM_ addr 0xfed40000/0x5000, device 0x0000104a rev 0x4e > > No idea what this does, I'm going to disable it again, but let me know > if you want further tests.
kettenis, jcs: Is this sufficient? Or do I need more tests? > OpenBSD 6.8-current (GENERIC.MP) #103: Fri Jan 8 20:11:51 CET 2021 > [email protected]:/usr/src/sys/arch/amd64/compile/GENERIC.MP > real mem = 8233394176 (7851MB) > avail mem = 7968542720 (7599MB) > random: good seed from bootblocks > mpath0 at root > scsibus0 at mpath0: 256 targets > mainbus0 at root > bios0 at mainbus0: SMBIOS rev. 2.7 @ 0xdcd3d000 (61 entries) > bios0: vendor LENOVO version "GRET40WW (1.17 )" date 09/02/2014 > bios0: LENOVO 20A7006VUS > acpi0 at bios0: ACPI 5.0 > acpi0: sleep states S0 S3 S4 S5 > acpi0: tables DSDT FACP SLIC DBGP ECDT HPET APIC MCFG SSDT SSDT SSDT SSDT > SSDT SSDT SSDT PCCT SSDT TCPA UEFI MSDM ASF! BATB FPDT UEFI DMAR > acpi0: wakeup devices LID_(S4) SLPB(S3) IGBE(S4) EXP2(S4) XHCI(S3) EHC1(S3) > acpitimer0 at acpi0: 3579545 Hz, 24 bits > acpiec0 at acpi0 > acpihpet0 at acpi0: 14318179 Hz > acpimadt0 at acpi0 addr 0xfee00000: PC-AT compat > cpu0 at mainbus0: apid 0 (boot processor) > cpu0: Intel(R) Core(TM) i7-4600U CPU @ 2.10GHz, 798.27 MHz, 06-45-01 > cpu0: > FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,PERF,ITSC,FSGSBASE,TSC_ADJUST,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,SRBDS_CTRL,MD_CLEAR,IBRS,IBPB,STIBP,L1DF,SSBD,SENSOR,ARAT,XSAVEOPT,MELTDOWN > cpu0: 256KB 64b/line 8-way L2 cache > cpu0: smt 0, core 0, package 0 > mtrr: Pentium Pro MTRR support, 10 var ranges, 88 fixed ranges > cpu0: apic clock running at 99MHz > cpu0: mwait min=64, max=64, C-substates=0.2.1.2.4.1.1.1, IBE > cpu1 at mainbus0: apid 1 (application processor) > cpu1: Intel(R) Core(TM) i7-4600U CPU @ 2.10GHz, 798.16 MHz, 06-45-01 > cpu1: > FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,PERF,ITSC,FSGSBASE,TSC_ADJUST,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,SRBDS_CTRL,MD_CLEAR,IBRS,IBPB,STIBP,L1DF,SSBD,SENSOR,ARAT,XSAVEOPT,MELTDOWN > cpu1: 256KB 64b/line 8-way L2 cache > cpu1: smt 1, core 0, package 0 > cpu2 at mainbus0: apid 2 (application processor) > cpu2: Intel(R) Core(TM) i7-4600U CPU @ 2.10GHz, 798.16 MHz, 06-45-01 > cpu2: > FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,PERF,ITSC,FSGSBASE,TSC_ADJUST,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,SRBDS_CTRL,MD_CLEAR,IBRS,IBPB,STIBP,L1DF,SSBD,SENSOR,ARAT,XSAVEOPT,MELTDOWN > cpu2: 256KB 64b/line 8-way L2 cache > cpu2: smt 0, core 1, package 0 > cpu3 at mainbus0: apid 3 (application processor) > cpu3: Intel(R) Core(TM) i7-4600U CPU @ 2.10GHz, 798.16 MHz, 06-45-01 > cpu3: > FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,SMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,PERF,ITSC,FSGSBASE,TSC_ADJUST,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,SRBDS_CTRL,MD_CLEAR,IBRS,IBPB,STIBP,L1DF,SSBD,SENSOR,ARAT,XSAVEOPT,MELTDOWN > cpu3: 256KB 64b/line 8-way L2 cache > cpu3: smt 1, core 1, package 0 > ioapic0 at mainbus0: apid 2 pa 0xfec00000, version 20, 40 pins > acpimcfg0 at acpi0 > acpimcfg0: addr 0xf8000000, bus 0-63 > acpiprt0 at acpi0: bus 0 (PCI0) > acpiprt1 at acpi0: bus -1 (PEG_) > acpiprt2 at acpi0: bus 2 (EXP1) > acpiprt3 at acpi0: bus 3 (EXP2) > acpiprt4 at acpi0: bus -1 (EXP3) > dwiic0 at acpi0 I2C1 addr 0xfe105000/0x1000 irq 7 > iic0 at dwiic0 > "CPLM3218" at iic0 addr 0x48 not configured > acpibtn0 at acpi0: LID_ > acpibtn1 at acpi0: SLPB > acpipci0 at acpi0 PCI0: 0x00000000 0x00000011 0x00000001 > acpicmos0 at acpi0 > tpm0 at acpi0 TPM_ addr 0xfed40000/0x5000, device 0x0000104a rev 0x4e > acpibat0 at acpi0: BAT0 model "45N1703" serial 3191 type LiP oem "SMP" > acpiac0 at acpi0: AC unit offline > acpithinkpad0 at acpi0: version 2.0 > "PNP0C14" at acpi0 not configured > "PNP0C14" at acpi0 not configured > "PNP0C14" at acpi0 not configured > "INT340F" at acpi0 not configured > acpicpu0 at acpi0: C3(200@506 mwait.1@0x60), C2(200@148 mwait.1@0x33), > C1(1000@1 mwait.1), PSS > acpicpu1 at acpi0: C3(200@506 mwait.1@0x60), C2(200@148 mwait.1@0x33), > C1(1000@1 mwait.1), PSS > acpicpu2 at acpi0: C3(200@506 mwait.1@0x60), C2(200@148 mwait.1@0x33), > C1(1000@1 mwait.1), PSS > acpicpu3 at acpi0: C3(200@506 mwait.1@0x60), C2(200@148 mwait.1@0x33), > C1(1000@1 mwait.1), PSS > acpipwrres0 at acpi0: PUBS, resource for XHCI, EHC1 > acpitz0 at acpi0: critical temperature is 200 degC > acpivideo0 at acpi0: VID_ > acpivout0 at acpivideo0: LCD0 > acpivideo1 at acpi0: VID_ > cpu0: using VERW MDS workaround (except on vmm entry) > cpu0: Enhanced SpeedStep 798 MHz: speeds: 2701, 2700, 2600, 2400, 2300, 2100, > 2000, 1800, 1700, 1600, 1400, 1300, 1100, 1000, 800, 756 MHz > pci0 at mainbus0 bus 0 > pchb0 at pci0 dev 0 function 0 "Intel Core 4G Host" rev 0x0b > inteldrm0 at pci0 dev 2 function 0 "Intel HD Graphics" rev 0x0b > drm0 at inteldrm0 > inteldrm0: msi, HASWELL, gen 7 > azalia0 at pci0 dev 3 function 0 "Intel Core 4G HD Audio" rev 0x0b: msi > azalia0: No codecs found > xhci0 at pci0 dev 20 function 0 "Intel 8 Series xHCI" rev 0x04: msi, xHCI 1.0 > usb0 at xhci0: USB revision 3.0 > uhub0 at usb0 configuration 1 interface 0 "Intel xHCI root hub" rev 3.00/1.00 > addr 1 > "Intel 8 Series MEI" rev 0x04 at pci0 dev 22 function 0 not configured > puc0 at pci0 dev 22 function 3 "Intel 8 Series KT" rev 0x04: ports: 16 com > com4 at puc0 port 0 apic 2 int 17: ns16550a, 16 byte fifo > com4: probed fifo depth: 0 bytes > em0 at pci0 dev 25 function 0 "Intel I218-LM" rev 0x04: msi, address > 54:ee:75:3d:fb:31 > azalia1 at pci0 dev 27 function 0 "Intel 8 Series HD Audio" rev 0x04: msi > azalia1: codecs: Realtek ALC292 > audio0 at azalia1 > ppb0 at pci0 dev 28 function 0 "Intel 8 Series PCIE" rev 0xe4: msi > pci1 at ppb0 bus 2 > ppb1 at pci0 dev 28 function 1 "Intel 8 Series PCIE" rev 0xe4: msi > pci2 at ppb1 bus 3 > iwm0 at pci2 dev 0 function 0 "Intel Dual Band Wireless AC 7260" rev 0x83, msi > ehci0 at pci0 dev 29 function 0 "Intel 8 Series USB" rev 0x04: apic 2 int 23 > usb1 at ehci0: USB revision 2.0 > uhub1 at usb1 configuration 1 interface 0 "Intel EHCI root hub" rev 2.00/1.00 > addr 1 > pcib0 at pci0 dev 31 function 0 "Intel 8 Series LPC" rev 0x04 > ahci0 at pci0 dev 31 function 2 "Intel 8 Series AHCI" rev 0x04: msi, AHCI 1.3 > ahci0: port 0: 6.0Gb/s > scsibus1 at ahci0: 32 targets > sd0 at scsibus1 targ 0 lun 0: <ATA, TOSHIBA THNSNJ51, JULA> > naa.500080dc001bf1e2 > sd0: 488386MB, 512 bytes/sector, 1000215216 sectors, thin > ichiic0 at pci0 dev 31 function 3 "Intel 8 Series SMBus" rev 0x04: apic 2 int > 18 > iic1 at ichiic0 > isa0 at pcib0 > isadma0 at isa0 > pckbc0 at isa0 port 0x60/5 irq 1 irq 12 > pckbd0 at pckbc0 (kbd slot) > wskbd0 at pckbd0: console keyboard > pms0 at pckbc0 (aux slot) > wsmouse0 at pms0 mux 0 > wsmouse1 at pms0 mux 0 > pms0: Synaptics clickpad, firmware 8.1, 0x1e2b1 0x940300 0x2d9240 0xd001a3 > 0x12e800 > pcppi0 at isa0 port 0x61 > spkr0 at pcppi0 > vmm0 at mainbus0: VMX/EPT > umb0 at uhub0 port 4 configuration 1 interface 0 "Sierra Wireless Inc. Sierra > Wireless EM7345 4G LTE" rev 2.00/17.29 addr 2 > umodem0 at uhub0 port 4 configuration 1 interface 2 "Sierra Wireless Inc. > Sierra Wireless EM7345 4G LTE" rev 2.00/17.29 addr 2 > umodem0: data interface 3, has no CM over data, has break > umodem0: status change notification available > ucom0 at umodem0 > uhub2 at uhub1 port 1 configuration 1 interface 0 "Intel Rate Matching Hub" > rev 2.00/0.04 addr 2 > uhidev0 at uhub2 port 5 configuration 1 interface 0 "ELAN Touchscreen" rev > 2.00/0.12 addr 3 > uhidev0: iclass 3/0, 68 report ids > ums0 at uhidev0 reportid 1: 1 button, tip > wsmouse2 at ums0 mux 0 > uhid0 at uhidev0 reportid 2: input=64, output=0, feature=0 > uhid1 at uhidev0 reportid 3: input=0, output=31, feature=0 > uhid2 at uhidev0 reportid 4: input=19, output=0, feature=0 > uhid3 at uhidev0 reportid 10: input=0, output=0, feature=1 > ums1 at uhidev0 reportid 68 > ums1: mouse has no X report > uvideo0 at uhub2 port 8 configuration 1 interface 0 "Chicony Electronics > Co.,Ltd. Integrated Camera" rev 2.00/25.09 addr 4 > video0 at uvideo0 > vscsi0 at root > scsibus2 at vscsi0: 256 targets > softraid0 at root > scsibus3 at softraid0: 256 targets > sd1 at scsibus3 targ 1 lun 0: <OPENBSD, SR CRYPTO, 006> > sd1: 488381MB, 512 bytes/sector, 1000206308 sectors > root on sd1a (f3054894d8af4b99.a) swap on sd1b dump on sd1b > inteldrm0: 2560x1440, 32bpp > wsdisplay0 at inteldrm0 mux 1: console (std, vt100 emulation), using wskbd0 > wsdisplay0: screen 1-5 added (std, vt100 emulation) > iwm0: hw rev 0x140, fw ver 17.3216344376.0, address 5c:c5:d4:63:b3:d9 > > > > > > 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); > > } > > > > > > -- > I'm not entirely sure you are real.
