| Message ID | 20230113161017.1079299-2-eajames@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tpm: Add reserved memory event log | expand |
On Fri, Jan 13, 2023 at 10:10:16AM -0600, Eddie James wrote: > Since the bios event log is freed in the device release function, > let devres handle the deallocation. This will allow other memory > allocation/mapping functions to be used for the bios event log. > > Signed-off-by: Eddie James <eajames@linux.ibm.com> > --- > drivers/char/tpm/eventlog/acpi.c | 5 +++-- > drivers/char/tpm/eventlog/efi.c | 13 +++++++------ > drivers/char/tpm/eventlog/of.c | 3 ++- > drivers/char/tpm/tpm-chip.c | 1 - > 4 files changed, 12 insertions(+), 10 deletions(-) > > diff --git a/drivers/char/tpm/eventlog/acpi.c b/drivers/char/tpm/eventlog/acpi.c > index 0913d3eb8d51..40360e599bc3 100644 > --- a/drivers/char/tpm/eventlog/acpi.c > +++ b/drivers/char/tpm/eventlog/acpi.c > @@ -14,6 +14,7 @@ > * Access to the event log extended by the TCG BIOS of PC platform > */ > > +#include <linux/device.h> > #include <linux/seq_file.h> > #include <linux/fs.h> > #include <linux/security.h> > @@ -135,7 +136,7 @@ int tpm_read_log_acpi(struct tpm_chip *chip) > } > > /* malloc EventLog space */ > - log->bios_event_log = kmalloc(len, GFP_KERNEL); > + log->bios_event_log = devm_kmalloc(&chip->dev, len, GFP_KERNEL); > if (!log->bios_event_log) > return -ENOMEM; > > @@ -160,7 +161,7 @@ int tpm_read_log_acpi(struct tpm_chip *chip) > return format; > > err: > - kfree(log->bios_event_log); > + devm_kfree(&chip->dev, log->bios_event_log); I wonder do we want to do devm_kfree's at all as the memory is freed during detach, i.e. taken care by devres. > log->bios_event_log = NULL; > return ret; > } > diff --git a/drivers/char/tpm/eventlog/efi.c b/drivers/char/tpm/eventlog/efi.c > index e6cb9d525e30..4e9d7c2bf32e 100644 > --- a/drivers/char/tpm/eventlog/efi.c > +++ b/drivers/char/tpm/eventlog/efi.c > @@ -6,6 +6,7 @@ > * Thiebaud Weksteen <tweek@google.com> > */ > > +#include <linux/device.h> > #include <linux/efi.h> > #include <linux/tpm_eventlog.h> > > @@ -55,7 +56,7 @@ int tpm_read_log_efi(struct tpm_chip *chip) > } > > /* malloc EventLog space */ > - log->bios_event_log = kmemdup(log_tbl->log, log_size, GFP_KERNEL); > + log->bios_event_log = devm_kmemdup(&chip->dev, log_tbl->log, log_size, GFP_KERNEL); > if (!log->bios_event_log) { > ret = -ENOMEM; > goto out; > @@ -76,7 +77,7 @@ int tpm_read_log_efi(struct tpm_chip *chip) > MEMREMAP_WB); > if (!final_tbl) { > pr_err("Could not map UEFI TPM final log\n"); > - kfree(log->bios_event_log); > + devm_kfree(&chip->dev, log->bios_event_log); > ret = -ENOMEM; > goto out; > } > @@ -91,11 +92,11 @@ int tpm_read_log_efi(struct tpm_chip *chip) > * Allocate memory for the 'combined log' where we will append the > * 'final events log' to. > */ > - tmp = krealloc(log->bios_event_log, > - log_size + final_events_log_size, > - GFP_KERNEL); > + tmp = devm_krealloc(&chip->dev, log->bios_event_log, > + log_size + final_events_log_size, > + GFP_KERNEL); > if (!tmp) { > - kfree(log->bios_event_log); > + devm_kfree(&chip->dev, log->bios_event_log); > ret = -ENOMEM; > goto out; > } > diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c > index a9ce66d09a75..741ab2204b11 100644 > --- a/drivers/char/tpm/eventlog/of.c > +++ b/drivers/char/tpm/eventlog/of.c > @@ -10,6 +10,7 @@ > * Read the event log created by the firmware on PPC64 > */ > > +#include <linux/device.h> > #include <linux/slab.h> > #include <linux/of.h> > #include <linux/tpm_eventlog.h> > @@ -65,7 +66,7 @@ int tpm_read_log_of(struct tpm_chip *chip) > return -EIO; > } > > - log->bios_event_log = kmemdup(__va(base), size, GFP_KERNEL); > + log->bios_event_log = devm_kmemdup(&chip->dev, __va(base), size, GFP_KERNEL); > if (!log->bios_event_log) > return -ENOMEM; > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c > index 741d8f3e8fb3..b99f55f2d4fd 100644 > --- a/drivers/char/tpm/tpm-chip.c > +++ b/drivers/char/tpm/tpm-chip.c > @@ -267,7 +267,6 @@ static void tpm_dev_release(struct device *dev) > idr_remove(&dev_nums_idr, chip->dev_num); > mutex_unlock(&idr_lock); > > - kfree(chip->log.bios_event_log); > kfree(chip->work_space.context_buf); > kfree(chip->work_space.session_buf); > kfree(chip->allocated_banks); > -- > 2.31.1 > BR, Jarkko
On 1/20/23 18:11, Jarkko Sakkinen wrote: > On Fri, Jan 13, 2023 at 10:10:16AM -0600, Eddie James wrote: >> Since the bios event log is freed in the device release function, >> let devres handle the deallocation. This will allow other memory >> allocation/mapping functions to be used for the bios event log. >> >> Signed-off-by: Eddie James <eajames@linux.ibm.com> >> --- >> drivers/char/tpm/eventlog/acpi.c | 5 +++-- >> drivers/char/tpm/eventlog/efi.c | 13 +++++++------ >> drivers/char/tpm/eventlog/of.c | 3 ++- >> drivers/char/tpm/tpm-chip.c | 1 - >> 4 files changed, 12 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/char/tpm/eventlog/acpi.c b/drivers/char/tpm/eventlog/acpi.c >> index 0913d3eb8d51..40360e599bc3 100644 >> --- a/drivers/char/tpm/eventlog/acpi.c >> +++ b/drivers/char/tpm/eventlog/acpi.c >> @@ -14,6 +14,7 @@ >> * Access to the event log extended by the TCG BIOS of PC platform >> */ >> >> +#include <linux/device.h> >> #include <linux/seq_file.h> >> #include <linux/fs.h> >> #include <linux/security.h> >> @@ -135,7 +136,7 @@ int tpm_read_log_acpi(struct tpm_chip *chip) >> } >> >> /* malloc EventLog space */ >> - log->bios_event_log = kmalloc(len, GFP_KERNEL); >> + log->bios_event_log = devm_kmalloc(&chip->dev, len, GFP_KERNEL); >> if (!log->bios_event_log) >> return -ENOMEM; >> >> @@ -160,7 +161,7 @@ int tpm_read_log_acpi(struct tpm_chip *chip) >> return format; >> >> err: >> - kfree(log->bios_event_log); >> + devm_kfree(&chip->dev, log->bios_event_log); > I wonder do we want to do devm_kfree's at all as the memory is freed during > detach, i.e. taken care by devres. I think we should since the chip/tpm driver will continue to probe without the bios event log. Therefore that memory will be wasted if there is some error during bios log setup. Thanks, Eddie > >> log->bios_event_log = NULL; >> return ret; >> } >> diff --git a/drivers/char/tpm/eventlog/efi.c b/drivers/char/tpm/eventlog/efi.c >> index e6cb9d525e30..4e9d7c2bf32e 100644 >> --- a/drivers/char/tpm/eventlog/efi.c >> +++ b/drivers/char/tpm/eventlog/efi.c >> @@ -6,6 +6,7 @@ >> * Thiebaud Weksteen <tweek@google.com> >> */ >> >> +#include <linux/device.h> >> #include <linux/efi.h> >> #include <linux/tpm_eventlog.h> >> >> @@ -55,7 +56,7 @@ int tpm_read_log_efi(struct tpm_chip *chip) >> } >> >> /* malloc EventLog space */ >> - log->bios_event_log = kmemdup(log_tbl->log, log_size, GFP_KERNEL); >> + log->bios_event_log = devm_kmemdup(&chip->dev, log_tbl->log, log_size, GFP_KERNEL); >> if (!log->bios_event_log) { >> ret = -ENOMEM; >> goto out; >> @@ -76,7 +77,7 @@ int tpm_read_log_efi(struct tpm_chip *chip) >> MEMREMAP_WB); >> if (!final_tbl) { >> pr_err("Could not map UEFI TPM final log\n"); >> - kfree(log->bios_event_log); >> + devm_kfree(&chip->dev, log->bios_event_log); >> ret = -ENOMEM; >> goto out; >> } >> @@ -91,11 +92,11 @@ int tpm_read_log_efi(struct tpm_chip *chip) >> * Allocate memory for the 'combined log' where we will append the >> * 'final events log' to. >> */ >> - tmp = krealloc(log->bios_event_log, >> - log_size + final_events_log_size, >> - GFP_KERNEL); >> + tmp = devm_krealloc(&chip->dev, log->bios_event_log, >> + log_size + final_events_log_size, >> + GFP_KERNEL); >> if (!tmp) { >> - kfree(log->bios_event_log); >> + devm_kfree(&chip->dev, log->bios_event_log); >> ret = -ENOMEM; >> goto out; >> } >> diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c >> index a9ce66d09a75..741ab2204b11 100644 >> --- a/drivers/char/tpm/eventlog/of.c >> +++ b/drivers/char/tpm/eventlog/of.c >> @@ -10,6 +10,7 @@ >> * Read the event log created by the firmware on PPC64 >> */ >> >> +#include <linux/device.h> >> #include <linux/slab.h> >> #include <linux/of.h> >> #include <linux/tpm_eventlog.h> >> @@ -65,7 +66,7 @@ int tpm_read_log_of(struct tpm_chip *chip) >> return -EIO; >> } >> >> - log->bios_event_log = kmemdup(__va(base), size, GFP_KERNEL); >> + log->bios_event_log = devm_kmemdup(&chip->dev, __va(base), size, GFP_KERNEL); >> if (!log->bios_event_log) >> return -ENOMEM; >> >> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c >> index 741d8f3e8fb3..b99f55f2d4fd 100644 >> --- a/drivers/char/tpm/tpm-chip.c >> +++ b/drivers/char/tpm/tpm-chip.c >> @@ -267,7 +267,6 @@ static void tpm_dev_release(struct device *dev) >> idr_remove(&dev_nums_idr, chip->dev_num); >> mutex_unlock(&idr_lock); >> >> - kfree(chip->log.bios_event_log); >> kfree(chip->work_space.context_buf); >> kfree(chip->work_space.session_buf); >> kfree(chip->allocated_banks); >> -- >> 2.31.1 >> > BR, Jarkko
On Wed, Jan 25, 2023 at 01:06:31PM -0600, Eddie James wrote: > > On 1/20/23 18:11, Jarkko Sakkinen wrote: > > On Fri, Jan 13, 2023 at 10:10:16AM -0600, Eddie James wrote: > > > Since the bios event log is freed in the device release function, > > > let devres handle the deallocation. This will allow other memory > > > allocation/mapping functions to be used for the bios event log. > > > > > > Signed-off-by: Eddie James <eajames@linux.ibm.com> > > > --- > > > drivers/char/tpm/eventlog/acpi.c | 5 +++-- > > > drivers/char/tpm/eventlog/efi.c | 13 +++++++------ > > > drivers/char/tpm/eventlog/of.c | 3 ++- > > > drivers/char/tpm/tpm-chip.c | 1 - > > > 4 files changed, 12 insertions(+), 10 deletions(-) > > > > > > diff --git a/drivers/char/tpm/eventlog/acpi.c b/drivers/char/tpm/eventlog/acpi.c > > > index 0913d3eb8d51..40360e599bc3 100644 > > > --- a/drivers/char/tpm/eventlog/acpi.c > > > +++ b/drivers/char/tpm/eventlog/acpi.c > > > @@ -14,6 +14,7 @@ > > > * Access to the event log extended by the TCG BIOS of PC platform > > > */ > > > +#include <linux/device.h> > > > #include <linux/seq_file.h> > > > #include <linux/fs.h> > > > #include <linux/security.h> > > > @@ -135,7 +136,7 @@ int tpm_read_log_acpi(struct tpm_chip *chip) > > > } > > > /* malloc EventLog space */ > > > - log->bios_event_log = kmalloc(len, GFP_KERNEL); > > > + log->bios_event_log = devm_kmalloc(&chip->dev, len, GFP_KERNEL); > > > if (!log->bios_event_log) > > > return -ENOMEM; > > > @@ -160,7 +161,7 @@ int tpm_read_log_acpi(struct tpm_chip *chip) > > > return format; > > > err: > > > - kfree(log->bios_event_log); > > > + devm_kfree(&chip->dev, log->bios_event_log); > > I wonder do we want to do devm_kfree's at all as the memory is freed during > > detach, i.e. taken care by devres. > > > I think we should since the chip/tpm driver will continue to probe without > the bios event log. Therefore that memory will be wasted if there is some > error during bios log setup. OK, I buy this! For this patch: Tested-by: Jarkko Sakkinen <jarkko@kernel.org> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org> If you could refine the description for the 2nd, then these would be ready to be picked. > Thanks, > > Eddie BR, Jarkko
diff --git a/drivers/char/tpm/eventlog/acpi.c b/drivers/char/tpm/eventlog/acpi.c index 0913d3eb8d51..40360e599bc3 100644 --- a/drivers/char/tpm/eventlog/acpi.c +++ b/drivers/char/tpm/eventlog/acpi.c @@ -14,6 +14,7 @@ * Access to the event log extended by the TCG BIOS of PC platform */ +#include <linux/device.h> #include <linux/seq_file.h> #include <linux/fs.h> #include <linux/security.h> @@ -135,7 +136,7 @@ int tpm_read_log_acpi(struct tpm_chip *chip) } /* malloc EventLog space */ - log->bios_event_log = kmalloc(len, GFP_KERNEL); + log->bios_event_log = devm_kmalloc(&chip->dev, len, GFP_KERNEL); if (!log->bios_event_log) return -ENOMEM; @@ -160,7 +161,7 @@ int tpm_read_log_acpi(struct tpm_chip *chip) return format; err: - kfree(log->bios_event_log); + devm_kfree(&chip->dev, log->bios_event_log); log->bios_event_log = NULL; return ret; } diff --git a/drivers/char/tpm/eventlog/efi.c b/drivers/char/tpm/eventlog/efi.c index e6cb9d525e30..4e9d7c2bf32e 100644 --- a/drivers/char/tpm/eventlog/efi.c +++ b/drivers/char/tpm/eventlog/efi.c @@ -6,6 +6,7 @@ * Thiebaud Weksteen <tweek@google.com> */ +#include <linux/device.h> #include <linux/efi.h> #include <linux/tpm_eventlog.h> @@ -55,7 +56,7 @@ int tpm_read_log_efi(struct tpm_chip *chip) } /* malloc EventLog space */ - log->bios_event_log = kmemdup(log_tbl->log, log_size, GFP_KERNEL); + log->bios_event_log = devm_kmemdup(&chip->dev, log_tbl->log, log_size, GFP_KERNEL); if (!log->bios_event_log) { ret = -ENOMEM; goto out; @@ -76,7 +77,7 @@ int tpm_read_log_efi(struct tpm_chip *chip) MEMREMAP_WB); if (!final_tbl) { pr_err("Could not map UEFI TPM final log\n"); - kfree(log->bios_event_log); + devm_kfree(&chip->dev, log->bios_event_log); ret = -ENOMEM; goto out; } @@ -91,11 +92,11 @@ int tpm_read_log_efi(struct tpm_chip *chip) * Allocate memory for the 'combined log' where we will append the * 'final events log' to. */ - tmp = krealloc(log->bios_event_log, - log_size + final_events_log_size, - GFP_KERNEL); + tmp = devm_krealloc(&chip->dev, log->bios_event_log, + log_size + final_events_log_size, + GFP_KERNEL); if (!tmp) { - kfree(log->bios_event_log); + devm_kfree(&chip->dev, log->bios_event_log); ret = -ENOMEM; goto out; } diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c index a9ce66d09a75..741ab2204b11 100644 --- a/drivers/char/tpm/eventlog/of.c +++ b/drivers/char/tpm/eventlog/of.c @@ -10,6 +10,7 @@ * Read the event log created by the firmware on PPC64 */ +#include <linux/device.h> #include <linux/slab.h> #include <linux/of.h> #include <linux/tpm_eventlog.h> @@ -65,7 +66,7 @@ int tpm_read_log_of(struct tpm_chip *chip) return -EIO; } - log->bios_event_log = kmemdup(__va(base), size, GFP_KERNEL); + log->bios_event_log = devm_kmemdup(&chip->dev, __va(base), size, GFP_KERNEL); if (!log->bios_event_log) return -ENOMEM; diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c index 741d8f3e8fb3..b99f55f2d4fd 100644 --- a/drivers/char/tpm/tpm-chip.c +++ b/drivers/char/tpm/tpm-chip.c @@ -267,7 +267,6 @@ static void tpm_dev_release(struct device *dev) idr_remove(&dev_nums_idr, chip->dev_num); mutex_unlock(&idr_lock); - kfree(chip->log.bios_event_log); kfree(chip->work_space.context_buf); kfree(chip->work_space.session_buf); kfree(chip->allocated_banks);
Since the bios event log is freed in the device release function, let devres handle the deallocation. This will allow other memory allocation/mapping functions to be used for the bios event log. Signed-off-by: Eddie James <eajames@linux.ibm.com> --- drivers/char/tpm/eventlog/acpi.c | 5 +++-- drivers/char/tpm/eventlog/efi.c | 13 +++++++------ drivers/char/tpm/eventlog/of.c | 3 ++- drivers/char/tpm/tpm-chip.c | 1 - 4 files changed, 12 insertions(+), 10 deletions(-)