| Message ID | 20230426185833.289868-1-jarkko@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tpm: Re-enable TPM chip boostrapping non-tpm_tis TPM drivers | expand |
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 <pengfei.xu@intel.com> 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 <pengfei.xu@intel.com> > > 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 <pengfei.xu@intel.com> > > 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 <pengfei.xu@intel.com> > > > > 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
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)
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 <l.sanfilippo@kunbus.com> Reported-by: Pengfei Xu <pengfei.xu@intel.com> Link: https://lore.kernel.org/all/ZEjqhwHWBnxcaRV5@xpf.sh.intel.com/ Fixes: 548eb516ec0f ("tpm, tpm_tis: startup chip before testing for interrupts") Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> --- 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(-)