by Michael Niewöhner

[permalink] [raw]

Subject: Re: [PATCH v4 5/6] tpm, tpm_tis: Move irq test from tpm_tis_send() to tpm_tis_probe_irq_single()

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 <[email protected]>
> > > > >
> > > > > 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 <[email protected]>
> > > > > ---
> > > > > 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

2022-05-18 16:13:01

by Michael Niewöhner

[permalink] [raw]

Subject: Re: [PATCH v4 5/6] tpm, tpm_tis: Move irq test from tpm_tis_send() to tpm_tis_probe_irq_single()

On Wed, 2022-05-18 at 04:26 +0300, Jarkko Sakkinen wrote:
> On Tue, 2022-05-17 at 20:19 +0200, Michael Niewöhner wrote:
> > 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 <[email protected]>
> > > > > > >
> > > > > > > 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 <[email protected]>
> > > > > > > ---
> > > > > > > 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.
>
> Show me the test data to back this up.

Why do you need test data as an argument for not doing useless actions? o.O

>
> BR, Jarkko