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

[Jarkko Sakkinen]

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.

> Regards,
> Lino

BR, Jarkko