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

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

[Alexey Dobriyan]

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

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

--- a/drivers/parisc/led.c
+++ b/drivers/parisc/led.c
@@ -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;

Re: [PATCH] parisc: convert /proc/pdc/{lcd,led} to seq_file

[Grant Grundler]

On Fri, Nov 27, 2009 at 09:51:12AM +0300, Alexey Dobriyan wrote:
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>

No commit comment?

> ---
> 
>  drivers/parisc/led.c |   59 +++++++++++++++++++++++++--------------------------
>  1 file changed, 29 insertions(+), 30 deletions(-)
> 
> --- a/drivers/parisc/led.c
> +++ b/drivers/parisc/led.c
> @@ -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 */

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 <grundler@parisc-linux.org>

cheers,
grant

>  	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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] parisc: convert /proc/pdc/{lcd,led} to seq_file

[Alexey Dobriyan]

On Fri, Nov 27, 2009 at 11:13:54PM -0700, Grant Grundler wrote:
> On Fri, Nov 27, 2009 at 09:51:12AM +0300, Alexey Dobriyan wrote:
> > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> 
> No commit comment?

No, subject says it's all.

> > -	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.

Re: [PATCH] parisc: convert /proc/pdc/{lcd,led} to seq_file

[James Bottomley]

On Sat, 2009-11-28 at 12:25 +0300, Alexey Dobriyan wrote:
> On Fri, Nov 27, 2009 at 11:13:54PM -0700, Grant Grundler wrote:
> > On Fri, Nov 27, 2009 at 09:51:12AM +0300, Alexey Dobriyan wrote:
> > > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> > 
> > 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

Re: [PATCH] parisc: convert /proc/pdc/{lcd,led} to seq_file

[Grant Grundler]

On Sat, Nov 28, 2009 at 12:25:09PM +0300, Alexey Dobriyan wrote:
> On Fri, Nov 27, 2009 at 11:13:54PM -0700, Grant Grundler wrote:
> > On Fri, Nov 27, 2009 at 09:51:12AM +0300, Alexey Dobriyan wrote:
> > > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> > 
> > 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.

> 
> > > -	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

Re: [PATCH] parisc: convert /proc/pdc/{lcd,led} to seq_file

[Andrew Morton]

On Sat, 28 Nov 2009 22:17:52 -0700
Grant Grundler <grundler@parisc-linux.org> wrote:

> > 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().

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

Re: [PATCH] parisc: convert /proc/pdc/{lcd,led} to seq_file

[Alexey Dobriyan]

On Tue, Dec 01, 2009 at 02:07:46PM -0800, Andrew Morton wrote:
> On Sat, 28 Nov 2009 22:17:52 -0700
> Grant Grundler <grundler@parisc-linux.org> wrote:
> 
> > > 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().
> 
> 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

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"