[PATCHv4 0/4] Introduce the /proc/socinfo and use it to export OMAP data

[PATCHv4 0/4] Introduce the /proc/socinfo and use it to export OMAP data

[Eduardo Valentin]

Hello all,

Here is the patch set to export OMAP id code, production id and die id.
The history of this change is as follows:

1. First attempt tried adding those data under sysfs node. Original patch by
Peter De Schrijver [1]. Then, it was suggested to move it to debugfs, which was
rejected as this info is actually needed in production systems.

2. Then it has been moved to /proc/cpuinfo [2]. Then, it was suggested to do
not add this kind of info there, as it is not really CPU data.

Now, I'm sending this version which will introduce first the /proc/socinfo.
Then, following patches will add OMAP data into that node. And finally,
will add OMAP id, production and die id code.

Just for reference, I'm also pasting the previous 0/4 message:

-------------------------------------------------------------------------------
I'm resending this series with minor change in the subject prefix of patch 4/4.
Removed the PM: prefix. Everything else is same. So, I kept the v3 prefix as well.

v3:
And now v3 of this series. Basically a minor change wrt string manipulation.
No need to use strlen in so many places. Previous log for reference:

v2:

Here is the version 2 of this series. Now die id is protected using
same x86 protection mechanism to hide x86 product number. Besides,
a compilation Kconfig option has been added for DIE ID as well.

Here is previous PATCH 0/4 message:

v1:

This series is to continue what has been discussed several weeks ago
wrt IDCODE patch. Original patch was made by Peter and discussion is here:
http://www.mail-archive.com/linux-omap at vger.kernel.org/msg17553.html

So, the conclusion was that IDCODE info is useful even in production systems,
and for that debugfs is not a good choice to export it. One suggestion was to
add it under /proc/cpuinfo. However this entry nowadays exports only ARM related
information.

So this series does the trick by extending the ARM /proc/cpuinfo to include
soc info data. There are a few ways to add a hook for soc specific data. But
I've decided to implement it via the simplest way I found. Basically it is
same thing which is done for system_rev, system_serial_low and system_serial_high.
Then, now we have system_soc_info, which is printed only if there is something
useful there.

As usual, comments are welcome.

BR,

Eduardo Valentin (4):
  procfs: Introduce socinfo under /proc
  mach-omap2: export omap2 info under /proc/socinfo
  mach-omap1: export omap1 info under /proc/socinfo
  OMAP3: export chip IDCODE, Production ID and Die ID

 Documentation/filesystems/proc.txt  |    1 +
 Documentation/kernel-parameters.txt |    2 +
 arch/arm/Kconfig                    |    1 +
 arch/arm/mach-omap1/id.c            |   45 +++++++++++++++++---
 arch/arm/mach-omap2/Kconfig         |   10 ++++
 arch/arm/mach-omap2/id.c            |   81 ++++++++++++++++++++++++++++++++---
 fs/proc/Kconfig                     |    7 +++
 fs/proc/Makefile                    |    1 +
 fs/proc/socinfo.c                   |   33 ++++++++++++++
 9 files changed, 169 insertions(+), 12 deletions(-)
 create mode 100644 fs/proc/socinfo.c

-------------------------------------------------------------------------------
[1] - http://www.mail-archive.com/linux-omap at vger.kernel.org/msg17553.html
[2] - http://marc.info/?l=linux-omap&m=127304890312365&w=2

[PATCHv4 1/4] procfs: Introduce socinfo under /proc

[Eduardo Valentin]

From: Eduardo Valentin <eduardo.valentin@nokia.com>

This patch introduce the /proc/socinfo node. Its purpose is to
export System on Chip information and specific bits.

The way it is done is basically same structure which is used to build
/proc/cpuinfo. Thus, it relies on the existence of socinfo_op seq_operation
structure. This structure must be provided by soc core code.

Signed-off-by: Eduardo Valentin <eduardo.valentin@nokia.com>
---
 Documentation/filesystems/proc.txt |    1 +
 fs/proc/Kconfig                    |    7 +++++++
 fs/proc/Makefile                   |    1 +
 fs/proc/socinfo.c                  |   33 +++++++++++++++++++++++++++++++++
 4 files changed, 42 insertions(+), 0 deletions(-)
 create mode 100644 fs/proc/socinfo.c

diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
index a4f30fa..039bcb7 100644
--- a/Documentation/filesystems/proc.txt
+++ b/Documentation/filesystems/proc.txt
@@ -415,6 +415,7 @@ Table 1-5: Kernel info in /proc
  bus         Directory containing bus specific information     
  cmdline     Kernel command line                               
  cpuinfo     Info about the CPU                                
+ socinfo     Info about the System on Chip                     
  devices     Available devices (block and character)           
  dma         Used DMS channels                                 
  filesystems Supported filesystems                             
diff --git a/fs/proc/Kconfig b/fs/proc/Kconfig
index 50f8f06..e683d62 100644
--- a/fs/proc/Kconfig
+++ b/fs/proc/Kconfig
@@ -67,3 +67,10 @@ config PROC_PAGE_MONITOR
 	  /proc/pid/smaps, /proc/pid/clear_refs, /proc/pid/pagemap,
 	  /proc/kpagecount, and /proc/kpageflags. Disabling these
           interfaces will reduce the size of the kernel by approximately 4kb.
+
+config PROC_SOCINFO
+ 	default y
+	depends on PROC_FS
+	bool "Enable /proc/socinfo" if EMBEDDED
+ 	help
+	  Say Y here if you need to see information about the your System on Chip.
diff --git a/fs/proc/Makefile b/fs/proc/Makefile
index 11a7b5c..7757d44 100644
--- a/fs/proc/Makefile
+++ b/fs/proc/Makefile
@@ -26,3 +26,4 @@ proc-$(CONFIG_PROC_VMCORE)	+= vmcore.o
 proc-$(CONFIG_PROC_DEVICETREE)	+= proc_devtree.o
 proc-$(CONFIG_PRINTK)	+= kmsg.o
 proc-$(CONFIG_PROC_PAGE_MONITOR)	+= page.o
+proc-$(CONFIG_PROC_SOCINFO)	+= socinfo.o
diff --git a/fs/proc/socinfo.c b/fs/proc/socinfo.c
new file mode 100644
index 0000000..05bfc4f
--- /dev/null
+++ b/fs/proc/socinfo.c
@@ -0,0 +1,33 @@
+/*
+ *  fs/proc/socinfo.c
+ *
+ *  Copyright (C) 2010 Nokia Corporation
+ *
+ *  Contact: Eduardo Valentin <eduardo.valentin@nokia.com>
+ *
+ *  proc socinfo file
+ */
+#include <linux/fs.h>
+#include <linux/init.h>
+#include <linux/proc_fs.h>
+#include <linux/seq_file.h>
+
+extern const struct seq_operations socinfo_op;
+static int socinfo_open(struct inode *inode, struct file *file)
+{
+	return seq_open(file, &socinfo_op);
+}
+
+static const struct file_operations proc_socinfo_operations = {
+	.open		= socinfo_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= seq_release,
+};
+
+static int __init proc_socinfo_init(void)
+{
+	proc_create("socinfo", 0, NULL, &proc_socinfo_operations);
+	return 0;
+}
+module_init(proc_socinfo_init);
-- 
1.7.0.4.361.g8b5fe.dirty

[PATCHv4 2/4] mach-omap2: export omap2 info under /proc/socinfo

[Eduardo Valentin]

From: Eduardo Valentin <eduardo.valentin@nokia.com>

Report OMAP name and rev under /proc/socinfo node.

Signed-off-by: Eduardo Valentin <eduardo.valentin@nokia.com>
---
 arch/arm/Kconfig         |    1 +
 arch/arm/mach-omap2/id.c |   48 ++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index c5408bf..7456967 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -798,6 +798,7 @@ config ARCH_OMAP
 	select GENERIC_TIME
 	select GENERIC_CLOCKEVENTS
 	select ARCH_HAS_HOLES_MEMORYMODEL
+	select PROC_SOC_INFO
 	help
 	  Support for TI's OMAP platform (OMAP1 and OMAP2).
 
diff --git a/arch/arm/mach-omap2/id.c b/arch/arm/mach-omap2/id.c
index 37b8a1a..8ecd8e2 100644
--- a/arch/arm/mach-omap2/id.c
+++ b/arch/arm/mach-omap2/id.c
@@ -18,6 +18,7 @@
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/io.h>
+#include <linux/seq_file.h>
 
 #include <asm/cputype.h>
 
@@ -101,10 +102,12 @@ static struct omap_id omap_ids[] __initdata = {
 
 static void __iomem *tap_base;
 static u16 tap_prod_id;
+#define SOCINFO_SZ		128
+static char socinfo[SOCINFO_SZ];
 
 void __init omap24xx_check_revision(void)
 {
-	int i, j;
+	int i, j, sz;
 	u32 idcode, prod_id;
 	u16 hawkeye;
 	u8  dev_type, rev;
@@ -152,10 +155,11 @@ void __init omap24xx_check_revision(void)
 		j = i;
 	}
 
-	pr_info("OMAP%04x", omap_rev() >> 16);
+	sz = snprintf(socinfo, SOCINFO_SZ, "OMAP%04x", omap_rev() >> 16);
 	if ((omap_rev() >> 8) & 0x0f)
-		pr_info("ES%x", (omap_rev() >> 12) & 0xf);
-	pr_info("\n");
+		snprintf(socinfo + sz, SOCINFO_SZ - sz, "ES%x",
+						(omap_rev() >> 12) & 0xf);
+	pr_info("%s\n", socinfo);
 }
 
 #define OMAP3_CHECK_FEATURE(status,feat)				\
@@ -286,7 +290,9 @@ void __init omap4_check_revision(void)
 	if ((hawkeye == 0xb852) && (rev == 0x0)) {
 		omap_revision = OMAP4430_REV_ES1_0;
 		omap_chip.oc |= CHIP_IS_OMAP4430ES1;
-		pr_info("OMAP%04x %s\n", omap_rev() >> 16, rev_name);
+		snprintf(socinfo, SOCINFO_SZ, "OMAP%04x %s\n",
+				omap_rev() >> 16, rev_name);
+		pr_info("%s\n", socinfo);
 		return;
 	}
 
@@ -356,7 +362,8 @@ void __init omap3_cpuinfo(void)
 	}
 
 	/* Print verbose information */
-	pr_info("%s ES%s (", cpu_name, cpu_rev);
+	snprintf(socinfo, SOCINFO_SZ, "%s ES%s", cpu_name, cpu_rev);
+	pr_info("%s (", socinfo);
 
 	OMAP3_SHOW_FEATURE(l2cache);
 	OMAP3_SHOW_FEATURE(iva);
@@ -425,3 +432,32 @@ void __init omap2_set_globals_tap(struct omap_globals *omap2_globals)
 	else
 		tap_prod_id = 0x0208;
 }
+
+static int c_show(struct seq_file *m, void *v)
+{
+	seq_printf(m, "SoC\t: %s\n", socinfo);
+
+	return 0;
+}
+
+static void *c_start(struct seq_file *m, loff_t *pos)
+{
+	return *pos < 1 ? (void *)1 : NULL;
+}
+
+static void *c_next(struct seq_file *m, void *v, loff_t *pos)
+{
+	++*pos;
+	return NULL;
+}
+
+static void c_stop(struct seq_file *m, void *v)
+{
+}
+
+const struct seq_operations socinfo_op = {
+	.start	= c_start,
+	.next	= c_next,
+	.stop	= c_stop,
+	.show	= c_show
+};
-- 
1.7.0.4.361.g8b5fe.dirty

[PATCHv4 3/4] mach-omap1: export omap1 info under /proc/socinfo

[Eduardo Valentin]

From: Eduardo Valentin <eduardo.valentin@nokia.com>

Report OMAP name and rev under /proc/socinfo node.

Signed-off-by: Eduardo Valentin <eduardo.valentin@nokia.com>
---
 arch/arm/mach-omap1/id.c |   45 +++++++++++++++++++++++++++++++++++++++------
 1 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-omap1/id.c b/arch/arm/mach-omap1/id.c
index a0e3560..917892b 100644
--- a/arch/arm/mach-omap1/id.c
+++ b/arch/arm/mach-omap1/id.c
@@ -15,6 +15,7 @@
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/io.h>
+#include <linux/seq_file.h>
 #include <plat/cpu.h>
 
 #define OMAP_DIE_ID_0		0xfffe1800
@@ -118,9 +119,12 @@ static u8 __init omap_get_die_rev(void)
 	return die_rev;
 }
 
+#define SOCINFO_SZ		128
+static char socinfo[SOCINFO_SZ];
+
 void __init omap_check_revision(void)
 {
-	int i;
+	int i, sz;
 	u16 jtag_id;
 	u8 die_rev;
 	u32 omap_id;
@@ -194,11 +198,40 @@ void __init omap_check_revision(void)
 		printk(KERN_INFO "Unknown OMAP cpu type: 0x%02x\n", cpu_type);
 	}
 
-	printk(KERN_INFO "OMAP%04x", omap_revision >> 16);
+	sz = snprintf(socinfo, SOCINFO_SZ, "OMAP%04x", omap_revision >> 16);
 	if ((omap_revision >> 8) & 0xff)
-		printk(KERN_INFO "%x", (omap_revision >> 8) & 0xff);
-	printk(KERN_INFO " revision %i handled as %02xxx id: %08x%08x\n",
-	       die_rev, omap_revision & 0xff, system_serial_low,
-	       system_serial_high);
+		snprintf(socinfo + sz, SOCINFO_SZ - sz, "%x",
+						(omap_revision >> 8) & 0xff);
+	pr_info("%s revision %i handled as %02xxx id: %08x%08x\n",
+		socinfo, die_rev, omap_revision & 0xff, system_serial_low,
+		system_serial_high);
+}
+
+static int c_show(struct seq_file *m, void *v)
+{
+	seq_printf(m, "SoC\t: %s\n", socinfo);
+
+	return 0;
+}
+
+static void *c_start(struct seq_file *m, loff_t *pos)
+{
+	return *pos < 1 ? (void *)1 : NULL;
 }
 
+static void *c_next(struct seq_file *m, void *v, loff_t *pos)
+{
+	++*pos;
+	return NULL;
+}
+
+static void c_stop(struct seq_file *m, void *v)
+{
+}
+
+const struct seq_operations socinfo_op = {
+	.start	= c_start,
+	.next	= c_next,
+	.stop	= c_stop,
+	.show	= c_show
+};
-- 
1.7.0.4.361.g8b5fe.dirty

[PATCHv4 4/4] OMAP3: export chip IDCODE, Production ID and Die ID

[Eduardo Valentin]

From: Eduardo Valentin <eduardo.valentin@nokia.com>

This patch exports the OMAP3 IDCODE and Production ID to userspace
via /proc/socinfo.

Die ID is also exported depending on what users pass as kernel
parameter. It is same protection mechanism made for x86 product
number. So, if user passes "omap3_die_id" parameter, it will append
die id code into /proc/socinfo as well. A Kconfig option has been
added as well, so it can be configurable during compilation time.

This can be used to track down silicon specific issues. The info is
exported via /proc/socinfo because then it can be possible to include this
in corematic dumps.

This is based on Peter De Schrijver patch, which export same info via sysfs.

Signed-off-by: Eduardo Valentin <eduardo.valentin@nokia.com>
---
 Documentation/kernel-parameters.txt |    2 ++
 arch/arm/mach-omap2/Kconfig         |   10 ++++++++++
 arch/arm/mach-omap2/id.c            |   35 ++++++++++++++++++++++++++++++++++-
 3 files changed, 46 insertions(+), 1 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 839b21b..8010cfb 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1809,6 +1809,8 @@ and is between 256 and 4096 characters. It is defined in the file
 			waiting for the ACK, so if this is set too high
 			interrupts *may* be lost!
 
+	omap3_die_id	[OMAP] Append DIE ID info under /proc/socinfo
+
 	omap_mux=	[OMAP] Override bootloader pin multiplexing.
 			Format: <mux_mode0.mode_name=value>...
 			For example, to override I2C bus2:
diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
index 2455dcc..fb8abed 100644
--- a/arch/arm/mach-omap2/Kconfig
+++ b/arch/arm/mach-omap2/Kconfig
@@ -169,3 +169,13 @@ config OMAP3_SDRC_AC_TIMING
 	  wish to say no.  Selecting yes without understanding what is
 	  going on could result in system crashes;
 
+config OMAP3_EXPORT_DIE_ID
+	bool "Export DIE ID code under /proc/socinfo"
+	depends on ARCH_OMAP3
+	default n
+	help
+	  Say Y here if you need DIE ID code to be exported via /proc/socinfo
+	  in production systems. You will need also to explicitly flag it by
+	  appending the "omap3_die_id" parameter to your boot command line.
+
+
diff --git a/arch/arm/mach-omap2/id.c b/arch/arm/mach-omap2/id.c
index 8ecd8e2..9663066 100644
--- a/arch/arm/mach-omap2/id.c
+++ b/arch/arm/mach-omap2/id.c
@@ -77,6 +77,10 @@ EXPORT_SYMBOL(omap_type);
 /*----------------------------------------------------------------------------*/
 
 #define OMAP_TAP_IDCODE		0x0204
+#define OMAP_TAP_PROD_ID_0      0x0208
+#define OMAP_TAP_PROD_ID_1      0x020c
+#define OMAP_TAP_PROD_ID_2      0x0210
+#define OMAP_TAP_PROD_ID_3      0x0214
 #define OMAP_TAP_DIE_ID_0	0x0218
 #define OMAP_TAP_DIE_ID_1	0x021C
 #define OMAP_TAP_DIE_ID_2	0x0220
@@ -305,6 +309,7 @@ void __init omap4_check_revision(void)
 
 void __init omap3_cpuinfo(void)
 {
+	int sz;
 	u8 rev = GET_OMAP_REVISION();
 	char cpu_name[16], cpu_rev[16];
 
@@ -362,7 +367,7 @@ void __init omap3_cpuinfo(void)
 	}
 
 	/* Print verbose information */
-	snprintf(socinfo, SOCINFO_SZ, "%s ES%s", cpu_name, cpu_rev);
+	sz = snprintf(socinfo, SOCINFO_SZ, "%s ES%s", cpu_name, cpu_rev);
 	pr_info("%s (", socinfo);
 
 	OMAP3_SHOW_FEATURE(l2cache);
@@ -373,7 +378,35 @@ void __init omap3_cpuinfo(void)
 	OMAP3_SHOW_FEATURE(192mhz_clk);
 
 	printk(")\n");
+
+	/* Append OMAP3 IDCODE and Production ID to system_soc_info */
+	snprintf(socinfo + sz, SOCINFO_SZ - sz,
+			"\nIDCODE\t: %08x\nPr. ID\t: %08x %08x %08x %08x",
+			read_tap_reg(OMAP_TAP_IDCODE),
+			read_tap_reg(OMAP_TAP_PROD_ID_0),
+			read_tap_reg(OMAP_TAP_PROD_ID_1),
+			read_tap_reg(OMAP_TAP_PROD_ID_2),
+			read_tap_reg(OMAP_TAP_PROD_ID_3));
+
+}
+
+#ifdef CONFIG_OMAP3_EXPORT_DIE_ID
+static int __init omap3_die_id_setup(char *s)
+{
+	int sz;
+
+	sz = strlen(socinfo);
+	snprintf(socinfo + sz, SOCINFO_SZ - sz,
+			"\nDie ID\t: %08x %08x %08x %08x",
+			read_tap_reg(OMAP_TAP_DIE_ID_0),
+			read_tap_reg(OMAP_TAP_DIE_ID_1),
+			read_tap_reg(OMAP_TAP_DIE_ID_2),
+			read_tap_reg(OMAP_TAP_DIE_ID_3));
+
+	return 1;
 }
+__setup("omap3_die_id", omap3_die_id_setup);
+#endif
 
 /*
  * Try to detect the exact revision of the omap we're running on
-- 
1.7.0.4.361.g8b5fe.dirty

[PATCHv4 3/4] mach-omap1: export omap1 info under /proc/socinfo

[Russell King - ARM Linux]

On Mon, May 10, 2010 at 01:37:36PM +0300, Eduardo Valentin wrote:
> From: Eduardo Valentin <eduardo.valentin@nokia.com>
> 
> Report OMAP name and rev under /proc/socinfo node.

I think this needs to be combined with the previous patch.  The previous
patch enables the socinfo file, which means with patch 2 applied but not
patch 3, OMAP1 breaks.

[PATCHv4 1/4] procfs: Introduce socinfo under /proc

[Paul Mundt]

On Mon, May 10, 2010 at 01:37:34PM +0300, Eduardo Valentin wrote:
> + */
> +#include <linux/fs.h>
> +#include <linux/init.h>
> +#include <linux/proc_fs.h>
> +#include <linux/seq_file.h>
> +
> +extern const struct seq_operations socinfo_op;

This doesn't look promising..

> +static int socinfo_open(struct inode *inode, struct file *file)
> +{
> +	return seq_open(file, &socinfo_op);
> +}
> +
If you use single_open() ..

> +static const struct file_operations proc_socinfo_operations = {
> +	.open		= socinfo_open,
> +	.read		= seq_read,
> +	.llseek		= seq_lseek,
> +	.release	= seq_release,
> +};
> +
.. and single_release(), then none of the seq_operations are necessary.

> +static int __init proc_socinfo_init(void)
> +{
> +	proc_create("socinfo", 0, NULL, &proc_socinfo_operations);
> +	return 0;
> +}
> +module_init(proc_socinfo_init);

proc_create() can fail, so some error handling here would be nice.

On Mon, May 10, 2010 at 01:37:35PM +0300, Eduardo Valentin wrote:
> +static int c_show(struct seq_file *m, void *v)
> +{
> +	seq_printf(m, "SoC\t: %s\n", socinfo);
> +
> +	return 0;
> +}
> +
> +static void *c_start(struct seq_file *m, loff_t *pos)
> +{
> +	return *pos < 1 ? (void *)1 : NULL;
> +}
> +
> +static void *c_next(struct seq_file *m, void *v, loff_t *pos)
> +{
> +	++*pos;
> +	return NULL;
> +}
> +
> +static void c_stop(struct seq_file *m, void *v)
> +{
> +}
> +
> +const struct seq_operations socinfo_op = {
> +	.start	= c_start,
> +	.next	= c_next,
> +	.stop	= c_stop,
> +	.show	= c_show
> +};

You'll still need the show function, but all of the rest of this is just
duplicating what single_open() already does. If the socinfo string is
static you may also want to rework this a bit so you can just stash the
string in the proc_dir_entry private data. Combine this with something
like kstrdup() and you'll save yourself a bit of stack while you're at
it.

[PATCHv4 3/4] mach-omap1: export omap1 info under /proc/socinfo

[Eduardo Valentin]

On Mon, May 10, 2010 at 12:52:00PM +0200, Russell King wrote:
> On Mon, May 10, 2010 at 01:37:36PM +0300, Eduardo Valentin wrote:
> > From: Eduardo Valentin <eduardo.valentin@nokia.com>
> > 
> > Report OMAP name and rev under /proc/socinfo node.
> 
> I think this needs to be combined with the previous patch.  The previous
> patch enables the socinfo file, which means with patch 2 applied but not
> patch 3, OMAP1 breaks.

yes, indeed. I will squash both.

Tks,

--
Eduardo Valentin

[PATCHv4 1/4] procfs: Introduce socinfo under /proc

[Eduardo Valentin]

On Mon, May 10, 2010 at 01:13:00PM +0200, ext Paul Mundt wrote:
> On Mon, May 10, 2010 at 01:37:34PM +0300, Eduardo Valentin wrote:
> > + */
> > +#include <linux/fs.h>
> > +#include <linux/init.h>
> > +#include <linux/proc_fs.h>
> > +#include <linux/seq_file.h>
> > +
> > +extern const struct seq_operations socinfo_op;
> 
> This doesn't look promising..

Right.. as stated in patch description (maybe not that clear), this was
basically same thing which you see under fs/proc/cpuinfo.c

> 
> > +static int socinfo_open(struct inode *inode, struct file *file)
> > +{
> > +	return seq_open(file, &socinfo_op);
> > +}
> > +
> If you use single_open() ..
> 
> > +static const struct file_operations proc_socinfo_operations = {
> > +	.open		= socinfo_open,
> > +	.read		= seq_read,
> > +	.llseek		= seq_lseek,
> > +	.release	= seq_release,
> > +};
> > +
> .. and single_release(), then none of the seq_operations are necessary.

Good! will replace than with single_[open,release].

> 
> > +static int __init proc_socinfo_init(void)
> > +{
> > +	proc_create("socinfo", 0, NULL, &proc_socinfo_operations);
> > +	return 0;
> > +}
> > +module_init(proc_socinfo_init);
> 
> proc_create() can fail, so some error handling here would be nice.

right. will add it.

> 
> On Mon, May 10, 2010 at 01:37:35PM +0300, Eduardo Valentin wrote:
> > +static int c_show(struct seq_file *m, void *v)
> > +{
> > +	seq_printf(m, "SoC\t: %s\n", socinfo);
> > +
> > +	return 0;
> > +}
> > +
> > +static void *c_start(struct seq_file *m, loff_t *pos)
> > +{
> > +	return *pos < 1 ? (void *)1 : NULL;
> > +}
> > +
> > +static void *c_next(struct seq_file *m, void *v, loff_t *pos)
> > +{
> > +	++*pos;
> > +	return NULL;
> > +}
> > +
> > +static void c_stop(struct seq_file *m, void *v)
> > +{
> > +}
> > +
> > +const struct seq_operations socinfo_op = {
> > +	.start	= c_start,
> > +	.next	= c_next,
> > +	.stop	= c_stop,
> > +	.show	= c_show
> > +};
> 
> You'll still need the show function, but all of the rest of this is just
> duplicating what single_open() already does. If the socinfo string is
> static you may also want to rework this a bit so you can just stash the
> string in the proc_dir_entry private data. Combine this with something
> like kstrdup() and you'll save yourself a bit of stack while you're at
> it.

yeah. will add those comments as well.


Thanks for reviewing,

--
Eduardo Valentin

[PATCHv4 1/4] procfs: Introduce socinfo under /proc

[Paul Mundt]

On Mon, May 10, 2010 at 03:35:14PM +0300, Eduardo Valentin wrote:
> On Mon, May 10, 2010 at 01:13:00PM +0200, ext Paul Mundt wrote:
> > On Mon, May 10, 2010 at 01:37:34PM +0300, Eduardo Valentin wrote:
> > > + */
> > > +#include <linux/fs.h>
> > > +#include <linux/init.h>
> > > +#include <linux/proc_fs.h>
> > > +#include <linux/seq_file.h>
> > > +
> > > +extern const struct seq_operations socinfo_op;
> > 
> > This doesn't look promising..
> 
> Right.. as stated in patch description (maybe not that clear), this was
> basically same thing which you see under fs/proc/cpuinfo.c
> 
The cpuinfo case is a bit more complex since you have actual real work to
do in the ->start op and you will iterate over the ->show op for each
CPU. In your socinfo case you're just following the single_xxx()
semantics so using those helpers there simplifies things quite a bit.

Architectures that do not support SMP technically employ single_open()
semantics, but the fs/proc/cpuinfo.c abstraction requires the
architecture to provide seq ops regardless.

Note that in the cpuinfo case there is often special handling for the
single (or boot CPU) case, such as printing out a descriptor for the
machine type and so on. You might be better off just extending cpuinfo
rather than introducing another /proc abstraction, presumably your
socinfo string will be fixed regardless of whether it's SMP or not.

[PATCHv4 1/4] procfs: Introduce socinfo under /proc

[Felipe Balbi]

On Mon, May 10, 2010 at 01:13:00PM +0200, ext Paul Mundt wrote:
>You'll still need the show function, but all of the rest of this is just
>duplicating what single_open() already does. If the socinfo string is
>static you may also want to rework this a bit so you can just stash the
>string in the proc_dir_entry private data. Combine this with something
>like kstrdup() and you'll save yourself a bit of stack while you're at
>it.

doesn't ksrtdup() cause memleak ?? Or is it only when used with 
module parameters ??

-- 
balbi

DefectiveByDesign.org

[PATCHv4 1/4] procfs: Introduce socinfo under /proc

[Eduardo Valentin]

On Mon, May 10, 2010 at 02:39:02PM +0200, ext Paul Mundt wrote:
> On Mon, May 10, 2010 at 03:35:14PM +0300, Eduardo Valentin wrote:
> > On Mon, May 10, 2010 at 01:13:00PM +0200, ext Paul Mundt wrote:
> > > On Mon, May 10, 2010 at 01:37:34PM +0300, Eduardo Valentin wrote:
> > > > + */
> > > > +#include <linux/fs.h>
> > > > +#include <linux/init.h>
> > > > +#include <linux/proc_fs.h>
> > > > +#include <linux/seq_file.h>
> > > > +
> > > > +extern const struct seq_operations socinfo_op;
> > > 
> > > This doesn't look promising..
> > 
> > Right.. as stated in patch description (maybe not that clear), this was
> > basically same thing which you see under fs/proc/cpuinfo.c
> > 
> The cpuinfo case is a bit more complex since you have actual real work to
> do in the ->start op and you will iterate over the ->show op for each
> CPU. In your socinfo case you're just following the single_xxx()
> semantics so using those helpers there simplifies things quite a bit.

Yeah,

> 
> Architectures that do not support SMP technically employ single_open()
> semantics, but the fs/proc/cpuinfo.c abstraction requires the
> architecture to provide seq ops regardless.

OK.

> 
> Note that in the cpuinfo case there is often special handling for the
> single (or boot CPU) case, such as printing out a descriptor for the
> machine type and so on. You might be better off just extending cpuinfo
> rather than introducing another /proc abstraction, presumably your
> socinfo string will be fixed regardless of whether it's SMP or not.

Yeah, I wouldn't expect it to change if it SMP or not. It should be fixed.
Previous version of this change was actually extending ARM cpuinfo. The previous
thread starts here:
http://marc.info/?l=linux-omap&m=127304890312365&w=2

But, the point of moving that to specific file was that soc info is not really cpu info.


BR,

--
Eduardo Valentin

[PATCHv4 1/4] procfs: Introduce socinfo under /proc

[Eduardo Valentin]

On Mon, May 10, 2010 at 02:54:40PM +0200, Balbi Felipe (Nokia-D/Helsinki) wrote:
> On Mon, May 10, 2010 at 01:13:00PM +0200, ext Paul Mundt wrote:
> >You'll still need the show function, but all of the rest of this is just
> >duplicating what single_open() already does. If the socinfo string is
> >static you may also want to rework this a bit so you can just stash the
> >string in the proc_dir_entry private data. Combine this with something
> >like kstrdup() and you'll save yourself a bit of stack while you're at
> >it.
> 
> doesn't ksrtdup() cause memleak ?? Or is it only when used with 
> module parameters ??

I'm not aware of the module parameter stuff.. but the leak might be other thing
than kstrdup?
> 
> -- 
> balbi
> 
> DefectiveByDesign.org

[PATCHv4 1/4] procfs: Introduce socinfo under /proc

[Eduardo Valentin]

Hello again,

On Mon, May 10, 2010 at 01:13:00PM +0200, ext Paul Mundt wrote:
> On Mon, May 10, 2010 at 01:37:34PM +0300, Eduardo Valentin wrote:
> > + */
> > +#include <linux/fs.h>
> > +#include <linux/init.h>
> > +#include <linux/proc_fs.h>
> > +#include <linux/seq_file.h>
> > +
> > +extern const struct seq_operations socinfo_op;
> 
> This doesn't look promising..
> 
> > +static int socinfo_open(struct inode *inode, struct file *file)
> > +{
> > +	return seq_open(file, &socinfo_op);
> > +}
> > +
> If you use single_open() ..
> 
> > +static const struct file_operations proc_socinfo_operations = {
> > +	.open		= socinfo_open,
> > +	.read		= seq_read,
> > +	.llseek		= seq_lseek,
> > +	.release	= seq_release,
> > +};
> > +
> .. and single_release(), then none of the seq_operations are necessary.
> 
> > +static int __init proc_socinfo_init(void)
> > +{
> > +	proc_create("socinfo", 0, NULL, &proc_socinfo_operations);
> > +	return 0;
> > +}
> > +module_init(proc_socinfo_init);
> 
> proc_create() can fail, so some error handling here would be nice.
> 
> On Mon, May 10, 2010 at 01:37:35PM +0300, Eduardo Valentin wrote:
> > +static int c_show(struct seq_file *m, void *v)
> > +{
> > +	seq_printf(m, "SoC\t: %s\n", socinfo);
> > +
> > +	return 0;
> > +}
> > +
> > +static void *c_start(struct seq_file *m, loff_t *pos)
> > +{
> > +	return *pos < 1 ? (void *)1 : NULL;
> > +}
> > +
> > +static void *c_next(struct seq_file *m, void *v, loff_t *pos)
> > +{
> > +	++*pos;
> > +	return NULL;
> > +}
> > +
> > +static void c_stop(struct seq_file *m, void *v)
> > +{
> > +}
> > +
> > +const struct seq_operations socinfo_op = {
> > +	.start	= c_start,
> > +	.next	= c_next,
> > +	.stop	= c_stop,
> > +	.show	= c_show
> > +};
> 
> You'll still need the show function, but all of the rest of this is just
> duplicating what single_open() already does. If the socinfo string is
> static you may also want to rework this a bit so you can just stash the
> string in the proc_dir_entry private data. Combine this with something
> like kstrdup() and you'll save yourself a bit of stack while you're at
> it.

While still here, about cleaning this, so, let me see if I got your point.
Basically, the file under fs/proc/socinfo.c whould do the thing with single_open &
single_release, as you stated. But then there is the .show and its data.
One idea would then be to have a function:

int register_socinfo_show(int (*show)(struct seq_file *, void *), void *data);

Which would be exported to other parts of the kernel (something placed under
include/linux/socinfo.h for instance). Then the soc core code
(like arch/arm/mach-omap[1,2]) would then register its local show function and pass its data.

This way I think we can avoid the exports inside .c files (as in this patch)
and also pass the static char * needed during the show.

What do you think?

BR,

--
Eduardo Valentin

[PATCHv4 1/4] procfs: Introduce socinfo under /proc

[Felipe Balbi]

On Mon, May 10, 2010 at 03:08:09PM +0200, Valentin Eduardo (Nokia-D/Helsinki) wrote:
>I'm not aware of the module parameter stuff.. but the leak might be other thing
>than kstrdup?

yeah, I was following the code and the problem is how the kernel handles 
charp module parameters

-- 
balbi

DefectiveByDesign.org

[PATCHv4 1/4] procfs: Introduce socinfo under /proc

[Paul Mundt]

On Mon, May 10, 2010 at 05:22:48PM +0300, Eduardo Valentin wrote:
> On Mon, May 10, 2010 at 01:13:00PM +0200, ext Paul Mundt wrote:
> > You'll still need the show function, but all of the rest of this is just
> > duplicating what single_open() already does. If the socinfo string is
> > static you may also want to rework this a bit so you can just stash the
> > string in the proc_dir_entry private data. Combine this with something
> > like kstrdup() and you'll save yourself a bit of stack while you're at
> > it.
> 
> While still here, about cleaning this, so, let me see if I got your point.
> Basically, the file under fs/proc/socinfo.c whould do the thing with single_open &
> single_release, as you stated. But then there is the .show and its data.
> One idea would then be to have a function:
> 
> int register_socinfo_show(int (*show)(struct seq_file *, void *), void *data);
> 
> Which would be exported to other parts of the kernel (something placed under
> include/linux/socinfo.h for instance). Then the soc core code
> (like arch/arm/mach-omap[1,2]) would then register its local show function and pass its data.
> 
> This way I think we can avoid the exports inside .c files (as in this patch)
> and also pass the static char * needed during the show.
> 
> What do you think?
> 
Yes, you'll need something like that. kstrdup() also does an allocation,
but you're only going to be registering once and are unlikely to ever
unregister (particular since you have this configured as a bool) so that
doesn't really matter. On the other hand if the string itself is static
you can just pass that in with a static initializer, or have some sort of
opaque socinfo data structure that contains the strings you care about.
You'll always be able to get back at the pointer through the
proc_dir_entry private data.

[PATCHv4 1/4] procfs: Introduce socinfo under /proc

[Paul Mundt]

On Mon, May 10, 2010 at 03:55:49PM +0300, Eduardo Valentin wrote:
> On Mon, May 10, 2010 at 02:39:02PM +0200, ext Paul Mundt wrote:
> > Note that in the cpuinfo case there is often special handling for the
> > single (or boot CPU) case, such as printing out a descriptor for the
> > machine type and so on. You might be better off just extending cpuinfo
> > rather than introducing another /proc abstraction, presumably your
> > socinfo string will be fixed regardless of whether it's SMP or not.
> 
> Yeah, I wouldn't expect it to change if it SMP or not. It should be fixed.
> Previous version of this change was actually extending ARM cpuinfo. The previous
> thread starts here:
> http://marc.info/?l=linux-omap&m=127304890312365&w=2
> 
> But, the point of moving that to specific file was that soc info is not really cpu info.
> 
It's up to you of course, but adding an extra file because of SoC/CPU
ambiguity seems pretty ugly. Almost all architectures already include
machine type descriptors in their cpuinfo output (as ARM does also) and
if you can justify that then certainly adding in some SoC-specific bits
isn't exactly much of a stretch.

These days you should have a pretty strong justification for adding new
procfs files, and this is certainly not one of them.

[PATCHv4 1/4] procfs: Introduce socinfo under /proc

[Russell King - ARM Linux]

On Tue, May 11, 2010 at 12:14:47PM +0900, Paul Mundt wrote:
> On Mon, May 10, 2010 at 03:55:49PM +0300, Eduardo Valentin wrote:
> > On Mon, May 10, 2010 at 02:39:02PM +0200, ext Paul Mundt wrote:
> > > Note that in the cpuinfo case there is often special handling for the
> > > single (or boot CPU) case, such as printing out a descriptor for the
> > > machine type and so on. You might be better off just extending cpuinfo
> > > rather than introducing another /proc abstraction, presumably your
> > > socinfo string will be fixed regardless of whether it's SMP or not.
> > 
> > Yeah, I wouldn't expect it to change if it SMP or not. It should be fixed.
> > Previous version of this change was actually extending ARM cpuinfo. The previous
> > thread starts here:
> > http://marc.info/?l=linux-omap&m=127304890312365&w=2
> > 
> > But, the point of moving that to specific file was that soc info is not really cpu info.
> > 
> It's up to you of course, but adding an extra file because of SoC/CPU
> ambiguity seems pretty ugly. Almost all architectures already include
> machine type descriptors in their cpuinfo output (as ARM does also) and
> if you can justify that then certainly adding in some SoC-specific bits
> isn't exactly much of a stretch.
> 
> These days you should have a pretty strong justification for adding new
> procfs files, and this is certainly not one of them.

I disagree.