| Message ID | 20230522143105.8617-2-LinoSanfilippo@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] tpm, tpm_tis: Handle interrupt storm | expand |
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
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,