Re: [v7] powerpc/powernv: add hdat attribute to sysfs
Hi Matt,
Sorry if you're getting sick of this at v7, but here's a few more
comments :D
Matt Brown writes:
> The HDAT data area is consumed by skiboot and turned into a device-tree.
> In some cases we would like to look directly at the HDAT, so this patch
> adds a sysfs node to allow it to be viewed. This is not possible through
> /dev/mem as it is reserved memory which is stopped by the /dev/mem filter.
You need to update the change log now that the patch has expanded in its
purpose.
> diff --git a/arch/powerpc/platforms/powernv/opal.c
> b/arch/powerpc/platforms/powernv/opal.c
> index 2822935..b8f057f 100644
> --- a/arch/powerpc/platforms/powernv/opal.c
> +++ b/arch/powerpc/platforms/powernv/opal.c
> @@ -604,6 +604,87 @@ static void opal_export_symmap(void)
> pr_warn("Error %d creating OPAL symbols file\n", rc);
> }
>
> +static ssize_t export_attr_read(struct file *fp, struct kobject *kobj,
> + struct bin_attribute *bin_attr, char *buf,
> + loff_t off, size_t count)
> +{
> + return memory_read_from_buffer(buf, count, &off, bin_attr->private,
> +bin_attr->size);
> +}
> +
> +/*
> + * opal_export_attrs: creates a sysfs node for each property listed in
> + * the device-tree under /ibm,opal/firmware/exports/
> + * All new sysfs nodes are created under /opal/exports/.
> + * This allows for reserved memory regions (e.g. HDAT) to be read.
> + * The new sysfs nodes are only readable by root.
> + */
> +static void opal_export_attrs(void)
> +{
> + /* /sys/firmware/opal/exports */
> + struct kobject *opal_export_kobj;
> + struct bin_attribute *exported_attrs;
> + char **attr_name;
> +
You should pretty much never have a blank line in your variable
declarations.
> + struct bin_attribute *attr_tmp;
> + const __be64 *syms;
> + unsigned int size;
> + struct device_node *fw;
It's traditional to call these "np" for "node pointer" (or less commonly "dn").
> + struct property *prop;
> + int rc;
> + int attr_count = 0;
> + int n = 0;
It's usually better to delay initialisation of variables until when
they're needed, so in this case just prior to the loop that uses them.
Also notice attr_count is only used prior to the use of n, so you could
just use n for both. Though see below.
You should also group common types on one line when possible. And I and
some other folks with good taste, like the longest variable lines first
followed by the shorter ones, so in this case eg:
struct bin_attribute *exported_attrs, *attr_tmp;
struct kobject *opal_export_kobj;
struct device_node *np;
struct property *prop;
int rc, attr_count, n;
const __be64 *syms;
unsigned int size;
char **attr_name;
> +
> + /* Create new 'exports' directory */
> + opal_export_kobj = kobject_create_and_add("exports", opal_kobj);
Now that this is a local it doesn't need such a verbose name, it could
just be kobj.
> + if (!opal_export_kobj) {
> + pr_warn("kobject_create_and_add opal_exports failed\n");
> + return;
> + }
> +
> + fw = of_find_node_by_path("/ibm,opal/firmware/exports");
> + if (!fw)
> + return;
> +
> + for (prop = fw->properties; prop != NULL; prop = prop->next)
> + attr_count++;
So here we count the properties of the node.
> + if (attr_count > 2) {
> + exported_attrs = kzalloc(sizeof(exported_attrs)*(attr_count-2),
> + GFP_KERNEL);
> + attr_name = kzalloc(sizeof(char *)*(attr_count-2), GFP_KERNEL);
And here we alloc space for that number - 2.
> + }
> +
> + for_each_property_of_node(fw, prop) {
But this loop does no checking vs attr_count. I know you have the check
for "name" and "phandle" below, but if ever you didn't have those
properties this loop would overflow the end of exported_attrs and
corrupt something else on the heap.
> + attr_name[n] = kstrdup(prop->name, GFP_KERNEL);
Here we allocate memory for the property name ...
> + syms = of_get_property(fw, attr_name[n], &size);
> +
> + if (!strcmp(attr_name[n], "name") ||
> + !strcmp(attr_name[n], "phandle"))
> + continue;
But then here we don't free the attr_name we just kstrdup'ed.
>
> + if (!syms || size != 2 * sizeof(__be64))
> + continue;
And same here, as well as we've now allocated an extra bin_attr in
exported_attrs that we'll never use.
> + syms = of_get_property(fw, attr_name[n], &size);
...
> + if (!syms || size != 2 * sizeof(__be64))
> + continue;
...
> + attr_tmp->private = __va(be64_to_cpu(syms[0]));
> + attr_tmp->size = be64_to_cpu(syms[1]);
The actual reading of the property can be done in a cleaner way using
one of the he
Re: [v7] powerpc/powernv: add hdat attribute to sysfs
Hi Matt,
[auto build test ERROR on powerpc/next]
[also build test ERROR on v4.11-rc3 next-20170324]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Matt-Brown/powerpc-powernv-add-hdat-attribute-to-sysfs/20170324-191306
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-defconfig (attached as .config)
compiler: powerpc64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget
https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=powerpc
Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings
All errors (new ones prefixed by >>):
arch/powerpc/platforms/powernv/opal.c: In function
'__machine_initcall_powernv_opal_init':
>> arch/powerpc/platforms/powernv/opal.c:651:12: error: 'attr_name' may be used
>> uninitialized in this function [-Werror=maybe-uninitialized]
attr_name[n] = kstrdup(prop->name, GFP_KERNEL);
^
arch/powerpc/platforms/powernv/opal.c:618:9: note: 'attr_name' was declared
here
char **attr_name;
^
>> arch/powerpc/platforms/powernv/opal.c:661:12: error: 'exported_attrs' may be
>> used uninitialized in this function [-Werror=maybe-uninitialized]
attr_tmp = &exported_attrs[n];
~^~~~
arch/powerpc/platforms/powernv/opal.c:617:24: note: 'exported_attrs' was
declared here
struct bin_attribute *exported_attrs;
^~
cc1: all warnings being treated as errors
vim +/attr_name +651 arch/powerpc/platforms/powernv/opal.c
645 GFP_KERNEL);
646 attr_name = kzalloc(sizeof(char *)*(attr_count-2),
GFP_KERNEL);
647 }
648
649 for_each_property_of_node(fw, prop) {
650
> 651 attr_name[n] = kstrdup(prop->name, GFP_KERNEL);
652 syms = of_get_property(fw, attr_name[n], &size);
653
654 if (!strcmp(attr_name[n], "name") ||
655 !strcmp(attr_name[n], "phandle"))
656 continue;
657
658 if (!syms || size != 2 * sizeof(__be64))
659 continue;
660
> 661 attr_tmp = &exported_attrs[n];
662 attr_tmp->attr.name = attr_name[n];
663 attr_tmp->attr.mode = 0400;
664 attr_tmp->read = export_attr_read;
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
Re: [v7] powerpc/powernv: add hdat attribute to sysfs
On 23/03/17 09:27, Matt Brown wrote:
The HDAT data area is consumed by skiboot and turned into a device-tree.
In some cases we would like to look directly at the HDAT, so this patch
adds a sysfs node to allow it to be viewed. This is not possible through
/dev/mem as it is reserved memory which is stopped by the /dev/mem filter.
Signed-off-by: Matt Brown
Reviewed-by: Andrew Donnellan
---
Changelog:
v7:
- moved exported_attrs and attr_name into opal_export_attrs
---
arch/powerpc/platforms/powernv/opal.c | 84 +++
1 file changed, 84 insertions(+)
diff --git a/arch/powerpc/platforms/powernv/opal.c
b/arch/powerpc/platforms/powernv/opal.c
index 2822935..b8f057f 100644
--- a/arch/powerpc/platforms/powernv/opal.c
+++ b/arch/powerpc/platforms/powernv/opal.c
@@ -604,6 +604,87 @@ static void opal_export_symmap(void)
pr_warn("Error %d creating OPAL symbols file\n", rc);
}
+static ssize_t export_attr_read(struct file *fp, struct kobject *kobj,
+struct bin_attribute *bin_attr, char *buf,
+loff_t off, size_t count)
+{
+ return memory_read_from_buffer(buf, count, &off, bin_attr->private,
+ bin_attr->size);
+}
+
+/*
+ * opal_export_attrs: creates a sysfs node for each property listed in
+ * the device-tree under /ibm,opal/firmware/exports/
+ * All new sysfs nodes are created under /opal/exports/.
+ * This allows for reserved memory regions (e.g. HDAT) to be read.
+ * The new sysfs nodes are only readable by root.
+ */
+static void opal_export_attrs(void)
+{
+ /* /sys/firmware/opal/exports */
+ struct kobject *opal_export_kobj;
+ struct bin_attribute *exported_attrs;
+ char **attr_name;
+
+ struct bin_attribute *attr_tmp;
+ const __be64 *syms;
+ unsigned int size;
+ struct device_node *fw;
+ struct property *prop;
+ int rc;
+ int attr_count = 0;
+ int n = 0;
+
+ /* Create new 'exports' directory */
+ opal_export_kobj = kobject_create_and_add("exports", opal_kobj);
+ if (!opal_export_kobj) {
+ pr_warn("kobject_create_and_add opal_exports failed\n");
+ return;
+ }
+
+ fw = of_find_node_by_path("/ibm,opal/firmware/exports");
+ if (!fw)
+ return;
+
+ for (prop = fw->properties; prop != NULL; prop = prop->next)
+ attr_count++;
+
+ if (attr_count > 2) {
+ exported_attrs = kzalloc(sizeof(exported_attrs)*(attr_count-2),
+ GFP_KERNEL);
+ attr_name = kzalloc(sizeof(char *)*(attr_count-2), GFP_KERNEL);
+ }
+
+ for_each_property_of_node(fw, prop) {
+
+ attr_name[n] = kstrdup(prop->name, GFP_KERNEL);
+ syms = of_get_property(fw, attr_name[n], &size);
+
+ if (!strcmp(attr_name[n], "name") ||
+ !strcmp(attr_name[n], "phandle"))
+ continue;
+
+ if (!syms || size != 2 * sizeof(__be64))
+ continue;
+
+ attr_tmp = &exported_attrs[n];
+ attr_tmp->attr.name = attr_name[n];
+ attr_tmp->attr.mode = 0400;
+ attr_tmp->read = export_attr_read;
+ attr_tmp->private = __va(be64_to_cpu(syms[0]));
+ attr_tmp->size = be64_to_cpu(syms[1]);
+
+ rc = sysfs_create_bin_file(opal_export_kobj, attr_tmp);
+ if (rc)
+ pr_warn("Error %d creating OPAL sysfs exports/%s
file\n",
+ rc, attr_name[n]);
+ n++;
+ }
+
+ of_node_put(fw);
+
+}
+
static void __init opal_dump_region_init(void)
{
void *addr;
@@ -742,6 +823,9 @@ static int __init opal_init(void)
opal_msglog_sysfs_init();
}
+ /* Export all properties */
+ opal_export_attrs();
+
/* Initialize platform devices: IPMI backend, PRD & flash interface */
opal_pdev_init("ibm,opal-ipmi");
opal_pdev_init("ibm,opal-flash");
--
Andrew Donnellan OzLabs, ADL Canberra
andrew.donnel...@au1.ibm.com IBM Australia Limited
