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