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,
Lukas Wunner <lukas@wunner.de>,
Philipp Rosenberger <p.rosenberger@kunbus.com>
Subject: Re: [PATCH v3 0/4] Fixes for TPM interrupt handling
Date: Mon, 25 Apr 2022 16:57:51 +0300 [thread overview]
Message-ID: <8ed6cf56ddd8e75a26cc8ea56be36dc54356f02d.camel@kernel.org> (raw)
In-Reply-To: <77374443-dc9b-5d1d-c652-0d093b6029f4@gmx.de>
On Sun, 2022-04-24 at 04:22 +0200, Lino Sanfilippo wrote:
>
> Hi,
>
> On 20.04.22 at 07:30, 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.
>
> Ok, we could extend tpm_tis_flags by a flag "TPM_TIS_USE_THREADED_IRQ"
> for this.
>
> >
> > # [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.
> >
>
> Ok, I will split the fix for the irq test into two patches then.
>
> >
> > # [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.
> >
>
> Yes, I can see that point.
>
> > BR, Jarkko
> >
>
> Thanks a lot for the review. I will prepare new patches with the suggested
> changes.
Yeah, I mean the point being: it's OK to suggest clean ups but with bug fixes
you should aim for the lowest common denominator as far as you possibly can.
BR, Jarkko
next prev parent reply other threads:[~2022-04-25 13:56 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
2022-04-24 2:22 ` Lino Sanfilippo
2022-04-25 13:57 ` Jarkko Sakkinen [this message]
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=8ed6cf56ddd8e75a26cc8ea56be36dc54356f02d.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=lukas@wunner.de \
--cc=ml.linux@elloe.vision \
--cc=p.rosenberger@kunbus.com \
--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).