On Thu, 2023-01-05 at 18:28 +1100, Andrew Donnellan wrote:

On Fri, 2022-12-30 at 15:20 +1100, Russell Currey wrote:

quoted

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 <redacted>

Minor comments below, but regardless:

Reviewed-by: Andrew Donnellan <redacted>

quoted

---
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.

quoted

 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.

quoted

+
+       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.

Yeah we should fix that for all of them, good catch.

quoted

+               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

quoted

+                       goto err;
+               }
        }
 
        secvar_sysfs_load();
 
        return 0;
+err:
+       kobject_put(secvar_kobj);
+       return rc;
 }
 
 late_initcall(secvar_sysfs_init);