2022-06-29 23:29:12

by Lino Sanfilippo

[permalink] [raw]
Subject: [PATCH v7 05/10] tpm, tpm_tis: Only handle supported interrupts

From: Lino Sanfilippo <[email protected]>

According to the TPM Interface Specification (TIS) support for "stsValid"
and "commandReady" interrupts is only optional.
This has to be taken into account when handling the interrupts in functions
like wait_for_tpm_stat(). To determine the supported interrupts use the
capability query.

Also adjust wait_for_tpm_stat() to only wait for interrupt reported status
changes. After that process all the remaining status changes by polling
the status register.

Signed-off-by: Lino Sanfilippo <[email protected]>
Tested-by: Michael Niewöhner <[email protected]>
---
drivers/char/tpm/tpm_tis_core.c | 119 +++++++++++++++++++-------------
drivers/char/tpm/tpm_tis_core.h | 1 +
2 files changed, 72 insertions(+), 48 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 09d8f04cbc81..c13599e94ab6 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -53,41 +53,63 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
long rc;
u8 status;
bool canceled = false;
+ u8 sts_mask = 0;
+ int ret = 0;

/* check current status */
status = chip->ops->status(chip);
if ((status & mask) == mask)
return 0;

- stop = jiffies + timeout;
+ /* check which status changes can be handled by irqs */
+ if (priv->int_mask & TPM_INTF_STS_VALID_INT)
+ sts_mask |= TPM_STS_VALID;

- if (chip->flags & TPM_CHIP_FLAG_IRQ) {
+ if (priv->int_mask & TPM_INTF_DATA_AVAIL_INT)
+ sts_mask |= TPM_STS_DATA_AVAIL;
+
+ if (priv->int_mask & TPM_INTF_CMD_READY_INT)
+ sts_mask |= TPM_STS_COMMAND_READY;
+
+ sts_mask &= mask;
+
+ stop = jiffies + timeout;
+ /* process status changes with irq support */
+ if (sts_mask) {
+ ret = -ETIME;
again:
timeout = stop - jiffies;
if ((long)timeout <= 0)
return -ETIME;
rc = wait_event_interruptible_timeout(*queue,
- wait_for_tpm_stat_cond(chip, mask, check_cancel,
+ wait_for_tpm_stat_cond(chip, sts_mask, check_cancel,
&canceled),
timeout);
if (rc > 0) {
if (canceled)
return -ECANCELED;
- return 0;
+ ret = 0;
}
if (rc == -ERESTARTSYS && freezing(current)) {
clear_thread_flag(TIF_SIGPENDING);
goto again;
}
- } else {
- do {
- usleep_range(priv->timeout_min,
- priv->timeout_max);
- status = chip->ops->status(chip);
- if ((status & mask) == mask)
- return 0;
- } while (time_before(jiffies, stop));
}
+
+ if (ret)
+ return ret;
+
+ mask &= ~sts_mask;
+ if (!mask) /* all done */
+ return 0;
+ /* process status changes without irq support */
+ do {
+ status = chip->ops->status(chip);
+ if ((status & mask) == mask)
+ return 0;
+ usleep_range(priv->timeout_min,
+ priv->timeout_max);
+ } while (time_before(jiffies, stop));
return -ETIME;
}

@@ -1007,8 +1029,39 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
if (rc < 0)
goto out_err;

- intmask |= TPM_INTF_CMD_READY_INT | TPM_INTF_LOCALITY_CHANGE_INT |
- TPM_INTF_DATA_AVAIL_INT | TPM_INTF_STS_VALID_INT;
+ /* Figure out the capabilities */
+ rc = tpm_tis_read32(priv, TPM_INTF_CAPS(priv->locality), &intfcaps);
+ if (rc < 0)
+ goto out_err;
+
+ dev_dbg(dev, "TPM interface capabilities (0x%x):\n",
+ intfcaps);
+ if (intfcaps & TPM_INTF_BURST_COUNT_STATIC)
+ dev_dbg(dev, "\tBurst Count Static\n");
+ if (intfcaps & TPM_INTF_CMD_READY_INT) {
+ intmask |= TPM_INTF_CMD_READY_INT;
+ dev_dbg(dev, "\tCommand Ready Int Support\n");
+ }
+ if (intfcaps & TPM_INTF_INT_EDGE_FALLING)
+ dev_dbg(dev, "\tInterrupt Edge Falling\n");
+ if (intfcaps & TPM_INTF_INT_EDGE_RISING)
+ dev_dbg(dev, "\tInterrupt Edge Rising\n");
+ if (intfcaps & TPM_INTF_INT_LEVEL_LOW)
+ dev_dbg(dev, "\tInterrupt Level Low\n");
+ if (intfcaps & TPM_INTF_INT_LEVEL_HIGH)
+ dev_dbg(dev, "\tInterrupt Level High\n");
+ if (intfcaps & TPM_INTF_LOCALITY_CHANGE_INT)
+ intmask |= TPM_INTF_LOCALITY_CHANGE_INT;
+ dev_dbg(dev, "\tLocality Change Int Support\n");
+ if (intfcaps & TPM_INTF_STS_VALID_INT) {
+ intmask |= TPM_INTF_STS_VALID_INT;
+ dev_dbg(dev, "\tSts Valid Int Support\n");
+ }
+ if (intfcaps & TPM_INTF_DATA_AVAIL_INT) {
+ intmask |= TPM_INTF_DATA_AVAIL_INT;
+ dev_dbg(dev, "\tData Avail Int Support\n");
+ }
+
intmask &= ~TPM_GLOBAL_INT_ENABLE;

rc = request_locality(chip, 0);
@@ -1042,32 +1095,6 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
goto out_err;
}

- /* Figure out the capabilities */
- rc = tpm_tis_read32(priv, TPM_INTF_CAPS(priv->locality), &intfcaps);
- if (rc < 0)
- goto out_err;
-
- dev_dbg(dev, "TPM interface capabilities (0x%x):\n",
- intfcaps);
- if (intfcaps & TPM_INTF_BURST_COUNT_STATIC)
- dev_dbg(dev, "\tBurst Count Static\n");
- if (intfcaps & TPM_INTF_CMD_READY_INT)
- dev_dbg(dev, "\tCommand Ready Int Support\n");
- if (intfcaps & TPM_INTF_INT_EDGE_FALLING)
- dev_dbg(dev, "\tInterrupt Edge Falling\n");
- if (intfcaps & TPM_INTF_INT_EDGE_RISING)
- dev_dbg(dev, "\tInterrupt Edge Rising\n");
- if (intfcaps & TPM_INTF_INT_LEVEL_LOW)
- dev_dbg(dev, "\tInterrupt Level Low\n");
- if (intfcaps & TPM_INTF_INT_LEVEL_HIGH)
- dev_dbg(dev, "\tInterrupt Level High\n");
- if (intfcaps & TPM_INTF_LOCALITY_CHANGE_INT)
- dev_dbg(dev, "\tLocality Change Int Support\n");
- if (intfcaps & TPM_INTF_STS_VALID_INT)
- dev_dbg(dev, "\tSts Valid Int Support\n");
- if (intfcaps & TPM_INTF_DATA_AVAIL_INT)
- dev_dbg(dev, "\tData Avail Int Support\n");
-
/* INTERRUPT Setup */
init_waitqueue_head(&priv->read_queue);
init_waitqueue_head(&priv->int_queue);
@@ -1098,7 +1125,9 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
else
tpm_tis_probe_irq(chip, intmask);

- if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
+ if (chip->flags & TPM_CHIP_FLAG_IRQ) {
+ priv->int_mask = intmask;
+ } else {
dev_err(&chip->dev, FW_BUG
"TPM interrupt not working, polling instead\n");

@@ -1145,13 +1174,7 @@ static void tpm_tis_reenable_interrupts(struct tpm_chip *chip)
if (rc < 0)
goto out;

- rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
- if (rc < 0)
- goto out;
-
- intmask |= TPM_INTF_CMD_READY_INT
- | TPM_INTF_LOCALITY_CHANGE_INT | TPM_INTF_DATA_AVAIL_INT
- | TPM_INTF_STS_VALID_INT | TPM_GLOBAL_INT_ENABLE;
+ intmask = priv->int_mask | TPM_GLOBAL_INT_ENABLE;

tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);

diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index bf07379dea42..e005eb99480e 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -93,6 +93,7 @@ struct tpm_tis_data {
u16 manufacturer_id;
int locality;
int irq;
+ unsigned int int_mask;
unsigned long flags;
void __iomem *ilb_base_addr;
u16 clkrun_enabled;
--
2.25.1


2022-07-01 00:01:02

by Jarkko Sakkinen

[permalink] [raw]
Subject: Re: [PATCH v7 05/10] tpm, tpm_tis: Only handle supported interrupts

On Thu, Jun 30, 2022 at 01:26:48AM +0200, Lino Sanfilippo wrote:
> From: Lino Sanfilippo <[email protected]>
>
> According to the TPM Interface Specification (TIS) support for "stsValid"
> and "commandReady" interrupts is only optional.
> This has to be taken into account when handling the interrupts in functions
> like wait_for_tpm_stat(). To determine the supported interrupts use the
> capability query.
>
> Also adjust wait_for_tpm_stat() to only wait for interrupt reported status
> changes. After that process all the remaining status changes by polling
> the status register.
>
> Signed-off-by: Lino Sanfilippo <[email protected]>
> Tested-by: Michael Niew??hner <[email protected]>
> ---
> drivers/char/tpm/tpm_tis_core.c | 119 +++++++++++++++++++-------------
> drivers/char/tpm/tpm_tis_core.h | 1 +
> 2 files changed, 72 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 09d8f04cbc81..c13599e94ab6 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -53,41 +53,63 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
> long rc;
> u8 status;
> bool canceled = false;
> + u8 sts_mask = 0;
> + int ret = 0;
>
> /* check current status */
> status = chip->ops->status(chip);
> if ((status & mask) == mask)
> return 0;
>
> - stop = jiffies + timeout;
> + /* check which status changes can be handled by irqs */
> + if (priv->int_mask & TPM_INTF_STS_VALID_INT)
> + sts_mask |= TPM_STS_VALID;
>
> - if (chip->flags & TPM_CHIP_FLAG_IRQ) {
> + if (priv->int_mask & TPM_INTF_DATA_AVAIL_INT)
> + sts_mask |= TPM_STS_DATA_AVAIL;
> +
> + if (priv->int_mask & TPM_INTF_CMD_READY_INT)
> + sts_mask |= TPM_STS_COMMAND_READY;
> +
> + sts_mask &= mask;
> +
> + stop = jiffies + timeout;
> + /* process status changes with irq support */
> + if (sts_mask) {
> + ret = -ETIME;
> again:
> timeout = stop - jiffies;
> if ((long)timeout <= 0)
> return -ETIME;
> rc = wait_event_interruptible_timeout(*queue,
> - wait_for_tpm_stat_cond(chip, mask, check_cancel,
> + wait_for_tpm_stat_cond(chip, sts_mask, check_cancel,
> &canceled),
> timeout);
> if (rc > 0) {
> if (canceled)
> return -ECANCELED;
> - return 0;
> + ret = 0;
> }
> if (rc == -ERESTARTSYS && freezing(current)) {
> clear_thread_flag(TIF_SIGPENDING);
> goto again;
> }
> - } else {
> - do {
> - usleep_range(priv->timeout_min,
> - priv->timeout_max);
> - status = chip->ops->status(chip);
> - if ((status & mask) == mask)
> - return 0;
> - } while (time_before(jiffies, stop));
> }
> +
> + if (ret)
> + return ret;
> +
> + mask &= ~sts_mask;
> + if (!mask) /* all done */
> + return 0;
> + /* process status changes without irq support */
> + do {
> + status = chip->ops->status(chip);
> + if ((status & mask) == mask)
> + return 0;
> + usleep_range(priv->timeout_min,
> + priv->timeout_max);
> + } while (time_before(jiffies, stop));
> return -ETIME;
> }
>
> @@ -1007,8 +1029,39 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
> if (rc < 0)
> goto out_err;
>
> - intmask |= TPM_INTF_CMD_READY_INT | TPM_INTF_LOCALITY_CHANGE_INT |
> - TPM_INTF_DATA_AVAIL_INT | TPM_INTF_STS_VALID_INT;
> + /* Figure out the capabilities */
> + rc = tpm_tis_read32(priv, TPM_INTF_CAPS(priv->locality), &intfcaps);
> + if (rc < 0)
> + goto out_err;
> +
> + dev_dbg(dev, "TPM interface capabilities (0x%x):\n",
> + intfcaps);
> + if (intfcaps & TPM_INTF_BURST_COUNT_STATIC)
> + dev_dbg(dev, "\tBurst Count Static\n");
> + if (intfcaps & TPM_INTF_CMD_READY_INT) {
> + intmask |= TPM_INTF_CMD_READY_INT;
> + dev_dbg(dev, "\tCommand Ready Int Support\n");
> + }
> + if (intfcaps & TPM_INTF_INT_EDGE_FALLING)
> + dev_dbg(dev, "\tInterrupt Edge Falling\n");
> + if (intfcaps & TPM_INTF_INT_EDGE_RISING)
> + dev_dbg(dev, "\tInterrupt Edge Rising\n");
> + if (intfcaps & TPM_INTF_INT_LEVEL_LOW)
> + dev_dbg(dev, "\tInterrupt Level Low\n");
> + if (intfcaps & TPM_INTF_INT_LEVEL_HIGH)
> + dev_dbg(dev, "\tInterrupt Level High\n");
> + if (intfcaps & TPM_INTF_LOCALITY_CHANGE_INT)
> + intmask |= TPM_INTF_LOCALITY_CHANGE_INT;
> + dev_dbg(dev, "\tLocality Change Int Support\n");
> + if (intfcaps & TPM_INTF_STS_VALID_INT) {
> + intmask |= TPM_INTF_STS_VALID_INT;
> + dev_dbg(dev, "\tSts Valid Int Support\n");
> + }
> + if (intfcaps & TPM_INTF_DATA_AVAIL_INT) {
> + intmask |= TPM_INTF_DATA_AVAIL_INT;
> + dev_dbg(dev, "\tData Avail Int Support\n");
> + }
> +
> intmask &= ~TPM_GLOBAL_INT_ENABLE;
>
> rc = request_locality(chip, 0);
> @@ -1042,32 +1095,6 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
> goto out_err;
> }
>
> - /* Figure out the capabilities */
> - rc = tpm_tis_read32(priv, TPM_INTF_CAPS(priv->locality), &intfcaps);
> - if (rc < 0)
> - goto out_err;
> -
> - dev_dbg(dev, "TPM interface capabilities (0x%x):\n",
> - intfcaps);
> - if (intfcaps & TPM_INTF_BURST_COUNT_STATIC)
> - dev_dbg(dev, "\tBurst Count Static\n");
> - if (intfcaps & TPM_INTF_CMD_READY_INT)
> - dev_dbg(dev, "\tCommand Ready Int Support\n");
> - if (intfcaps & TPM_INTF_INT_EDGE_FALLING)
> - dev_dbg(dev, "\tInterrupt Edge Falling\n");
> - if (intfcaps & TPM_INTF_INT_EDGE_RISING)
> - dev_dbg(dev, "\tInterrupt Edge Rising\n");
> - if (intfcaps & TPM_INTF_INT_LEVEL_LOW)
> - dev_dbg(dev, "\tInterrupt Level Low\n");
> - if (intfcaps & TPM_INTF_INT_LEVEL_HIGH)
> - dev_dbg(dev, "\tInterrupt Level High\n");
> - if (intfcaps & TPM_INTF_LOCALITY_CHANGE_INT)
> - dev_dbg(dev, "\tLocality Change Int Support\n");
> - if (intfcaps & TPM_INTF_STS_VALID_INT)
> - dev_dbg(dev, "\tSts Valid Int Support\n");
> - if (intfcaps & TPM_INTF_DATA_AVAIL_INT)
> - dev_dbg(dev, "\tData Avail Int Support\n");
> -
> /* INTERRUPT Setup */
> init_waitqueue_head(&priv->read_queue);
> init_waitqueue_head(&priv->int_queue);
> @@ -1098,7 +1125,9 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
> else
> tpm_tis_probe_irq(chip, intmask);
>
> - if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) {
> + if (chip->flags & TPM_CHIP_FLAG_IRQ) {
> + priv->int_mask = intmask;
> + } else {
> dev_err(&chip->dev, FW_BUG
> "TPM interrupt not working, polling instead\n");
>
> @@ -1145,13 +1174,7 @@ static void tpm_tis_reenable_interrupts(struct tpm_chip *chip)
> if (rc < 0)
> goto out;
>
> - rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
> - if (rc < 0)
> - goto out;
> -
> - intmask |= TPM_INTF_CMD_READY_INT
> - | TPM_INTF_LOCALITY_CHANGE_INT | TPM_INTF_DATA_AVAIL_INT
> - | TPM_INTF_STS_VALID_INT | TPM_GLOBAL_INT_ENABLE;
> + intmask = priv->int_mask | TPM_GLOBAL_INT_ENABLE;
>
> tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
>
> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
> index bf07379dea42..e005eb99480e 100644
> --- a/drivers/char/tpm/tpm_tis_core.h
> +++ b/drivers/char/tpm/tpm_tis_core.h
> @@ -93,6 +93,7 @@ struct tpm_tis_data {
> u16 manufacturer_id;
> int locality;
> int irq;
> + unsigned int int_mask;
> unsigned long flags;
> void __iomem *ilb_base_addr;
> u16 clkrun_enabled;
> --
> 2.25.1
>


Reviewed-by: Jarkko Sakkinen <[email protected]>

BR, Jarkko

2022-08-26 18:10:23

by Jason Andryuk

[permalink] [raw]
Subject: Re: [PATCH v7 05/10] tpm, tpm_tis: Only handle supported interrupts

On Wed, Jun 29, 2022 at 7:28 PM Lino Sanfilippo <[email protected]> wrote:

> @@ -1007,8 +1029,39 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
> if (rc < 0)
> goto out_err;
>
> - intmask |= TPM_INTF_CMD_READY_INT | TPM_INTF_LOCALITY_CHANGE_INT |
> - TPM_INTF_DATA_AVAIL_INT | TPM_INTF_STS_VALID_INT;
> + /* Figure out the capabilities */
> + rc = tpm_tis_read32(priv, TPM_INTF_CAPS(priv->locality), &intfcaps);
> + if (rc < 0)
> + goto out_err;
> +
> + dev_dbg(dev, "TPM interface capabilities (0x%x):\n",
> + intfcaps);
> + if (intfcaps & TPM_INTF_BURST_COUNT_STATIC)
> + dev_dbg(dev, "\tBurst Count Static\n");
> + if (intfcaps & TPM_INTF_CMD_READY_INT) {
> + intmask |= TPM_INTF_CMD_READY_INT;
> + dev_dbg(dev, "\tCommand Ready Int Support\n");
> + }
> + if (intfcaps & TPM_INTF_INT_EDGE_FALLING)
> + dev_dbg(dev, "\tInterrupt Edge Falling\n");
> + if (intfcaps & TPM_INTF_INT_EDGE_RISING)
> + dev_dbg(dev, "\tInterrupt Edge Rising\n");
> + if (intfcaps & TPM_INTF_INT_LEVEL_LOW)
> + dev_dbg(dev, "\tInterrupt Level Low\n");
> + if (intfcaps & TPM_INTF_INT_LEVEL_HIGH)
> + dev_dbg(dev, "\tInterrupt Level High\n");
> + if (intfcaps & TPM_INTF_LOCALITY_CHANGE_INT)

Hi, you may already have fixed this, but I just saw:

error: this ‘if’ clause does not guard... [-Werror=misleading-indentation]
1144 | if (intfcaps & TPM_INTF_LOCALITY_CHANGE_INT)
| ^~

> + intmask |= TPM_INTF_LOCALITY_CHANGE_INT;
> + dev_dbg(dev, "\tLocality Change Int Support\n");

You need { } for the block.

Regards,
Jason

2022-08-29 08:10:43

by Lino Sanfilippo

[permalink] [raw]
Subject: Re: [PATCH v7 05/10] tpm, tpm_tis: Only handle supported interrupts


Hi Jason,

On 26.08.22 19:43, Jason Andryuk wrote:
> On Wed, Jun 29, 2022 at 7:28 PM Lino Sanfilippo <[email protected]> wrote:
>
>> @@ -1007,8 +1029,39 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>> if (rc < 0)
>> goto out_err;
>>
>> - intmask |= TPM_INTF_CMD_READY_INT | TPM_INTF_LOCALITY_CHANGE_INT |
>> - TPM_INTF_DATA_AVAIL_INT | TPM_INTF_STS_VALID_INT;
>> + /* Figure out the capabilities */
>> + rc = tpm_tis_read32(priv, TPM_INTF_CAPS(priv->locality), &intfcaps);
>> + if (rc < 0)
>> + goto out_err;
>> +
>> + dev_dbg(dev, "TPM interface capabilities (0x%x):\n",
>> + intfcaps);
>> + if (intfcaps & TPM_INTF_BURST_COUNT_STATIC)
>> + dev_dbg(dev, "\tBurst Count Static\n");
>> + if (intfcaps & TPM_INTF_CMD_READY_INT) {
>> + intmask |= TPM_INTF_CMD_READY_INT;
>> + dev_dbg(dev, "\tCommand Ready Int Support\n");
>> + }
>> + if (intfcaps & TPM_INTF_INT_EDGE_FALLING)
>> + dev_dbg(dev, "\tInterrupt Edge Falling\n");
>> + if (intfcaps & TPM_INTF_INT_EDGE_RISING)
>> + dev_dbg(dev, "\tInterrupt Edge Rising\n");
>> + if (intfcaps & TPM_INTF_INT_LEVEL_LOW)
>> + dev_dbg(dev, "\tInterrupt Level Low\n");
>> + if (intfcaps & TPM_INTF_INT_LEVEL_HIGH)
>> + dev_dbg(dev, "\tInterrupt Level High\n");
>> + if (intfcaps & TPM_INTF_LOCALITY_CHANGE_INT)
>
> Hi, you may already have fixed this, but I just saw:
>
> error: this ‘if’ clause does not guard... [-Werror=misleading-indentation]
> 1144 | if (intfcaps & TPM_INTF_LOCALITY_CHANGE_INT)
> | ^~
>
>> + intmask |= TPM_INTF_LOCALITY_CHANGE_INT;
>> + dev_dbg(dev, "\tLocality Change Int Support\n");
>
> You need { } for the block.
>

thanks for pointing at this, I will fix it in the next version of this series.

Regards,
Lino


> Regards,
> Jason

2022-08-30 06:39:15

by Lino Sanfilippo

[permalink] [raw]
Subject: Re: [PATCH v7 05/10] tpm, tpm_tis: Only handle supported interrupts


Hi Jason,

On 26.08.22 at 19:43, Jason Andryuk wrote:
> On Wed, Jun 29, 2022 at 7:28 PM Lino Sanfilippo <[email protected]> wrote:
>
>> @@ -1007,8 +1029,39 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>> if (rc < 0)
>> goto out_err;
>>
>> - intmask |= TPM_INTF_CMD_READY_INT | TPM_INTF_LOCALITY_CHANGE_INT |
>> - TPM_INTF_DATA_AVAIL_INT | TPM_INTF_STS_VALID_INT;
>> + /* Figure out the capabilities */
>> + rc = tpm_tis_read32(priv, TPM_INTF_CAPS(priv->locality), &intfcaps);
>> + if (rc < 0)
>> + goto out_err;
>> +
>> + dev_dbg(dev, "TPM interface capabilities (0x%x):\n",
>> + intfcaps);
>> + if (intfcaps & TPM_INTF_BURST_COUNT_STATIC)
>> + dev_dbg(dev, "\tBurst Count Static\n");
>> + if (intfcaps & TPM_INTF_CMD_READY_INT) {
>> + intmask |= TPM_INTF_CMD_READY_INT;
>> + dev_dbg(dev, "\tCommand Ready Int Support\n");
>> + }
>> + if (intfcaps & TPM_INTF_INT_EDGE_FALLING)
>> + dev_dbg(dev, "\tInterrupt Edge Falling\n");
>> + if (intfcaps & TPM_INTF_INT_EDGE_RISING)
>> + dev_dbg(dev, "\tInterrupt Edge Rising\n");
>> + if (intfcaps & TPM_INTF_INT_LEVEL_LOW)
>> + dev_dbg(dev, "\tInterrupt Level Low\n");
>> + if (intfcaps & TPM_INTF_INT_LEVEL_HIGH)
>> + dev_dbg(dev, "\tInterrupt Level High\n");
>> + if (intfcaps & TPM_INTF_LOCALITY_CHANGE_INT)
>
> Hi, you may already have fixed this, but I just saw:
>
> error: this ‘if’ clause does not guard... [-Werror=misleading-indentation]
> 1144 | if (intfcaps & TPM_INTF_LOCALITY_CHANGE_INT)
> | ^~
>
>> + intmask |= TPM_INTF_LOCALITY_CHANGE_INT;
>> + dev_dbg(dev, "\tLocality Change Int Support\n");
>
> You need { } for the block.
>

Thanks for pointing at this. I will fix it in the next version of the series.

Best Regards,
Lino


> Regards,
> Jason
>