Hi guys, On Thu, 2021-05-06 at 04:47 +0300, Jarkko Sakkinen wrote: > On Wed, May 05, 2021 at 01:15:29AM +0200, Lino Sanfilippo wrote: > > Hi, > > > > On 03.05.21 at 17:50, Jarkko Sakkinen wrote: > > > What the heck is "simplification" and what that has to do with fixing > > > anything? I don't understand your terminology. > > > > > > The intention for this patch is not to fix anything. Please read the cover > > letter and the commit message. > > This patch is about making the locality handling easier by not > > claiming/releasing > > it multiple times over the driver life time, but claiming it once at driver > > startup and only releasing it at driver shutdown. > > > > Right now we have locality request/release combos in > > > > - probe_itpm() > > - tpm_tis_gen_interrupt() > > - tpm_tis_core_init() > > - tpm_chip_start() > > > > and there is still one combo missing for > > > > - tpm2_get_timeouts() > > > > which is the reason why we get the "TPM returned invalid status" bug in case > > of TPM2 (and this is the bug which is _incidentally_ fixed by this patch, > > see > > below). > > > > And if we are going to enable interrupts, we have to introduce yet another > > combo, > > for accessing the status register in the interrupt handler, since TPM 2.0 > > requires holding the locality for writing to the status register. That makes > > 6 different code places in which we take and release the locality. > > > > With this patch applied we only take the locality at one place. Furthermore > > with interrupts enabled we dont have to claim the locality for each handler > > execution, saving us countless claim/release combinations at runtime. > > > > Hence the term "simplification" which is perfectly justified IMO. > > > > So again, this patch is "only" in preparation for the next patch when > > interrupts > > are actually enabled and we would have to take the locality in the interrupt > > handler without this patch. > > So: what problem this patch does solve? > > /Jarkko > first, thank you very much, Lino, for working on this! I've been debugging issues with the tis driver in the last days and was about to start with the same approach as yours when I luckily discovered your patch! Jarkko, while I agree, that the commit message is not optimal, Lino tried hard to explain what the problems with the current code are and how they are / can be fixed. Further, I too don't see why simplification / optimization is such a bad thing. This driver is actually a very good example. I had a hard time, too, figuring out what's going on there. A clean rewrite is a very valid approach here IMO. It's not "polishing for nothing", as you described it, but actually solving problems. Interrupt detection is broken for years now and finally a volunteer worked on a solution. Don't you think this should be valued? Let's get this problem sorted out :-) Lino, I'd be happy to test the patches, when you have time and interest to work on this again! Thanks, Michael
