From: Jarkko Sakkinen <jarkko@kernel.org>
To: "Lino Sanfilippo" <LinoSanfilippo@gmx.de>,
"Michael Niewöhner" <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
Subject: Re: [PATCH v3 0/4] Fixes for TPM interrupt handling
Date: Wed, 20 Apr 2022 08:32:19 +0300 [thread overview]
Message-ID: <59d5026897bff15e371f2770335270cf1b540766.camel@kernel.org> (raw)
In-Reply-To: <c8c0c5fb614d8b2de2a5faee2ef5ff3214281064.camel@kernel.org>
On Wed, 2022-04-20 at 08:30 +0300, Jarkko Sakkinen wrote:
> n Sat, 2022-03-26 at 04:24 +0100, Lino Sanfilippo wrote:
> >
> > Hi Michael,
> >
> > On 25.03.22 at 13:32, Michael Niewöhner wrote:
> > > > >
> > > > > Lino, I'd be happy to test the patches, when you have time and interest to
> > > > > work
> > > > > on this again!
> > > > >
> > > > > Thanks, Michael
> > > >
> > > > 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:
> > > >
> > > > linux-integrity@vger.kernel.org
> > > >
> > > > BR, Jarkko
> > >
> > > I don't see Linos patches on mainline. Also, the series included four patches:
> > > [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
> > >
> > > Three of them are relevant for the interrupt problem, which is still present 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
> > >
> > > Michael
> > >
> >
> > You are right, the interrupts are still not working in the mainline kernel.
> > 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.
> >
> > A first step could be to have a sleepable context for the interrupt handling,
> > since in case of SPI the accesses to the irq status register may sleep.
> >
> > I sent a patch for this purpose once, but it seems to have gone lost:
> >
> > https://lore.kernel.org/all/20210620023444.14684-1-LinoSanfilippo@gmx.de/
> >
> >
> > Best regards,
> > Lino
>
> I went these through one by one.
>
> # Above linked patch
>
> 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
> 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.
I.e. to move forward create first localized fixes, and only after those
clean ups if there is point. Removing code (like in 2/4) is not a bug
fix fo anything. This not to say that some changes would be illegit, I'm
only saying that the patches are badly scoped.
BR, Jarkko
next prev parent reply other threads:[~2022-04-20 5:33 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-01 13:57 [PATCH v3 0/4] Fixes for TPM interrupt handling Lino Sanfilippo
2021-05-01 13:57 ` [PATCH v3 1/4] tpm: Use a threaded interrupt handler Lino Sanfilippo
2021-05-03 15:14 ` Jarkko Sakkinen
2021-05-04 22:54 ` Lino Sanfilippo
2021-05-06 1:46 ` Jarkko Sakkinen
2021-05-01 13:57 ` [PATCH v3 2/4] tpm: Simplify locality handling Lino Sanfilippo
2021-05-03 15:50 ` Jarkko Sakkinen
2021-05-04 23:15 ` Lino Sanfilippo
2021-05-06 1:47 ` Jarkko Sakkinen
2022-03-24 17:04 ` [PATCH v3 0/4] Fixes for TPM interrupt handling Michael Niewöhner
2022-03-25 2:14 ` Jarkko Sakkinen
2022-03-25 12:32 ` Michael Niewöhner
2022-03-26 3:24 ` Lino Sanfilippo
2022-03-26 8:59 ` Michael Niewöhner
2022-03-30 15:19 ` Jarkko Sakkinen
2022-04-20 5:30 ` Jarkko Sakkinen
2022-04-20 5:32 ` Jarkko Sakkinen [this message]
2022-04-24 2:22 ` Lino Sanfilippo
2022-04-25 13:57 ` Jarkko Sakkinen
2022-03-30 15:18 ` Jarkko Sakkinen
2021-05-01 13:57 ` [PATCH v3 3/4] tpm: Fix test for interrupts Lino Sanfilippo
2021-05-03 15:52 ` Jarkko Sakkinen
2021-05-04 23:18 ` Lino Sanfilippo
2021-05-01 13:57 ` [PATCH v3 4/4] tpm: Only enable supported irqs Lino Sanfilippo
2021-05-01 19:09 ` Stefan Berger
2021-05-02 3:15 ` Lino Sanfilippo
2021-05-03 15:52 ` Jarkko Sakkinen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=59d5026897bff15e371f2770335270cf1b540766.camel@kernel.org \
--to=jarkko@kernel.org \
--cc=James.Bottomley@hansenpartnership.com \
--cc=LinoSanfilippo@gmx.de \
--cc=jgg@ziepe.ca \
--cc=jsnitsel@redhat.com \
--cc=keescook@chromium.org \
--cc=linux-integrity@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@mniewoehner.de \
--cc=ml.linux@elloe.vision \
--cc=peterhuewe@gmx.de \
--cc=stefanb@linux.ibm.com \
--cc=stefanb@linux.vnet.ibm.com \
--cc=twawrzynczak@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).