parisc: convert /proc/pdc/{lcd,led} to seq_file

PATCH In review Alexey Dobriyan 4 participants 7 messages Nov 27, 2009 – Dec 5, 2009 PA-RISC architecture

lore

Share & tools

lore

Alexey Dobriyan

author

2009-11-27 06:51 UTC #0 lore raw

PATCH 1/1 parisc: convert /proc/pdc/{lcd,led} to seq_file

Signed-off-by Alexey Dobriyan <[email protected]>

---

 drivers/parisc/led.c |   59 +++++++++++++++++++++++++--------------------------
 1 file changed, 29 insertions(+), 30 deletions(-)

drivers/parisc/led.c +29 ~~-30~~

@@ -38,6 +38,7 @@
 #include <linux/kernel_stat.h>
 #include <linux/reboot.h>
 #include <linux/proc_fs.h>
+#include <linux/seq_file.h>
 #include <linux/ctype.h>
 #include <linux/blkdev.h>
 #include <linux/workqueue.h>

@@ -147,41 +148,34 @@ device_initcall(start_task);
 static void (*led_func_ptr) (unsigned char) __read_mostly;
 
 #ifdef CONFIG_PROC_FS
-static int led_proc_read(char *page, char **start, off_t off, int count, 
-	int *eof, void *data)
+static int led_proc_show(struct seq_file *m, void *v)
 {
-	char *out = page;
-	int len;
-
-	switch ((long)data)
+	switch ((long)m->private)
 	{
 	case LED_NOLCD:
-		out += sprintf(out, "Heartbeat: %d\n", led_heartbeat);
-		out += sprintf(out, "Disk IO: %d\n", led_diskio);
-		out += sprintf(out, "LAN Rx/Tx: %d\n", led_lanrxtx);
+		seq_printf(m, "Heartbeat: %d\n", led_heartbeat);
+		seq_printf(m, "Disk IO: %d\n", led_diskio);
+		seq_printf(m, "LAN Rx/Tx: %d\n", led_lanrxtx);
 		break;
 	case LED_HASLCD:
-		out += sprintf(out, "%s\n", lcd_text);
+		seq_printf(m, "%s\n", lcd_text);
 		break;
 	default:
-		*eof = 1;
 		return 0;
 	}
+	return 0;
+}
 
-	len = out - page - off;
-	if (len < count) {
-		*eof = 1;
-		if (len <= 0) return 0;
-	} else {
-		len = count;
-	}
-	*start = page + off;
-	return len;
+static int led_proc_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, led_proc_show, PDE(inode)->data);
 }
 
-static int led_proc_write(struct file *file, const char *buf, 
-	unsigned long count, void *data)
+
+static ssize_t led_proc_write(struct file *file, const char *buf,
+	size_t count, loff_t *pos)
 {
+	void *data = PDE(file->f_path.dentry->d_inode)->data;
 	char *cur, lbuf[count + 1];
 	int d;

@@ -234,6 +228,15 @@ parse_error:
 	return -EINVAL;
 }
 
+static const struct file_operations led_proc_fops = {
+	.owner		= THIS_MODULE,
+	.open		= led_proc_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+	.write		= led_proc_write,
+};
+
 static int __init led_create_procfs(void)
 {
 	struct proc_dir_entry *proc_pdc_root = NULL;

@@ -243,19 +246,15 @@ static int __init led_create_procfs(void)
 
 	proc_pdc_root = proc_mkdir("pdc", 0);
 	if (!proc_pdc_root) return -1;
-	ent = create_proc_entry("led", S_IFREG|S_IRUGO|S_IWUSR, proc_pdc_root);
+	ent = proc_create_data("led", S_IRUGO|S_IWUSR, proc_pdc_root,
+				&led_proc_fops, (void *)LED_NOLCD); /* LED */
 	if (!ent) return -1;
-	ent->data = (void *)LED_NOLCD; /* LED */
-	ent->read_proc = led_proc_read;
-	ent->write_proc = led_proc_write;
 
 	if (led_type == LED_HASLCD)
 	{
-		ent = create_proc_entry("lcd", S_IFREG|S_IRUGO|S_IWUSR, proc_pdc_root);
+		ent = proc_create_data("lcd", S_IRUGO|S_IWUSR, proc_pdc_root,
+					&led_proc_fops, (void *)LED_HASLCD); /* LCD */
 		if (!ent) return -1;
-		ent->data = (void *)LED_HASLCD; /* LCD */
-		ent->read_proc = led_proc_read;
-		ent->write_proc = led_proc_write;
 	}
 
 	return 0;

Grant Grundler

Reviewed-by on v1 1/1

2009-11-28 06:13 UTC #1 lore raw

Quoted 1 line · Alexey Dobriyan

Signed-off-by Alexey Dobriyan <[email protected]>

No commit comment?

Quoted 89 lines

---

 drivers/parisc/led.c |   59 +++++++++++++++++++++++++--------------------------
 1 file changed, 29 insertions(+), 30 deletions(-)

drivers/parisc/led.c +25 ~~-22~~

@@ -38,6 +38,7 @@
 #include <linux/kernel_stat.h>
 #include <linux/reboot.h>
 #include <linux/proc_fs.h>
+#include <linux/seq_file.h>
 #include <linux/ctype.h>
 #include <linux/blkdev.h>
 #include <linux/workqueue.h>

@@ -147,41 +148,34 @@ device_initcall(start_task);
 static void (*led_func_ptr) (unsigned char) __read_mostly;
 
 #ifdef CONFIG_PROC_FS
-static int led_proc_read(char *page, char **start, off_t off, int count, 
-	int *eof, void *data)
+static int led_proc_show(struct seq_file *m, void *v)
 {
-	char *out = page;
-	int len;
-
-	switch ((long)data)
+	switch ((long)m->private)
 	{
 	case LED_NOLCD:
-		out += sprintf(out, "Heartbeat: %d\n", led_heartbeat);
-		out += sprintf(out, "Disk IO: %d\n", led_diskio);
-		out += sprintf(out, "LAN Rx/Tx: %d\n", led_lanrxtx);
+		seq_printf(m, "Heartbeat: %d\n", led_heartbeat);
+		seq_printf(m, "Disk IO: %d\n", led_diskio);
+		seq_printf(m, "LAN Rx/Tx: %d\n", led_lanrxtx);
 		break;
 	case LED_HASLCD:
-		out += sprintf(out, "%s\n", lcd_text);
+		seq_printf(m, "%s\n", lcd_text);
 		break;
 	default:
-		*eof = 1;
 		return 0;
 	}
+	return 0;
+}
 
-	len = out - page - off;
-	if (len < count) {
-		*eof = 1;
-		if (len <= 0) return 0;
-	} else {
-		len = count;
-	}
-	*start = page + off;
-	return len;
+static int led_proc_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, led_proc_show, PDE(inode)->data);
 }
 
-static int led_proc_write(struct file *file, const char *buf, 
-	unsigned long count, void *data)
+
+static ssize_t led_proc_write(struct file *file, const char *buf,
+	size_t count, loff_t *pos)
 {
+	void *data = PDE(file->f_path.dentry->d_inode)->data;
 	char *cur, lbuf[count + 1];
 	int d;

@@ -234,6 +228,15 @@ parse_error:
 	return -EINVAL;
 }
 
+static const struct file_operations led_proc_fops = {
+	.owner		= THIS_MODULE,
+	.open		= led_proc_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+	.write		= led_proc_write,
+};
+
 static int __init led_create_procfs(void)
 {
 	struct proc_dir_entry *proc_pdc_root = NULL;

drivers/parisc/led.c static int __init led_create_procfs(void) · line 250

Quoted 6 lines

drivers/parisc/led.c +2 -1

@@ -243,19 +246,15 @@ static int __init led_create_procfs(void)
 
 	proc_pdc_root = proc_mkdir("pdc", 0);
 	if (!proc_pdc_root) return -1;
-	ent = create_proc_entry("led", S_IFREG|S_IRUGO|S_IWUSR, proc_pdc_root);
+	ent = proc_create_data("led", S_IRUGO|S_IWUSR, proc_pdc_root,
+				&led_proc_fops, (void *)LED_NOLCD); /* LED */

Documentation/filesystems/seq_file.txt says to use create_proc_entry(). I couldn't find any documentation on proc_create_data().
Is this really the preferred API?

I reviewed the code and it looks fine to me otherwise.

Reviewed-by Grant Grundler <[email protected]>

cheers,
grant

Quoted 21 lines

drivers/parisc/led.c +2 -8

 	if (!ent) return -1;
-	ent->data = (void *)LED_NOLCD; /* LED */
-	ent->read_proc = led_proc_read;
-	ent->write_proc = led_proc_write;
 
 	if (led_type == LED_HASLCD)
 	{
-		ent = create_proc_entry("lcd", S_IFREG|S_IRUGO|S_IWUSR, proc_pdc_root);
+		ent = proc_create_data("lcd", S_IRUGO|S_IWUSR, proc_pdc_root,
+					&led_proc_fops, (void *)LED_HASLCD); /* LCD */
 		if (!ent) return -1;
-		ent->data = (void *)LED_HASLCD; /* LCD */
-		ent->read_proc = led_proc_read;
-		ent->write_proc = led_proc_write;
 	}
 
 	return 0;
--

To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Alexey Dobriyan

author

2009-11-28 09:25 UTC #2 lore raw

Quoted 4 lines · Grant Grundler

Quoted 1 line · Alexey Dobriyan
Signed-off-by Alexey Dobriyan <[email protected]>
No commit comment?

No, subject says it's all.

Quoted 7 lines

drivers/parisc/led.c static int __init led_create_procfs(void) · line 250
Quoted 3 lines
drivers/parisc/led.c +2 -1
-	ent = create_proc_entry("led", S_IFREG|S_IRUGO|S_IWUSR, proc_pdc_root);
+	ent = proc_create_data("led", S_IRUGO|S_IWUSR, proc_pdc_root,
+				&led_proc_fops, (void *)LED_NOLCD); /* LED */
Documentation/filesystems/seq_file.txt says to use create_proc_entry(). I couldn't find any documentation on proc_create_data().
Is this really the preferred API?

Yes, it's slightly less racy than create_proc_entry().

create_proc_entry() is going to be removed in fact.

James Bottomley

maintainer

2009-11-28 14:45 UTC #3 lore raw

Quoted 7 lines · Alexey Dobriyan

Quoted 4 lines · Grant Grundler
Quoted 1 line · Alexey Dobriyan
Signed-off-by Alexey Dobriyan <[email protected]>
No commit comment?
No, subject says it's all.

He means rationale ... the output of the files you are altering is fixed size and atomic, so we don't need seq_file for fear of running off the fixed buffer and the data is reasonably fast changing, so anyone looking to seek within the file because they can't read the data in one go isn't going to get sensible results. Given that, why bother with the conversion?

James

Grant Grundler

2009-11-29 05:17 UTC #4 lore raw

In reply to Alexey Dobriyan

Quoted 7 lines · Alexey Dobriyan

Quoted 4 lines · Grant Grundler
Quoted 1 line · Alexey Dobriyan
Signed-off-by Alexey Dobriyan <[email protected]>
No commit comment?
No, subject says it's all.

The subject line says what the patch does, not why you wrote the patch. Your comments below are an example of what I was looking for.

Quoted 11 lines

Quoted 8 lines
drivers/parisc/led.c static int __init led_create_procfs(void) · line 250
Quoted 4 lines
drivers/parisc/led.c +2 -1
-	ent = create_proc_entry("led", S_IFREG|S_IRUGO|S_IWUSR, proc_pdc_root);
+	ent = proc_create_data("led", S_IRUGO|S_IWUSR, proc_pdc_root,
+				&led_proc_fops, (void *)LED_NOLCD); /* LED */
Documentation/filesystems/seq_file.txt says to use create_proc_entry(). I couldn't find any documentation on proc_create_data().
Is this really the preferred API?
Yes, it's slightly less racy than create_proc_entry(). create_proc_entry() is going to be removed in fact.

Ok. Please add a sentence about which race you are worried about. This isn't to discuss the race - it's just informative to explain why there is a plan to replace the API. I'm willing to go along with that.

Are you removing create_proc_entry() ? Is this patch part of a grand plan? (All good things to include in a commit comment)

Also updating Documentation/filesystems/seq_file.txt would be very helpful given you understand why proc_create_data() should replace create_proc_entry().

thanks!
grant

Andrew Morton

2009-12-01 22:07 UTC #5 lore raw

Quoted 8 lines · On Sat, 28 Nov 2009 22:17:52 -0700 Grant Grundler <[email protected]>…

Quoted 2 lines
Yes, it's slightly less racy than create_proc_entry(). create_proc_entry() is going to be removed in fact.
Ok. Please add a sentence about which race you are worried about. This isn't to discuss the race - it's just informative to explain why there is a plan to replace the API. I'm willing to go along with that.

Quoted 6 lines

Are you removing create_proc_entry() ? Is this patch part of a grand plan? (All good things to include in a commit comment)
Also updating Documentation/filesystems/seq_file.txt would be very helpful given you understand why proc_create_data() should replace create_proc_entry().

yeah. I didn't even know that this was the reason and I've been applying the patches like crazy. Heaven knows what a random developer of a remote subsystem is supposed to make of such a patch. Probably he just assumes that someone else knows what's going on.

Alexey, please, don't do this.

Please send me some boilerplate text which I can paste into

thinkpad_acpi-convert-to-seq_file.patch asus_acpi-convert-to-seq_file.patch toshiba_acpi-convert-to-seq_file.patch arm-convert-proc-cpu-aligment-to-seq_file.patch proc_fops-convert-drivers-isdn-to-seq_file.patch proc_fops-convert-drivers-isdn-to-seq_file-fix.patch mpt-fusion-convert-to-seq_file.patch pnpbios-convert-to-seq_file.patch uml-convert-to-seq_file-proc_fops.patch alpha-convert-srm-code-to-seq_file.patch parisc-convert-proc-pdc-lcdled-to-seq_file.patch via-pmu-convert-to-proc_fops-seq_file.patch

Alexey Dobriyan

author

2009-12-05 00:39 UTC #6 lore raw

Quoted 23 lines · Andrew Morton

Quoted 8 lines · On Sat, 28 Nov 2009 22:17:52 -0700 Grant Grundler <[email protected]>…
Quoted 2 lines
Yes, it's slightly less racy than create_proc_entry(). create_proc_entry() is going to be removed in fact.
Ok. Please add a sentence about which race you are worried about. This isn't to discuss the race - it's just informative to explain why there is a plan to replace the API. I'm willing to go along with that.
Quoted 6 lines
Are you removing create_proc_entry() ? Is this patch part of a grand plan? (All good things to include in a commit comment)
Also updating Documentation/filesystems/seq_file.txt would be very helpful given you understand why proc_create_data() should replace create_proc_entry().
yeah. I didn't even know that this was the reason and I've been applying the patches like crazy. Heaven knows what a random developer of a remote subsystem is supposed to make of such a patch. Probably he just assumes that someone else knows what's going on.

Quoted 4 lines

Alexey, please, don't do this.
Please send me some boilerplate text which I can paste into

Here it is:

Convert code away from ->read_proc/->write_proc interfaces. Switch to proc_create()/proc_create_data() which make addition of proc entries reliable wrt NULL ->proc_fops, NULL ->data and so on.

Problem with ->read_proc et al is described here
commit 786d7e1612f0b0adb6046f19b906609e4fe8b1ba
"Fix rmmod/read/write races in /proc entries"