[PATCH] Fix proc_file_write missing ppos update

[PATCH] Fix proc_file_write missing ppos update

[Stefani Seibold]

The following fix a long standing issue in the proc_file_write function,
which doesn't update the ppos file position pointer.

This prevent the usage of multiple sequently writes on an opened proc
file, because it is impossible to distinguish these due the offset is
always 0.

The patch has no side effects, because the old implementaion has
no ability to access the file offset pointer.

Out of tree should work, also if there not using the new prototype definition, 
because the new file offset pointer argument will be passed as last parameter.

The patch is against 2.6.31-rc8

Signed-off-by: Stefani Seibold <stefani@seibold.net>
---
 fs/proc/generic.c                                      |    5 ++--
 include/linux/proc_fs.h                                |    2 -
 Documentation/DocBook/procfs-guide.tmpl                |    1 
 Documentation/DocBook/procfs_example.c                 |    3 +-
 arch/alpha/kernel/srm_env.c                            |    2 -
 arch/arm/mm/alignment.c                                |    2 -
 arch/blackfin/kernel/kgdb_test.c                       |    2 -
 arch/mips/lasat/picvue_proc.c                          |    4 +--
 arch/powerpc/platforms/iseries/mf.c                    |    6 ++---
 arch/um/drivers/mconsole_kern.c                        |    2 -
 arch/um/kernel/exitcode.c                              |    2 -
 arch/um/kernel/process.c                               |    3 +-
 drivers/acpi/debug.c                                   |    2 -
 drivers/block/DAC960.c                                 |    3 +-
 drivers/ide/ide-proc.c                                 |    6 +++--
 drivers/isdn/hardware/eicon/divasproc.c                |    8 +++----
 drivers/macintosh/via-pmu.c                            |    4 +--
 drivers/media/dvb/ttpci/av7110_ir.c                    |    2 -
 drivers/media/video/cpia.c                             |    2 -
 drivers/net/wireless/ipw2x00/libipw_module.c           |    2 -
 drivers/net/wireless/ray_cs.c                          |    4 +--
 drivers/parisc/led.c                                   |    2 -
 drivers/platform/x86/asus_acpi.c                       |   18 ++++++++---------
 drivers/platform/x86/thinkpad_acpi.c                   |    2 -
 drivers/platform/x86/toshiba_acpi.c                    |    2 -
 drivers/pnp/pnpbios/proc.c                             |    2 -
 drivers/s390/block/dasd_proc.c                         |    2 -
 drivers/s390/crypto/zcrypt_api.c                       |    2 -
 drivers/scsi/scsi_proc.c                               |    2 -
 drivers/staging/epl/proc_fs.c                          |    4 +--
 drivers/staging/rtl8192su/ieee80211/ieee80211_module.c |    2 -
 drivers/video/clps711xfb.c                             |    4 +--
 drivers/video/via/viafbdev.c                           |   15 +++++++++-----
 kernel/profile.c                                       |    2 -
 34 files changed, 69 insertions(+), 57 deletions(-)

diff -u -N -r write_proc.orig/include/linux/proc_fs.h write_proc/include/linux/proc_fs.h
--- write_proc.orig/include/linux/proc_fs.h	2009-08-29 15:53:44.000000000 +0200
+++ write_proc/include/linux/proc_fs.h	2009-08-29 15:44:44.000000000 +0200
@@ -46,7 +46,7 @@
 typedef	int (read_proc_t)(char *page, char **start, off_t off,
 			  int count, int *eof, void *data);
 typedef	int (write_proc_t)(struct file *file, const char __user *buffer,
-			   unsigned long count, void *data);
+			   unsigned long count, void *data, off_t off);
 
 struct proc_dir_entry {
 	unsigned int low_ino;
diff -u -N -r write_proc.orig/fs/proc/generic.c write_proc/fs/proc/generic.c
--- write_proc.orig/fs/proc/generic.c	2009-06-10 05:05:27.000000000 +0200
+++ write_proc/fs/proc/generic.c	2009-08-29 15:43:46.000000000 +0200
@@ -219,9 +219,10 @@
 		pde->pde_users++;
 		spin_unlock(&pde->pde_unload_lock);
 
-		/* FIXME: does this routine need ppos?  probably... */
-		rv = pde->write_proc(file, buffer, count, pde->data);
+		rv = pde->write_proc(file, buffer, count, pde->data, *ppos);
 		pde_users_dec(pde);
+		if (rv > 0)
+			*ppos += rv;
 	}
 	return rv;
 }
diff -u -N -r write_proc.orig/arch/alpha/kernel/srm_env.c write_proc/arch/alpha/kernel/srm_env.c
--- write_proc.orig/arch/alpha/kernel/srm_env.c	2009-06-10 05:05:27.000000000 +0200
+++ write_proc/arch/alpha/kernel/srm_env.c	2009-08-29 15:43:46.000000000 +0200
@@ -106,7 +106,7 @@
 
 static int
 srm_env_write(struct file *file, const char __user *buffer, unsigned long count,
-		void *data)
+		void *data, off_t off)
 {
 	int res;
 	srm_env_t	*entry;
diff -u -N -r write_proc.orig/arch/arm/mm/alignment.c write_proc/arch/arm/mm/alignment.c
--- write_proc.orig/arch/arm/mm/alignment.c	2009-08-29 15:53:40.000000000 +0200
+++ write_proc/arch/arm/mm/alignment.c	2009-08-29 15:43:46.000000000 +0200
@@ -120,7 +120,7 @@
 }
 
 static int proc_alignment_write(struct file *file, const char __user *buffer,
-				unsigned long count, void *data)
+				unsigned long count, void *data, off_t off)
 {
 	char mode;
 
diff -u -N -r write_proc.orig/arch/blackfin/kernel/kgdb_test.c write_proc/arch/blackfin/kernel/kgdb_test.c
--- write_proc.orig/arch/blackfin/kernel/kgdb_test.c	2009-06-10 05:05:27.000000000 +0200
+++ write_proc/arch/blackfin/kernel/kgdb_test.c	2009-08-29 15:43:46.000000000 +0200
@@ -90,7 +90,7 @@
 }
 
 static int test_write_proc(struct file *file, const char *buffer,
-				 unsigned long count, void *data)
+				 unsigned long count, void *data, off_t off)
 {
 	if (count >= 256)
 		len = 255;
diff -u -N -r write_proc.orig/arch/mips/lasat/picvue_proc.c write_proc/arch/mips/lasat/picvue_proc.c
--- write_proc.orig/arch/mips/lasat/picvue_proc.c	2009-06-10 05:05:27.000000000 +0200
+++ write_proc/arch/mips/lasat/picvue_proc.c	2009-08-29 15:43:46.000000000 +0200
@@ -58,7 +58,7 @@
 }
 
 static int pvc_proc_write_line(struct file *file, const char *buffer,
-			   unsigned long count, void *data)
+			   unsigned long count, void *data, off_t off)
 {
 	int origcount = count;
 	int lineno = *(int *)data;
@@ -86,7 +86,7 @@
 }
 
 static int pvc_proc_write_scroll(struct file *file, const char *buffer,
-			   unsigned long count, void *data)
+			   unsigned long count, void *data, off_t off)
 {
 	int origcount = count;
 	int cmd = simple_strtol(buffer, NULL, 10);
diff -u -N -r write_proc.orig/arch/powerpc/platforms/iseries/mf.c write_proc/arch/powerpc/platforms/iseries/mf.c
--- write_proc.orig/arch/powerpc/platforms/iseries/mf.c	2009-08-29 15:53:40.000000000 +0200
+++ write_proc/arch/powerpc/platforms/iseries/mf.c	2009-08-29 15:43:46.000000000 +0200
@@ -1003,7 +1003,7 @@
 }
 
 static int proc_mf_change_side(struct file *file, const char __user *buffer,
-		unsigned long count, void *data)
+		unsigned long count, void *data, off_t off)
 {
 	char side;
 	u64 newSide;
@@ -1110,7 +1110,7 @@
 }
 
 static int proc_mf_change_src(struct file *file, const char __user *buffer,
-		unsigned long count, void *data)
+		unsigned long count, void *data, off_t off)
 {
 	char stkbuf[10];
 
@@ -1136,7 +1136,7 @@
 }
 
 static int proc_mf_change_cmdline(struct file *file, const char __user *buffer,
-		unsigned long count, void *data)
+		unsigned long count, void *data, off_t off)
 {
 	struct vsp_cmd_data vsp_cmd;
 	dma_addr_t dma_addr;
diff -u -N -r write_proc.orig/arch/um/drivers/mconsole_kern.c write_proc/arch/um/drivers/mconsole_kern.c
--- write_proc.orig/arch/um/drivers/mconsole_kern.c	2009-06-10 05:05:27.000000000 +0200
+++ write_proc/arch/um/drivers/mconsole_kern.c	2009-08-29 15:43:46.000000000 +0200
@@ -834,7 +834,7 @@
 __initcall(mconsole_init);
 
 static int write_proc_mconsole(struct file *file, const char __user *buffer,
-			       unsigned long count, void *data)
+			       unsigned long count, void *data, off_t off)
 {
 	char *buf;
 
diff -u -N -r write_proc.orig/arch/um/kernel/exitcode.c write_proc/arch/um/kernel/exitcode.c
--- write_proc.orig/arch/um/kernel/exitcode.c	2009-06-10 05:05:27.000000000 +0200
+++ write_proc/arch/um/kernel/exitcode.c	2009-08-29 15:43:46.000000000 +0200
@@ -39,7 +39,7 @@
 }
 
 static int write_proc_exitcode(struct file *file, const char __user *buffer,
-			       unsigned long count, void *data)
+			       unsigned long count, void *data, off_t off)
 {
 	char *end, buf[sizeof("nnnnn\0")];
 	int tmp;
diff -u -N -r write_proc.orig/arch/um/kernel/process.c write_proc/arch/um/kernel/process.c
--- write_proc.orig/arch/um/kernel/process.c	2009-06-10 05:05:27.000000000 +0200
+++ write_proc/arch/um/kernel/process.c	2009-08-29 15:43:46.000000000 +0200
@@ -345,7 +345,8 @@
 	return strlen(buf);
 }
 
-static int proc_write_sysemu(struct file *file,const char __user *buf, unsigned long count,void *data)
+static int proc_write_sysemu(struct file *file, const char __user *buf,
+		unsigned long count, void *data, off_t off)
 {
 	char tmp[2];
 
diff -u -N -r write_proc.orig/Documentation/DocBook/procfs_example.c write_proc/Documentation/DocBook/procfs_example.c
--- write_proc.orig/Documentation/DocBook/procfs_example.c	2009-06-10 05:05:27.000000000 +0200
+++ write_proc/Documentation/DocBook/procfs_example.c	2009-08-29 15:43:46.000000000 +0200
@@ -88,7 +88,8 @@
 static int proc_write_foobar(struct file *file,
 			     const char *buffer,
 			     unsigned long count, 
-			     void *data)
+			     void *data,
+			     off_t off)
 {
 	int len;
 	struct fb_data_t *fb_data = (struct fb_data_t *)data;
diff -u -N -r write_proc.orig/drivers/acpi/debug.c write_proc/drivers/acpi/debug.c
--- write_proc.orig/drivers/acpi/debug.c	2009-06-10 05:05:27.000000000 +0200
+++ write_proc/drivers/acpi/debug.c	2009-08-29 16:18:44.000000000 +0200
@@ -266,7 +266,7 @@
 static int
 acpi_system_write_debug(struct file *file,
 			const char __user * buffer,
-			unsigned long count, void *data)
+			unsigned long count, void *data, off_t off)
 {
 	char debug_string[12] = { '\0' };
 
diff -u -N -r write_proc.orig/drivers/block/DAC960.c write_proc/drivers/block/DAC960.c
--- write_proc.orig/drivers/block/DAC960.c	2009-08-29 15:53:41.000000000 +0200
+++ write_proc/drivers/block/DAC960.c	2009-08-29 15:43:46.000000000 +0200
@@ -6553,7 +6553,8 @@
 
 static int DAC960_ProcWriteUserCommand(struct file *file,
 				       const char __user *Buffer,
-				       unsigned long Count, void *Data)
+				       unsigned long Count, void *Data,
+				       off_t Offset)
 {
   DAC960_Controller_T *Controller = (DAC960_Controller_T *) Data;
   unsigned char CommandBuffer[80];
diff -u -N -r write_proc.orig/drivers/ide/ide-proc.c write_proc/drivers/ide/ide-proc.c
--- write_proc.orig/drivers/ide/ide-proc.c	2009-06-10 05:05:27.000000000 +0200
+++ write_proc/drivers/ide/ide-proc.c	2009-08-29 15:43:46.000000000 +0200
@@ -297,7 +297,8 @@
 #define MAX_LEN	30
 
 static int proc_ide_write_settings(struct file *file, const char __user *buffer,
-				   unsigned long count, void *data)
+				   unsigned long count, void *data,
+				   off_t off)
 {
 	ide_drive_t	*drive = (ide_drive_t *) data;
 	char		name[MAX_LEN + 1];
@@ -481,7 +482,8 @@
 }
 
 static int proc_ide_write_driver
-	(struct file *file, const char __user *buffer, unsigned long count, void *data)
+	(struct file *file, const char __user *buffer, unsigned long count,
+	 void *data, off_t off)
 {
 	ide_drive_t	*drive = (ide_drive_t *) data;
 	char name[32];
diff -u -N -r write_proc.orig/drivers/isdn/hardware/eicon/divasproc.c write_proc/drivers/isdn/hardware/eicon/divasproc.c
--- write_proc.orig/drivers/isdn/hardware/eicon/divasproc.c	2009-06-10 05:05:27.000000000 +0200
+++ write_proc/drivers/isdn/hardware/eicon/divasproc.c	2009-08-29 15:43:46.000000000 +0200
@@ -146,7 +146,7 @@
 */
 static int
 write_grp_opt(struct file *file, const char __user *buffer, unsigned long count,
-	      void *data)
+	      void *data, off_t off)
 {
 	diva_os_xdi_adapter_t *a = (diva_os_xdi_adapter_t *) data;
 	PISDN_ADAPTER IoAdapter = IoAdapters[a->controller - 1];
@@ -176,8 +176,8 @@
 ** write dynamic_l1_down
 */
 static int
-write_d_l1_down(struct file *file, const char __user *buffer, unsigned long count,
-		void *data)
+write_d_l1_down(struct file *file, const char __user *buffer,
+		unsigned long count, void *data, off_t off)
 {
 	diva_os_xdi_adapter_t *a = (diva_os_xdi_adapter_t *) data;
 	PISDN_ADAPTER IoAdapter = IoAdapters[a->controller - 1];
@@ -257,7 +257,7 @@
 */
 static int
 info_write(struct file *file, const char __user *buffer, unsigned long count,
-	   void *data)
+	   void *data, off_t off)
 {
 	diva_os_xdi_adapter_t *a = (diva_os_xdi_adapter_t *) data;
 	PISDN_ADAPTER IoAdapter = IoAdapters[a->controller - 1];
diff -u -N -r write_proc.orig/drivers/macintosh/via-pmu.c write_proc/drivers/macintosh/via-pmu.c
--- write_proc.orig/drivers/macintosh/via-pmu.c	2009-06-10 05:05:27.000000000 +0200
+++ write_proc/drivers/macintosh/via-pmu.c	2009-08-29 15:43:46.000000000 +0200
@@ -196,7 +196,7 @@
 static int proc_read_options(char *page, char **start, off_t off,
 			int count, int *eof, void *data);
 static int proc_write_options(struct file *file, const char __user *buffer,
-			unsigned long count, void *data);
+			unsigned long count, void *data, off_t off);
 
 #ifdef CONFIG_ADB
 struct adb_driver via_pmu_driver = {
@@ -880,7 +880,7 @@
 			
 static int
 proc_write_options(struct file *file, const char __user *buffer,
-			unsigned long count, void *data)
+			unsigned long count, void *data, off_t off)
 {
 	char tmp[33];
 	char *label, *val;
diff -u -N -r write_proc.orig/drivers/media/dvb/ttpci/av7110_ir.c write_proc/drivers/media/dvb/ttpci/av7110_ir.c
--- write_proc.orig/drivers/media/dvb/ttpci/av7110_ir.c	2009-06-10 05:05:27.000000000 +0200
+++ write_proc/drivers/media/dvb/ttpci/av7110_ir.c	2009-08-29 15:43:46.000000000 +0200
@@ -269,7 +269,7 @@
 
 /* /proc/av7110_ir interface */
 static int av7110_ir_write_proc(struct file *file, const char __user *buffer,
-				unsigned long count, void *data)
+				unsigned long count, void *data, off_t off)
 {
 	char *page;
 	u32 ir_config;
diff -u -N -r write_proc.orig/drivers/media/video/cpia.c write_proc/drivers/media/video/cpia.c
--- write_proc.orig/drivers/media/video/cpia.c	2009-06-10 05:05:27.000000000 +0200
+++ write_proc/drivers/media/video/cpia.c	2009-08-29 15:43:46.000000000 +0200
@@ -565,7 +565,7 @@
 }
 
 static int cpia_write_proc(struct file *file, const char __user *buf,
-			   unsigned long count, void *data)
+			   unsigned long count, void *data, off_t off)
 {
 	struct cam_data *cam = data;
 	struct cam_params new_params;
diff -u -N -r write_proc.orig/drivers/net/wireless/ipw2x00/libipw_module.c write_proc/drivers/net/wireless/ipw2x00/libipw_module.c
--- write_proc.orig/drivers/net/wireless/ipw2x00/libipw_module.c	2009-08-29 15:53:42.000000000 +0200
+++ write_proc/drivers/net/wireless/ipw2x00/libipw_module.c	2009-08-29 15:43:46.000000000 +0200
@@ -223,7 +223,7 @@
 }
 
 static int store_debug_level(struct file *file, const char __user * buffer,
-			     unsigned long count, void *data)
+			     unsigned long count, void *data, off_t off)
 {
 	char buf[] = "0x00000000\n";
 	unsigned long len = min((unsigned long)sizeof(buf) - 1, count);
diff -u -N -r write_proc.orig/drivers/net/wireless/ray_cs.c write_proc/drivers/net/wireless/ray_cs.c
--- write_proc.orig/drivers/net/wireless/ray_cs.c	2009-08-29 15:53:42.000000000 +0200
+++ write_proc/drivers/net/wireless/ray_cs.c	2009-08-29 15:43:46.000000000 +0200
@@ -2875,7 +2875,7 @@
 }
 
 static int write_essid(struct file *file, const char __user *buffer,
-		       unsigned long count, void *data)
+		       unsigned long count, void *data, off_t off)
 {
 	static char proc_essid[33];
 	int len = count;
@@ -2890,7 +2890,7 @@
 }
 
 static int write_int(struct file *file, const char __user *buffer,
-		     unsigned long count, void *data)
+		     unsigned long count, void *data, off_t off)
 {
 	static char proc_number[10];
 	char *p;
diff -u -N -r write_proc.orig/drivers/parisc/led.c write_proc/drivers/parisc/led.c
--- write_proc.orig/drivers/parisc/led.c	2009-06-10 05:05:27.000000000 +0200
+++ write_proc/drivers/parisc/led.c	2009-08-29 15:43:46.000000000 +0200
@@ -180,7 +180,7 @@
 }
 
 static int led_proc_write(struct file *file, const char *buf, 
-	unsigned long count, void *data)
+	unsigned long count, void *data, off_t off)
 {
 	char *cur, lbuf[count + 1];
 	int d;
diff -u -N -r write_proc.orig/drivers/platform/x86/asus_acpi.c write_proc/drivers/platform/x86/asus_acpi.c
--- write_proc.orig/drivers/platform/x86/asus_acpi.c	2009-08-29 15:53:42.000000000 +0200
+++ write_proc/drivers/platform/x86/asus_acpi.c	2009-08-29 15:43:46.000000000 +0200
@@ -648,7 +648,7 @@
 
 static int
 proc_write_mled(struct file *file, const char __user *buffer,
-		unsigned long count, void *data)
+		unsigned long count, void *data, off_t off)
 {
 	return write_led(buffer, count, hotk->methods->mt_mled, MLED_ON, 1);
 }
@@ -665,7 +665,7 @@
 
 static int
 proc_write_ledd(struct file *file, const char __user *buffer,
-		unsigned long count, void *data)
+		unsigned long count, void *data, off_t ppos)
 {
 	int rv, value;
 
@@ -694,7 +694,7 @@
 
 static int
 proc_write_wled(struct file *file, const char __user *buffer,
-		unsigned long count, void *data)
+		unsigned long count, void *data, off_t off)
 {
 	return write_led(buffer, count, hotk->methods->mt_wled, WLED_ON, 0);
 }
@@ -711,7 +711,7 @@
 
 static int
 proc_write_bluetooth(struct file *file, const char __user *buffer,
-		     unsigned long count, void *data)
+		     unsigned long count, void *data, off_t off)
 {
 	/* Note: mt_bt_switch controls both internal Bluetooth adapter's
 	   presence and its LED */
@@ -731,7 +731,7 @@
 
 static int
 proc_write_tled(struct file *file, const char __user *buffer,
-		unsigned long count, void *data)
+		unsigned long count, void *data, off_t off)
 {
 	return write_led(buffer, count, hotk->methods->mt_tled, TLED_ON, 0);
 }
@@ -837,7 +837,7 @@
 
 static int
 proc_write_lcd(struct file *file, const char __user *buffer,
-	       unsigned long count, void *data)
+	       unsigned long count, void *data, off_t off)
 {
 	int rv, value;
 
@@ -915,7 +915,7 @@
 
 static int
 proc_write_brn(struct file *file, const char __user *buffer,
-	       unsigned long count, void *data)
+	       unsigned long count, void *data, off_t off)
 {
 	int rv, value;
 
@@ -962,7 +962,7 @@
  */
 static int
 proc_write_disp(struct file *file, const char __user *buffer,
-		unsigned long count, void *data)
+		unsigned long count, void *data, off_t off)
 {
 	int rv, value;
 
@@ -975,7 +975,7 @@
 typedef int (proc_readfunc) (char *page, char **start, off_t off, int count,
 			     int *eof, void *data);
 typedef int (proc_writefunc) (struct file *file, const char __user *buffer,
-			      unsigned long count, void *data);
+			      unsigned long count, void *data, off_t off);
 
 static int
 asus_proc_add(char *name, proc_writefunc *writefunc,
diff -u -N -r write_proc.orig/drivers/platform/x86/thinkpad_acpi.c write_proc/drivers/platform/x86/thinkpad_acpi.c
--- write_proc.orig/drivers/platform/x86/thinkpad_acpi.c	2009-08-29 15:53:42.000000000 +0200
+++ write_proc/drivers/platform/x86/thinkpad_acpi.c	2009-08-29 16:25:21.000000000 +0200
@@ -758,7 +758,7 @@
 
 static int dispatch_procfs_write(struct file *file,
 			const char __user *userbuf,
-			unsigned long count, void *data)
+			unsigned long count, void *data, off_t off)
 {
 	struct ibm_struct *ibm = data;
 	char *kernbuf;
diff -u -N -r write_proc.orig/drivers/platform/x86/toshiba_acpi.c write_proc/drivers/platform/x86/toshiba_acpi.c
--- write_proc.orig/drivers/platform/x86/toshiba_acpi.c	2009-08-29 15:53:42.000000000 +0200
+++ write_proc/drivers/platform/x86/toshiba_acpi.c	2009-08-29 15:43:46.000000000 +0200
@@ -390,7 +390,7 @@
 
 static int
 dispatch_write(struct file *file, const char __user * buffer,
-	       unsigned long count, ProcItem * item)
+	       unsigned long count, ProcItem * item, off_t off)
 {
 	int result;
 	char *tmp_buffer;
diff -u -N -r write_proc.orig/drivers/pnp/pnpbios/proc.c write_proc/drivers/pnp/pnpbios/proc.c
--- write_proc.orig/drivers/pnp/pnpbios/proc.c	2009-06-10 05:05:27.000000000 +0200
+++ write_proc/drivers/pnp/pnpbios/proc.c	2009-08-29 15:47:33.000000000 +0200
@@ -186,7 +186,7 @@
 }
 
 static int proc_write_node(struct file *file, const char __user * buf,
-			   unsigned long count, void *data)
+			   unsigned long count, void *data, off_t off)
 {
 	struct pnp_bios_node *node;
 	int boot = (long)data >> 8;
diff -u -N -r write_proc.orig/drivers/s390/block/dasd_proc.c write_proc/drivers/s390/block/dasd_proc.c
--- write_proc.orig/drivers/s390/block/dasd_proc.c	2009-06-10 05:05:27.000000000 +0200
+++ write_proc/drivers/s390/block/dasd_proc.c	2009-08-29 15:43:46.000000000 +0200
@@ -259,7 +259,7 @@
 
 static int
 dasd_statistics_write(struct file *file, const char __user *user_buf,
-		      unsigned long user_len, void *data)
+		      unsigned long user_len, void *data, off_t off)
 {
 #ifdef CONFIG_DASD_PROFILE
 	char *buffer, *str;
diff -u -N -r write_proc.orig/drivers/s390/crypto/zcrypt_api.c write_proc/drivers/s390/crypto/zcrypt_api.c
--- write_proc.orig/drivers/s390/crypto/zcrypt_api.c	2009-06-10 05:05:27.000000000 +0200
+++ write_proc/drivers/s390/crypto/zcrypt_api.c	2009-08-29 15:43:46.000000000 +0200
@@ -1058,7 +1058,7 @@
 }
 
 static int zcrypt_status_write(struct file *file, const char __user *buffer,
-			       unsigned long count, void *data)
+			       unsigned long count, void *data, off_t off)
 {
 	unsigned char *lbuf, *ptr;
 	unsigned long local_count;
diff -u -N -r write_proc.orig/drivers/scsi/scsi_proc.c write_proc/drivers/scsi/scsi_proc.c
--- write_proc.orig/drivers/scsi/scsi_proc.c	2009-06-10 05:05:27.000000000 +0200
+++ write_proc/drivers/scsi/scsi_proc.c	2009-08-29 16:28:51.000000000 +0200
@@ -75,7 +75,7 @@
  * @data: pointer to &struct Scsi_Host
  */
 static int proc_scsi_write_proc(struct file *file, const char __user *buf,
-                           unsigned long count, void *data)
+                           unsigned long count, void *data, off_t off)
 {
 	struct Scsi_Host *shost = data;
 	ssize_t ret = -ENOMEM;
diff -u -N -r write_proc.orig/drivers/staging/epl/proc_fs.c write_proc/drivers/staging/epl/proc_fs.c
--- write_proc.orig/drivers/staging/epl/proc_fs.c	2009-06-10 05:05:27.000000000 +0200
+++ write_proc/drivers/staging/epl/proc_fs.c	2009-08-29 15:43:46.000000000 +0200
@@ -144,7 +144,7 @@
 static int EplLinProcRead(char *pcBuffer_p, char **ppcStart_p, off_t Offset_p,
 			  int nBufferSize_p, int *pEof_p, void *pData_p);
 static int EplLinProcWrite(struct file *file, const char __user * buffer,
-			   unsigned long count, void *data);
+			   unsigned long count, void *data, off_t off);
 
 void TgtDbgSignalTracePoint(u8 bTracePointNumber_p);
 void TgtDbgPostTraceValue(u32 dwTraceValue_p);
@@ -382,7 +382,7 @@
 //---------------------------------------------------------------------------
 
 static int EplLinProcWrite(struct file *file, const char __user * buffer,
-			   unsigned long count, void *data)
+			   unsigned long count, void *data, off_t off)
 {
 	char abBuffer[count + 1];
 	int iErr;
diff -u -N -r write_proc.orig/drivers/staging/rtl8192su/ieee80211/ieee80211_module.c write_proc/drivers/staging/rtl8192su/ieee80211/ieee80211_module.c
--- write_proc.orig/drivers/staging/rtl8192su/ieee80211/ieee80211_module.c	2009-08-29 15:53:42.000000000 +0200
+++ write_proc/drivers/staging/rtl8192su/ieee80211/ieee80211_module.c	2009-08-29 15:43:46.000000000 +0200
@@ -301,7 +301,7 @@
 }
 
 static int store_debug_level(struct file *file, const char *buffer,
-			     unsigned long count, void *data)
+			     unsigned long count, void *data, off_t off)
 {
 	char buf[] = "0x00000000";
 	unsigned long len = min(sizeof(buf) - 1, (u32)count);
diff -u -N -r write_proc.orig/drivers/video/clps711xfb.c write_proc/drivers/video/clps711xfb.c
--- write_proc.orig/drivers/video/clps711xfb.c	2009-06-10 05:05:27.000000000 +0200
+++ write_proc/drivers/video/clps711xfb.c	2009-08-29 15:43:46.000000000 +0200
@@ -44,7 +44,7 @@
 static int clps7111fb_proc_backlight_read(char *page, char **start, off_t off,
 		int count, int *eof, void *data);
 static int clps7111fb_proc_backlight_write(struct file *file, 
-		const char *buffer, unsigned long count, void *data);
+		const char *buffer, unsigned long count, void *data, off_t off);
 
 /*
  * LCD AC Prescale.  This comes from the LCD panel manufacturers specifications.
@@ -240,7 +240,7 @@
 
 static int 
 clps7111fb_proc_backlight_write(struct file *file, const char *buffer, 
-		unsigned long count, void *data)
+		unsigned long count, void *data, off_t off)
 {
 	unsigned char char_value;
 	int value;
diff -u -N -r write_proc.orig/drivers/video/via/viafbdev.c write_proc/drivers/video/via/viafbdev.c
--- write_proc.orig/drivers/video/via/viafbdev.c	2009-08-29 15:53:43.000000000 +0200
+++ write_proc/drivers/video/via/viafbdev.c	2009-08-29 16:20:28.000000000 +0200
@@ -1772,7 +1772,8 @@
 	return len;
 }
 static int viafb_dvp0_proc_write(struct file *file,
-	const char __user *buffer, unsigned long count, void *data)
+	const char __user *buffer, unsigned long count, void *data,
+	off_t offset)
 {
 	char buf[20], *value, *pbuf;
 	u8 reg_val = 0;
@@ -1830,7 +1831,8 @@
 	return len;
 }
 static int viafb_dvp1_proc_write(struct file *file,
-	const char __user *buffer, unsigned long count, void *data)
+	const char __user *buffer, unsigned long count, void *data,
+	off_t offset)
 {
 	char buf[20], *value, *pbuf;
 	u8 reg_val = 0;
@@ -1880,7 +1882,8 @@
 	return len;
 }
 static int viafb_dfph_proc_write(struct file *file,
-	const char __user *buffer, unsigned long count, void *data)
+	const char __user *buffer, unsigned long count, void *data,
+	off_t offset)
 {
 	char buf[20];
 	u8 reg_val = 0;
@@ -1906,7 +1909,8 @@
 	return len;
 }
 static int viafb_dfpl_proc_write(struct file *file,
-	const char __user *buffer, unsigned long count, void *data)
+	const char __user *buffer, unsigned long count, void *data,
+	off_t offset)
 {
 	char buf[20];
 	u8 reg_val = 0;
@@ -1956,7 +1960,8 @@
 	return len;
 }
 static int viafb_vt1636_proc_write(struct file *file,
-	const char __user *buffer, unsigned long count, void *data)
+	const char __user *buffer, unsigned long count, void *data,
+	off_t offset)
 {
 	char buf[30], *value, *pbuf;
 	struct IODATA reg_val;
diff -u -N -r write_proc.orig/kernel/profile.c write_proc/kernel/profile.c
--- write_proc.orig/kernel/profile.c	2009-08-29 15:53:44.000000000 +0200
+++ write_proc/kernel/profile.c	2009-08-29 16:15:35.000000000 +0200
@@ -455,7 +455,7 @@
 }
 
 static int prof_cpu_mask_write_proc(struct file *file,
-	const char __user *buffer,  unsigned long count, void *data)
+	const char __user *buffer,  unsigned long count, void *data, off_t off)
 {
 	struct cpumask *mask = data;
 	unsigned long full_count = count, err;
diff -u -N -r write_proc.orig/Documentation/DocBook/procfs-guide.tmpl write_proc/Documentation/DocBook/procfs-guide.tmpl
--- write_proc.orig/Documentation/DocBook/procfs-guide.tmpl	2009-08-29 18:21:36.000000000 +0200
+++ write_proc/Documentation/DocBook/procfs-guide.tmpl	2009-08-29 18:22:27.000000000 +0200
@@ -455,6 +455,7 @@
 	  <paramdef>const char* <parameter>buffer</parameter></paramdef>
 	  <paramdef>unsigned long <parameter>count</parameter></paramdef>
 	  <paramdef>void* <parameter>data</parameter></paramdef>
+	  <paramdef>off_t <parameter>off</parameter></paramdef>
 	</funcprototype>
       </funcsynopsis>
 

Re: [PATCH] Fix proc_file_write missing ppos update

[Christoph Hellwig]

On Sat, Aug 29, 2009 at 06:38:12PM +0200, Stefani Seibold wrote:
> The following fix a long standing issue in the proc_file_write function,
> which doesn't update the ppos file position pointer.

The right fix is to get rid of the last remaining
read_proc_t/write_proc_t instances and switch everyone to implement file
operations.  Alexey has been working on this on and off for a while.

Re: [PATCH] Fix proc_file_write missing ppos update

[Alexey Dobriyan]

On Sat, Aug 29, 2009 at 07:16:50PM -0400, Christoph Hellwig wrote:
> On Sat, Aug 29, 2009 at 06:38:12PM +0200, Stefani Seibold wrote:
> > The following fix a long standing issue in the proc_file_write function,
> > which doesn't update the ppos file position pointer.
> 
> The right fix is to get rid of the last remaining
> read_proc_t/write_proc_t instances and switch everyone to implement file
> operations.  Alexey has been working on this on and off for a while.

Yes, please, ->write_proc is going to end very soon.

As for prevoius arguments:
- changing ->write_proc signature is _of course_ going to break
  out-of-tree stuff
- ->proc_fops worked since forever, and since it's struct file_operations,
  nobody prohibits implementing ->write with pos update and what not.
- seq_file is not relevant to this issue because seq_file is for read path
  only.

Back into SCSI pile of users.

Re: [PATCH] Fix proc_file_write missing ppos update

[Stefani Seibold]

Am Samstag, den 29.08.2009, 19:16 -0400 schrieb Christoph Hellwig:
> On Sat, Aug 29, 2009 at 06:38:12PM +0200, Stefani Seibold wrote:
> > The following fix a long standing issue in the proc_file_write function,
> > which doesn't update the ppos file position pointer.
> 
> The right fix is to get rid of the last remaining
> read_proc_t/write_proc_t instances and switch everyone to implement file
> operations.  Alexey has been working on this on and off for a while.
> 

Switching all users of read_proc_t/write_proc_t to file operation is a
huge job. About 180 files must be fixed.

But the main reason not to do this is because the breakage of "out of
tree" drivers.

I like the current simplified proc interface. It saves a lot of code
duplication because the basic operations will be handled inside the
kernel and not in the driver.

There is no reason for Alexey to finish his work, submit it and maybe it
will be accepted. 

In the meantime i will see my patch as the preferred solution: a cleanup
of the current interface.

Re: [PATCH] Fix proc_file_write missing ppos update

[Stefani Seibold]

Am Sonntag, den 30.08.2009, 12:09 +0400 schrieb Alexey Dobriyan:
> On Sat, Aug 29, 2009 at 07:16:50PM -0400, Christoph Hellwig wrote:
> > On Sat, Aug 29, 2009 at 06:38:12PM +0200, Stefani Seibold wrote:
> > > The following fix a long standing issue in the proc_file_write function,
> > > which doesn't update the ppos file position pointer.
> > 
> > The right fix is to get rid of the last remaining
> > read_proc_t/write_proc_t instances and switch everyone to implement file
> > operations.  Alexey has been working on this on and off for a while.
> 
> Yes, please, ->write_proc is going to end very soon.
> 
> As for prevoius arguments:
> - changing ->write_proc signature is _of course_ going to break
>   out-of-tree stuff

No, it breaks nothing. You get only a warning. I think thats okay...

> - ->proc_fops worked since forever, and since it's struct file_operations,
>   nobody prohibits implementing ->write with pos update and what not.
> - seq_file is not relevant to this issue because seq_file is for read path
>   only.
> 
> Back into SCSI pile of users.

Re: [PATCH] Fix proc_file_write missing ppos update

[Alexey Dobriyan]

On Sun, Aug 30, 2009 at 09:05:44PM +0200, Stefani Seibold wrote:
> Am Samstag, den 29.08.2009, 19:16 -0400 schrieb Christoph Hellwig:
> > On Sat, Aug 29, 2009 at 06:38:12PM +0200, Stefani Seibold wrote:
> > > The following fix a long standing issue in the proc_file_write function,
> > > which doesn't update the ppos file position pointer.
> > 
> > The right fix is to get rid of the last remaining
> > read_proc_t/write_proc_t instances and switch everyone to implement file
> > operations.  Alexey has been working on this on and off for a while.
> > 
> 
> Switching all users of read_proc_t/write_proc_t to file operation is a
> huge job. About 180 files must be fixed.

Less, staging stuff is on it's own.

> But the main reason not to do this is because the breakage of "out of
> tree" drivers.

Just how many times it needs to be repeated.

Out-of-tree argument means almost nothing.

> I like the current simplified proc interface. It saves a lot of code
> duplication because the basic operations will be handled inside the
> kernel and not in the driver.

It's garbage.

Arbirtrary 3 KB limit on read,
kernel memory giving trailer in ->read_proc which is impossible to get right,
was always buggy wrt module unload (quickly fixed, but it was always meant
to be temporary).

Now that SCSI pile is mostly finished, what a horrible code interface provoked.

> There is no reason for Alexey to finish his work, submit it and maybe it
> will be accepted. 

You'd better start converting to struct file_operations::write now.

> In the meantime i will see my patch as the preferred solution: a cleanup
> of the current interface.

Re: [PATCH] Fix proc_file_write missing ppos update

[Arnd Bergmann]

On Monday 31 August 2009, Alexey Dobriyan wrote:
> Out-of-tree argument means almost nothing.
> 
> ...
>
> You'd better start converting to struct file_operations::write now.

Maybe a purely mechanical conversion to file_operations would be a nice first
step, just so we can remove read_proc and write_proc? Taking scsi_proc.c
as an example, this should be really straightforward and lets us get
rid of the write_proc and read_proc callbacks from proc_dir_entry without
having to rewrite all the remaining drivers that use it.

Obviously, someone who understands the specific driver code better should
then clean up the code by converting to seq_file operations or something
else that is appropriate there.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/scsi/scsi_proc.c |   27 ++++++++++++++++++++++-----
 fs/proc/generic.c        |   16 ++++++----------
 include/linux/proc_fs.h  |   11 +++++++++++
 3 files changed, 39 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/scsi_proc.c b/drivers/scsi/scsi_proc.c
index 77fbddb..f2a48a1 100644
--- a/drivers/scsi/scsi_proc.c
+++ b/drivers/scsi/scsi_proc.c
@@ -136,6 +136,26 @@ void scsi_proc_hostdir_rm(struct scsi_host_template *sht)
 	mutex_unlock(&global_host_template_mutex);
 }
 
+static ssize_t scsi_proc_read(struct file *file, char __user *buf,
+			size_t count, loff_t *off)
+{
+	return __proc_file_read(&proc_scsi_read, buf, count, off,
+			PDE(file->f_path.dentry->d_inode)->data);
+}
+
+static ssize_t scsi_proc_write(struct file *file, const char __user *buf,
+		size_t count, loff_t *off)
+{
+	return proc_scsi_write_proc(file, buf, count,
+			PDE(file->f_path.dentry->d_inode)->data);
+}
+
+static const struct file_operations scsi_proc_fops = {
+	.owner = THIS_MODULE,
+	.read = scsi_proc_read,
+	.write = scsi_proc_write,
+	.llseek = generic_file_llseek,
+};
 
 /**
  * scsi_proc_host_add - Add entry for this host to appropriate /proc dir
@@ -151,16 +170,14 @@ void scsi_proc_host_add(struct Scsi_Host *shost)
 		return;
 
 	sprintf(name,"%d", shost->host_no);
-	p = create_proc_read_entry(name, S_IFREG | S_IRUGO | S_IWUSR,
-			sht->proc_dir, proc_scsi_read, shost);
+	p = proc_create_data(name, S_IFREG | S_IRUGO | S_IWUSR,
+			sht->proc_dir, &scsi_proc_fops, shost);
 	if (!p) {
 		printk(KERN_ERR "%s: Failed to register host %d in"
 		       "%s\n", __func__, shost->host_no,
 		       sht->proc_name);
 		return;
-	} 
-
-	p->write_proc = proc_scsi_write_proc;
+	}
 }
 
 /**
diff --git a/fs/proc/generic.c b/fs/proc/generic.c
index fa678ab..ccf3949 100644
--- a/fs/proc/generic.c
+++ b/fs/proc/generic.c
@@ -36,17 +36,14 @@ static int proc_match(int len, const char *name, struct proc_dir_entry *de)
 /* buffer size is one page but our output routines use some slack for overruns */
 #define PROC_BLOCK_SIZE	(PAGE_SIZE - 1024)
 
-static ssize_t
-__proc_file_read(struct file *file, char __user *buf, size_t nbytes,
-	       loff_t *ppos)
+ssize_t __proc_file_read(read_proc_t read_proc, char __user *buf,
+			size_t nbytes, loff_t *ppos, void *data)
 {
-	struct inode * inode = file->f_path.dentry->d_inode;
 	char 	*page;
 	ssize_t	retval=0;
 	int	eof=0;
 	ssize_t	n, count;
 	char	*start;
-	struct proc_dir_entry * dp;
 	unsigned long long pos;
 
 	/*
@@ -60,7 +57,6 @@ __proc_file_read(struct file *file, char __user *buf, size_t nbytes,
 	if (nbytes > MAX_NON_LFS - pos)
 		nbytes = MAX_NON_LFS - pos;
 
-	dp = PDE(inode);
 	if (!(page = (char*) __get_free_page(GFP_TEMPORARY)))
 		return -ENOMEM;
 
@@ -68,7 +64,7 @@ __proc_file_read(struct file *file, char __user *buf, size_t nbytes,
 		count = min_t(size_t, PROC_BLOCK_SIZE, nbytes);
 
 		start = NULL;
-		if (dp->read_proc) {
+		if (read_proc) {
 			/*
 			 * How to be a proc read function
 			 * ------------------------------
@@ -116,8 +112,8 @@ __proc_file_read(struct file *file, char __user *buf, size_t nbytes,
 			 *    requested offset advanced by the number of bytes
 			 *    absorbed.
 			 */
-			n = dp->read_proc(page, &start, *ppos,
-					  count, &eof, dp->data);
+			n = read_proc(page, &start, *ppos,
+					  count, &eof, data);
 		} else
 			break;
 
@@ -197,7 +193,7 @@ proc_file_read(struct file *file, char __user *buf, size_t nbytes,
 	pde->pde_users++;
 	spin_unlock(&pde->pde_unload_lock);
 
-	rv = __proc_file_read(file, buf, nbytes, ppos);
+	rv = __proc_file_read(pde->read_proc, buf, nbytes, ppos, pde->data);
 
 	pde_users_dec(pde);
 	return rv;
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
index e6e77d3..949a382 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -103,6 +103,10 @@ struct proc_dir_entry *proc_create_data(const char *name, mode_t mode,
 				struct proc_dir_entry *parent,
 				const struct file_operations *proc_fops,
 				void *data);
+/* do not use in new code */
+ssize_t __proc_file_read(read_proc_t read_proc, char __user *buf,
+                        size_t nbytes, loff_t *ppos, void *data);
+
 extern void remove_proc_entry(const char *name, struct proc_dir_entry *parent);
 
 struct pid_namespace;
@@ -195,6 +199,13 @@ static inline struct proc_dir_entry *proc_create_data(const char *name,
 }
 #define remove_proc_entry(name, parent) do {} while (0)
 
+static inline ssize_t __proc_file_read(read_proc_t read_proc,
+			char __user *buf, size_t nbytes, loff_t *ppos,
+			void *data)
+{
+	return 0;
+}
+
 static inline struct proc_dir_entry *proc_symlink(const char *name,
 		struct proc_dir_entry *parent,const char *dest) {return NULL;}
 static inline struct proc_dir_entry *proc_mkdir(const char *name,

Re: [PATCH] Fix proc_file_write missing ppos update

[Alexey Dobriyan]

On Mon, Aug 31, 2009 at 05:44:52PM +0200, Arnd Bergmann wrote:
> On Monday 31 August 2009, Alexey Dobriyan wrote:
> > Out-of-tree argument means almost nothing.
> > 
> > ...
> >
> > You'd better start converting to struct file_operations::write now.
> 
> Maybe a purely mechanical conversion to file_operations would be a nice first
> step, just so we can remove read_proc and write_proc? Taking scsi_proc.c
> as an example, this should be really straightforward and lets us get
> rid of the write_proc and read_proc callbacks from proc_dir_entry without
> having to rewrite all the remaining drivers that use it.
> 
> Obviously, someone who understands the specific driver code better should
> then clean up the code by converting to seq_file operations or something
> else that is appropriate there.

I don't like any churn in that area. Everything is ready for ->proc_fops usage.
So we should simply convert users away.
Actually most of SCSI and IDE is already done, just wasn't sent yet.

After conversion, ->read_proc, ->write_proc, create_proc_read_entry(),
create_proc_entry() will become deprecated for a while, then gone.

Re: [PATCH] Fix proc_file_write missing ppos update

[Christoph Hellwig]

On Mon, Aug 31, 2009 at 09:19:56PM +0400, Alexey Dobriyan wrote:
> After conversion, ->read_proc, ->write_proc, create_proc_read_entry(),
> create_proc_entry() will become deprecated for a while, then gone.

Just kill it once all intree users are gone, deprecation is only useful
if we still have some in-tree user around but want to keep people from
adding new ones.

Re: [PATCH] Fix proc_file_write missing ppos update

[Arnd Bergmann]

On Monday 31 August 2009, Alexey Dobriyan wrote:
> I don't like any churn in that area. Everything is ready for ->proc_fops usage.
> So we should simply convert users away.
> Actually most of SCSI and IDE is already done, just wasn't sent yet.

Ok, fair enough. I was expecting that the remaining drivers are suffering from
lack of testing options, as many of them are rather obscure. If you think we can
just convert them in one step, that's even better.

	Arnd <><

Re: [PATCH] Fix proc_file_write missing ppos update

[Al Viro]

On Sun, Aug 30, 2009 at 09:05:44PM +0200, Stefani Seibold wrote:
> Switching all users of read_proc_t/write_proc_t to file operation is a
> huge job. About 180 files must be fixed.
> 
> But the main reason not to do this is because the breakage of "out of
> tree" drivers.

> I like the current simplified proc interface. It saves a lot of code
> duplication because the basic operations will be handled inside the
> kernel and not in the driver.

_What_ code duplication?  Would that be
        struct proc_dir_entry *pde = PDE(file->f_path.dentry->d_inode);
and calculation of pde->data currently done by proc_file_write()?
Pardon me, but that's hard to take seriously.  As for the ->read() side,
most of those suckers end up switched to use of seq_read() and there's
not a lot of boilerplate code in the resulting variants...

Re: [PATCH] Fix proc_file_write missing ppos update

[Stefani Seibold]

Am Samstag, den 12.09.2009, 16:28 +0100 schrieb Al Viro:
> On Sun, Aug 30, 2009 at 09:05:44PM +0200, Stefani Seibold wrote:
> > Switching all users of read_proc_t/write_proc_t to file operation is a
> > huge job. About 180 files must be fixed.
> > 
> > But the main reason not to do this is because the breakage of "out of
> > tree" drivers.
> 
> > I like the current simplified proc interface. It saves a lot of code
> > duplication because the basic operations will be handled inside the
> > kernel and not in the driver.
> 
> _What_ code duplication?  Would that be
>         struct proc_dir_entry *pde = PDE(file->f_path.dentry->d_inode);
> and calculation of pde->data currently done by proc_file_write()?
> Pardon me, but that's hard to take seriously.  As for the ->read() side,
> most of those suckers end up switched to use of seq_read() and there's
> not a lot of boilerplate code in the resulting variants...

Hi Al, you are 2 weeks to late, the discussion has already ended. But
lets start it again.... 

There is some code duplication, proc_file_write has 17 lines of code.
And together with my path it will be 19 lines of code. How much lines of
duplicated code are necessary for take them seriously? 

I know that the current mainstream development targets enterprise and
maybe sometime desktop systems. But the truth is that linux will be
mostly used in embedded system with very limited memory constrains.

BTW: I personally think the whole seq_.... interface is IMHO to
complicate to use, procfs is much simpler. But this is only my personal
opinion! 

Greetings,
Stefani

Re: [PATCH] Fix proc_file_write missing ppos update

[Eric W. Biederman]

Stefani Seibold <stefani@seibold.net> writes:

> Am Samstag, den 12.09.2009, 16:28 +0100 schrieb Al Viro:
>> On Sun, Aug 30, 2009 at 09:05:44PM +0200, Stefani Seibold wrote:
>> > Switching all users of read_proc_t/write_proc_t to file operation is a
>> > huge job. About 180 files must be fixed.
>> > 
>> > But the main reason not to do this is because the breakage of "out of
>> > tree" drivers.
>> 
>> > I like the current simplified proc interface. It saves a lot of code
>> > duplication because the basic operations will be handled inside the
>> > kernel and not in the driver.
>> 
>> _What_ code duplication?  Would that be
>>         struct proc_dir_entry *pde = PDE(file->f_path.dentry->d_inode);
>> and calculation of pde->data currently done by proc_file_write()?
>> Pardon me, but that's hard to take seriously.  As for the ->read() side,
>> most of those suckers end up switched to use of seq_read() and there's
>> not a lot of boilerplate code in the resulting variants...
>
> Hi Al, you are 2 weeks to late, the discussion has already ended. But
> lets start it again.... 
>
> There is some code duplication, proc_file_write has 17 lines of code.
> And together with my path it will be 19 lines of code. How much lines of
> duplicated code are necessary for take them seriously? 
>
> I know that the current mainstream development targets enterprise and
> maybe sometime desktop systems. But the truth is that linux will be
> mostly used in embedded system with very limited memory constrains.
>
> BTW: I personally think the whole seq_.... interface is IMHO to
> complicate to use, procfs is much simpler. But this is only my personal
> opinion! 

The point of all of this is that you are trying to enhance an
interface that is essentially deprecated.  We don't want to use it
anymore because at least on the read side there are recurring problems
with buffer overflows and reads not handling seeks properly, etc.  So
everything is slowing switching being fixed and taken off of the
current proc_file_read/proc_file_write handlers.

Eric

Re: [PATCH] Fix proc_file_write missing ppos update

[Stefani Seibold]

Am Freitag, den 07.08.2009, 23:59 -0700 schrieb Eric W. Biederman:
> Andrew Morton <akpm@linux-foundation.org> writes:
> 
> > On Fri, 07 Aug 2009 23:43:07 +0200
> > Stefani Seibold <stefani@seibold.net> wrote:
> >
> 
> >> So what is your suggestion? Should we drop this patch or should we
> >> analyze the users and fix it?
> >
> > Well.
> >
> > We could review all implementations of ->write_proc.  There only seem
> > to be twenty or so.
> >
> > If any of them will have their behaviour altered by this patch then
> > let's look at those on a case-by-case basis and decide whether making
> > this change will have an acceptable risk.
> >
> > If we _do_ find one for which we simply cannot make this behavioural
> > change then..  ugh.  We could perhaps add a new `bool
> > proc_dir_entry.implement_old_broken_behaviour' and set that flag for
> > the offending driver(s) and test it within proc_write_file().
> >
> > Or we could do
> >
> > 	if (pde->write_proc_new) {
> > 		rv = pde->write_proc_new(file, buffer, count, pde->data);
> > 		*ppos += rv;
> > 	} else {
> > 		rv = pde->write_proc(file, buffer, count, pde->data);
> > 	}
> >
> > which is really the same thing and isn't obviously better ;)
> >
> >> My opinion is to fix it, because it is wrong and it limits the usage of
> >> the proc_write operation. Many embedded developers like me count on proc
> >> support, because it is much simpler to use than the seqfile thing.
> 
> The simple and portable answer is to implement your own file_operations.
> 

This is what i still doing since a long time:

<CodeSnip>
 proc_entry = create_proc_entry(procname, S_IRUGO|S_IWUGO, NULL);

 proc_entry->read_proc = proc_read_foo;

 bar->proc_file_operations.llseek = proc_entry->proc_fops->llseek;
 bar->proc_file_operations.read = proc_entry->proc_fops->read;
 bar->proc_file_operations.write = proc_write_foo;

 proc_entry->proc_fops = &bar->proc_file_operations;
</CodeSnip>

This works very well for me, but it requires some additional step
because of the buggy interface.

But the question is: can we fix this bug? 

I will have a look on the current users of proc->write and if there are
no driver which is depending on the old behavior we can fix it. 
   
> It is unlikely that implementing a new totally unstructured proc file is
> a good idea.
> 

That is your opinion. I still use it f.e. to access a eeprom.
 
> I'm not quite up to speed on write_proc but I believe we have been spraying
> read_proc and write_proc because of problems with the interface.
> 

First: I never noticed a problem with the current proc interface. The
only issue i figured out is the proc_write ppos problem.

Second: If speed matters or not is a question of the use case. Sometimes
a simple solution is required.  

Stefani

Re: [PATCH] Fix proc_file_write missing ppos update

[Eric W. Biederman]

Andrew Morton <akpm@linux-foundation.org> writes:

> On Fri, 07 Aug 2009 23:43:07 +0200
> Stefani Seibold <stefani@seibold.net> wrote:
>

>> So what is your suggestion? Should we drop this patch or should we
>> analyze the users and fix it?
>
> Well.
>
> We could review all implementations of ->write_proc.  There only seem
> to be twenty or so.
>
> If any of them will have their behaviour altered by this patch then
> let's look at those on a case-by-case basis and decide whether making
> this change will have an acceptable risk.
>
> If we _do_ find one for which we simply cannot make this behavioural
> change then..  ugh.  We could perhaps add a new `bool
> proc_dir_entry.implement_old_broken_behaviour' and set that flag for
> the offending driver(s) and test it within proc_write_file().
>
> Or we could do
>
> 	if (pde->write_proc_new) {
> 		rv = pde->write_proc_new(file, buffer, count, pde->data);
> 		*ppos += rv;
> 	} else {
> 		rv = pde->write_proc(file, buffer, count, pde->data);
> 	}
>
> which is really the same thing and isn't obviously better ;)
>
>> My opinion is to fix it, because it is wrong and it limits the usage of
>> the proc_write operation. Many embedded developers like me count on proc
>> support, because it is much simpler to use than the seqfile thing.

The simple and portable answer is to implement your own file_operations.

It is unlikely that implementing a new totally unstructured proc file is
a good idea.

I'm not quite up to speed on write_proc but I believe we have been spraying
read_proc and write_proc because of problems with the interface.

Eric

Re: [PATCH] Fix proc_file_write missing ppos update

[Andrew Morton]

On Fri, 07 Aug 2009 23:43:07 +0200
Stefani Seibold <stefani@seibold.net> wrote:

> > > +			*ppos += rv;
> > >  	}
> > >  	return rv;
> > >  }
> > 
> > Yes, that's odd.
> > 
> > I worry that there might be procfs write handlers which are looking at
> > *ppos and whose behaviour might be altered by this patch.
> > 
> > <searches a bit>
> > 
> > Look at arch/s390/appldata/appldata_base.c:appldata_timer_handler().
> > 
> > static int
> > appldata_timer_handler(ctl_table *ctl, int write, struct file *filp,
> > 			   void __user *buffer, size_t *lenp, loff_t *ppos)
> > {
> > 	int len;
> > 	char buf[2];
> > 
> > 	if (!*lenp || *ppos) {
> > 		*lenp = 0;
> > 		return 0;
> > 	}
> > 
> > 
> 
> This function will be handled IMHO by the proc_sys_call_handler which
> has nothing to do with the proc_file_write.
> /proc/sys/... file access should be not touched because there are
> handled differently. 

hm, OK, fail.

> > Prior to your change, an application which opened that proc file and
> > repeatedly wrote to the fd would repeatedly start and stop the timer.
> > 
> > After your change, the second and successive writes would have no
> > effect unless the application was changed to lseek back to the start of
> > the "file".
> > 
> > And that was just the second file I looked at via
> > 
> > 	$EDITOR $(grep -l '[*]ppos' $(grep -rl _proc_ .))
> 
> Yes, i think you are right, i have forseen also that there maybe some
> pitfalls. The question is: is there any appplication which will be
> broken by this patch?

There is no way of telling.  We have to assume that there will be such
code out there.

> So what is your suggestion? Should we drop this patch or should we
> analyze the users and fix it?

Well.

We could review all implementations of ->write_proc.  There only seem
to be twenty or so.

If any of them will have their behaviour altered by this patch then
let's look at those on a case-by-case basis and decide whether making
this change will have an acceptable risk.

If we _do_ find one for which we simply cannot make this behavioural
change then..  ugh.  We could perhaps add a new `bool
proc_dir_entry.implement_old_broken_behaviour' and set that flag for
the offending driver(s) and test it within proc_write_file().

Or we could do

	if (pde->write_proc_new) {
		rv = pde->write_proc_new(file, buffer, count, pde->data);
		*ppos += rv;
	} else {
		rv = pde->write_proc(file, buffer, count, pde->data);
	}

which is really the same thing and isn't obviously better ;)

> My opinion is to fix it, because it is wrong and it limits the usage of
> the proc_write operation. Many embedded developers like me count on proc
> support, because it is much simpler to use than the seqfile thing.
> 

Re: [PATCH] Fix proc_file_write missing ppos update

[Stefani Seibold]

Am Freitag, den 07.08.2009, 13:58 -0700 schrieb Andrew Morton: 
> On Fri, 07 Aug 2009 22:27:10 +0200
> Stefani Seibold <stefani@seibold.net> wrote:
> 
> > The following fix a long standing issue in the proc_file_write function,
> > which doesn't update the ppos file position pointer.
> > 
> > This prevent the usage of multiple sequently writes on an opened proc
> > file, because it is impossible to distinguish these due the offset is
> > always 0.
> > 
> > Signed-off-by: Stefani Seibold <stefani@seibold.net>
> > 
> >  generic.c |    3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > --- linux-2.6.31-rc4.orig/fs/proc/generic.c	2009-08-07 22:05:57.000000000 +0200
> > +++ linux-2.6.30-rc4/fs/proc/generic.c	2009-08-07 22:06:22.000000000 +0200
> > @@ -219,9 +219,10 @@
> >  		pde->pde_users++;
> >  		spin_unlock(&pde->pde_unload_lock);
> >  
> > -		/* FIXME: does this routine need ppos?  probably... */
> >  		rv = pde->write_proc(file, buffer, count, pde->data);
> >  		pde_users_dec(pde);
> > +		if (rv > 0)
> > +			*ppos += rv;
> >  	}
> >  	return rv;
> >  }
> 
> Yes, that's odd.
> 
> I worry that there might be procfs write handlers which are looking at
> *ppos and whose behaviour might be altered by this patch.
> 
> <searches a bit>
> 
> Look at arch/s390/appldata/appldata_base.c:appldata_timer_handler().
> 
> static int
> appldata_timer_handler(ctl_table *ctl, int write, struct file *filp,
> 			   void __user *buffer, size_t *lenp, loff_t *ppos)
> {
> 	int len;
> 	char buf[2];
> 
> 	if (!*lenp || *ppos) {
> 		*lenp = 0;
> 		return 0;
> 	}
> 
> 

This function will be handled IMHO by the proc_sys_call_handler which
has nothing to do with the proc_file_write.
/proc/sys/... file access should be not touched because there are
handled differently. 

> Prior to your change, an application which opened that proc file and
> repeatedly wrote to the fd would repeatedly start and stop the timer.
> 
> After your change, the second and successive writes would have no
> effect unless the application was changed to lseek back to the start of
> the "file".
> 
> And that was just the second file I looked at via
> 
> 	$EDITOR $(grep -l '[*]ppos' $(grep -rl _proc_ .))

Yes, i think you are right, i have forseen also that there maybe some
pitfalls. The question is: is there any appplication which will be
broken by this patch?

So what is your suggestion? Should we drop this patch or should we
analyze the users and fix it?

My opinion is to fix it, because it is wrong and it limits the usage of
the proc_write operation. Many embedded developers like me count on proc
support, because it is much simpler to use than the seqfile thing.

Re: [PATCH] Fix proc_file_write missing ppos update

[Andrew Morton]

On Fri, 07 Aug 2009 22:27:10 +0200
Stefani Seibold <stefani@seibold.net> wrote:

> The following fix a long standing issue in the proc_file_write function,
> which doesn't update the ppos file position pointer.
> 
> This prevent the usage of multiple sequently writes on an opened proc
> file, because it is impossible to distinguish these due the offset is
> always 0.
> 
> Signed-off-by: Stefani Seibold <stefani@seibold.net>
> 
>  generic.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> --- linux-2.6.31-rc4.orig/fs/proc/generic.c	2009-08-07 22:05:57.000000000 +0200
> +++ linux-2.6.30-rc4/fs/proc/generic.c	2009-08-07 22:06:22.000000000 +0200
> @@ -219,9 +219,10 @@
>  		pde->pde_users++;
>  		spin_unlock(&pde->pde_unload_lock);
>  
> -		/* FIXME: does this routine need ppos?  probably... */
>  		rv = pde->write_proc(file, buffer, count, pde->data);
>  		pde_users_dec(pde);
> +		if (rv > 0)
> +			*ppos += rv;
>  	}
>  	return rv;
>  }

Yes, that's odd.

I worry that there might be procfs write handlers which are looking at
*ppos and whose behaviour might be altered by this patch.

<searches a bit>

Look at arch/s390/appldata/appldata_base.c:appldata_timer_handler().

static int
appldata_timer_handler(ctl_table *ctl, int write, struct file *filp,
			   void __user *buffer, size_t *lenp, loff_t *ppos)
{
	int len;
	char buf[2];

	if (!*lenp || *ppos) {
		*lenp = 0;
		return 0;
	}


Prior to your change, an application which opened that proc file and
repeatedly wrote to the fd would repeatedly start and stop the timer.

After your change, the second and successive writes would have no
effect unless the application was changed to lseek back to the start of
the "file".

And that was just the second file I looked at via

	$EDITOR $(grep -l '[*]ppos' $(grep -rl _proc_ .))

[PATCH] Fix proc_file_write missing ppos update

[Stefani Seibold]

The following fix a long standing issue in the proc_file_write function,
which doesn't update the ppos file position pointer.

This prevent the usage of multiple sequently writes on an opened proc
file, because it is impossible to distinguish these due the offset is
always 0.

Signed-off-by: Stefani Seibold <stefani@seibold.net>

 generic.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

--- linux-2.6.31-rc4.orig/fs/proc/generic.c	2009-08-07 22:05:57.000000000 +0200
+++ linux-2.6.30-rc4/fs/proc/generic.c	2009-08-07 22:06:22.000000000 +0200
@@ -219,9 +219,10 @@
 		pde->pde_users++;
 		spin_unlock(&pde->pde_unload_lock);
 
-		/* FIXME: does this routine need ppos?  probably... */
 		rv = pde->write_proc(file, buffer, count, pde->data);
 		pde_users_dec(pde);
+		if (rv > 0)
+			*ppos += rv;
 	}
 	return rv;
 }