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:
>> + 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.
Regards,
Lino
