Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <c8c0c5fb614d8b2de2a5faee2ef5ff3214281064.camel@kernel.org>
Subject: Re: [PATCH v3 0/4] Fixes for TPM interrupt handling
From:   Jarkko Sakkinen <jarkko@kernel.org>
To:     Lino Sanfilippo <LinoSanfilippo@gmx.de>,
        Michael =?ISO-8859-1?Q?Niew=F6hner?= <linux@mniewoehner.de>
Cc:     peterhuewe@gmx.de, jgg@ziepe.ca, stefanb@linux.vnet.ibm.com,
        stefanb@linux.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,
        twawrzynczak@chromium.org
Date:   Wed, 20 Apr 2022 08:30:09 +0300
In-Reply-To: <efdb99b3-6d33-38b1-64a0-671821101631@gmx.de>
References: <20210501135727.17747-1-LinoSanfilippo@gmx.de>
         <20210501135727.17747-3-LinoSanfilippo@gmx.de>
         <YJAby8mmiJ74qWAh@kernel.org> <6722bf6f-1a3f-ee9c-55e2-cf63c64266a9@gmx.de>
         <YJNKs8bUMGOzFre+@kernel.org>
         <2a1a1cf61732eff1608aeae74054a0c135c1671f.camel@mniewoehner.de>
         <Yj0lhqTP1RoedxSc@iki.fi>
         <0d6c22b40a2f17d4b260f287d4c479a96a88b0b1.camel@mniewoehner.de>
         <efdb99b3-6d33-38b1-64a0-671821101631@gmx.de>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.0 
MIME-Version: 1.0
Precedence: bulk

n Sat, 2022-03-26 at 04:24 +0100, Lino Sanfilippo wrote:
>=20
> Hi Michael,
>=20
> On 25.03.22 at 13:32, Michael Niew=C3=B6hner wrote:
> > > >=20
> > > > Lino, I'd be happy to test the patches, when you have time and inte=
rest to
> > > > work
> > > > on this again!
> > > >=20
> > > > Thanks, Michael
> > >=20
> > > It's quite easy to test them out. Both fixes are in the mainline GIT =
tree.
> > > E.g. give a shot rc1, and please report if any issues persists to:
> > >=20
> > > =C2=A0 linux-integrity@vger.kernel.org=C2=A0
> > >=20
> > > BR, Jarkko
> >=20
> > I don't see Linos patches on mainline. Also, the series included four p=
atches:
> > [PATCH v3 0/4] Fixes for TPM interrupt handling
> > [PATCH v3 1/4] tpm: Use a threaded interrupt handler
> > [PATCH v3 2/4] tpm: Simplify locality handling
> > [PATCH v3 3/4] tpm: Fix test for interrupts
> > [PATCH v3 4/4] tpm: Only enable supported irqs
> >=20
> > Three of them are relevant for the interrupt problem, which is still pr=
esent in
> > mainline, as these patches were refused:
> > [PATCH v3 1/4] tpm: Use a threaded interrupt handler
> > [PATCH v3 2/4] tpm: Simplify locality handling
> > [PATCH v3 3/4] tpm: Fix test for interrupts
> >=20
> > Michael
> >=20
>=20
> You are right, the interrupts are still not working in the mainline kerne=
l.
> I would gladly make another attempt to fix this but rather step by step
> than in a series that tries to fix (different) things at once.
>=20
> A first step could be to have a sleepable context for the interrupt handl=
ing,
> since in case of SPI the accesses to the irq status register may sleep.
>=20
> I sent a patch for this purpose once, but it seems to have gone lost:
>=20
> https://lore.kernel.org/all/20210620023444.14684-1-LinoSanfilippo@gmx.de/
>=20
>=20
> Best regards,
> Lino

I went these through one by one.

# Above linked patch=20

Boolean parameters are considered bad. I.e. use named flags
instead. This is for above linked patch.

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

1. Please remove "unnecessarily complicated" sentence because
   it cannot be evaluated. It's your opinion, which might perhaps
   be correct, but it is irrelevant for any possible patch
   description.
2. There's no such thing as "fix by re-implementation". Please
   explain instead code change is relevant for the bug fix.
3. If set_bit() et al necessarily to fix a possible race condition
   you need to have a separate patch for that.

To move forward, start with a better summary such as

"tpm: move interrupt test to tpm_tis_probe_irq_single()"

I'd also either revert the change for flags, or alternatively
move it to separate patch explaining race condition. Otherwise,
there's no argument of saying that using set_bit() is more=20
proper. This will make the change more localized.


# [PATCH v3 2/4] tpm: Simplify locality handling

"As a side-effect these modifications fix a bug which results in the
following warning when using TPM 2:"

Generally speaking, the simplifications should be done on top of code
that does not have known bugs, even if the simplification renders out
the bug. This is because then we have code that have potentially unknown
unknown bugs.

I hope you see my point. The problem with these patches were then
and is still that they intermix bug fixes and other modifications and
thus cannot be taken in.

BR, Jarkko