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

[Lino Sanfilippo <LinoSanfilippo@gmx.de>]

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@kunbus.com>
>>>>
>>>> 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@kunbus.com>
>>>> ---
>>>>  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.
>

Ok, I will drop this patch in the next version of this series then.

Regards,
Lino