Re: [PATCH] tpm_tis_spi: Prefer async probe

From: Doug Anderson <dianders@chromium.org>
To: Peter Huewe <peterhuewe@gmx.de>,
	Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: Nicolas Boichat <drinkcat@chromium.org>,
	Andrey Pronin <apronin@chromium.org>,
	Stephen Boyd <swboyd@chromium.org>, Arnd Bergmann <arnd@arndb.de>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jason Gunthorpe <jgg@ziepe.ca>,
	linux-integrity@vger.kernel.org,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] tpm_tis_spi: Prefer async probe
Date: Mon, 22 Jun 2020 07:56:54 -0700	[thread overview]
Message-ID: <CAD=FV=VCX27cMs4q+jJ528UdEM93yZ96+kYTQPgU3ZqZ-whgEw@mail.gmail.com> (raw)
In-Reply-To: <20200619141958.1.I58d549fded1fd2299543ede6a103fe2bb94c805d@changeid>

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