diff mbox series

[v6,5/9] tpm, tpm_tis: Only handle supported interrupts

Message ID 20220621132447.16281-6-LinoSanfilippo@gmx.de (mailing list archive)
State New, archived
Headers show
Series TPM IRQ fixes | expand

Commit Message

Lino Sanfilippo June 21, 2022, 1:24 p.m. UTC 
From: Lino Sanfilippo <l.sanfilippo@kunbus.com>

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 <l.sanfilippo@kunbus.com>
---
 drivers/char/tpm/tpm_tis_core.c | 119 +++++++++++++++++++-------------
 drivers/char/tpm/tpm_tis_core.h |   1 +
 2 files changed, 72 insertions(+), 48 deletions(-)

Comments

Jarkko Sakkinen June 26, 2022, 6:40 a.m. UTC | #1 
On Tue, Jun 21, 2022 at 03:24:43PM +0200, Lino Sanfilippo wrote:
> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> 
> 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 <l.sanfilippo@kunbus.com>
> ---
>  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..cb469591373a 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 what 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;

I would instead mask out bits and write a helper function
taking care of this:

static u8 tpm_tis_filter_sts_mask(u8 int_mask, u8 sts_mask)
{
        struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);

        if (!(int_mask & TPM_INTF_STS_VALID_INT))
                sts_mask &= ~TPM_STS_VALID;

        if (!(int_mask & TPM_INTF_DATA_AVAIL_INT))
                sts_mask &= ~TPM_STS_DATA_AVAIL;

        if (!(int_mask & TPM_INTF_CMD_READY_INT))
		sts_mask &= ~TPM_STS_COMMAND_READY;

        return sts_mask;
}

Less operations and imho somewhat cleaner structure.

Add suggested-by if you want.

BR, Jarkko
Jarkko Sakkinen June 26, 2022, 6:44 a.m. UTC | #2 
On Sun, Jun 26, 2022 at 09:40:43AM +0300, Jarkko Sakkinen wrote:
> On Tue, Jun 21, 2022 at 03:24:43PM +0200, Lino Sanfilippo wrote:
> > From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> > 
> > 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 <l.sanfilippo@kunbus.com>
> > ---
> >  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..cb469591373a 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 what 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;
> 
> I would instead mask out bits and write a helper function
> taking care of this:
> 
> static u8 tpm_tis_filter_sts_mask(u8 int_mask, u8 sts_mask)
> {
>         struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);

Ignore this line (wrote this out of top of my head).

>         if (!(int_mask & TPM_INTF_STS_VALID_INT))
>                 sts_mask &= ~TPM_STS_VALID;
> 
>         if (!(int_mask & TPM_INTF_DATA_AVAIL_INT))
>                 sts_mask &= ~TPM_STS_DATA_AVAIL;
> 
>         if (!(int_mask & TPM_INTF_CMD_READY_INT))
> 		sts_mask &= ~TPM_STS_COMMAND_READY;
> 
>         return sts_mask;
> }
> 
> Less operations and imho somewhat cleaner structure.
> 
> Add suggested-by if you want.
> 
> BR, Jarkko
Lino Sanfilippo June 26, 2022, 12:18 p.m. UTC | #3 
On 26.06.22 at 08:40, Jarkko Sakkinen wrote:
>
> I would instead mask out bits and write a helper function
> taking care of this:
>
> static u8 tpm_tis_filter_sts_mask(u8 int_mask, u8 sts_mask)
> {
>         struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>
>         if (!(int_mask & TPM_INTF_STS_VALID_INT))
>                 sts_mask &= ~TPM_STS_VALID;
>
>         if (!(int_mask & TPM_INTF_DATA_AVAIL_INT))
>                 sts_mask &= ~TPM_STS_DATA_AVAIL;
>
>         if (!(int_mask & TPM_INTF_CMD_READY_INT))
> 		sts_mask &= ~TPM_STS_COMMAND_READY;
>
>         return sts_mask;
> }
>
> Less operations and imho somewhat cleaner structure.
>
> Add suggested-by if you want.

I thought of a helper like this before but then decided to
not introduce another function to keep the code changes minimal. But yes,
it is indeed cleaner. I will do the change and resubmit the series.

Thanks for the review!

Regards,
Lino
Jarkko Sakkinen June 27, 2022, 11:09 p.m. UTC | #4 
On Sun, Jun 26, 2022 at 02:18:17PM +0200, Lino Sanfilippo wrote:
> On 26.06.22 at 08:40, Jarkko Sakkinen wrote:
> >
> > I would instead mask out bits and write a helper function
> > taking care of this:
> >
> > static u8 tpm_tis_filter_sts_mask(u8 int_mask, u8 sts_mask)
> > {
> >         struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> >
> >         if (!(int_mask & TPM_INTF_STS_VALID_INT))
> >                 sts_mask &= ~TPM_STS_VALID;
> >
> >         if (!(int_mask & TPM_INTF_DATA_AVAIL_INT))
> >                 sts_mask &= ~TPM_STS_DATA_AVAIL;
> >
> >         if (!(int_mask & TPM_INTF_CMD_READY_INT))
> > 		sts_mask &= ~TPM_STS_COMMAND_READY;
> >
> >         return sts_mask;
> > }
> >
> > Less operations and imho somewhat cleaner structure.
> >
> > Add suggested-by if you want.
> 
> I thought of a helper like this before but then decided to
> not introduce another function to keep the code changes minimal. But yes,
> it is indeed cleaner. I will do the change and resubmit the series.
> 
> Thanks for the review!
> 
> Regards,
> Lino

Yeah, please don't add suggested-by, it's such a minor detail
in the overall patch :-) Thanks for taking time to fix these
glitches and also taking all the feedback into account (and
also being patient).
 
BR, Jarkko
Lino Sanfilippo June 29, 2022, 9:20 a.m. UTC | #5 
On 28.06.22 01:09, Jarkko Sakkinen wrote:
> On Sun, Jun 26, 2022 at 02:18:17PM +0200, Lino Sanfilippo wrote:
>> On 26.06.22 at 08:40, Jarkko Sakkinen wrote:
>>>
>>> I would instead mask out bits and write a helper function
>>> taking care of this:
>>>
>>> static u8 tpm_tis_filter_sts_mask(u8 int_mask, u8 sts_mask)
>>> {
>>>         struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>>>
>>>         if (!(int_mask & TPM_INTF_STS_VALID_INT))
>>>                 sts_mask &= ~TPM_STS_VALID;
>>>
>>>         if (!(int_mask & TPM_INTF_DATA_AVAIL_INT))
>>>                 sts_mask &= ~TPM_STS_DATA_AVAIL;
>>>
>>>         if (!(int_mask & TPM_INTF_CMD_READY_INT))
>>> 		sts_mask &= ~TPM_STS_COMMAND_READY;
>>>
>>>         return sts_mask;
>>> }
>>>
>>> Less operations and imho somewhat cleaner structure.
>>>
>>> Add suggested-by if you want.
>>
>> I thought of a helper like this before but then decided to
>> not introduce another function to keep the code changes minimal. But yes,
>> it is indeed cleaner. I will do the change and resubmit the series.
>>
>> Thanks for the review!
>>
>> Regards,
>> Lino
>
> Yeah, please don't add suggested-by, it's such a minor detail
> in the overall patch :-)

I already created a separate patch which only contains moving the bit checks into the
helper function. For that patch the Suggested-by is fully justified IMHO.


Thanks for taking time to fix these
> glitches and also taking all the feedback into account (and
> also being patient).
>

No problem. Its always good to have some feedback from people that have a deeper insight
into the code. Especially when it is as complex as the TPM subsystem and drivers.

Best regards,
Lino
diff mbox series

Patch

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 09d8f04cbc81..cb469591373a 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 what 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;