TPM chip bootstrapping was removed from tpm_chip_register(), and it
was relocated to tpm_tis_core. This breaks all drivers which are not
based on tpm_tis because the chip will not get properly initialized.
Take the corrective steps:
1. Rename tpm_chip_startup() as tpm_chip_bootstrap() and make it one-shot.
2. Call tpm_chip_bootstrap() in tpm_chip_register(), which reverts the
things as tehy used to be.
Cc: Lino Sanfilippo <[email protected]>
Reported-by: Pengfei Xu <[email protected]>
Link: https://lore.kernel.org/all/[email protected]/
Fixes: 548eb516ec0f ("tpm, tpm_tis: startup chip before testing for interrupts")
Signed-off-by: Jarkko Sakkinen <[email protected]>
---
drivers/char/tpm/tpm-chip.c | 22 +++++++++++++++++++---
drivers/char/tpm/tpm.h | 2 +-
drivers/char/tpm/tpm_tis_core.c | 2 +-
include/linux/tpm.h | 13 +++++++------
4 files changed, 28 insertions(+), 11 deletions(-)
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 6fdfa65a00c3..b8a484b4ff7a 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -606,13 +606,19 @@ static int tpm_get_pcr_allocation(struct tpm_chip *chip)
}
/*
- * tpm_chip_startup() - performs auto startup and allocates the PCRs
+ * tpm_chip_bootstrap() - Boostrap TPM chip after power on
* @chip: TPM chip to use.
+ *
+ * Initialize TPM chip after power on. This a one-shot function: subsequent
+ * calls will have no effect.
*/
-int tpm_chip_startup(struct tpm_chip *chip)
+int tpm_chip_bootstrap(struct tpm_chip *chip)
{
int rc;
+ if (chip->flags & TPM_CHIP_FLAG_BOOTSTRAP)
+ return 0;
+
rc = tpm_chip_start(chip);
if (rc)
return rc;
@@ -625,9 +631,15 @@ int tpm_chip_startup(struct tpm_chip *chip)
stop:
tpm_chip_stop(chip);
+ /*
+ * Unconditionally set, as driver initialization should cease, when the
+ * boostrapping process fails.
+ */
+ chip->flags |= TPM_CHIP_FLAG_BOOTSTRAP;
+
return rc;
}
-EXPORT_SYMBOL_GPL(tpm_chip_startup);
+EXPORT_SYMBOL_GPL(tpm_chip_bootstrap);
/*
* tpm_chip_register() - create a character device for the TPM chip
@@ -644,6 +656,10 @@ int tpm_chip_register(struct tpm_chip *chip)
{
int rc;
+ rc = tpm_chip_bootstrap(chip);
+ if (rc)
+ return rc;
+
tpm_sysfs_add_device(chip);
tpm_bios_log_setup(chip);
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 88d3bd76e076..f6c99b3f0045 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -263,7 +263,7 @@ static inline void tpm_msleep(unsigned int delay_msec)
delay_msec * 1000);
};
-int tpm_chip_startup(struct tpm_chip *chip);
+int tpm_chip_bootstrap(struct tpm_chip *chip);
int tpm_chip_start(struct tpm_chip *chip);
void tpm_chip_stop(struct tpm_chip *chip);
struct tpm_chip *tpm_find_get_ops(struct tpm_chip *chip);
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index c2421162cf34..02945d53fcef 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -1139,7 +1139,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
init_waitqueue_head(&priv->read_queue);
init_waitqueue_head(&priv->int_queue);
- rc = tpm_chip_startup(chip);
+ rc = tpm_chip_bootstrap(chip);
if (rc)
goto out_err;
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 4dc97b9f65fb..50bcdee19b12 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -274,13 +274,14 @@ enum tpm2_cc_attrs {
#define TPM_VID_ATML 0x1114
enum tpm_chip_flags {
- TPM_CHIP_FLAG_TPM2 = BIT(1),
- TPM_CHIP_FLAG_IRQ = BIT(2),
- TPM_CHIP_FLAG_VIRTUAL = BIT(3),
- TPM_CHIP_FLAG_HAVE_TIMEOUTS = BIT(4),
- TPM_CHIP_FLAG_ALWAYS_POWERED = BIT(5),
+ TPM_CHIP_FLAG_TPM2 = BIT(1),
+ TPM_CHIP_FLAG_IRQ = BIT(2),
+ TPM_CHIP_FLAG_VIRTUAL = BIT(3),
+ TPM_CHIP_FLAG_HAVE_TIMEOUTS = BIT(4),
+ TPM_CHIP_FLAG_ALWAYS_POWERED = BIT(5),
TPM_CHIP_FLAG_FIRMWARE_POWER_MANAGED = BIT(6),
- TPM_CHIP_FLAG_FIRMWARE_UPGRADE = BIT(7),
+ TPM_CHIP_FLAG_FIRMWARE_UPGRADE = BIT(7),
+ TPM_CHIP_FLAG_BOOTSTRAP = BIT(8),
};
#define to_tpm_chip(d) container_of(d, struct tpm_chip, dev)
--
2.39.2
On 2023-04-26 at 21:58:33 +0300, Jarkko Sakkinen wrote:
> TPM chip bootstrapping was removed from tpm_chip_register(), and it
> was relocated to tpm_tis_core. This breaks all drivers which are not
> based on tpm_tis because the chip will not get properly initialized.
>
> Take the corrective steps:
> 1. Rename tpm_chip_startup() as tpm_chip_bootstrap() and make it one-shot.
> 2. Call tpm_chip_bootstrap() in tpm_chip_register(), which reverts the
> things as tehy used to be.
^ two space and one typo "they"
Thanks for your fixed patch in short time!
And I tested this patch, it could not be reproduced in 155s, and the patch
fixed this issue.
Fixed dmesg is in attached.
Tested-by: Pengfei Xu <[email protected]>
Thanks!
BR.
>
> Cc: Lino Sanfilippo <[email protected]>
> Reported-by: Pengfei Xu <[email protected]>
> Link: https://lore.kernel.org/all/[email protected]/
> Fixes: 548eb516ec0f ("tpm, tpm_tis: startup chip before testing for interrupts")
> Signed-off-by: Jarkko Sakkinen <[email protected]>
> ---
> drivers/char/tpm/tpm-chip.c | 22 +++++++++++++++++++---
> drivers/char/tpm/tpm.h | 2 +-
> drivers/char/tpm/tpm_tis_core.c | 2 +-
> include/linux/tpm.h | 13 +++++++------
> 4 files changed, 28 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 6fdfa65a00c3..b8a484b4ff7a 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -606,13 +606,19 @@ static int tpm_get_pcr_allocation(struct tpm_chip *chip)
> }
>
> /*
> - * tpm_chip_startup() - performs auto startup and allocates the PCRs
> + * tpm_chip_bootstrap() - Boostrap TPM chip after power on
> * @chip: TPM chip to use.
> + *
> + * Initialize TPM chip after power on. This a one-shot function: subsequent
> + * calls will have no effect.
> */
> -int tpm_chip_startup(struct tpm_chip *chip)
> +int tpm_chip_bootstrap(struct tpm_chip *chip)
> {
> int rc;
>
> + if (chip->flags & TPM_CHIP_FLAG_BOOTSTRAP)
> + return 0;
> +
> rc = tpm_chip_start(chip);
> if (rc)
> return rc;
> @@ -625,9 +631,15 @@ int tpm_chip_startup(struct tpm_chip *chip)
> stop:
> tpm_chip_stop(chip);
>
> + /*
> + * Unconditionally set, as driver initialization should cease, when the
> + * boostrapping process fails.
> + */
> + chip->flags |= TPM_CHIP_FLAG_BOOTSTRAP;
> +
> return rc;
> }
> -EXPORT_SYMBOL_GPL(tpm_chip_startup);
> +EXPORT_SYMBOL_GPL(tpm_chip_bootstrap);
>
> /*
> * tpm_chip_register() - create a character device for the TPM chip
> @@ -644,6 +656,10 @@ int tpm_chip_register(struct tpm_chip *chip)
> {
> int rc;
>
> + rc = tpm_chip_bootstrap(chip);
> + if (rc)
> + return rc;
> +
> tpm_sysfs_add_device(chip);
>
> tpm_bios_log_setup(chip);
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 88d3bd76e076..f6c99b3f0045 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -263,7 +263,7 @@ static inline void tpm_msleep(unsigned int delay_msec)
> delay_msec * 1000);
> };
>
> -int tpm_chip_startup(struct tpm_chip *chip);
> +int tpm_chip_bootstrap(struct tpm_chip *chip);
> int tpm_chip_start(struct tpm_chip *chip);
> void tpm_chip_stop(struct tpm_chip *chip);
> struct tpm_chip *tpm_find_get_ops(struct tpm_chip *chip);
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index c2421162cf34..02945d53fcef 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -1139,7 +1139,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
> init_waitqueue_head(&priv->read_queue);
> init_waitqueue_head(&priv->int_queue);
>
> - rc = tpm_chip_startup(chip);
> + rc = tpm_chip_bootstrap(chip);
> if (rc)
> goto out_err;
>
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 4dc97b9f65fb..50bcdee19b12 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -274,13 +274,14 @@ enum tpm2_cc_attrs {
> #define TPM_VID_ATML 0x1114
>
> enum tpm_chip_flags {
> - TPM_CHIP_FLAG_TPM2 = BIT(1),
> - TPM_CHIP_FLAG_IRQ = BIT(2),
> - TPM_CHIP_FLAG_VIRTUAL = BIT(3),
> - TPM_CHIP_FLAG_HAVE_TIMEOUTS = BIT(4),
> - TPM_CHIP_FLAG_ALWAYS_POWERED = BIT(5),
> + TPM_CHIP_FLAG_TPM2 = BIT(1),
> + TPM_CHIP_FLAG_IRQ = BIT(2),
> + TPM_CHIP_FLAG_VIRTUAL = BIT(3),
> + TPM_CHIP_FLAG_HAVE_TIMEOUTS = BIT(4),
> + TPM_CHIP_FLAG_ALWAYS_POWERED = BIT(5),
> TPM_CHIP_FLAG_FIRMWARE_POWER_MANAGED = BIT(6),
> - TPM_CHIP_FLAG_FIRMWARE_UPGRADE = BIT(7),
> + TPM_CHIP_FLAG_FIRMWARE_UPGRADE = BIT(7),
> + TPM_CHIP_FLAG_BOOTSTRAP = BIT(8),
> };
>
> #define to_tpm_chip(d) container_of(d, struct tpm_chip, dev)
> --
> 2.39.2
>
On Thu, 2023-04-27 at 09:58 +0800, Pengfei Xu wrote:
> On 2023-04-26 at 21:58:33 +0300, Jarkko Sakkinen wrote:
> > TPM chip bootstrapping was removed from tpm_chip_register(), and it
> > was relocated to tpm_tis_core. This breaks all drivers which are not
> > based on tpm_tis because the chip will not get properly initialized.
> >
> > Take the corrective steps:
> > 1. Rename tpm_chip_startup() as tpm_chip_bootstrap() and make it one-shot.
> > 2. Call tpm_chip_bootstrap() in tpm_chip_register(), which reverts the
> > things as tehy used to be.
> ^ two space and one typo "they"
> Thanks for your fixed patch in short time!
>
> And I tested this patch, it could not be reproduced in 155s, and the patch
> fixed this issue.
> Fixed dmesg is in attached.
>
> Tested-by: Pengfei Xu <[email protected]>
Thanks a lot! I'll take immediate action to send PR to Linus.
Lino: no worries. It was expected that such a large refactorization
could break a thing or two. You did a good job in any case.
BR, Jarkko
On Thu, 2023-04-27 at 13:52 +0300, Jarkko Sakkinen wrote:
> On Thu, 2023-04-27 at 09:58 +0800, Pengfei Xu wrote:
> > On 2023-04-26 at 21:58:33 +0300, Jarkko Sakkinen wrote:
> > > TPM chip bootstrapping was removed from tpm_chip_register(), and it
> > > was relocated to tpm_tis_core. This breaks all drivers which are not
> > > based on tpm_tis because the chip will not get properly initialized.
> > >
> > > Take the corrective steps:
> > > 1. Rename tpm_chip_startup() as tpm_chip_bootstrap() and make it one-shot.
> > > 2. Call tpm_chip_bootstrap() in tpm_chip_register(), which reverts the
> > > things as tehy used to be.
> > ^ two space and one typo "they"
> > Thanks for your fixed patch in short time!
> >
> > And I tested this patch, it could not be reproduced in 155s, and the patch
> > fixed this issue.
> > Fixed dmesg is in attached.
> >
> > Tested-by: Pengfei Xu <[email protected]>
>
> Thanks a lot! I'll take immediate action to send PR to Linus.
>
> Lino: no worries. It was expected that such a large refactorization
> could break a thing or two. You did a good job in any case.
Done: https://lkml.org/lkml/2023/4/27/297
BR, Jarkko
Hi Jarkko,
On 27.04.23 12:52, Jarkko Sakkinen wrote:
> ATTENTION: This e-mail is from an external sender. Please check attachments and links before opening e.g. with mouseover.
>
>
> On Thu, 2023-04-27 at 09:58 +0800, Pengfei Xu wrote:
>> On 2023-04-26 at 21:58:33 +0300, Jarkko Sakkinen wrote:
>>> TPM chip bootstrapping was removed from tpm_chip_register(), and it
>>> was relocated to tpm_tis_core. This breaks all drivers which are not
>>> based on tpm_tis because the chip will not get properly initialized.
>>>
>>> Take the corrective steps:
>>> 1. Rename tpm_chip_startup() as tpm_chip_bootstrap() and make it one-shot.
>>> 2. Call tpm_chip_bootstrap() in tpm_chip_register(), which reverts the
>>> things as tehy used to be.
>> ^ two space and one typo "they"
>> Thanks for your fixed patch in short time!
>>
>> And I tested this patch, it could not be reproduced in 155s, and the patch
>> fixed this issue.
>> Fixed dmesg is in attached.
>>
>> Tested-by: Pengfei Xu <[email protected]>
>
> Thanks a lot! I'll take immediate action to send PR to Linus.
>
> Lino: no worries. It was expected that such a large refactorization
> could break a thing or two. You did a good job in any case.
>
> BR, Jarkko
I guess you are right, there will probably be more issues showing up
in the next time. I will try my best to help fixing them.
Thank you very much (also for fixing this issue)!
Best regards,
Lino
On Thu Apr 27, 2023 at 6:33 PM EEST, Lino Sanfilippo wrote:
>
> Hi Jarkko,
>
> On 27.04.23 12:52, Jarkko Sakkinen wrote:
> > ATTENTION: This e-mail is from an external sender. Please check attachments and links before opening e.g. with mouseover.
> >
> >
> > On Thu, 2023-04-27 at 09:58 +0800, Pengfei Xu wrote:
> >> On 2023-04-26 at 21:58:33 +0300, Jarkko Sakkinen wrote:
> >>> TPM chip bootstrapping was removed from tpm_chip_register(), and it
> >>> was relocated to tpm_tis_core. This breaks all drivers which are not
> >>> based on tpm_tis because the chip will not get properly initialized.
> >>>
> >>> Take the corrective steps:
> >>> 1. Rename tpm_chip_startup() as tpm_chip_bootstrap() and make it one-shot.
> >>> 2. Call tpm_chip_bootstrap() in tpm_chip_register(), which reverts the
> >>> things as tehy used to be.
> >> ^ two space and one typo "they"
> >> Thanks for your fixed patch in short time!
> >>
> >> And I tested this patch, it could not be reproduced in 155s, and the patch
> >> fixed this issue.
> >> Fixed dmesg is in attached.
> >>
> >> Tested-by: Pengfei Xu <[email protected]>
> >
> > Thanks a lot! I'll take immediate action to send PR to Linus.
> >
> > Lino: no worries. It was expected that such a large refactorization
> > could break a thing or two. You did a good job in any case.
> >
> > BR, Jarkko
>
> I guess you are right, there will probably be more issues showing up
> in the next time. I will try my best to help fixing them.
> Thank you very much (also for fixing this issue)!
Yeah, no worries, that is my responsibility in the end to take care of
whatever I accept :-) And yeah, this was complicated change to do, I
think you did your best and that is good enough as far as I'm concerned.
BR, Jarkko