Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH v4 5/6] tpm, tpm_tis: Move irq test from tpm_tis_send() to
 tpm_tis_probe_irq_single()
To:     Jarkko Sakkinen <jarkko@kernel.org>
Cc:     peterhuewe@gmx.de, jgg@ziepe.ca, stefanb@linux.vnet.ibm.com,
        linux@mniewoehner.de, linux-integrity@vger.kernel.org,
        linux-kernel@vger.kernel.org, lukas@wunner.de,
        p.rosenberger@kunbus.com, Lino Sanfilippo <l.sanfilippo@kunbus.com>
References: <20220509080559.4381-1-LinoSanfilippo@gmx.de>
 <20220509080559.4381-6-LinoSanfilippo@gmx.de> <YnvRrT19Pe2SPDNe@kernel.org>
From:   Lino Sanfilippo <LinoSanfilippo@gmx.de>
Message-ID: <34f47a0c-5c2d-1cdc-fb97-03666a5e1918@gmx.de>
Date:   Wed, 11 May 2022 21:56:59 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <YnvRrT19Pe2SPDNe@kernel.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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 *chi=
p)
>>   * tpm.c can skip polling for the data to be available as the interrup=
t is
>>   * waited for here
>>   */
>> -static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, siz=
e_t len)
>> +static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len)
>>  {
>>  	struct tpm_tis_data *priv =3D 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 =3D 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 =3D priv->irq;
>> -	priv->irq =3D 0;
>> -	chip->flags &=3D ~TPM_CHIP_FLAG_IRQ;
>> -	rc =3D tpm_tis_send_main(chip, buf, len);
>> -	priv->irq =3D irq;
>> -	chip->flags |=3D 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_ch=
ip *chip, u32 intmask,
>>
>>  	rc =3D tpm_tis_read8(priv, TPM_INT_VECTOR(priv->locality),
>>  			   &original_int_vec);
>> -	if (rc < 0)
>> +	if (rc < 0) {
>> +		disable_interrupts(chip);
>>  		return rc;
>> +	}
>>
>>  	rc =3D tpm_tis_write8(priv, TPM_INT_VECTOR(priv->locality), irq);
>>  	if (rc < 0)
>> -		return rc;
>> +		goto out_err;
>>
>>  	rc =3D tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &int_stat=
us);
>>  	if (rc < 0)
>> -		return rc;
>> +		goto out_err;
>>
>>  	/* Clear all existing */
>>  	rc =3D tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), int_stat=
us);
>>  	if (rc < 0)
>> -		return rc;
>> +		goto out_err;
>>
>>  	/* Turn on */
>>  	rc =3D 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 |=3D TPM_CHIP_FLAG_IRQ;
>>
>>  	/* Generate an interrupt by having the core call through to
>>  	 * tpm_tis_send
>>  	 */
>>  	rc =3D 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 =3D 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 |=3D 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 system=
s 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 doi=
ng 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