Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v3 3/4] tpm: Fix test for interrupts
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
References: <20210501135727.17747-1-LinoSanfilippo@gmx.de>
 <20210501135727.17747-4-LinoSanfilippo@gmx.de> <YJAcPAbFopeW7PYs@kernel.org>
From:   Lino Sanfilippo <LinoSanfilippo@gmx.de>
Message-ID: <eb0165fe-5f33-4779-e296-72ad8b386791@gmx.de>
Date:   Wed, 5 May 2021 01:18:05 +0200
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: <YJAcPAbFopeW7PYs@kernel.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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 transm=
ission:

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 fo=
r 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 th=
e
>> 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 an=
d
 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