Re: [PATCH v3 0/4] Fixes for TPM interrupt handling

[Jarkko Sakkinen <jarkko@kernel.org>]

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.

BR, Jarkko