[PATCH] tpm_tis_spi: Prefer async probe

[PATCH] tpm_tis_spi: Prefer async probe

[Douglas Anderson]

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

Re: [PATCH] tpm_tis_spi: Prefer async probe

[Doug Anderson]

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

Re: [PATCH] tpm_tis_spi: Prefer async probe

[Jarkko Sakkinen]

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