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:
> + 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
