Re: [PATCH v4 5/6] tpm, tpm_tis: Move irq test from tpm_tis_send() to tpm_tis_probe_irq_single()
From: Michael Niewöhner
Date: Tue May 17 2022 - 14:20:11 EST
Hi guys,
On Mon, 2022-05-16 at 22:25 +0200, Lino Sanfilippo wrote:
> On 16.05.22 at 19:51, Jarkko Sakkinen wrote:
> > On Wed, May 11, 2022 at 09:56:59PM +0200, Lino Sanfilippo wrote:
> > > On 11.05.22 at 17:09, Jarkko Sakkinen wrote:
> > > > On Mon, May 09, 2022 at 10:05:58AM +0200, Lino Sanfilippo wrote:
> > > > > From: Lino Sanfilippo <l.sanfilippo@xxxxxxxxxx>
> > > > >
> > > > > There is no need to check for the irq test completion at each data
> > > > > transmission during the driver livetime. Instead do the check only
> > > > > once at
> > > > > driver startup.
> > > > >
> > > > > Signed-off-by: Lino Sanfilippo <l.sanfilippo@xxxxxxxxxx>
> > > > > ---
> > > > > drivers/char/tpm/tpm_tis_core.c | 68 +++++++++++---------------------
> > > > > -
> > > > > 1 file changed, 22 insertions(+), 46 deletions(-)
> > > > >
> > > > > diff --git a/drivers/char/tpm/tpm_tis_core.c
> > > > > b/drivers/char/tpm/tpm_tis_core.c
> > > > > index bdfde1cd71fe..4c65718feb7d 100644
> > > > > --- a/drivers/char/tpm/tpm_tis_core.c
> > > > > +++ b/drivers/char/tpm/tpm_tis_core.c
> > > > > @@ -432,7 +432,7 @@ static void disable_interrupts(struct tpm_chip
> > > > > *chip)
> > > > > * tpm.c can skip polling for the data to be available as the
> > > > > interrupt is
> > > > > * waited for here
> > > > > */
> > > > > -static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf,
> > > > > size_t len)
> > > > > +static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
> > > > > {
> > > > > struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> > > > > int rc;
> > > > > @@ -465,30 +465,6 @@ static int tpm_tis_send_main(struct tpm_chip
> > > > > *chip, const u8 *buf, size_t len)
> > > > > return rc;
> > > > > }
> > > > >
> > > > > -static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
> > > > > -{
> > > > > - int rc, irq;
> > > > > - struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> > > > > -
> > > > > - if (!(chip->flags & TPM_CHIP_FLAG_IRQ) ||
> > > > > - test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
> > > > > - return tpm_tis_send_main(chip, buf, len);
> > > > > -
> > > > > - /* Verify receipt of the expected IRQ */
> > > > > - irq = priv->irq;
> > > > > - priv->irq = 0;
> > > > > - chip->flags &= ~TPM_CHIP_FLAG_IRQ;
> > > > > - rc = tpm_tis_send_main(chip, buf, len);
> > > > > - priv->irq = irq;
> > > > > - chip->flags |= TPM_CHIP_FLAG_IRQ;
> > > > > - if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
> > > > > - tpm_msleep(1);
> > > > > - if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
> > > > > - disable_interrupts(chip);
> > > > > - set_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags);
> > > > > - return rc;
> > > > > -}
> > > > > -
> > > > > struct tis_vendor_durations_override {
> > > > > u32 did_vid;
> > > > > struct tpm1_version version;
> > > > > @@ -759,51 +735,54 @@ static int tpm_tis_probe_irq_single(struct
> > > > > tpm_chip *chip, u32 intmask,
> > > > >
> > > > > rc = tpm_tis_read8(priv, TPM_INT_VECTOR(priv->locality),
> > > > > &original_int_vec);
> > > > > - if (rc < 0)
> > > > > + if (rc < 0) {
> > > > > + disable_interrupts(chip);
> > > > > return rc;
> > > > > + }
> > > > >
> > > > > rc = tpm_tis_write8(priv, TPM_INT_VECTOR(priv->locality),
> > > > > irq);
> > > > > if (rc < 0)
> > > > > - return rc;
> > > > > + goto out_err;
> > > > >
> > > > > rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality),
> > > > > &int_status);
> > > > > if (rc < 0)
> > > > > - return rc;
> > > > > + goto out_err;
> > > > >
> > > > > /* Clear all existing */
> > > > > rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality),
> > > > > int_status);
> > > > > if (rc < 0)
> > > > > - return rc;
> > > > > + goto out_err;
> > > > >
> > > > > /* Turn on */
> > > > > rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality),
> > > > > intmask | TPM_GLOBAL_INT_ENABLE);
> > > > > if (rc < 0)
> > > > > - return rc;
> > > > > + goto out_err;
> > > > >
> > > > > clear_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags);
> > > > > - chip->flags |= TPM_CHIP_FLAG_IRQ;
> > > > >
> > > > > /* Generate an interrupt by having the core call through to
> > > > > * tpm_tis_send
> > > > > */
> > > > > rc = tpm_tis_gen_interrupt(chip);
> > > > > if (rc < 0)
> > > > > - return rc;
> > > > > + goto out_err;
> > > > >
> > > > > - /* tpm_tis_send will either confirm the interrupt is working
> > > > > or it
> > > > > - * will call disable_irq which undoes all of the above.
> > > > > - */
> > > > > - if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
> > > > > - rc = tpm_tis_write8(priv, original_int_vec,
> > > > > - TPM_INT_VECTOR(priv->locality));
> > > > > - if (rc < 0)
> > > > > - return rc;
> > > > > + tpm_msleep(1);
> > > > >
> > > > > - return 1;
> > > > > - }
> > > > > + /* Verify receipt of the expected IRQ */
> > > > > + if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
> > > > > + goto out_err;
> > > > > +
> > > > > + chip->flags |= TPM_CHIP_FLAG_IRQ;
> > > > >
> > > > > return 0;
> > > > > +
> > > > > +out_err:
> >
> > Rename this as just 'err'.
> >
> > > > > + disable_interrupts(chip);
> > > > > + tpm_tis_write8(priv, original_int_vec, TPM_INT_VECTOR(priv-
> > > > > >locality));
> > > > > +
> > > > > + return rc;
> > > > > }
> > > > >
> > > > > /* Try to find the IRQ the TPM is using. This is for legacy x86
> > > > > systems that
> > > > > @@ -1075,12 +1054,9 @@ int tpm_tis_core_init(struct device *dev,
> > > > > struct tpm_tis_data *priv, int irq,
> > > > > if (irq) {
> > > > > tpm_tis_probe_irq_single(chip, intmask,
> > > > > IRQF_SHARED,
> > > > > irq);
> > > > > - if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
> > > > > + if (!(chip->flags & TPM_CHIP_FLAG_IRQ))
> > > > > dev_err(&chip->dev, FW_BUG
> > > > > "TPM interrupt not working,
> > > > > polling instead\n");
> > > > > -
> > > > > - disable_interrupts(chip);
> > > > > - }
> > > > > } else {
> > > > > tpm_tis_probe_irq(chip, intmask);
> > > > > }
> > > > > --
> > > > > 2.36.0
> > > > >
> > > >
> > > > For me this looks just code shuffling.
> > > >
> > > > I don't disagree but changing working code without actual semantical
> > > > reasons neither makes sense.
> > > >
> > > > BR, Jarkko
> > > >
> > >
> > > Well the semantical reason for this change is that the check for irq test
> > > completion
> > > only has to be done once for the driver livetime. There is no point in
> > > doing it
> > > over and over again for each transmission.
> > > So the code is not simply shuffled around, it is shifted to a place where
> > > it is only
> > > executed once.
> > >
> > > This is not a bugfix but it is clearly an improvement/cleanup. As far as I
> > > understood
> > > from your comments on the earlier versions of this patch set cleanups are
> > > also ok as
> > > long as they are not intermixed with bugfixes.
> >
> > The patch does not do anything particulary useful IMHO. There's no
> > stimulus to do this change.
> >
I don't agree. IMHO preventing useless actions (like checking the interrupt
again and again) *is* useful and I think it's reason enough.
>
> Ok, I will drop this patch in the next version of this series then.
>
> Regards,
> Lino
>
Michael