powerpc/powernv: add hdat attribute to sysfs

PATCH Superseded Matt Brown 4 participants 8 messages Feb 24, 2017 – Feb 28, 2017 PowerPC

Share & tools

Matt Brown

author

2017-02-24 06:21 UTC #0 lore raw

PATCH v3 1/1 powerpc/powernv: add hdat attribute to sysfs

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 <[email protected]>

---

Changes between v2 to v3:
- fixed header comments
- simplified if statement

---
 arch/powerpc/include/asm/opal.h            |  1 +
 arch/powerpc/platforms/powernv/Makefile    |  1 +
 arch/powerpc/platforms/powernv/opal-hdat.c | 65 ++++++++++++++++++++++++++++++
 arch/powerpc/platforms/powernv/opal.c      |  2 +
 4 files changed, 69 insertions(+)
 create mode 100644 arch/powerpc/platforms/powernv/opal-hdat.c

arch/powerpc/include/asm/opal.h +1 -0

index 5c7db0f..b26944e 100644
@@ -277,6 +277,7 @@ extern int opal_async_comp_init(void);
 extern int opal_sensor_init(void);
 extern int opal_hmi_handler_init(void);
 extern int opal_event_init(void);
+extern void opal_hdat_sysfs_init(void);
 
 extern int opal_machine_check(struct pt_regs *regs);
 extern bool opal_mce_check_early_recovery(struct pt_regs *regs);

arch/powerpc/platforms/powernv/Makefile +1 -0

index b5d98cb..9a0c9d6 100644
@@ -3,6 +3,7 @@ obj-y			+= opal-rtc.o opal-nvram.o opal-lpc.o opal-flash.o
 obj-y			+= rng.o opal-elog.o opal-dump.o opal-sysparam.o opal-sensor.o
 obj-y			+= opal-msglog.o opal-hmi.o opal-power.o opal-irqchip.o
 obj-y			+= opal-kmsg.o
+obj-y			+= opal-hdat.o
 
 obj-$(CONFIG_SMP)	+= smp.o subcore.o subcore-asm.o
 obj-$(CONFIG_PCI)	+= pci.o pci-ioda.o npu-dma.o

arch/powerpc/platforms/powernv/opal-hdat.c new file +65 -0

65 lines not on this page · raw message

arch/powerpc/platforms/powernv/opal.c +2 -0

index 2822935..cae3745 100644
@@ -740,6 +740,8 @@ static int __init opal_init(void)
 		opal_sys_param_init();
 		/* Setup message log sysfs interface. */
 		opal_msglog_sysfs_init();
+		/* Create hdat object under sys/firmware/opal */
+		opal_hdat_sysfs_init();
 	}
 
 	/* Initialize platform devices: IPMI backend, PRD & flash interface */

Signature · 1 line

2.9.3

Andrew Donnellan

Reviewed-by on v3 1/1

2017-02-27 02:00 UTC #1 lore raw

Quoted 6 lines · Matt Brown

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 <[email protected]>

Changes look good, thanks for addressing the comments! Still a couple of minor points below, otherwise:

Reviewed-by Andrew Donnellan <[email protected]>

Stewart: this might need your ACK?

Quoted 31 lines

---

Changes between v2 to v3:
- fixed header comments
- simplified if statement

---
 arch/powerpc/include/asm/opal.h            |  1 +
 arch/powerpc/platforms/powernv/Makefile    |  1 +
 arch/powerpc/platforms/powernv/opal-hdat.c | 65 ++++++++++++++++++++++++++++++
 arch/powerpc/platforms/powernv/opal.c      |  2 +
 4 files changed, 69 insertions(+)
 create mode 100644 arch/powerpc/platforms/powernv/opal-hdat.c

arch/powerpc/include/asm/opal.h +1 -0

index 5c7db0f..b26944e 100644
@@ -277,6 +277,7 @@ extern int opal_async_comp_init(void);
 extern int opal_sensor_init(void);
 extern int opal_hmi_handler_init(void);
 extern int opal_event_init(void);
+extern void opal_hdat_sysfs_init(void);
 
 extern int opal_machine_check(struct pt_regs *regs);
 extern bool opal_mce_check_early_recovery(struct pt_regs *regs);

arch/powerpc/platforms/powernv/Makefile +0 -0

index b5d98cb..9a0c9d6 100644

arch/powerpc/platforms/powernv/Makefile obj-y += opal-rtc.o opal-nvram.o opal-lpc.o opal-flash.o · line 6

Quoted 4 lines

arch/powerpc/platforms/powernv/Makefile +1 -0

@@ -3,6 +3,7 @@ obj-y			+= opal-rtc.o opal-nvram.o opal-lpc.o opal-flash.o
 obj-y			+= rng.o opal-elog.o opal-dump.o opal-sysparam.o opal-sensor.o
 obj-y			+= opal-msglog.o opal-hmi.o opal-power.o opal-irqchip.o
 obj-y			+= opal-kmsg.o
+obj-y			+= opal-hdat.o

Normally we keep putting new object files on the same line until it gets long enough that we have to break it. This is very minor though :)

Quoted 23 lines

arch/powerpc/platforms/powernv/Makefile +0 -0

 obj-$(CONFIG_SMP)	+= smp.o subcore.o subcore-asm.o
 obj-$(CONFIG_PCI)	+= pci.o pci-ioda.o npu-dma.o

arch/powerpc/platforms/powernv/opal-hdat.c new file +14 -0

new file mode 100644
index 0000000..3315dd3
@@ -0,0 +1,65 @@
+/*
+ * PowerNV OPAL HDAT interface
+ *
+ * Author: Matt Brown <[email protected]>
+ *
+ * Copyright 2017 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <asm/io.h>

arch/powerpc/platforms/powernv/opal-hdat.c line 24

Quoted 10 lines

arch/powerpc/platforms/powernv/opal-hdat.c new file +10 -0

+#include <asm/opal.h>
+#include <linux/of.h>
+#include <linux/types.h>
+
+struct hdat_info {
+	char *base;
+	u64 size;
+};
+
+static struct hdat_info hdat_inf;

As Oliver pointed out, we could do with a better name than hdat_inf - it's only one character away from the name of the struct type. Hmm, perhaps "hdat_location", or maybe Oliver has a better suggestion.

Signature · 2 lines

Andrew Donnellan              OzLabs, ADL Canberra
[email protected]  IBM Australia Limited

Oliver O'Halloran

Reviewed-by

2017-02-27 02:13 UTC #2 lore raw

Quoted 96 lines · Andrew Donnellan

Quoted 7 lines · Matt Brown
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 <[email protected]>
Changes look good, thanks for addressing the comments! Still a couple of minor points below, otherwise:
Reviewed-by Andrew Donnellan <[email protected]>
Stewart: this might need your ACK?
Quoted 34 lines
---
Changes between v2 to v3:
- fixed header comments
- simplified if statement
---
 arch/powerpc/include/asm/opal.h            |  1 +
 arch/powerpc/platforms/powernv/Makefile    |  1 +
 arch/powerpc/platforms/powernv/opal-hdat.c | 65
++++++++++++++++++++++++++++++
 arch/powerpc/platforms/powernv/opal.c      |  2 +
 4 files changed, 69 insertions(+)
 create mode 100644 arch/powerpc/platforms/powernv/opal-hdat.c
arch/powerpc/include/asm/opal.h +1 -0
index 5c7db0f..b26944e 100644
@@ -277,6 +277,7 @@ extern int opal_async_comp_init(void);
 extern int opal_sensor_init(void);
 extern int opal_hmi_handler_init(void);
 extern int opal_event_init(void);
+extern void opal_hdat_sysfs_init(void);
 
 extern int opal_machine_check(struct pt_regs *regs);
 extern bool opal_mce_check_early_recovery(struct pt_regs *regs);
arch/powerpc/platforms/powernv/Makefile +0 -0
index b5d98cb..9a0c9d6 100644
@@ -3,6 +3,7 @@ obj-y                   += opal-rtc.o opal-nvram.o
Quoted 7 lines
opal-lpc.o opal-flash.o
obj-y += rng.o opal-elog.o opal-dump.o opal-sysparam.o
opal-sensor.o
obj-y += opal-msglog.o opal-hmi.o opal-power.o
opal-irqchip.o
obj-y += opal-kmsg.o
+obj-y += opal-hdat.o
Normally we keep putting new object files on the same line until it gets long enough that we have to break it. This is very minor though :)
Quoted 31 lines
obj-$(CONFIG_SMP) += smp.o subcore.o subcore-asm.o
obj-$(CONFIG_PCI) += pci.o pci-ioda.o npu-dma.o
arch/powerpc/platforms/powernv/opal-hdat.c new file +21 -0
new file mode 100644
index 0000000..3315dd3
@@ -0,0 +1,65 @@
+/*
+ * PowerNV OPAL HDAT interface
+ *
+ * Author: Matt Brown <[email protected]>
+ *
+ * Copyright 2017 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <asm/io.h>
+#include <asm/opal.h>
+#include <linux/of.h>
+#include <linux/types.h>
+
+struct hdat_info {
+       char *base;
+       u64 size;

Quoted 8 lines

arch/powerpc/platforms/powernv/opal-hdat.c line 24
Quoted 3 lines
arch/powerpc/platforms/powernv/opal-hdat.c new file +3 -0
+};
+
+static struct hdat_info hdat_inf;
As Oliver pointed out, we could do with a better name than hdat_inf - it's only one character away from the name of the struct type. Hmm, perhaps "hdat_location", or maybe Oliver has a better suggestion.

I'm not that bothered by it.

Reviewed-by Oliver O'Halloran <[email protected]>

Quoted 6 lines

--
Andrew Donnellan OzLabs, ADL Canberra
[email protected] IBM Australia Limited

Michael Ellerman

reviewer

2017-02-27 10:52 UTC #3 lore raw

In reply to Andrew Donnellan

Quoted 6 lines · Andrew Donnellan

Quoted 5 lines · Matt Brown
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.

Quoted 8 lines

Quoted 1 line
Signed-off-by Matt Brown <[email protected]>
Changes look good, thanks for addressing the comments! Still a couple of minor points below, otherwise:
Reviewed-by Andrew Donnellan <[email protected]>
Stewart: this might need your ACK?

I don't see it in skiboot yet ?

Quoted 4 lines

Quoted 4 lines
arch/powerpc/platforms/powernv/Makefile +0 -0
index b5d98cb..9a0c9d6 100644

Quoted 8 lines

arch/powerpc/platforms/powernv/Makefile obj-y += opal-rtc.o opal-nvram.o opal-lpc.o opal-flash.o · line 6
Quoted 5 lines
arch/powerpc/platforms/powernv/Makefile +1 -0
@@ -3,6 +3,7 @@ obj-y			+= opal-rtc.o opal-nvram.o opal-lpc.o opal-flash.o
 obj-y			+= rng.o opal-elog.o opal-dump.o opal-sysparam.o opal-sensor.o
 obj-y			+= opal-msglog.o opal-hmi.o opal-power.o opal-irqchip.o
 obj-y			+= opal-kmsg.o
+obj-y			+= opal-hdat.o
Normally we keep putting new object files on the same line until it gets long enough that we have to break it. This is very minor though :)

Yeah, please put it on the same line as opal-kmsg.o

Quoted 4 lines

Quoted 4 lines
arch/powerpc/platforms/powernv/opal-hdat.c new file +0 -1
new file mode 100644
index 0000000..3315dd3
--- /dev/null

Quoted 8 lines

arch/powerpc/platforms/powernv/opal-hdat.c line 6
Quoted 8 lines
+++ b/arch/powerpc/platforms/powernv/opal-hdat.c
arch/powerpc/platforms/powernv/opal-hdat.c new file +6 -0
@@ -0,0 +1,65 @@
+/*
+ * PowerNV OPAL HDAT interface
+ *
+ * Author: Matt Brown <[email protected]>
+ *
+ * Copyright 2017 IBM Corp.

I dislike email addresses in source files, they just end up being wrong and needing to be updated. We have your email in the commit log anyway.

So I prefer:

Which I believe is also blessed by the lawyers.

Quoted 10 lines

arch/powerpc/platforms/powernv/opal-hdat.c line 17

Quoted 10 lines

arch/powerpc/platforms/powernv/opal-hdat.c new file +10 -0

+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#include <asm/io.h>
+#include <asm/opal.h>
+#include <linux/of.h>
+#include <linux/types.h>

Preferred style is to put the linux includes first, before the asm ones.

Quoted 10 lines

arch/powerpc/platforms/powernv/opal-hdat.c line 24
Quoted 6 lines
arch/powerpc/platforms/powernv/opal-hdat.c new file +6 -0
+struct hdat_info {
+	char *base;
+	u64 size;
+};
+
+static struct hdat_info hdat_inf;
As Oliver pointed out, we could do with a better name than hdat_inf - it's only one character away from the name of the struct type. Hmm, perhaps "hdat_location", or maybe Oliver has a better suggestion.

Why not hdat_info ?

In fact for bonus points, you can just do:

static struct {
	char *base;
	u64 size;
} hdat_info;

cheers

Michael Ellerman

reviewer

2017-02-27 10:56 UTC #4 lore raw

In reply to Matt Brown

Quoted 6 lines · Matt Brown

arch/powerpc/platforms/powernv/opal-hdat.c new file +0 -0
new file mode 100644
index 0000000..3315dd3
@@ -0,0 +1,65 @@

...

arch/powerpc/platforms/powernv/opal-hdat.c line 41

Quoted 5 lines

arch/powerpc/platforms/powernv/opal-hdat.c new file +5 -0

+
+
+/* HDAT attribute for sysfs */
+static struct bin_attribute hdat_attr = {
+	.attr = {.name = "hdat", .mode = 0444},

                                         ^^^^

ajd and oohal report to my office.

cheers

Oliver O'Halloran

2017-02-27 11:41 UTC #5 lore raw

Quoted 8 lines · Michael Ellerman

Quoted 6 lines · Matt Brown
arch/powerpc/platforms/powernv/opal-hdat.c new file +0 -0
new file mode 100644
index 0000000..3315dd3
@@ -0,0 +1,65 @@
...

Quoted 7 lines

arch/powerpc/platforms/powernv/opal-hdat.c line 41
Quoted 5 lines
arch/powerpc/platforms/powernv/opal-hdat.c new file +5 -0
+
+
+/* HDAT attribute for sysfs */
+static struct bin_attribute hdat_attr = {
+     .attr = {.name = "hdat", .mode = 0444},
                                         ^^^^
ajd and oohal report to my office.

I don't think there's anything in the HDAT that's sensitive. That said, this might not be true in the future so making it only readable by root might be a good idea.

Oliver

Michael Ellerman

reviewer

2017-02-28 03:14 UTC #6 lore raw

Quoted 17 lines · Oliver O'Halloran

Quoted 8 lines · Michael Ellerman
Quoted 6 lines · Matt Brown
arch/powerpc/platforms/powernv/opal-hdat.c new file +0 -0
new file mode 100644
index 0000000..3315dd3
@@ -0,0 +1,65 @@
...
Quoted 7 lines
arch/powerpc/platforms/powernv/opal-hdat.c line 41
Quoted 5 lines
arch/powerpc/platforms/powernv/opal-hdat.c new file +5 -0
+
+
+/* HDAT attribute for sysfs */
+static struct bin_attribute hdat_attr = {
+     .attr = {.name = "hdat", .mode = 0444},
                                         ^^^^
ajd and oohal report to my office.

Quoted 3 lines

I don't think there's anything in the HDAT that's sensitive. That said, this might not be true in the future so making it only readable by root might be a good idea.

Right. What's sensitive can also change over time, in addition to the actual content changing without our knowledge.

This is also firmly a debugging thing, so unless there's a compelling reason why it should be world readable then it shouldn't be.

So 0400 please.

cheers

Andrew Donnellan

2017-02-28 23:20 UTC #7 lore raw

In reply to Michael Ellerman

Quoted 5 lines · Michael Ellerman

arch/powerpc/platforms/powernv/opal-hdat.c line 41
Quoted 3 lines
arch/powerpc/platforms/powernv/opal-hdat.c new file +3 -0
+/* HDAT attribute for sysfs */
+static struct bin_attribute hdat_attr = {
+	.attr = {.name = "hdat", .mode = 0444},
                                         ^^^^
ajd and oohal report to my office.

...ACK... :/

Signature · 2 lines

Andrew Donnellan              OzLabs, ADL Canberra
[email protected]  IBM Australia Limited