* [v7] powerpc/powernv: add hdat attribute to sysfs
@ 2017-03-22 22:27 Matt Brown
2017-03-24 12:37 ` kbuild test robot
2017-03-27 11:34 ` Michael Ellerman
0 siblings, 2 replies; 4+ messages in thread
From: Matt Brown @ 2017-03-22 22:27 UTC (permalink / raw)
To: linuxppc-dev
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 <matthew.brown.dev@gmail.com>
---
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");
--
2.9.3
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [v7] powerpc/powernv: add hdat attribute to sysfs
2017-03-22 22:27 [v7] powerpc/powernv: add hdat attribute to sysfs Matt Brown
@ 2017-03-24 12:37 ` kbuild test robot
2017-03-27 11:34 ` Michael Ellerman
1 sibling, 0 replies; 4+ messages in thread
From: kbuild test robot @ 2017-03-24 12:37 UTC (permalink / raw)
To: Matt Brown; +Cc: kbuild-all, linuxppc-dev
[-- Attachment #1: Type: text/plain, Size: 2677 bytes --]
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 infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 23094 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [v7] powerpc/powernv: add hdat attribute to sysfs
2017-03-22 22:27 [v7] powerpc/powernv: add hdat attribute to sysfs Matt Brown
2017-03-24 12:37 ` kbuild test robot
@ 2017-03-27 11:34 ` Michael Ellerman
1 sibling, 0 replies; 4+ messages in thread
From: Michael Ellerman @ 2017-03-27 11:34 UTC (permalink / raw)
To: Matt Brown, linuxppc-dev
Hi Matt,
Sorry if you're getting sick of this at v7, but here's a few more
comments :D
Matt Brown <matthew.brown.dev@gmail.com> 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 helpers, eg:
u64 vals[2];
...
if (of_property_read_u64_array(np, prop->name, &vals, 2))
continue.
And if it succeeds then vals has the two values in CPU endian.
> + attr_tmp = &exported_attrs[n];
Notice here we use exported_attrs, ...
> + attr_tmp->attr.name = attr_name[n];
And here we use attr_name.
But we never use them again. We do need to keep the bin_attr alive, we
gave it to sysfs_create_bin_file() after all, but we don't actually need
an array of them. And attr_name is completely unnecessary once we've
assigned the pointer to attr.name.
So the solution is to drop the allocation of exported_attrs and
attr_name prior to the loop.
Then in the loop we do:
for_each_property_of_node(fw, prop)
Check for name or phandle, else continue
Read the property value using read_u64_array, else continue
Allocate one attr else warn and continue
Strdup the prop->name into attr->name, else kfree(attr), warn, continue
Intialise the attr
Call sysfs_create_bin_file(), if it fails, warn and kfree(attr)
cheers
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [v7] powerpc/powernv: add hdat attribute to sysfs
[not found] <20170322222916.8480278038@b03ledav004.gho.boulder.ibm.com>
@ 2017-03-23 1:12 ` Andrew Donnellan
0 siblings, 0 replies; 4+ messages in thread
From: Andrew Donnellan @ 2017-03-23 1:12 UTC (permalink / raw)
To: Matt Brown, linuxppc-dev
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 <matthew.brown.dev@gmail.com>
Reviewed-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>
> ---
> 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.donnellan@au1.ibm.com IBM Australia Limited
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-03-27 11:34 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-22 22:27 [v7] powerpc/powernv: add hdat attribute to sysfs Matt Brown
2017-03-24 12:37 ` kbuild test robot
2017-03-27 11:34 ` Michael Ellerman
[not found] <20170322222916.8480278038@b03ledav004.gho.boulder.ibm.com>
2017-03-23 1:12 ` Andrew Donnellan
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.