[2/2] tpm, tpm_tis: reuse code in disable_interrupts()

  From: Lino Sanfilippo <l.sanfilippo@kunbus.com>

Avoid code redundancy by shifting part of the code in disable_interrupts()
into a subfunction and reusing this function in tpm_tis_handle_irq_storm().
Make sure that in the subfunction the INT_ENABLE register is written with a
claimed locality even if the caller did not claim it before.

In the shifted code get rid of the variable "rc" by initializing the
interrupt mask to zero at variable declaration.

Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
---
 drivers/char/tpm/tpm_tis_core.c | 36 ++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 19 deletions(-)
  

On Mon, May 22, 2023 at 04:31:05PM +0200, Lino Sanfilippo wrote:
> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> 
> Avoid code redundancy by shifting part of the code in disable_interrupts()
> into a subfunction and reusing this function in tpm_tis_handle_irq_storm().
> Make sure that in the subfunction the INT_ENABLE register is written with a
> claimed locality even if the caller did not claim it before.
> 
> In the shifted code get rid of the variable "rc" by initializing the
> interrupt mask to zero at variable declaration.
> 
> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>

Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>

> ---
>  drivers/char/tpm/tpm_tis_core.c | 36 ++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 458ebf8c2f16..8f4f2cb5520f 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -468,25 +468,32 @@ static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len)
>  	return rc;
>  }
>  
> +static void __tpm_tis_disable_interrupts(struct tpm_chip *chip)
> +{
> +	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> +	u32 intmask = 0;
> +
> +	tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
> +	intmask &= ~TPM_GLOBAL_INT_ENABLE;
> +
> +	tpm_tis_request_locality(chip, 0);
> +	tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
> +	tpm_tis_relinquish_locality(chip, 0);
> +
> +	chip->flags &= ~TPM_CHIP_FLAG_IRQ;
> +}
> +
>  static void disable_interrupts(struct tpm_chip *chip)
>  {
>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> -	u32 intmask;
> -	int rc;
>  
>  	if (priv->irq == 0)
>  		return;
>  
> -	rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
> -	if (rc < 0)
> -		intmask = 0;
> -
> -	intmask &= ~TPM_GLOBAL_INT_ENABLE;
> -	rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
> +	__tpm_tis_disable_interrupts(chip);
>  
>  	devm_free_irq(chip->dev.parent, priv->irq, chip);
>  	priv->irq = 0;
> -	chip->flags &= ~TPM_CHIP_FLAG_IRQ;
>  }
>  
>  /*
> @@ -755,20 +762,11 @@ static bool tpm_tis_req_canceled(struct tpm_chip *chip, u8 status)
>  static void tpm_tis_handle_irq_storm(struct tpm_chip *chip)
>  {
>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> -	int intmask = 0;
>  
>  	dev_err(&chip->dev, HW_ERR
>  		"TPM interrupt storm detected, polling instead\n");
>  
> -	tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
> -
> -	intmask &= ~TPM_GLOBAL_INT_ENABLE;
> -
> -	tpm_tis_request_locality(chip, 0);
> -	tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
> -	tpm_tis_relinquish_locality(chip, 0);
> -
> -	chip->flags &= ~TPM_CHIP_FLAG_IRQ;
> +	__tpm_tis_disable_interrupts(chip);
>  
>  	/*
>  	 * We must not call devm_free_irq() from within the interrupt handler,
> -- 
> 2.40.1
>
  

On 22/05/2023 17:31, Lino Sanfilippo wrote:
> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> 
> Avoid code redundancy by shifting part of the code in disable_interrupts()
> into a subfunction and reusing this function in tpm_tis_handle_irq_storm().
> Make sure that in the subfunction the INT_ENABLE register is written with a
> claimed locality even if the caller did not claim it before.
> 
> In the shifted code get rid of the variable "rc" by initializing the
> interrupt mask to zero at variable declaration.

Reviewed-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>

> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> ---
>  drivers/char/tpm/tpm_tis_core.c | 36 ++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 458ebf8c2f16..8f4f2cb5520f 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -468,25 +468,32 @@ static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len)
>  	return rc;
>  }
>  
> +static void __tpm_tis_disable_interrupts(struct tpm_chip *chip)
> +{
> +	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> +	u32 intmask = 0;
> +
> +	tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
> +	intmask &= ~TPM_GLOBAL_INT_ENABLE;
> +
> +	tpm_tis_request_locality(chip, 0);
> +	tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
> +	tpm_tis_relinquish_locality(chip, 0);
> +
> +	chip->flags &= ~TPM_CHIP_FLAG_IRQ;
> +}
> +
>  static void disable_interrupts(struct tpm_chip *chip)
>  {
>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> -	u32 intmask;
> -	int rc;
>  
>  	if (priv->irq == 0)
>  		return;
>  
> -	rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
> -	if (rc < 0)
> -		intmask = 0;
> -
> -	intmask &= ~TPM_GLOBAL_INT_ENABLE;
> -	rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
> +	__tpm_tis_disable_interrupts(chip);
>  
>  	devm_free_irq(chip->dev.parent, priv->irq, chip);
>  	priv->irq = 0;
> -	chip->flags &= ~TPM_CHIP_FLAG_IRQ;
>  }
>  
>  /*
> @@ -755,20 +762,11 @@ static bool tpm_tis_req_canceled(struct tpm_chip *chip, u8 status)
>  static void tpm_tis_handle_irq_storm(struct tpm_chip *chip)
>  {
>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> -	int intmask = 0;
>  
>  	dev_err(&chip->dev, HW_ERR
>  		"TPM interrupt storm detected, polling instead\n");
>  
> -	tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
> -
> -	intmask &= ~TPM_GLOBAL_INT_ENABLE;
> -
> -	tpm_tis_request_locality(chip, 0);
> -	tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
> -	tpm_tis_relinquish_locality(chip, 0);
> -
> -	chip->flags &= ~TPM_CHIP_FLAG_IRQ;
> +	__tpm_tis_disable_interrupts(chip);
>  
>  	/*
>  	 * We must not call devm_free_irq() from within the interrupt handler,
  

On Mon May 22, 2023 at 5:31 PM EEST, Lino Sanfilippo wrote:
> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>
> Avoid code redundancy by shifting part of the code in disable_interrupts()
> into a subfunction and reusing this function in tpm_tis_handle_irq_storm().
> Make sure that in the subfunction the INT_ENABLE register is written with a
> claimed locality even if the caller did not claim it before.
>
> In the shifted code get rid of the variable "rc" by initializing the
> interrupt mask to zero at variable declaration.
>
> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
> ---
>  drivers/char/tpm/tpm_tis_core.c | 36 ++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 458ebf8c2f16..8f4f2cb5520f 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -468,25 +468,32 @@ static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, size_t len)
>  	return rc;
>  }
>  
> +static void __tpm_tis_disable_interrupts(struct tpm_chip *chip)
> +{
> +	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> +	u32 intmask = 0;
> +
> +	tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
> +	intmask &= ~TPM_GLOBAL_INT_ENABLE;
> +
> +	tpm_tis_request_locality(chip, 0);
> +	tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
> +	tpm_tis_relinquish_locality(chip, 0);
> +
> +	chip->flags &= ~TPM_CHIP_FLAG_IRQ;
> +}
> +
>  static void disable_interrupts(struct tpm_chip *chip)
>  {
>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> -	u32 intmask;
> -	int rc;
>  
>  	if (priv->irq == 0)
>  		return;
>  
> -	rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
> -	if (rc < 0)
> -		intmask = 0;
> -
> -	intmask &= ~TPM_GLOBAL_INT_ENABLE;
> -	rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
> +	__tpm_tis_disable_interrupts(chip);
>  
>  	devm_free_irq(chip->dev.parent, priv->irq, chip);
>  	priv->irq = 0;
> -	chip->flags &= ~TPM_CHIP_FLAG_IRQ;
>  }
>  
>  /*
> @@ -755,20 +762,11 @@ static bool tpm_tis_req_canceled(struct tpm_chip *chip, u8 status)
>  static void tpm_tis_handle_irq_storm(struct tpm_chip *chip)
>  {
>  	struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
> -	int intmask = 0;
>  
>  	dev_err(&chip->dev, HW_ERR
>  		"TPM interrupt storm detected, polling instead\n");
>  
> -	tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
> -
> -	intmask &= ~TPM_GLOBAL_INT_ENABLE;
> -
> -	tpm_tis_request_locality(chip, 0);
> -	tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
> -	tpm_tis_relinquish_locality(chip, 0);
> -
> -	chip->flags &= ~TPM_CHIP_FLAG_IRQ;
> +	__tpm_tis_disable_interrupts(chip);
>  
>  	/*
>  	 * We must not call devm_free_irq() from within the interrupt handler,
> -- 
> 2.40.1

NAK as invidual change w/o further discussion.

Would need to be seen in context. This does not change kernel for
better.

If you want to wrap, please do it in 1/2 and then we can evaluate
whether it makes sense or not.

BR, Jarkko
  

On 23.05.23 21:07, Jarkko Sakkinen wrote:

> 
> NAK as invidual change w/o further discussion.
> 
> Would need to be seen in context. This does not change kernel for
> better.
> 
> If you want to wrap, please do it in 1/2 and then we can evaluate
> whether it makes sense or not.
> 

Ok, will do so.

Regards,
Lino
  

On Tue May 23, 2023 at 11:52 PM EEST, Lino Sanfilippo wrote:
>
>
> On 23.05.23 21:07, Jarkko Sakkinen wrote:
>
> > 
> > NAK as invidual change w/o further discussion.
> > 
> > Would need to be seen in context. This does not change kernel for
> > better.
> > 
> > If you want to wrap, please do it in 1/2 and then we can evaluate
> > whether it makes sense or not.
> > 
>
> Ok, will do so.

And generally: keep it *minimal* :-) We want to exactly fix the bug,
and take absolutely no other action.

BR, Jarkko