| Message ID | 20220509080559.4381-6-LinoSanfilippo@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | TPM irq fixes | expand |
On Mon, May 09, 2022 at 10:05:58AM +0200, Lino Sanfilippo wrote: > From: Lino Sanfilippo <l.sanfilippo@kunbus.com> > > There is no need to check for the irq test completion at each data > transmission during the driver livetime. Instead do the check only once at > driver startup. > > Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com> > --- > drivers/char/tpm/tpm_tis_core.c | 68 +++++++++++---------------------- > 1 file changed, 22 insertions(+), 46 deletions(-) > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c > index bdfde1cd71fe..4c65718feb7d 100644 > --- a/drivers/char/tpm/tpm_tis_core.c > +++ b/drivers/char/tpm/tpm_tis_core.c > @@ -432,7 +432,7 @@ static void disable_interrupts(struct tpm_chip *chip) > * tpm.c can skip polling for the data to be available as the interrupt is > * waited for here > */ > -static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, size_t len) > +static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len) > { > struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > int rc; > @@ -465,30 +465,6 @@ static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, size_t len) > return rc; > } > > -static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len) > -{ > - int rc, irq; > - struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > - > - if (!(chip->flags & TPM_CHIP_FLAG_IRQ) || > - test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags)) > - return tpm_tis_send_main(chip, buf, len); > - > - /* Verify receipt of the expected IRQ */ > - irq = priv->irq; > - priv->irq = 0; > - chip->flags &= ~TPM_CHIP_FLAG_IRQ; > - rc = tpm_tis_send_main(chip, buf, len); > - priv->irq = irq; > - chip->flags |= TPM_CHIP_FLAG_IRQ; > - if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags)) > - tpm_msleep(1); > - if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags)) > - disable_interrupts(chip); > - set_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags); > - return rc; > -} > - > struct tis_vendor_durations_override { > u32 did_vid; > struct tpm1_version version; > @@ -759,51 +735,54 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask, > > rc = tpm_tis_read8(priv, TPM_INT_VECTOR(priv->locality), > &original_int_vec); > - if (rc < 0) > + if (rc < 0) { > + disable_interrupts(chip); > return rc; > + } > > rc = tpm_tis_write8(priv, TPM_INT_VECTOR(priv->locality), irq); > if (rc < 0) > - return rc; > + goto out_err; > > rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &int_status); > if (rc < 0) > - return rc; > + goto out_err; > > /* Clear all existing */ > rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), int_status); > if (rc < 0) > - return rc; > + goto out_err; > > /* Turn on */ > rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), > intmask | TPM_GLOBAL_INT_ENABLE); > if (rc < 0) > - return rc; > + goto out_err; > > clear_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags); > - chip->flags |= TPM_CHIP_FLAG_IRQ; > > /* Generate an interrupt by having the core call through to > * tpm_tis_send > */ > rc = tpm_tis_gen_interrupt(chip); > if (rc < 0) > - return rc; > + goto out_err; > > - /* tpm_tis_send will either confirm the interrupt is working or it > - * will call disable_irq which undoes all of the above. > - */ > - if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) { > - rc = tpm_tis_write8(priv, original_int_vec, > - TPM_INT_VECTOR(priv->locality)); > - if (rc < 0) > - return rc; > + tpm_msleep(1); > > - return 1; > - } > + /* Verify receipt of the expected IRQ */ > + if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags)) > + goto out_err; > + > + chip->flags |= TPM_CHIP_FLAG_IRQ; > > return 0; > + > +out_err: > + disable_interrupts(chip); > + tpm_tis_write8(priv, original_int_vec, TPM_INT_VECTOR(priv->locality)); > + > + return rc; > } > > /* Try to find the IRQ the TPM is using. This is for legacy x86 systems that > @@ -1075,12 +1054,9 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, > if (irq) { > tpm_tis_probe_irq_single(chip, intmask, IRQF_SHARED, > irq); > - if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) { > + if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) > dev_err(&chip->dev, FW_BUG > "TPM interrupt not working, polling instead\n"); > - > - disable_interrupts(chip); > - } > } else { > tpm_tis_probe_irq(chip, intmask); > } > -- > 2.36.0 > For me this looks just code shuffling. I don't disagree but changing working code without actual semantical reasons neither makes sense. BR, Jarkko
On 11.05.22 at 17:09, Jarkko Sakkinen wrote: > On Mon, May 09, 2022 at 10:05:58AM +0200, Lino Sanfilippo wrote: >> From: Lino Sanfilippo <l.sanfilippo@kunbus.com> >> >> There is no need to check for the irq test completion at each data >> transmission during the driver livetime. Instead do the check only once at >> driver startup. >> >> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com> >> --- >> drivers/char/tpm/tpm_tis_core.c | 68 +++++++++++---------------------- >> 1 file changed, 22 insertions(+), 46 deletions(-) >> >> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c >> index bdfde1cd71fe..4c65718feb7d 100644 >> --- a/drivers/char/tpm/tpm_tis_core.c >> +++ b/drivers/char/tpm/tpm_tis_core.c >> @@ -432,7 +432,7 @@ static void disable_interrupts(struct tpm_chip *chip) >> * tpm.c can skip polling for the data to be available as the interrupt is >> * waited for here >> */ >> -static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, size_t len) >> +static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len) >> { >> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); >> int rc; >> @@ -465,30 +465,6 @@ static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, size_t len) >> return rc; >> } >> >> -static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len) >> -{ >> - int rc, irq; >> - struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); >> - >> - if (!(chip->flags & TPM_CHIP_FLAG_IRQ) || >> - test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags)) >> - return tpm_tis_send_main(chip, buf, len); >> - >> - /* Verify receipt of the expected IRQ */ >> - irq = priv->irq; >> - priv->irq = 0; >> - chip->flags &= ~TPM_CHIP_FLAG_IRQ; >> - rc = tpm_tis_send_main(chip, buf, len); >> - priv->irq = irq; >> - chip->flags |= TPM_CHIP_FLAG_IRQ; >> - if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags)) >> - tpm_msleep(1); >> - if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags)) >> - disable_interrupts(chip); >> - set_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags); >> - return rc; >> -} >> - >> struct tis_vendor_durations_override { >> u32 did_vid; >> struct tpm1_version version; >> @@ -759,51 +735,54 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask, >> >> rc = tpm_tis_read8(priv, TPM_INT_VECTOR(priv->locality), >> &original_int_vec); >> - if (rc < 0) >> + if (rc < 0) { >> + disable_interrupts(chip); >> return rc; >> + } >> >> rc = tpm_tis_write8(priv, TPM_INT_VECTOR(priv->locality), irq); >> if (rc < 0) >> - return rc; >> + goto out_err; >> >> rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &int_status); >> if (rc < 0) >> - return rc; >> + goto out_err; >> >> /* Clear all existing */ >> rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), int_status); >> if (rc < 0) >> - return rc; >> + goto out_err; >> >> /* Turn on */ >> rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), >> intmask | TPM_GLOBAL_INT_ENABLE); >> if (rc < 0) >> - return rc; >> + goto out_err; >> >> clear_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags); >> - chip->flags |= TPM_CHIP_FLAG_IRQ; >> >> /* Generate an interrupt by having the core call through to >> * tpm_tis_send >> */ >> rc = tpm_tis_gen_interrupt(chip); >> if (rc < 0) >> - return rc; >> + goto out_err; >> >> - /* tpm_tis_send will either confirm the interrupt is working or it >> - * will call disable_irq which undoes all of the above. >> - */ >> - if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) { >> - rc = tpm_tis_write8(priv, original_int_vec, >> - TPM_INT_VECTOR(priv->locality)); >> - if (rc < 0) >> - return rc; >> + tpm_msleep(1); >> >> - return 1; >> - } >> + /* Verify receipt of the expected IRQ */ >> + if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags)) >> + goto out_err; >> + >> + chip->flags |= TPM_CHIP_FLAG_IRQ; >> >> return 0; >> + >> +out_err: >> + disable_interrupts(chip); >> + tpm_tis_write8(priv, original_int_vec, TPM_INT_VECTOR(priv->locality)); >> + >> + return rc; >> } >> >> /* Try to find the IRQ the TPM is using. This is for legacy x86 systems that >> @@ -1075,12 +1054,9 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, >> if (irq) { >> tpm_tis_probe_irq_single(chip, intmask, IRQF_SHARED, >> irq); >> - if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) { >> + if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) >> dev_err(&chip->dev, FW_BUG >> "TPM interrupt not working, polling instead\n"); >> - >> - disable_interrupts(chip); >> - } >> } else { >> tpm_tis_probe_irq(chip, intmask); >> } >> -- >> 2.36.0 >> > > For me this looks just code shuffling. > > I don't disagree but changing working code without actual semantical > reasons neither makes sense. > > BR, Jarkko > Well the semantical reason for this change is that the check for irq test completion only has to be done once for the driver livetime. There is no point in doing it over and over again for each transmission. So the code is not simply shuffled around, it is shifted to a place where it is only executed once. This is not a bugfix but it is clearly an improvement/cleanup. As far as I understood from your comments on the earlier versions of this patch set cleanups are also ok as long as they are not intermixed with bugfixes. Regards, Lino
On Wed, May 11, 2022 at 09:56:59PM +0200, Lino Sanfilippo wrote: > On 11.05.22 at 17:09, Jarkko Sakkinen wrote: > > On Mon, May 09, 2022 at 10:05:58AM +0200, Lino Sanfilippo wrote: > >> From: Lino Sanfilippo <l.sanfilippo@kunbus.com> > >> > >> There is no need to check for the irq test completion at each data > >> transmission during the driver livetime. Instead do the check only once at > >> driver startup. > >> > >> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com> > >> --- > >> drivers/char/tpm/tpm_tis_core.c | 68 +++++++++++---------------------- > >> 1 file changed, 22 insertions(+), 46 deletions(-) > >> > >> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c > >> index bdfde1cd71fe..4c65718feb7d 100644 > >> --- a/drivers/char/tpm/tpm_tis_core.c > >> +++ b/drivers/char/tpm/tpm_tis_core.c > >> @@ -432,7 +432,7 @@ static void disable_interrupts(struct tpm_chip *chip) > >> * tpm.c can skip polling for the data to be available as the interrupt is > >> * waited for here > >> */ > >> -static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, size_t len) > >> +static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len) > >> { > >> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > >> int rc; > >> @@ -465,30 +465,6 @@ static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, size_t len) > >> return rc; > >> } > >> > >> -static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len) > >> -{ > >> - int rc, irq; > >> - struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > >> - > >> - if (!(chip->flags & TPM_CHIP_FLAG_IRQ) || > >> - test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags)) > >> - return tpm_tis_send_main(chip, buf, len); > >> - > >> - /* Verify receipt of the expected IRQ */ > >> - irq = priv->irq; > >> - priv->irq = 0; > >> - chip->flags &= ~TPM_CHIP_FLAG_IRQ; > >> - rc = tpm_tis_send_main(chip, buf, len); > >> - priv->irq = irq; > >> - chip->flags |= TPM_CHIP_FLAG_IRQ; > >> - if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags)) > >> - tpm_msleep(1); > >> - if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags)) > >> - disable_interrupts(chip); > >> - set_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags); > >> - return rc; > >> -} > >> - > >> struct tis_vendor_durations_override { > >> u32 did_vid; > >> struct tpm1_version version; > >> @@ -759,51 +735,54 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask, > >> > >> rc = tpm_tis_read8(priv, TPM_INT_VECTOR(priv->locality), > >> &original_int_vec); > >> - if (rc < 0) > >> + if (rc < 0) { > >> + disable_interrupts(chip); > >> return rc; > >> + } > >> > >> rc = tpm_tis_write8(priv, TPM_INT_VECTOR(priv->locality), irq); > >> if (rc < 0) > >> - return rc; > >> + goto out_err; > >> > >> rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &int_status); > >> if (rc < 0) > >> - return rc; > >> + goto out_err; > >> > >> /* Clear all existing */ > >> rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), int_status); > >> if (rc < 0) > >> - return rc; > >> + goto out_err; > >> > >> /* Turn on */ > >> rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), > >> intmask | TPM_GLOBAL_INT_ENABLE); > >> if (rc < 0) > >> - return rc; > >> + goto out_err; > >> > >> clear_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags); > >> - chip->flags |= TPM_CHIP_FLAG_IRQ; > >> > >> /* Generate an interrupt by having the core call through to > >> * tpm_tis_send > >> */ > >> rc = tpm_tis_gen_interrupt(chip); > >> if (rc < 0) > >> - return rc; > >> + goto out_err; > >> > >> - /* tpm_tis_send will either confirm the interrupt is working or it > >> - * will call disable_irq which undoes all of the above. > >> - */ > >> - if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) { > >> - rc = tpm_tis_write8(priv, original_int_vec, > >> - TPM_INT_VECTOR(priv->locality)); > >> - if (rc < 0) > >> - return rc; > >> + tpm_msleep(1); > >> > >> - return 1; > >> - } > >> + /* Verify receipt of the expected IRQ */ > >> + if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags)) > >> + goto out_err; > >> + > >> + chip->flags |= TPM_CHIP_FLAG_IRQ; > >> > >> return 0; > >> + > >> +out_err: Rename this as just 'err'. > >> + disable_interrupts(chip); > >> + tpm_tis_write8(priv, original_int_vec, TPM_INT_VECTOR(priv->locality)); > >> + > >> + return rc; > >> } > >> > >> /* Try to find the IRQ the TPM is using. This is for legacy x86 systems that > >> @@ -1075,12 +1054,9 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, > >> if (irq) { > >> tpm_tis_probe_irq_single(chip, intmask, IRQF_SHARED, > >> irq); > >> - if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) { > >> + if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) > >> dev_err(&chip->dev, FW_BUG > >> "TPM interrupt not working, polling instead\n"); > >> - > >> - disable_interrupts(chip); > >> - } > >> } else { > >> tpm_tis_probe_irq(chip, intmask); > >> } > >> -- > >> 2.36.0 > >> > > > > For me this looks just code shuffling. > > > > I don't disagree but changing working code without actual semantical > > reasons neither makes sense. > > > > BR, Jarkko > > > > Well the semantical reason for this change is that the check for irq test completion > only has to be done once for the driver livetime. There is no point in doing it > over and over again for each transmission. > So the code is not simply shuffled around, it is shifted to a place where it is only > executed once. > > This is not a bugfix but it is clearly an improvement/cleanup. As far as I understood > from your comments on the earlier versions of this patch set cleanups are also ok as > long as they are not intermixed with bugfixes. The patch does not do anything particulary useful IMHO. There's no stimulus to do this change. > Regards, > Lino BR, Jarkko
On 16.05.22 at 19:51, Jarkko Sakkinen wrote: > On Wed, May 11, 2022 at 09:56:59PM +0200, Lino Sanfilippo wrote: >> On 11.05.22 at 17:09, Jarkko Sakkinen wrote: >>> On Mon, May 09, 2022 at 10:05:58AM +0200, Lino Sanfilippo wrote: >>>> From: Lino Sanfilippo <l.sanfilippo@kunbus.com> >>>> >>>> There is no need to check for the irq test completion at each data >>>> transmission during the driver livetime. Instead do the check only once at >>>> driver startup. >>>> >>>> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com> >>>> --- >>>> drivers/char/tpm/tpm_tis_core.c | 68 +++++++++++---------------------- >>>> 1 file changed, 22 insertions(+), 46 deletions(-) >>>> >>>> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c >>>> index bdfde1cd71fe..4c65718feb7d 100644 >>>> --- a/drivers/char/tpm/tpm_tis_core.c >>>> +++ b/drivers/char/tpm/tpm_tis_core.c >>>> @@ -432,7 +432,7 @@ static void disable_interrupts(struct tpm_chip *chip) >>>> * tpm.c can skip polling for the data to be available as the interrupt is >>>> * waited for here >>>> */ >>>> -static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, size_t len) >>>> +static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len) >>>> { >>>> struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); >>>> int rc; >>>> @@ -465,30 +465,6 @@ static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, size_t len) >>>> return rc; >>>> } >>>> >>>> -static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len) >>>> -{ >>>> - int rc, irq; >>>> - struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); >>>> - >>>> - if (!(chip->flags & TPM_CHIP_FLAG_IRQ) || >>>> - test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags)) >>>> - return tpm_tis_send_main(chip, buf, len); >>>> - >>>> - /* Verify receipt of the expected IRQ */ >>>> - irq = priv->irq; >>>> - priv->irq = 0; >>>> - chip->flags &= ~TPM_CHIP_FLAG_IRQ; >>>> - rc = tpm_tis_send_main(chip, buf, len); >>>> - priv->irq = irq; >>>> - chip->flags |= TPM_CHIP_FLAG_IRQ; >>>> - if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags)) >>>> - tpm_msleep(1); >>>> - if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags)) >>>> - disable_interrupts(chip); >>>> - set_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags); >>>> - return rc; >>>> -} >>>> - >>>> struct tis_vendor_durations_override { >>>> u32 did_vid; >>>> struct tpm1_version version; >>>> @@ -759,51 +735,54 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask, >>>> >>>> rc = tpm_tis_read8(priv, TPM_INT_VECTOR(priv->locality), >>>> &original_int_vec); >>>> - if (rc < 0) >>>> + if (rc < 0) { >>>> + disable_interrupts(chip); >>>> return rc; >>>> + } >>>> >>>> rc = tpm_tis_write8(priv, TPM_INT_VECTOR(priv->locality), irq); >>>> if (rc < 0) >>>> - return rc; >>>> + goto out_err; >>>> >>>> rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &int_status); >>>> if (rc < 0) >>>> - return rc; >>>> + goto out_err; >>>> >>>> /* Clear all existing */ >>>> rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), int_status); >>>> if (rc < 0) >>>> - return rc; >>>> + goto out_err; >>>> >>>> /* Turn on */ >>>> rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), >>>> intmask | TPM_GLOBAL_INT_ENABLE); >>>> if (rc < 0) >>>> - return rc; >>>> + goto out_err; >>>> >>>> clear_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags); >>>> - chip->flags |= TPM_CHIP_FLAG_IRQ; >>>> >>>> /* Generate an interrupt by having the core call through to >>>> * tpm_tis_send >>>> */ >>>> rc = tpm_tis_gen_interrupt(chip); >>>> if (rc < 0) >>>> - return rc; >>>> + goto out_err; >>>> >>>> - /* tpm_tis_send will either confirm the interrupt is working or it >>>> - * will call disable_irq which undoes all of the above. >>>> - */ >>>> - if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) { >>>> - rc = tpm_tis_write8(priv, original_int_vec, >>>> - TPM_INT_VECTOR(priv->locality)); >>>> - if (rc < 0) >>>> - return rc; >>>> + tpm_msleep(1); >>>> >>>> - return 1; >>>> - } >>>> + /* Verify receipt of the expected IRQ */ >>>> + if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags)) >>>> + goto out_err; >>>> + >>>> + chip->flags |= TPM_CHIP_FLAG_IRQ; >>>> >>>> return 0; >>>> + >>>> +out_err: > > Rename this as just 'err'. > >>>> + disable_interrupts(chip); >>>> + tpm_tis_write8(priv, original_int_vec, TPM_INT_VECTOR(priv->locality)); >>>> + >>>> + return rc; >>>> } >>>> >>>> /* Try to find the IRQ the TPM is using. This is for legacy x86 systems that >>>> @@ -1075,12 +1054,9 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, >>>> if (irq) { >>>> tpm_tis_probe_irq_single(chip, intmask, IRQF_SHARED, >>>> irq); >>>> - if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) { >>>> + if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) >>>> dev_err(&chip->dev, FW_BUG >>>> "TPM interrupt not working, polling instead\n"); >>>> - >>>> - disable_interrupts(chip); >>>> - } >>>> } else { >>>> tpm_tis_probe_irq(chip, intmask); >>>> } >>>> -- >>>> 2.36.0 >>>> >>> >>> For me this looks just code shuffling. >>> >>> I don't disagree but changing working code without actual semantical >>> reasons neither makes sense. >>> >>> BR, Jarkko >>> >> >> Well the semantical reason for this change is that the check for irq test completion >> only has to be done once for the driver livetime. There is no point in doing it >> over and over again for each transmission. >> So the code is not simply shuffled around, it is shifted to a place where it is only >> executed once. >> >> This is not a bugfix but it is clearly an improvement/cleanup. As far as I understood >> from your comments on the earlier versions of this patch set cleanups are also ok as >> long as they are not intermixed with bugfixes. > > The patch does not do anything particulary useful IMHO. There's no > stimulus to do this change. > Ok, I will drop this patch in the next version of this series then. Regards, Lino
Hi guys, On Mon, 2022-05-16 at 22:25 +0200, Lino Sanfilippo wrote: > On 16.05.22 at 19:51, Jarkko Sakkinen wrote: > > On Wed, May 11, 2022 at 09:56:59PM +0200, Lino Sanfilippo wrote: > > > On 11.05.22 at 17:09, Jarkko Sakkinen wrote: > > > > On Mon, May 09, 2022 at 10:05:58AM +0200, Lino Sanfilippo wrote: > > > > > From: Lino Sanfilippo <l.sanfilippo@kunbus.com> > > > > > > > > > > There is no need to check for the irq test completion at each data > > > > > transmission during the driver livetime. Instead do the check only > > > > > once at > > > > > driver startup. > > > > > > > > > > Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com> > > > > > --- > > > > > drivers/char/tpm/tpm_tis_core.c | 68 +++++++++++--------------------- > > > > > - > > > > > 1 file changed, 22 insertions(+), 46 deletions(-) > > > > > > > > > > diff --git a/drivers/char/tpm/tpm_tis_core.c > > > > > b/drivers/char/tpm/tpm_tis_core.c > > > > > index bdfde1cd71fe..4c65718feb7d 100644 > > > > > --- a/drivers/char/tpm/tpm_tis_core.c > > > > > +++ b/drivers/char/tpm/tpm_tis_core.c > > > > > @@ -432,7 +432,7 @@ static void disable_interrupts(struct tpm_chip > > > > > *chip) > > > > > * tpm.c can skip polling for the data to be available as the > > > > > interrupt is > > > > > * waited for here > > > > > */ > > > > > -static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, > > > > > size_t len) > > > > > +static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len) > > > > > { > > > > > struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > > > > > int rc; > > > > > @@ -465,30 +465,6 @@ static int tpm_tis_send_main(struct tpm_chip > > > > > *chip, const u8 *buf, size_t len) > > > > > return rc; > > > > > } > > > > > > > > > > -static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len) > > > > > -{ > > > > > - int rc, irq; > > > > > - struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > > > > > - > > > > > - if (!(chip->flags & TPM_CHIP_FLAG_IRQ) || > > > > > - test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags)) > > > > > - return tpm_tis_send_main(chip, buf, len); > > > > > - > > > > > - /* Verify receipt of the expected IRQ */ > > > > > - irq = priv->irq; > > > > > - priv->irq = 0; > > > > > - chip->flags &= ~TPM_CHIP_FLAG_IRQ; > > > > > - rc = tpm_tis_send_main(chip, buf, len); > > > > > - priv->irq = irq; > > > > > - chip->flags |= TPM_CHIP_FLAG_IRQ; > > > > > - if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags)) > > > > > - tpm_msleep(1); > > > > > - if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags)) > > > > > - disable_interrupts(chip); > > > > > - set_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags); > > > > > - return rc; > > > > > -} > > > > > - > > > > > struct tis_vendor_durations_override { > > > > > u32 did_vid; > > > > > struct tpm1_version version; > > > > > @@ -759,51 +735,54 @@ static int tpm_tis_probe_irq_single(struct > > > > > tpm_chip *chip, u32 intmask, > > > > > > > > > > rc = tpm_tis_read8(priv, TPM_INT_VECTOR(priv->locality), > > > > > &original_int_vec); > > > > > - if (rc < 0) > > > > > + if (rc < 0) { > > > > > + disable_interrupts(chip); > > > > > return rc; > > > > > + } > > > > > > > > > > rc = tpm_tis_write8(priv, TPM_INT_VECTOR(priv->locality), > > > > > irq); > > > > > if (rc < 0) > > > > > - return rc; > > > > > + goto out_err; > > > > > > > > > > rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), > > > > > &int_status); > > > > > if (rc < 0) > > > > > - return rc; > > > > > + goto out_err; > > > > > > > > > > /* Clear all existing */ > > > > > rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), > > > > > int_status); > > > > > if (rc < 0) > > > > > - return rc; > > > > > + goto out_err; > > > > > > > > > > /* Turn on */ > > > > > rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), > > > > > intmask | TPM_GLOBAL_INT_ENABLE); > > > > > if (rc < 0) > > > > > - return rc; > > > > > + goto out_err; > > > > > > > > > > clear_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags); > > > > > - chip->flags |= TPM_CHIP_FLAG_IRQ; > > > > > > > > > > /* Generate an interrupt by having the core call through to > > > > > * tpm_tis_send > > > > > */ > > > > > rc = tpm_tis_gen_interrupt(chip); > > > > > if (rc < 0) > > > > > - return rc; > > > > > + goto out_err; > > > > > > > > > > - /* tpm_tis_send will either confirm the interrupt is working > > > > > or it > > > > > - * will call disable_irq which undoes all of the above. > > > > > - */ > > > > > - if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) { > > > > > - rc = tpm_tis_write8(priv, original_int_vec, > > > > > - TPM_INT_VECTOR(priv->locality)); > > > > > - if (rc < 0) > > > > > - return rc; > > > > > + tpm_msleep(1); > > > > > > > > > > - return 1; > > > > > - } > > > > > + /* Verify receipt of the expected IRQ */ > > > > > + if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags)) > > > > > + goto out_err; > > > > > + > > > > > + chip->flags |= TPM_CHIP_FLAG_IRQ; > > > > > > > > > > return 0; > > > > > + > > > > > +out_err: > > > > Rename this as just 'err'. > > > > > > > + disable_interrupts(chip); > > > > > + tpm_tis_write8(priv, original_int_vec, TPM_INT_VECTOR(priv- > > > > > >locality)); > > > > > + > > > > > + return rc; > > > > > } > > > > > > > > > > /* Try to find the IRQ the TPM is using. This is for legacy x86 > > > > > systems that > > > > > @@ -1075,12 +1054,9 @@ int tpm_tis_core_init(struct device *dev, > > > > > struct tpm_tis_data *priv, int irq, > > > > > if (irq) { > > > > > tpm_tis_probe_irq_single(chip, intmask, > > > > > IRQF_SHARED, > > > > > irq); > > > > > - if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) { > > > > > + if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) > > > > > dev_err(&chip->dev, FW_BUG > > > > > "TPM interrupt not working, > > > > > polling instead\n"); > > > > > - > > > > > - disable_interrupts(chip); > > > > > - } > > > > > } else { > > > > > tpm_tis_probe_irq(chip, intmask); > > > > > } > > > > > -- > > > > > 2.36.0 > > > > > > > > > > > > > For me this looks just code shuffling. > > > > > > > > I don't disagree but changing working code without actual semantical > > > > reasons neither makes sense. > > > > > > > > BR, Jarkko > > > > > > > > > > Well the semantical reason for this change is that the check for irq test > > > completion > > > only has to be done once for the driver livetime. There is no point in > > > doing it > > > over and over again for each transmission. > > > So the code is not simply shuffled around, it is shifted to a place where > > > it is only > > > executed once. > > > > > > This is not a bugfix but it is clearly an improvement/cleanup. As far as I > > > understood > > > from your comments on the earlier versions of this patch set cleanups are > > > also ok as > > > long as they are not intermixed with bugfixes. > > > > The patch does not do anything particulary useful IMHO. There's no > > stimulus to do this change. > > I don't agree. IMHO preventing useless actions (like checking the interrupt again and again) *is* useful and I think it's reason enough. > > Ok, I will drop this patch in the next version of this series then. > > Regards, > Lino > Michael
On Tue, 2022-05-17 at 20:19 +0200, Michael Niewöhner wrote: > Hi guys, > > > On Mon, 2022-05-16 at 22:25 +0200, Lino Sanfilippo wrote: > > On 16.05.22 at 19:51, Jarkko Sakkinen wrote: > > > On Wed, May 11, 2022 at 09:56:59PM +0200, Lino Sanfilippo wrote: > > > > On 11.05.22 at 17:09, Jarkko Sakkinen wrote: > > > > > On Mon, May 09, 2022 at 10:05:58AM +0200, Lino Sanfilippo wrote: > > > > > > From: Lino Sanfilippo <l.sanfilippo@kunbus.com> > > > > > > > > > > > > There is no need to check for the irq test completion at each data > > > > > > transmission during the driver livetime. Instead do the check only > > > > > > once at > > > > > > driver startup. > > > > > > > > > > > > Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com> > > > > > > --- > > > > > > drivers/char/tpm/tpm_tis_core.c | 68 +++++++++++--------------------- > > > > > > - > > > > > > 1 file changed, 22 insertions(+), 46 deletions(-) > > > > > > > > > > > > diff --git a/drivers/char/tpm/tpm_tis_core.c > > > > > > b/drivers/char/tpm/tpm_tis_core.c > > > > > > index bdfde1cd71fe..4c65718feb7d 100644 > > > > > > --- a/drivers/char/tpm/tpm_tis_core.c > > > > > > +++ b/drivers/char/tpm/tpm_tis_core.c > > > > > > @@ -432,7 +432,7 @@ static void disable_interrupts(struct tpm_chip > > > > > > *chip) > > > > > > * tpm.c can skip polling for the data to be available as the > > > > > > interrupt is > > > > > > * waited for here > > > > > > */ > > > > > > -static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, > > > > > > size_t len) > > > > > > +static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len) > > > > > > { > > > > > > struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > > > > > > int rc; > > > > > > @@ -465,30 +465,6 @@ static int tpm_tis_send_main(struct tpm_chip > > > > > > *chip, const u8 *buf, size_t len) > > > > > > return rc; > > > > > > } > > > > > > > > > > > > -static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len) > > > > > > -{ > > > > > > - int rc, irq; > > > > > > - struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > > > > > > - > > > > > > - if (!(chip->flags & TPM_CHIP_FLAG_IRQ) || > > > > > > - test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags)) > > > > > > - return tpm_tis_send_main(chip, buf, len); > > > > > > - > > > > > > - /* Verify receipt of the expected IRQ */ > > > > > > - irq = priv->irq; > > > > > > - priv->irq = 0; > > > > > > - chip->flags &= ~TPM_CHIP_FLAG_IRQ; > > > > > > - rc = tpm_tis_send_main(chip, buf, len); > > > > > > - priv->irq = irq; > > > > > > - chip->flags |= TPM_CHIP_FLAG_IRQ; > > > > > > - if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags)) > > > > > > - tpm_msleep(1); > > > > > > - if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags)) > > > > > > - disable_interrupts(chip); > > > > > > - set_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags); > > > > > > - return rc; > > > > > > -} > > > > > > - > > > > > > struct tis_vendor_durations_override { > > > > > > u32 did_vid; > > > > > > struct tpm1_version version; > > > > > > @@ -759,51 +735,54 @@ static int tpm_tis_probe_irq_single(struct > > > > > > tpm_chip *chip, u32 intmask, > > > > > > > > > > > > rc = tpm_tis_read8(priv, TPM_INT_VECTOR(priv->locality), > > > > > > &original_int_vec); > > > > > > - if (rc < 0) > > > > > > + if (rc < 0) { > > > > > > + disable_interrupts(chip); > > > > > > return rc; > > > > > > + } > > > > > > > > > > > > rc = tpm_tis_write8(priv, TPM_INT_VECTOR(priv->locality), > > > > > > irq); > > > > > > if (rc < 0) > > > > > > - return rc; > > > > > > + goto out_err; > > > > > > > > > > > > rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), > > > > > > &int_status); > > > > > > if (rc < 0) > > > > > > - return rc; > > > > > > + goto out_err; > > > > > > > > > > > > /* Clear all existing */ > > > > > > rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), > > > > > > int_status); > > > > > > if (rc < 0) > > > > > > - return rc; > > > > > > + goto out_err; > > > > > > > > > > > > /* Turn on */ > > > > > > rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), > > > > > > intmask | TPM_GLOBAL_INT_ENABLE); > > > > > > if (rc < 0) > > > > > > - return rc; > > > > > > + goto out_err; > > > > > > > > > > > > clear_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags); > > > > > > - chip->flags |= TPM_CHIP_FLAG_IRQ; > > > > > > > > > > > > /* Generate an interrupt by having the core call through to > > > > > > * tpm_tis_send > > > > > > */ > > > > > > rc = tpm_tis_gen_interrupt(chip); > > > > > > if (rc < 0) > > > > > > - return rc; > > > > > > + goto out_err; > > > > > > > > > > > > - /* tpm_tis_send will either confirm the interrupt is working > > > > > > or it > > > > > > - * will call disable_irq which undoes all of the above. > > > > > > - */ > > > > > > - if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) { > > > > > > - rc = tpm_tis_write8(priv, original_int_vec, > > > > > > - TPM_INT_VECTOR(priv->locality)); > > > > > > - if (rc < 0) > > > > > > - return rc; > > > > > > + tpm_msleep(1); > > > > > > > > > > > > - return 1; > > > > > > - } > > > > > > + /* Verify receipt of the expected IRQ */ > > > > > > + if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags)) > > > > > > + goto out_err; > > > > > > + > > > > > > + chip->flags |= TPM_CHIP_FLAG_IRQ; > > > > > > > > > > > > return 0; > > > > > > + > > > > > > +out_err: > > > > > > Rename this as just 'err'. > > > > > > > > > + disable_interrupts(chip); > > > > > > + tpm_tis_write8(priv, original_int_vec, TPM_INT_VECTOR(priv- > > > > > > > locality)); > > > > > > + > > > > > > + return rc; > > > > > > } > > > > > > > > > > > > /* Try to find the IRQ the TPM is using. This is for legacy x86 > > > > > > systems that > > > > > > @@ -1075,12 +1054,9 @@ int tpm_tis_core_init(struct device *dev, > > > > > > struct tpm_tis_data *priv, int irq, > > > > > > if (irq) { > > > > > > tpm_tis_probe_irq_single(chip, intmask, > > > > > > IRQF_SHARED, > > > > > > irq); > > > > > > - if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) { > > > > > > + if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) > > > > > > dev_err(&chip->dev, FW_BUG > > > > > > "TPM interrupt not working, > > > > > > polling instead\n"); > > > > > > - > > > > > > - disable_interrupts(chip); > > > > > > - } > > > > > > } else { > > > > > > tpm_tis_probe_irq(chip, intmask); > > > > > > } > > > > > > -- > > > > > > 2.36.0 > > > > > > > > > > > > > > > > For me this looks just code shuffling. > > > > > > > > > > I don't disagree but changing working code without actual semantical > > > > > reasons neither makes sense. > > > > > > > > > > BR, Jarkko > > > > > > > > > > > > > Well the semantical reason for this change is that the check for irq test > > > > completion > > > > only has to be done once for the driver livetime. There is no point in > > > > doing it > > > > over and over again for each transmission. > > > > So the code is not simply shuffled around, it is shifted to a place where > > > > it is only > > > > executed once. > > > > > > > > This is not a bugfix but it is clearly an improvement/cleanup. As far as I > > > > understood > > > > from your comments on the earlier versions of this patch set cleanups are > > > > also ok as > > > > long as they are not intermixed with bugfixes. > > > > > > The patch does not do anything particulary useful IMHO. There's no > > > stimulus to do this change. > > > > > I don't agree. IMHO preventing useless actions (like checking the interrupt > again and again) *is* useful and I think it's reason enough. Show me the test data to back this up. BR, Jarkko
On Wed, 2022-05-18 at 04:26 +0300, Jarkko Sakkinen wrote: > On Tue, 2022-05-17 at 20:19 +0200, Michael Niewöhner wrote: > > Hi guys, > > > > > > On Mon, 2022-05-16 at 22:25 +0200, Lino Sanfilippo wrote: > > > On 16.05.22 at 19:51, Jarkko Sakkinen wrote: > > > > On Wed, May 11, 2022 at 09:56:59PM +0200, Lino Sanfilippo wrote: > > > > > On 11.05.22 at 17:09, Jarkko Sakkinen wrote: > > > > > > On Mon, May 09, 2022 at 10:05:58AM +0200, Lino Sanfilippo wrote: > > > > > > > From: Lino Sanfilippo <l.sanfilippo@kunbus.com> > > > > > > > > > > > > > > There is no need to check for the irq test completion at each data > > > > > > > transmission during the driver livetime. Instead do the check only > > > > > > > once at > > > > > > > driver startup. > > > > > > > > > > > > > > Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com> > > > > > > > --- > > > > > > > drivers/char/tpm/tpm_tis_core.c | 68 +++++++++++----------------- > > > > > > > ---- > > > > > > > - > > > > > > > 1 file changed, 22 insertions(+), 46 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/char/tpm/tpm_tis_core.c > > > > > > > b/drivers/char/tpm/tpm_tis_core.c > > > > > > > index bdfde1cd71fe..4c65718feb7d 100644 > > > > > > > --- a/drivers/char/tpm/tpm_tis_core.c > > > > > > > +++ b/drivers/char/tpm/tpm_tis_core.c > > > > > > > @@ -432,7 +432,7 @@ static void disable_interrupts(struct tpm_chip > > > > > > > *chip) > > > > > > > * tpm.c can skip polling for the data to be available as the > > > > > > > interrupt is > > > > > > > * waited for here > > > > > > > */ > > > > > > > -static int tpm_tis_send_main(struct tpm_chip *chip, const u8 > > > > > > > *buf, > > > > > > > size_t len) > > > > > > > +static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t > > > > > > > len) > > > > > > > { > > > > > > > struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > > > > > > > int rc; > > > > > > > @@ -465,30 +465,6 @@ static int tpm_tis_send_main(struct tpm_chip > > > > > > > *chip, const u8 *buf, size_t len) > > > > > > > return rc; > > > > > > > } > > > > > > > > > > > > > > -static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t > > > > > > > len) > > > > > > > -{ > > > > > > > - int rc, irq; > > > > > > > - struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > > > > > > > - > > > > > > > - if (!(chip->flags & TPM_CHIP_FLAG_IRQ) || > > > > > > > - test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags)) > > > > > > > - return tpm_tis_send_main(chip, buf, len); > > > > > > > - > > > > > > > - /* Verify receipt of the expected IRQ */ > > > > > > > - irq = priv->irq; > > > > > > > - priv->irq = 0; > > > > > > > - chip->flags &= ~TPM_CHIP_FLAG_IRQ; > > > > > > > - rc = tpm_tis_send_main(chip, buf, len); > > > > > > > - priv->irq = irq; > > > > > > > - chip->flags |= TPM_CHIP_FLAG_IRQ; > > > > > > > - if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags)) > > > > > > > - tpm_msleep(1); > > > > > > > - if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags)) > > > > > > > - disable_interrupts(chip); > > > > > > > - set_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags); > > > > > > > - return rc; > > > > > > > -} > > > > > > > - > > > > > > > struct tis_vendor_durations_override { > > > > > > > u32 did_vid; > > > > > > > struct tpm1_version version; > > > > > > > @@ -759,51 +735,54 @@ static int tpm_tis_probe_irq_single(struct > > > > > > > tpm_chip *chip, u32 intmask, > > > > > > > > > > > > > > rc = tpm_tis_read8(priv, TPM_INT_VECTOR(priv->locality), > > > > > > > &original_int_vec); > > > > > > > - if (rc < 0) > > > > > > > + if (rc < 0) { > > > > > > > + disable_interrupts(chip); > > > > > > > return rc; > > > > > > > + } > > > > > > > > > > > > > > rc = tpm_tis_write8(priv, TPM_INT_VECTOR(priv->locality), > > > > > > > irq); > > > > > > > if (rc < 0) > > > > > > > - return rc; > > > > > > > + goto out_err; > > > > > > > > > > > > > > rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), > > > > > > > &int_status); > > > > > > > if (rc < 0) > > > > > > > - return rc; > > > > > > > + goto out_err; > > > > > > > > > > > > > > /* Clear all existing */ > > > > > > > rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), > > > > > > > int_status); > > > > > > > if (rc < 0) > > > > > > > - return rc; > > > > > > > + goto out_err; > > > > > > > > > > > > > > /* Turn on */ > > > > > > > rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), > > > > > > > intmask | TPM_GLOBAL_INT_ENABLE); > > > > > > > if (rc < 0) > > > > > > > - return rc; > > > > > > > + goto out_err; > > > > > > > > > > > > > > clear_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags); > > > > > > > - chip->flags |= TPM_CHIP_FLAG_IRQ; > > > > > > > > > > > > > > /* Generate an interrupt by having the core call through > > > > > > > to > > > > > > > * tpm_tis_send > > > > > > > */ > > > > > > > rc = tpm_tis_gen_interrupt(chip); > > > > > > > if (rc < 0) > > > > > > > - return rc; > > > > > > > + goto out_err; > > > > > > > > > > > > > > - /* tpm_tis_send will either confirm the interrupt is > > > > > > > working > > > > > > > or it > > > > > > > - * will call disable_irq which undoes all of the above. > > > > > > > - */ > > > > > > > - if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) { > > > > > > > - rc = tpm_tis_write8(priv, original_int_vec, > > > > > > > - TPM_INT_VECTOR(priv->locality)); > > > > > > > - if (rc < 0) > > > > > > > - return rc; > > > > > > > + tpm_msleep(1); > > > > > > > > > > > > > > - return 1; > > > > > > > - } > > > > > > > + /* Verify receipt of the expected IRQ */ > > > > > > > + if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags)) > > > > > > > + goto out_err; > > > > > > > + > > > > > > > + chip->flags |= TPM_CHIP_FLAG_IRQ; > > > > > > > > > > > > > > return 0; > > > > > > > + > > > > > > > +out_err: > > > > > > > > Rename this as just 'err'. > > > > > > > > > > > + disable_interrupts(chip); > > > > > > > + tpm_tis_write8(priv, original_int_vec, > > > > > > > TPM_INT_VECTOR(priv- > > > > > > > > locality)); > > > > > > > + > > > > > > > + return rc; > > > > > > > } > > > > > > > > > > > > > > /* Try to find the IRQ the TPM is using. This is for legacy x86 > > > > > > > systems that > > > > > > > @@ -1075,12 +1054,9 @@ int tpm_tis_core_init(struct device *dev, > > > > > > > struct tpm_tis_data *priv, int irq, > > > > > > > if (irq) { > > > > > > > tpm_tis_probe_irq_single(chip, intmask, > > > > > > > IRQF_SHARED, > > > > > > > irq); > > > > > > > - if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) { > > > > > > > + if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) > > > > > > > dev_err(&chip->dev, FW_BUG > > > > > > > "TPM interrupt not > > > > > > > working, > > > > > > > polling instead\n"); > > > > > > > - > > > > > > > - disable_interrupts(chip); > > > > > > > - } > > > > > > > } else { > > > > > > > tpm_tis_probe_irq(chip, intmask); > > > > > > > } > > > > > > > -- > > > > > > > 2.36.0 > > > > > > > > > > > > > > > > > > > For me this looks just code shuffling. > > > > > > > > > > > > I don't disagree but changing working code without actual semantical > > > > > > reasons neither makes sense. > > > > > > > > > > > > BR, Jarkko > > > > > > > > > > > > > > > > Well the semantical reason for this change is that the check for irq > > > > > test > > > > > completion > > > > > only has to be done once for the driver livetime. There is no point in > > > > > doing it > > > > > over and over again for each transmission. > > > > > So the code is not simply shuffled around, it is shifted to a place > > > > > where > > > > > it is only > > > > > executed once. > > > > > > > > > > This is not a bugfix but it is clearly an improvement/cleanup. As far > > > > > as I > > > > > understood > > > > > from your comments on the earlier versions of this patch set cleanups > > > > > are > > > > > also ok as > > > > > long as they are not intermixed with bugfixes. > > > > > > > > The patch does not do anything particulary useful IMHO. There's no > > > > stimulus to do this change. > > > > > > > > I don't agree. IMHO preventing useless actions (like checking the interrupt > > again and again) *is* useful and I think it's reason enough. > > Show me the test data to back this up. Why do you need test data as an argument for not doing useless actions? o.O > > BR, Jarkko
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c index bdfde1cd71fe..4c65718feb7d 100644 --- a/drivers/char/tpm/tpm_tis_core.c +++ b/drivers/char/tpm/tpm_tis_core.c @@ -432,7 +432,7 @@ static void disable_interrupts(struct tpm_chip *chip) * tpm.c can skip polling for the data to be available as the interrupt is * waited for here */ -static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, size_t len) +static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len) { struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); int rc; @@ -465,30 +465,6 @@ static int tpm_tis_send_main(struct tpm_chip *chip, const u8 *buf, size_t len) return rc; } -static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len) -{ - int rc, irq; - struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); - - if (!(chip->flags & TPM_CHIP_FLAG_IRQ) || - test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags)) - return tpm_tis_send_main(chip, buf, len); - - /* Verify receipt of the expected IRQ */ - irq = priv->irq; - priv->irq = 0; - chip->flags &= ~TPM_CHIP_FLAG_IRQ; - rc = tpm_tis_send_main(chip, buf, len); - priv->irq = irq; - chip->flags |= TPM_CHIP_FLAG_IRQ; - if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags)) - tpm_msleep(1); - if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags)) - disable_interrupts(chip); - set_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags); - return rc; -} - struct tis_vendor_durations_override { u32 did_vid; struct tpm1_version version; @@ -759,51 +735,54 @@ static int tpm_tis_probe_irq_single(struct tpm_chip *chip, u32 intmask, rc = tpm_tis_read8(priv, TPM_INT_VECTOR(priv->locality), &original_int_vec); - if (rc < 0) + if (rc < 0) { + disable_interrupts(chip); return rc; + } rc = tpm_tis_write8(priv, TPM_INT_VECTOR(priv->locality), irq); if (rc < 0) - return rc; + goto out_err; rc = tpm_tis_read32(priv, TPM_INT_STATUS(priv->locality), &int_status); if (rc < 0) - return rc; + goto out_err; /* Clear all existing */ rc = tpm_tis_write32(priv, TPM_INT_STATUS(priv->locality), int_status); if (rc < 0) - return rc; + goto out_err; /* Turn on */ rc = tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask | TPM_GLOBAL_INT_ENABLE); if (rc < 0) - return rc; + goto out_err; clear_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags); - chip->flags |= TPM_CHIP_FLAG_IRQ; /* Generate an interrupt by having the core call through to * tpm_tis_send */ rc = tpm_tis_gen_interrupt(chip); if (rc < 0) - return rc; + goto out_err; - /* tpm_tis_send will either confirm the interrupt is working or it - * will call disable_irq which undoes all of the above. - */ - if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) { - rc = tpm_tis_write8(priv, original_int_vec, - TPM_INT_VECTOR(priv->locality)); - if (rc < 0) - return rc; + tpm_msleep(1); - return 1; - } + /* Verify receipt of the expected IRQ */ + if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags)) + goto out_err; + + chip->flags |= TPM_CHIP_FLAG_IRQ; return 0; + +out_err: + disable_interrupts(chip); + tpm_tis_write8(priv, original_int_vec, TPM_INT_VECTOR(priv->locality)); + + return rc; } /* Try to find the IRQ the TPM is using. This is for legacy x86 systems that @@ -1075,12 +1054,9 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq, if (irq) { tpm_tis_probe_irq_single(chip, intmask, IRQF_SHARED, irq); - if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) { + if (!(chip->flags & TPM_CHIP_FLAG_IRQ)) dev_err(&chip->dev, FW_BUG "TPM interrupt not working, polling instead\n"); - - disable_interrupts(chip); - } } else { tpm_tis_probe_irq(chip, intmask); }