| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 16 May 2022 22:25:57 +0200
From: Lino Sanfilippo <LinoSanfilippo@....de>
To: Jarkko Sakkinen <jarkko@...nel.org>
Cc: peterhuewe@....de, jgg@...pe.ca, stefanb@...ux.vnet.ibm.com,
linux@...ewoehner.de, linux-integrity@...r.kernel.org,
linux-kernel@...r.kernel.org, lukas@...ner.de,
p.rosenberger@...bus.com, Lino Sanfilippo <l.sanfilippo@...bus.com>
Subject: Re: [PATCH v4 5/6] tpm, tpm_tis: Move irq test from tpm_tis_send() to
tpm_tis_probe_irq_single()
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@...bus.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@...bus.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
Powered by blists - more mailing lists