On Wed, Aug 21, 2019 at 11:08:21AM -0400, Nayna Jain wrote:

Can you break this out into one-entry-per-file like most other entries
are defined?  That makes it easier for tools to parse (specifically the
tool in the tree right now...)

No depends on SYSFS?

+        help
+          POWER secure variables are managed and controlled by firmware.
+          These variables are exposed to userspace via sysfs to enable
+          read/write operations on these variables. Say Y if you have
+	  secure boot enabled and want to expose variables to userspace.

Mix of tabs and spaces :(

No tab?

" " before "A" here please.

+#define VARIABLE_MAX_SIZE  32000
+
+static struct kobject *powerpc_kobj;
+static struct secvar_operations *secvarops;
+struct kset *secvar_kset;
+
+static ssize_t name_show(struct kobject *kobj, struct kobj_attribute *attr,
+			 char *buf)
+{
+	return sprintf(buf, "%s", kobj->name);
+}

Why do you need this entry as it is the directory name?  Userspace
already "knows" it if they can open this file.

+
+static ssize_t size_show(struct kobject *kobj, struct kobj_attribute *attr,
+			 char *buf)
+{
+	unsigned long dsize;
+	int rc;
+
+	rc = secvarops->get_variable(kobj->name, strlen(kobj->name) + 1, NULL,
+				     &dsize);
+	if (rc) {
+		pr_err("Error retrieving variable size %d\n", rc);
+		return rc;
+	}
+
+	rc = sprintf(buf, "%ld", dsize);
+
+	return rc;
+}
+
+static ssize_t data_read(struct file *filep, struct kobject *kobj,
+			 struct bin_attribute *attr, char *buf, loff_t off,
+			 size_t count)
+{
+	unsigned long dsize;
+	int rc;
+	char *data;
+
+	rc = secvarops->get_variable(kobj->name, strlen(kobj->name) + 1, NULL,
+				     &dsize);
+	if (rc) {
+		pr_err("Error getting variable size %d\n", rc);
+		return rc;
+	}
+	pr_debug("dsize is %ld\n", dsize);
+
+	data = kzalloc(dsize, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	rc = secvarops->get_variable(kobj->name, strlen(kobj->name)+1, data,
+				     &dsize);
+	if (rc) {
+		pr_err("Error getting variable %d\n", rc);
+		goto data_fail;
+	}
+
+	rc = memory_read_from_buffer(buf, count, &off, data, dsize);
+
+data_fail:
+	kfree(data);
+	return rc;
+}
+
+static ssize_t update_write(struct file *filep, struct kobject *kobj,
+			    struct bin_attribute *attr, char *buf, loff_t off,
+			    size_t count)
+{
+	int rc;
+
+	pr_debug("count is %ld\n", count);
+	rc = secvarops->set_variable(kobj->name, strlen(kobj->name)+1, buf,
+				     count);
+	if (rc) {
+		pr_err("Error setting the variable %s\n", kobj->name);
+		return rc;
+	}
+
+	return count;
+}
+
+static struct kobj_attribute name_attr =
+__ATTR(name, 0444, name_show, NULL);

__ATTR_RO()?

+
+static struct kobj_attribute size_attr =
+__ATTR(size, 0444, size_show, NULL);

__ATTR_RO()?

+
+static struct bin_attribute data_attr = {
+	.attr = {.name = "data", .mode = 0444},
+	.size = VARIABLE_MAX_SIZE,
+	.read = data_read,
+};

__BIN_ATTR_RO()?

+
+
+static struct bin_attribute update_attr = {
+	.attr = {.name = "update", .mode = 0200},
+	.size = VARIABLE_MAX_SIZE,
+	.write = update_write,
+};

__BIN_ATTR_RO()?

+
+static struct bin_attribute  *secvar_bin_attrs[] = {
+	&data_attr,
+	&update_attr,
+	NULL,
+};
+
+static struct attribute *secvar_attrs[] = {
+	&name_attr.attr,
+	&size_attr.attr,
+	NULL,
+};
+
+const struct attribute_group secvar_attr_group = {
+	.attrs = secvar_attrs,
+	.bin_attrs = secvar_bin_attrs,
+};

static?

+
+int secvar_sysfs_load(void)
+{
+
+	char *name;

No blank line.  You didn't run this this through checkpatch, did you :(

+	unsigned long namesize;
+	struct kobject *kobj;
+	int status;
+	int rc = 0;
+
+	name = kzalloc(1024, GFP_KERNEL);

Why 1024?

+	if (!name)
+		return -ENOMEM;
+
+	do {
+
+		status = secvarops->get_next_variable(name, &namesize, 1024);
+		if (status != OPAL_SUCCESS)
+			break;
+
+		pr_info("name is %s\n", name);

Please delete debugging messages.

+		kobj = kobject_create_and_add(name, &(secvar_kset->kobj));
+		if (kobj) {
+			rc = sysfs_create_group(kobj, &secvar_attr_group);

You just raced userspace and lost :(

If you set your kobj_type to have the attribute group you will not race
and loose, the core will handle it for you.

+			if (rc)
+				pr_err("Error creating attributes for %s variable\n",
+				name);
+		} else {
+			pr_err("Error creating sysfs entry for %s variable\n",
+				name);
+			rc = -EINVAL;
+		}
+
+	} while ((status == OPAL_SUCCESS) && (rc == 0));
+
+	kfree(name);
+	return rc;
+}
+
+int secvar_sysfs_init(void)
+{
+	powerpc_kobj = kobject_create_and_add("secvar", firmware_kobj);
+	if (!powerpc_kobj) {
+		pr_err("secvar: Failed to create firmware kobj\n");
+		return -ENODEV;
+	}
+
+	secvar_kset = kset_create_and_add("vars", NULL, powerpc_kobj);
+	if (!secvar_kset) {
+		pr_err("secvar: sysfs kobject registration failed.\n");

You juat leaked a kobject :(

+		return -ENODEV;
+	}
+
+	secvarops = get_secvar_ops();
+	if (!secvarops) {
+		kobject_put(powerpc_kobj);
+		pr_err("secvar: failed to retrieve secvar operations.\n");
+		return -ENODEV;

You just leaked 2 things from above :(

+	}
+
+	secvar_sysfs_load();
+	pr_info("Secure variables sysfs initialized");

Do not be noisy when all goes just fine.  The kernel log should be quiet
when all goes well.

thanks,

greg k-h