From: Andrew Donnellan <ajd@linux.ibm.com> To: Russell Currey <ruscur@russell.cc>, linuxppc-dev@lists.ozlabs.org Cc: gregkh@linuxfoundation.org, gcwilson@linux.ibm.com, linux-kernel@vger.kernel.org, nayna@linux.ibm.com, zohar@linux.ibm.com, mpe@ellerman.id.au Subject: Re: [PATCH v2 6/7] powerpc/secvar: Extend sysfs to include config vars Date: Thu, 05 Jan 2023 18:28:16 +1100 [thread overview] Message-ID: <20729da992c608ed40f5f037a7314395075a4254.camel@linux.ibm.com> (raw) In-Reply-To: <20221230042014.154483-7-ruscur@russell.cc> On Fri, 2022-12-30 at 15:20 +1100, Russell Currey wrote: > The forthcoming pseries consumer of the secvar API wants to expose a > number of config variables. Allowing secvar implementations to > provide > their own sysfs attributes makes it easy for consumers to expose what > they need to. > > This is not being used by the OPAL secvar implementation at present, > and > the config directory will not be created if no attributes are set. > > Signed-off-by: Russell Currey <ruscur@russell.cc> Minor comments below, but regardless: Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com> > --- > I played around with adding an API call to facilitate a more generic > key/value interface for config variables and it seemed like > unnecessary > complexity. I think this is cleaner. If there was ever a secvar > interface other than sysfs we'd have to rework it, though. I concur, this can be dealt with if/when the secvar interface is exposed by some other means than sysfs. > > arch/powerpc/include/asm/secvar.h | 3 +++ > arch/powerpc/kernel/secvar-sysfs.c | 40 ++++++++++++++++++++++++++-- > -- > 2 files changed, 38 insertions(+), 5 deletions(-) > > diff --git a/arch/powerpc/include/asm/secvar.h > b/arch/powerpc/include/asm/secvar.h > index 92d2c051918b..250e7066b6da 100644 > --- a/arch/powerpc/include/asm/secvar.h > +++ b/arch/powerpc/include/asm/secvar.h > @@ -10,6 +10,7 @@ > > #include <linux/types.h> > #include <linux/errno.h> > +#include <linux/sysfs.h> > > extern const struct secvar_operations *secvar_ops; > > @@ -27,10 +28,12 @@ struct secvar_operations { > #ifdef CONFIG_PPC_SECURE_BOOT > > extern void set_secvar_ops(const struct secvar_operations *ops); > +extern void set_secvar_config_attrs(const struct attribute **attrs); > > #else > > static inline void set_secvar_ops(const struct secvar_operations > *ops) { } > +static inline void set_secvar_config_attrs(const struct attribute > **attrs) { } > > #endif > > diff --git a/arch/powerpc/kernel/secvar-sysfs.c > b/arch/powerpc/kernel/secvar-sysfs.c > index aa1daec480e1..ad1e1d72d2ae 100644 > --- a/arch/powerpc/kernel/secvar-sysfs.c > +++ b/arch/powerpc/kernel/secvar-sysfs.c > @@ -15,9 +15,17 @@ > > #define NAME_MAX_SIZE 1024 > > +const struct attribute **secvar_config_attrs __ro_after_init = NULL; > + > static struct kobject *secvar_kobj; > static struct kset *secvar_kset; > > +void set_secvar_config_attrs(const struct attribute **attrs) > +{ > + WARN_ON_ONCE(secvar_config_attrs); > + secvar_config_attrs = attrs; > +} > + > static ssize_t format_show(struct kobject *kobj, struct > kobj_attribute *attr, > char *buf) > { > @@ -134,6 +142,16 @@ static int update_kobj_size(void) > return 0; > } > > +static int secvar_sysfs_config(struct kobject *kobj) > +{ > + struct attribute_group config_group = { > + .name = "config", > + .attrs = (struct attribute **)secvar_config_attrs, > + }; I was slightly concerned that you're putting this on the stack, but it doesn't appear that sysfs_create_group() keeps any references to the group around after it creates all the files, so I think this is fine. > + > + return sysfs_create_group(kobj, &config_group); > +} > + > static int secvar_sysfs_load(void) > { > char *name; > @@ -196,26 +214,38 @@ static int secvar_sysfs_init(void) > > rc = sysfs_create_file(secvar_kobj, &format_attr.attr); > if (rc) { > - kobject_put(secvar_kobj); > - return -ENOMEM; > + pr_err("secvar: Failed to create format object\n"); This file defines pr_fmt, so the secvar: prefix here can go away, though I notice that is the case for all the existing prints in this function too. > + rc = -ENOMEM; > + goto err; > } > > secvar_kset = kset_create_and_add("vars", NULL, secvar_kobj); > if (!secvar_kset) { > pr_err("secvar: sysfs kobject registration > failed.\n"); > - kobject_put(secvar_kobj); > - return -ENOMEM; > + rc = -ENOMEM; > + goto err; > } > > rc = update_kobj_size(); > if (rc) { > pr_err("Cannot read the size of the attribute\n"); > - return rc; > + goto err; > + } > + > + if (secvar_config_attrs) { > + rc = secvar_sysfs_config(secvar_kobj); > + if (rc) { > + pr_err("secvar: Failed to create config > directory\n"); Same comment as above > + goto err; > + } > } > > secvar_sysfs_load(); > > return 0; > +err: > + kobject_put(secvar_kobj); > + return rc; > } > > late_initcall(secvar_sysfs_init); -- Andrew Donnellan OzLabs, ADL Canberra ajd@linux.ibm.com IBM Australia Limited
WARNING: multiple messages have this Message-ID (diff)
From: Andrew Donnellan <ajd@linux.ibm.com> To: Russell Currey <ruscur@russell.cc>, linuxppc-dev@lists.ozlabs.org Cc: gregkh@linuxfoundation.org, nayna@linux.ibm.com, linux-kernel@vger.kernel.org, zohar@linux.ibm.com, gcwilson@linux.ibm.com Subject: Re: [PATCH v2 6/7] powerpc/secvar: Extend sysfs to include config vars Date: Thu, 05 Jan 2023 18:28:16 +1100 [thread overview] Message-ID: <20729da992c608ed40f5f037a7314395075a4254.camel@linux.ibm.com> (raw) In-Reply-To: <20221230042014.154483-7-ruscur@russell.cc> On Fri, 2022-12-30 at 15:20 +1100, Russell Currey wrote: > The forthcoming pseries consumer of the secvar API wants to expose a > number of config variables. Allowing secvar implementations to > provide > their own sysfs attributes makes it easy for consumers to expose what > they need to. > > This is not being used by the OPAL secvar implementation at present, > and > the config directory will not be created if no attributes are set. > > Signed-off-by: Russell Currey <ruscur@russell.cc> Minor comments below, but regardless: Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com> > --- > I played around with adding an API call to facilitate a more generic > key/value interface for config variables and it seemed like > unnecessary > complexity. I think this is cleaner. If there was ever a secvar > interface other than sysfs we'd have to rework it, though. I concur, this can be dealt with if/when the secvar interface is exposed by some other means than sysfs. > > arch/powerpc/include/asm/secvar.h | 3 +++ > arch/powerpc/kernel/secvar-sysfs.c | 40 ++++++++++++++++++++++++++-- > -- > 2 files changed, 38 insertions(+), 5 deletions(-) > > diff --git a/arch/powerpc/include/asm/secvar.h > b/arch/powerpc/include/asm/secvar.h > index 92d2c051918b..250e7066b6da 100644 > --- a/arch/powerpc/include/asm/secvar.h > +++ b/arch/powerpc/include/asm/secvar.h > @@ -10,6 +10,7 @@ > > #include <linux/types.h> > #include <linux/errno.h> > +#include <linux/sysfs.h> > > extern const struct secvar_operations *secvar_ops; > > @@ -27,10 +28,12 @@ struct secvar_operations { > #ifdef CONFIG_PPC_SECURE_BOOT > > extern void set_secvar_ops(const struct secvar_operations *ops); > +extern void set_secvar_config_attrs(const struct attribute **attrs); > > #else > > static inline void set_secvar_ops(const struct secvar_operations > *ops) { } > +static inline void set_secvar_config_attrs(const struct attribute > **attrs) { } > > #endif > > diff --git a/arch/powerpc/kernel/secvar-sysfs.c > b/arch/powerpc/kernel/secvar-sysfs.c > index aa1daec480e1..ad1e1d72d2ae 100644 > --- a/arch/powerpc/kernel/secvar-sysfs.c > +++ b/arch/powerpc/kernel/secvar-sysfs.c > @@ -15,9 +15,17 @@ > > #define NAME_MAX_SIZE 1024 > > +const struct attribute **secvar_config_attrs __ro_after_init = NULL; > + > static struct kobject *secvar_kobj; > static struct kset *secvar_kset; > > +void set_secvar_config_attrs(const struct attribute **attrs) > +{ > + WARN_ON_ONCE(secvar_config_attrs); > + secvar_config_attrs = attrs; > +} > + > static ssize_t format_show(struct kobject *kobj, struct > kobj_attribute *attr, > char *buf) > { > @@ -134,6 +142,16 @@ static int update_kobj_size(void) > return 0; > } > > +static int secvar_sysfs_config(struct kobject *kobj) > +{ > + struct attribute_group config_group = { > + .name = "config", > + .attrs = (struct attribute **)secvar_config_attrs, > + }; I was slightly concerned that you're putting this on the stack, but it doesn't appear that sysfs_create_group() keeps any references to the group around after it creates all the files, so I think this is fine. > + > + return sysfs_create_group(kobj, &config_group); > +} > + > static int secvar_sysfs_load(void) > { > char *name; > @@ -196,26 +214,38 @@ static int secvar_sysfs_init(void) > > rc = sysfs_create_file(secvar_kobj, &format_attr.attr); > if (rc) { > - kobject_put(secvar_kobj); > - return -ENOMEM; > + pr_err("secvar: Failed to create format object\n"); This file defines pr_fmt, so the secvar: prefix here can go away, though I notice that is the case for all the existing prints in this function too. > + rc = -ENOMEM; > + goto err; > } > > secvar_kset = kset_create_and_add("vars", NULL, secvar_kobj); > if (!secvar_kset) { > pr_err("secvar: sysfs kobject registration > failed.\n"); > - kobject_put(secvar_kobj); > - return -ENOMEM; > + rc = -ENOMEM; > + goto err; > } > > rc = update_kobj_size(); > if (rc) { > pr_err("Cannot read the size of the attribute\n"); > - return rc; > + goto err; > + } > + > + if (secvar_config_attrs) { > + rc = secvar_sysfs_config(secvar_kobj); > + if (rc) { > + pr_err("secvar: Failed to create config > directory\n"); Same comment as above > + goto err; > + } > } > > secvar_sysfs_load(); > > return 0; > +err: > + kobject_put(secvar_kobj); > + return rc; > } > > late_initcall(secvar_sysfs_init); -- Andrew Donnellan OzLabs, ADL Canberra ajd@linux.ibm.com IBM Australia Limited
next prev parent reply other threads:[~2023-01-05 7:28 UTC|newest] Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-12-30 4:20 [PATCH v2 0/7] pseries dynamic secure boot interface using secvar Russell Currey 2022-12-30 4:20 ` Russell Currey 2022-12-30 4:20 ` [PATCH v2 1/7] powerpc/pseries: Log hcall return codes for PLPKS debug Russell Currey 2022-12-30 4:20 ` Russell Currey 2023-01-04 4:45 ` Andrew Donnellan 2023-01-04 4:45 ` Andrew Donnellan 2022-12-30 4:20 ` [PATCH v2 2/7] powerpc/secvar: WARN_ON_ONCE() if multiple secvar ops are set Russell Currey 2022-12-30 4:20 ` Russell Currey 2023-01-04 7:10 ` Andrew Donnellan 2023-01-04 7:10 ` Andrew Donnellan 2022-12-30 4:20 ` [PATCH v2 3/7] powerpc/secvar: Use sysfs_emit() instead of sprintf() Russell Currey 2022-12-30 4:20 ` Russell Currey 2023-01-04 7:12 ` Andrew Donnellan 2023-01-04 7:12 ` Andrew Donnellan 2022-12-30 4:20 ` [PATCH v2 4/7] powerpc/secvar: Handle format string in the consumer Russell Currey 2022-12-30 4:20 ` Russell Currey 2023-01-04 7:31 ` Andrew Donnellan 2023-01-04 7:31 ` Andrew Donnellan 2022-12-30 4:20 ` [PATCH v2 5/7] powerpc/secvar: Handle max object size " Russell Currey 2022-12-30 4:20 ` Russell Currey 2023-01-04 7:50 ` Andrew Donnellan 2023-01-04 7:50 ` Andrew Donnellan 2022-12-30 4:20 ` [PATCH v2 6/7] powerpc/secvar: Extend sysfs to include config vars Russell Currey 2022-12-30 4:20 ` Russell Currey 2023-01-05 7:28 ` Andrew Donnellan [this message] 2023-01-05 7:28 ` Andrew Donnellan 2023-01-06 6:33 ` Russell Currey 2023-01-06 6:33 ` Russell Currey 2023-01-06 4:15 ` Michael Ellerman 2023-01-06 4:15 ` Michael Ellerman 2023-01-06 6:35 ` Russell Currey 2023-01-06 6:35 ` Russell Currey 2022-12-30 4:20 ` [PATCH v2 7/7] powerpc/pseries: Implement secvars for dynamic secure boot Russell Currey 2022-12-30 4:20 ` Russell Currey 2023-01-05 8:15 ` Andrew Donnellan 2023-01-05 8:15 ` Andrew Donnellan 2023-01-06 6:49 ` Russell Currey 2023-01-06 6:49 ` Russell Currey 2023-01-09 4:42 ` Andrew Donnellan 2023-01-09 4:42 ` Andrew Donnellan 2023-01-06 10:49 ` Michael Ellerman 2023-01-06 10:49 ` Michael Ellerman 2023-01-09 3:33 ` Andrew Donnellan 2023-01-09 3:33 ` Andrew Donnellan 2023-01-09 3:34 ` Russell Currey 2023-01-09 3:34 ` Russell Currey 2023-01-09 5:20 ` Andrew Donnellan 2023-01-09 5:20 ` Andrew Donnellan 2023-01-10 1:27 ` Russell Currey 2023-01-10 1:27 ` Russell Currey 2023-01-10 3:59 ` Andrew Donnellan 2023-01-10 3:59 ` Andrew Donnellan 2023-01-11 3:57 ` Andrew Donnellan 2023-01-11 3:57 ` Andrew Donnellan
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20729da992c608ed40f5f037a7314395075a4254.camel@linux.ibm.com \ --to=ajd@linux.ibm.com \ --cc=gcwilson@linux.ibm.com \ --cc=gregkh@linuxfoundation.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=mpe@ellerman.id.au \ --cc=nayna@linux.ibm.com \ --cc=ruscur@russell.cc \ --cc=zohar@linux.ibm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.