Subject: Re: [PATCH v11 00/14] TPM IRQ fixes
To:     peterhuewe@gmx.de, jarkko@kernel.org, jgg@ziepe.ca
Cc:     stefanb@linux.vnet.ibm.com, linux@mniewoehner.de,
        linux-integrity@vger.kernel.org, linux-kernel@vger.kernel.org,
        jandryuk@gmail.com, pmenzel@molgen.mpg.de, l.sanfilippo@kunbus.com,
        lukas@wunner.de, p.rosenberger@kunbus.com
References: <20221124135538.31020-1-LinoSanfilippo@gmx.de>
From:   Lino Sanfilippo <LinoSanfilippo@gmx.de>
Message-ID: <4c094418-7725-b815-f1f9-8b12f1ac4d72@gmx.de>
Date:   Fri, 24 Feb 2023 15:48:23 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <20221124135538.31020-1-LinoSanfilippo@gmx.de>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: quoted-printable
UI-OutboundReport: notjunk:1;M01:P0:d6qHNZNlTRw=;DAJCia3xrEr0b7Mx3Z3z10KbfNp
 t8MbwQ7yVXho/DXZvPWd3PIhKqrc9Lwu0mNCQOwU4gZ61yWzw9aEa5j4GeODoo7CSMRL8IhuO
 bsNuLcuYduT7H1ex7P/+OtWiFBifZSbgM8muoNNxVa1z9eHpKmNzfX5QN5YkJvKvRyXKxaiMJ
 11jAprCy/aglhYiRhGf7ChqcGiG+K3AuzMnYfYsImfCJLHtELHHTyo09vrFZpfNQBMLT26G0C
 xN7rrujvtGvKuMztzWdPUrCRNpWreRgSYbsADtMQoYQ3lXVRh/LRDxTadXp9ReY6aQwaD1YRP
 GnWW/dKv2VveP9IxcqARp0orusD99r/gWXMhuwOHwbJJf0Nh42UhOsYCQlQVTYEt39HRYKfqg
 15IBhpuP59IqZMU/It5XRDrnTCBGd42s8tlgmms6QDFoIyyO02ITk3yB9tORLHfH17sP4d/Z9
 NlSLTpKSdfv24kvO+GuHnRbpSIHkECIiVIif9+WPwBNY6dQj+Mitx6hvpej2fuddaLz0WRh5q
 Zs/iFnynapTbW/oDkhdRGzBzZxF9ATAKJ7KYnaz41YZOxEyBxBnM+s63G60waU+BIeqAXJJK9
 21Yr7wKEQAscAVM3xU924JiEhCJ7Cwgw5k16COYHERHjaSDzJeYXtw/R1OuxkpsHftdy6KMB6
 pPzNPUHFmc/6aXX/NAfmE7GpK0c+hjSBxuSBZJ4cqYiGxggtqv4lb8VS6lWPI47+8cBHkrJVD
 VWlGfUg5w7iS17E54+bObMn6GCL3oAHuct3iC35Eju58yWSqK1l3YI/lDwgxfZ7XpHzOaTUZD
 h3jJSNoE6BUB9uJPZFzWJm8XXxlLdWrMhO0t2U7Kpr31iz/lIKyY3uZpRMtCq7AuUtnwJ3I4K
 Bk0s7swt8lbkkSwFCrk2xbe1FbYk2Tdk2ScTBYx8o2fDcP2sWcXkP3Zw/5JvKRN8CS7ouqhw9
 Y5ACjw==
Precedence: bulk

On 24.11.22 at 14:55, Lino Sanfilippo wrote:
> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>
> This series enables IRQ support for the TPM TIS core. For this reason a
> number of bugfixes around the interrupt handling are required (patches 1=
 to
> 5).
>
> Patch 6 takes into account that according to the TPM Interface
> Specification stsValid and commandRead interrupts might not be supported
> by the hardware. For this reason the supported interrupts are first quer=
ied
> and stored. Then wait_for_tpm_stat() is adjusted to not wait for status
> changes that are not reported by interrupts.
>
> Patch 7 moves the interrupt flag checks into an own function as suggeste=
d
> by Jarkko.
>
> Patch 8 Removes the possibility that tpm_tis_data->locality can be chang=
ed
> at driver runtime so this variable can be read without the need to prote=
ct
> it against concurrent modification.
>
> Patch 9 addresses the issue with concurrent locality handling:
> Since the interrupt handler writes the interrupt status registers it nee=
ds
> to hold the locality. However it runs concurrently to the thread which
> triggered the interrupt (e.g. by reading or writing data to the TPM). So
> it must take care when claiming and releasing the locality itself,
> because it may race with the concurrent running thread which also claims
> and releases the locality.
> To avoid that both interrupt and concurrent running thread interfere wit=
h
> each other a locality counter is used which guarantees that at any time
> the locality is held as long as it is required by one of both execution
> paths.
>
> Patch 10 implements the request of a threaded interrupt handler. This is
> needed since SPI uses a mutex for data transmission and since we access =
the
> interrupt status register via SPI in the irq handler we need a sleepable
> context.
>
> Patch 11 makes sure that writes to the interrupt register are effective =
if
> done in the interrupt handler.
>
> Patch 12 makes sure that writes to the interrupt and INTERRUPT_VECTOR
> and INTERRUPT_ENABLE registers are effective by holding the locality.
>
> Patch 13 makes sure that the TPM is properly initialized after power cyc=
le
> before the irq test is done.
>
> Patch 14 enables the test for interrupts by setting the required flag
> before the test is executed.
>
> Changes in v11:
> - adjust patches 4,5 and 14 slightly to void the consecutive removal and
>   re-addition of the "ret" variable in tpm_tis_gen_interrupt()
>
> Changes in v10:
> - fix typo in subject line as pointed out by Jason Andryuk
> - improve patch "tpm, tpm_tis: Claim locality before writing interrupt
>   registers" by extending the scope with held locality". For this reason
>   the "Reviewed-by" tag by Jarko and the "Tested-by" tag by Michael have
>   been removed.
> - add fix to avoid TPM_RC_INITIALIZE after power cycle when testing irqs
>   (PATCH 13)
> - add fix to restore the old interrupt vector at error (PATCH 4)
>
>
> Changes in v9:
> - add a fix for an issue when interrupts are reenabled on resume (PATCH =
11)
> - improve the commit message for patch 8 as requested by Jarkko
> - improved functions naming
> - changed patch 12 (tpm, tpm_tis: Enable interrupt test) to not delete t=
he
>   TPM_CHIP_FLAG_IRQ flag any more when tpm2_get_tpm_pt() fails. Due to t=
his
>   change the 'Tested-by' tag from Michael and the 'Reviewed-by:' tag fro=
m
>   Jarko has been removed
>
> Changes in v8:
> - tpm_tis_data->locality is not changed at runtime any more so that it c=
an
> be read without any protection against concurrent modification.
> - add missing brackets as pointed out by Jason Andryuk
>
> Changes in v7:
> - moved interrupt flag checks into an own function as suggested by Jarkk=
o
> - added "Tested-by" tags for Tests from Michael Niew=C3=B6hner
> - fixed one comment
>
> Changes in v6:
> - set TPM_TIS_IRQ_TESTED in flag member of the tpm_tis_data struct inste=
ad
> in an own bitfield
> - improve commit messages
> - use int_mask instead of irqs_in_use as variable name
> - use sts_mask instead of active_irqs as variable name
> - squash patch 5 and 6
> - prefix functions with tpm_tis_
> - remove "fixes" tag
>
> Changes in v5:
> - improve commit message of patch 1 as requested by Jarko
> - drop patch that makes locality handling simpler by only claiming it at
>   driver startup and releasing it at driver shutdown (requested by Jarko=
)
> - drop patch that moves the interrupt test from tpm_tis_send()
>   to tmp_tis_probe_irq_single() as requested by Jarko
> - add patch to make locality handling threadsafe so that it can also be
>   done by the irq handler
> - separate logical changes into own patches
> - always request threaded interrupt handler
>
> Changes in v4:
> - only request threaded irq in case of SPI as requested by Jarko.
> - reimplement patch 2 to limit locality handling changes to the TIS core=
.
> - separate fixes from cleanups as requested by Jarko.
> - rephrase commit messages
>
> Changes in v3:
> - fixed compiler error reported by kernel test robot
> - rephrased commit message as suggested by Jarko Sakkinen
> - added Reviewed-by tag
>
> Changes in v2:
> - rebase against 5.12
> - free irq on error path
>
>
> Lino Sanfilippo (14):
>   tpm, tpm_tis: Avoid cache incoherency in test for interrupts
>   tpm, tpm_tis: Claim locality before writing TPM_INT_ENABLE register
>   tpm, tpm_tis: Disable interrupts if tpm_tis_probe_irq() failed
>   tpm, tpm_tis: Do not skip reset of original interrupt vector
>   tpm, tpm_tis: Claim locality before writing interrupt registers
>   tpm, tpm_tis: Only handle supported interrupts
>   tpm, tpm_tis: Move interrupt mask checks into own function
>   tpm, tpm_tis: do not check for the active locality in interrupt
>     handler
>   tpm, tpm: Implement usage counter for locality
>   tpm, tpm_tis: Request threaded interrupt handler
>   tpm, tpm_tis: Claim locality in interrupt handler
>   tpm, tpm_tis: Claim locality when interrupts are reenabled on resume
>   tpm, tpm_tis: startup chip before testing for interrupts
>   tpm, tpm_tis: Enable interrupt test
>
>  drivers/char/tpm/tpm-chip.c     |  38 ++--
>  drivers/char/tpm/tpm.h          |   1 +
>  drivers/char/tpm/tpm_tis.c      |   2 +-
>  drivers/char/tpm/tpm_tis_core.c | 299 ++++++++++++++++++++------------
>  drivers/char/tpm/tpm_tis_core.h |   5 +-
>  5 files changed, 214 insertions(+), 131 deletions(-)
>

Hi Jarkko,

its been a while now since the review of this series has been completed. W=
ill it be merged in the
near future? Or is there anything left to do (from my side)?

Regards,
Lino