| Message ID | 20200619141958.1.I58d549fded1fd2299543ede6a103fe2bb94c805d@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tpm_tis_spi: Prefer async probe | expand |
Hi, On Fri, Jun 19, 2020 at 2:20 PM Douglas Anderson <dianders@chromium.org> wrote: > > On a Chromebook I'm working on I noticed a big (~1 second) delay > during bootup where nothing was happening. Right around this big > delay there were messages about the TPM: > > [ 2.311352] tpm_tis_spi spi0.0: TPM ready IRQ confirmed on attempt 2 > [ 3.332790] tpm_tis_spi spi0.0: Cr50 firmware version: ... > > I put a few printouts in and saw that tpm_tis_spi_init() (specifically > tpm_chip_register() in that function) was taking the lion's share of > this time, though ~115 ms of the time was in cr50_print_fw_version(). > > Let's make a one-line change to prefer async probe for tpm_tis_spi. > There's no reason we need to block other drivers from probing while we > load. > > NOTES: > * It's possible that other hardware runs through the init sequence > faster than Cr50 and this isn't such a big problem for them. > However, even if they are faster they are still doing _some_ > transfers over a SPI bus so this should benefit everyone even if to > a lesser extent. > * It's possible that there are extra delays in the code that could be > optimized out. I didn't dig since once I enabled async probe they > no longer impacted me. I will note that I did continue to dig into the delays, actually. I haven't fully resolved all of them, but I'm fairly sure that most of them are actually inefficiencies in the SPI driver on my platform, which seems to have a lot of overhead in starting a new transfer. I'll work on fixing that, but in any case we should still do the async probe because it's very safe and gives a perf boost. Why do I say it's safe? 1. It's not like our probe order was defined by anything anyway. When we probe is at the whim of when the SPI controller probes and that can be any time. 2. If someone needs to access the TPM from another driver then they have to handle the fact that it might not have probed yet anyway because perhaps the SPI controller hasn't probed yet. 3. There may be other drivers probing at the same time as us anyway because _they_ used async probe. While I won't say that it's impossible to tickle a bug by turning on async probe, I would assert that in almost all cases the bug was already there and needed to be fixed anyway. ;-) -Doug
On Fri, Jun 19, 2020 at 02:20:01PM -0700, Douglas Anderson wrote: > On a Chromebook I'm working on I noticed a big (~1 second) delay > during bootup where nothing was happening. Right around this big > delay there were messages about the TPM: > > [ 2.311352] tpm_tis_spi spi0.0: TPM ready IRQ confirmed on attempt 2 > [ 3.332790] tpm_tis_spi spi0.0: Cr50 firmware version: ... > > I put a few printouts in and saw that tpm_tis_spi_init() (specifically > tpm_chip_register() in that function) was taking the lion's share of > this time, though ~115 ms of the time was in cr50_print_fw_version(). > > Let's make a one-line change to prefer async probe for tpm_tis_spi. > There's no reason we need to block other drivers from probing while we > load. > > NOTES: > * It's possible that other hardware runs through the init sequence > faster than Cr50 and this isn't such a big problem for them. > However, even if they are faster they are still doing _some_ > transfers over a SPI bus so this should benefit everyone even if to > a lesser extent. > * It's possible that there are extra delays in the code that could be > optimized out. I didn't dig since once I enabled async probe they > no longer impacted me. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > drivers/char/tpm/tpm_tis_spi_main.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/char/tpm/tpm_tis_spi_main.c b/drivers/char/tpm/tpm_tis_spi_main.c > index d96755935529..422766445373 100644 > --- a/drivers/char/tpm/tpm_tis_spi_main.c > +++ b/drivers/char/tpm/tpm_tis_spi_main.c > @@ -288,6 +288,7 @@ static struct spi_driver tpm_tis_spi_driver = { > .pm = &tpm_tis_pm, > .of_match_table = of_match_ptr(of_tis_spi_match), > .acpi_match_table = ACPI_PTR(acpi_tis_spi_match), > + .probe_type = PROBE_PREFER_ASYNCHRONOUS, > }, > .probe = tpm_tis_spi_driver_probe, > .remove = tpm_tis_spi_remove, > -- > 2.27.0.111.gc72c7da667-goog > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> /Jarkko
diff --git a/drivers/char/tpm/tpm_tis_spi_main.c b/drivers/char/tpm/tpm_tis_spi_main.c index d96755935529..422766445373 100644 --- a/drivers/char/tpm/tpm_tis_spi_main.c +++ b/drivers/char/tpm/tpm_tis_spi_main.c @@ -288,6 +288,7 @@ static struct spi_driver tpm_tis_spi_driver = { .pm = &tpm_tis_pm, .of_match_table = of_match_ptr(of_tis_spi_match), .acpi_match_table = ACPI_PTR(acpi_tis_spi_match), + .probe_type = PROBE_PREFER_ASYNCHRONOUS, }, .probe = tpm_tis_spi_driver_probe, .remove = tpm_tis_spi_remove,
On a Chromebook I'm working on I noticed a big (~1 second) delay during bootup where nothing was happening. Right around this big delay there were messages about the TPM: [ 2.311352] tpm_tis_spi spi0.0: TPM ready IRQ confirmed on attempt 2 [ 3.332790] tpm_tis_spi spi0.0: Cr50 firmware version: ... I put a few printouts in and saw that tpm_tis_spi_init() (specifically tpm_chip_register() in that function) was taking the lion's share of this time, though ~115 ms of the time was in cr50_print_fw_version(). Let's make a one-line change to prefer async probe for tpm_tis_spi. There's no reason we need to block other drivers from probing while we load. NOTES: * It's possible that other hardware runs through the init sequence faster than Cr50 and this isn't such a big problem for them. However, even if they are faster they are still doing _some_ transfers over a SPI bus so this should benefit everyone even if to a lesser extent. * It's possible that there are extra delays in the code that could be optimized out. I didn't dig since once I enabled async probe they no longer impacted me. Signed-off-by: Douglas Anderson <dianders@chromium.org> --- drivers/char/tpm/tpm_tis_spi_main.c | 1 + 1 file changed, 1 insertion(+)