Re: [PATCH v3 3/4] tpm: Fix test for interrupts

From: Lino Sanfilippo <LinoSanfilippo@gmx.de>
To: Jarkko Sakkinen <jarkko@kernel.org>
Cc: peterhuewe@gmx.de, jgg@ziepe.ca, stefanb@linux.vnet.ibm.com,
	James.Bottomley@hansenpartnership.com, keescook@chromium.org,
	jsnitsel@redhat.com, ml.linux@elloe.vision,
	linux-integrity@vger.kernel.org, linux-kernel@vger.kernel.org,
	LinoSanfilippo@gmx.de
Subject: Re: [PATCH v3 3/4] tpm: Fix test for interrupts
Date: Wed, 5 May 2021 01:18:05 +0200	[thread overview]
Message-ID: <eb0165fe-5f33-4779-e296-72ad8b386791@gmx.de> (raw)
In-Reply-To: <YJAcPAbFopeW7PYs@kernel.org>

On 03.05.21 at 17:52, Jarkko Sakkinen wrote:
> On Sat, May 01, 2021 at 03:57:26PM +0200, Lino Sanfilippo wrote:
>> The current test for functional interrupts is broken in multiple ways:
>> 1. The needed flag TPM_CHIP_FLAG_IRQ is never set, so the test is never
>> executed.
>> 2. The test includes the check for two variables and the check is done for
>> each transmission which is unnecessarily complicated.
>
> Unnecessary complicated is again terminolgy that I don't decipher,
> unfortunately. I get "something that works" or "has a regression".
> We don't polish things for nothing.

In this case "unnecessary complicated" means that we can achieve the same
effect (test for interrupts) with fewer code and fewer runtime impact.
Note that in the current code we do the same check for irq for each transmission:

if (!(chip->flags & TPM_CHIP_FLAG_IRQ) || priv->irq_tested)
		return tpm_tis_send_main(chip, buf, len);

With this patch the check for irqs has already be done at this time, so we dont have to
do this over and over again for each transmission.

>> 3. Part of the test is setting a bool variable in an interrupt handler and
>> then check the value in the main thread. However there is nothing that
>> guarantees the visibility of the value set in the interrupt handler for
>> any other thread. Some kind of synchronization primitive is required for this purpose.
>>
>> Fix all these issues by a reimplementation:
>> Instead of doing the test in tpm_tis_send() which is called for each
>> transmission do it only once in tpm_tis_probe_irq_single(). Furthermore
>> use proper accessor functions like get_bit()/set_bit() which include the
>> required synchronization primitives to guarantee visibility between the
>> interrupt handler and threads.
>> Finally remove one function which is no longer needed.
>>
>> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
>
> Not sure why to take this patch.

All I can say is that this patch is supposed to

- fix one bug:
"...Part of the test is setting a bool variable in an interrupt handler and
 then check the value in the main thread. However there is nothing that
 guarantees the visibility of the value set in the interrupt handler for
 any other thread. Some kind of synchronization primitive is required for this purpose..."

- simplify the irq test

- provide interrupts instead of polling

If this is worth taking is up to you, of course.

Regards,
Lino