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;
Date:   Fri, 1 Jul 2022 02:18:41 +0300
From:   Jarkko Sakkinen <jarkko@kernel.org>
To:     Lino Sanfilippo <LinoSanfilippo@gmx.de>
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, l.sanfilippo@kunbus.com,
        lukas@wunner.de, p.rosenberger@kunbus.com
Subject: Re: [PATCH v7 05/10] tpm, tpm_tis: Only handle supported interrupts
Message-ID: <Yr4vUSrShAWntLeP@kernel.org>
References: <20220629232653.1306735-1-LinoSanfilippo@gmx.de>
 <20220629232653.1306735-6-LinoSanfilippo@gmx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220629232653.1306735-6-LinoSanfilippo@gmx.de>
Precedence: bulk

On Thu, Jun 30, 2022 at 01:26:48AM +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>
> Tested-by: Michael Niew??hner <linux@mniewoehner.de>
> ---
>  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 <jarkko@kernel.org>

BR, Jarkko