Linux-Integrity Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] tpm_tis_spi: Prefer async probe
@ 2020-06-19 21:20 Douglas Anderson
  2020-06-22 14:56 ` Doug Anderson
  2020-06-23  1:36 ` Jarkko Sakkinen
  0 siblings, 2 replies; 3+ messages in thread
From: Douglas Anderson @ 2020-06-19 21:20 UTC (permalink / raw)
  To: Peter Huewe, Jarkko Sakkinen
  Cc: Nicolas Boichat, Andrey Pronin, Stephen Boyd, Douglas Anderson,
	Arnd Bergmann, Greg Kroah-Hartman, Jason Gunthorpe,
	linux-integrity, linux-kernel

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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] tpm_tis_spi: Prefer async probe
  2020-06-19 21:20 [PATCH] tpm_tis_spi: Prefer async probe Douglas Anderson
@ 2020-06-22 14:56 ` Doug Anderson
  2020-06-23  1:36 ` Jarkko Sakkinen
  1 sibling, 0 replies; 3+ messages in thread
From: Doug Anderson @ 2020-06-22 14:56 UTC (permalink / raw)
  To: Peter Huewe, Jarkko Sakkinen
  Cc: Nicolas Boichat, Andrey Pronin, Stephen Boyd, Arnd Bergmann,
	Greg Kroah-Hartman, Jason Gunthorpe, linux-integrity, LKML

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] tpm_tis_spi: Prefer async probe
  2020-06-19 21:20 [PATCH] tpm_tis_spi: Prefer async probe Douglas Anderson
  2020-06-22 14:56 ` Doug Anderson
@ 2020-06-23  1:36 ` Jarkko Sakkinen
  1 sibling, 0 replies; 3+ messages in thread
From: Jarkko Sakkinen @ 2020-06-23  1:36 UTC (permalink / raw)
  To: Douglas Anderson
  Cc: Peter Huewe, Nicolas Boichat, Andrey Pronin, Stephen Boyd,
	Arnd Bergmann, Greg Kroah-Hartman, Jason Gunthorpe,
	linux-integrity, linux-kernel

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-19 21:20 [PATCH] tpm_tis_spi: Prefer async probe Douglas Anderson
2020-06-22 14:56 ` Doug Anderson
2020-06-23  1:36 ` Jarkko Sakkinen

Linux-Integrity Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-integrity/0 linux-integrity/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-integrity linux-integrity/ https://lore.kernel.org/linux-integrity \
		linux-integrity@vger.kernel.org
	public-inbox-index linux-integrity

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-integrity


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git