On Thu, Jan 12, 2017 at 05:17:23PM -0800, James Bottomley wrote:
> On Thu, 2017-01-12 at 19:46 +0200, Jarkko Sakkinen wrote:
> > @@ -189,6 +190,12 @@ struct tpm_chip *tpm_chip_alloc(struct device
> > *pdev,
> > chip->cdev.owner = THIS_MODULE;
> > chip->cdev.kobj.parent = &chip->dev.kobj;
> >
> > + chip->work_space.context_buf = kzalloc(PAGE_SIZE,
> > GFP_KERNEL);
> > + if (!chip->work_space.context_buf) {
> > + rc = -ENOMEM;
> > + goto out;
> > + }
> > +
>
> I think the work_buf handling can be greatly simplified by making it a
> pointer to the space: it's only usable between tpm2_prepare_space() and
> tpm2_commit_space() which are protected by the chip mutex, so there's
> no need for it to exist outside of these calls (i.e. it can be NULL).
>
> Doing it this way also saves the allocation and copying overhead of
> work_space.
>
> The patch below can be folded to effect this.
>
> James
>
> ---
>
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 13cac09..770a8c0 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -131,7 +131,6 @@ static void tpm_dev_release(struct device *dev)
> mutex_unlock(&idr_lock);
>
> kfree(chip->log.bios_event_log);
> - kfree(chip->work_space.context_buf);
> kfree(chip);
> }
>
> @@ -206,12 +205,6 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
> chip->cdev.kobj.parent = &chip->dev.kobj;
> chip->cdevrm.kobj.parent = &chip->devrm.kobj;
>
> - chip->work_space.context_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
> - if (!chip->work_space.context_buf) {
> - rc = -ENOMEM;
> - goto out;
> - }
> -
> return chip;
>
> out:
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 8009ed4..adf7810 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -211,7 +211,7 @@ struct tpm_chip {
> char ppi_version[TPM_PPI_VERSION_LEN + 1];
> #endif /* CONFIG_ACPI */
>
> - struct tpm_space work_space;
> + struct tpm_space *work_space;
> u32 nr_commands;
> u32 *cc_attrs_tbl;
> };
> diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
> index 44e5501..285361e 100644
> --- a/drivers/char/tpm/tpm2-space.c
> +++ b/drivers/char/tpm/tpm2-space.c
> @@ -27,7 +27,7 @@ enum tpm2_handle_types {
>
> static void tpm2_flush_space(struct tpm_chip *chip)
> {
> - struct tpm_space *space = &chip->work_space;
> + struct tpm_space *space = chip->work_space;
> int i;
>
> for (i = 0; i < ARRAY_SIZE(space->context_tbl); i++)
> @@ -45,7 +45,7 @@ struct tpm2_context {
>
> static int tpm2_load_space(struct tpm_chip *chip)
> {
> - struct tpm_space *space = &chip->work_space;
> + struct tpm_space *space = chip->work_space;
> struct tpm2_context *ctx;
> struct tpm_buf buf;
> int i;
> @@ -99,7 +99,7 @@ static int tpm2_load_space(struct tpm_chip *chip)
>
> static int tpm2_map_command(struct tpm_chip *chip, u32 cc, u8 *cmd, size_t
> len)
> {
> - struct tpm_space *space = &chip->work_space;
> + struct tpm_space *space = chip->work_space;
> unsigned int nr_handles;
> u32 vhandle;
> u32 phandle;
> @@ -147,9 +147,7 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct
> tpm_space *space,
> if (!space)
> return 0;
>
> - memcpy(&chip->work_space.context_tbl, &space->context_tbl,
> - sizeof(space->context_tbl));
> - memcpy(chip->work_space.context_buf, space->context_buf, PAGE_SIZE);
> + chip->work_space = space;
>
> rc = tpm2_load_space(chip);
> if (rc)
> @@ -164,7 +162,7 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct
> tpm_space *space,
>
> static int tpm2_map_response(struct tpm_chip *chip, u32 cc, u8 *rsp, size_t
> len)
> {
> - struct tpm_space *space = &chip->work_space;
> + struct tpm_space *space = chip->work_space;
> u32 phandle;
> u32 vhandle;
> u32 attrs;
> @@ -222,7 +220,7 @@ static int tpm2_map_response(struct tpm_chip *chip, u32
> cc, u8 *rsp, size_t len)
>
> static int tpm2_save_space(struct tpm_chip *chip)
> {
> - struct tpm_space *space = &chip->work_space;
> + struct tpm_space *space = chip->work_space;
> struct tpm_buf buf;
> int i;
> int j;
> @@ -295,9 +293,7 @@ int tpm2_commit_space(struct tpm_chip *chip, struct
> tpm_space *space,
> if (rc)
> return rc;
>
> - memcpy(&space->context_tbl, &chip->work_space.context_tbl,
> - sizeof(space->context_tbl));
> - memcpy(space->context_buf, chip->work_space.context_buf, PAGE_SIZE);
> + chip->work_space = NULL;
>
> return 0;
> }
[x]
/Jarkko
