[PATCH 0/9] vme_user checkpatch fixes and read()/write() rework

[PATCH 0/9] vme_user checkpatch fixes and read()/write() rework

[Dmitry Kalinkin]

First four patches are fixes for various checpatch warnings.  Next there is a
change to drop large read()/write() stub followed by a change to rework user
copy error codes.  Last three changes are refactorings.

Dmitry Kalinkin (9):
  staging: vme_user: fix code alignment
  staging: vme_user: fix blank lines
  staging: vme_user: fix NULL comparison style
  staging: vme_user: fix kmalloc style
  staging: vme_user: allow large read()/write()
  staging: vme_user: return -EFAULT on __copy_*_user errors
  staging: vme_user: remove unused variable
  staging: vme_user: remove distracting comment
  staging: vme_user: remove okcount variable

 drivers/staging/vme/devices/vme_user.c | 158 +++++++++++----------------------
 1 file changed, 51 insertions(+), 107 deletions(-)

-- 
1.8.3.1

[PATCH 1/9] staging: vme_user: fix code alignment

[Dmitry Kalinkin]

Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
---
 drivers/staging/vme/devices/vme_user.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
index 5ff44fb..285e00e 100644
--- a/drivers/staging/vme/devices/vme_user.c
+++ b/drivers/staging/vme/devices/vme_user.c
@@ -128,7 +128,7 @@ struct vme_user_vma_priv {
  * transfer the data directly into the user space buffers.
  */
 static ssize_t resource_to_user(int minor, char __user *buf, size_t count,
-	loff_t *ppos)
+				loff_t *ppos)
 {
 	ssize_t retval;
 	ssize_t copied = 0;
@@ -167,7 +167,7 @@ static ssize_t resource_to_user(int minor, char __user *buf, size_t count,
  * transfer the data directly from the user space buffers out to VME.
  */
 static ssize_t resource_from_user(unsigned int minor, const char __user *buf,
-	size_t count, loff_t *ppos)
+				  size_t count, loff_t *ppos)
 {
 	ssize_t retval;
 	ssize_t copied = 0;
@@ -195,7 +195,7 @@ static ssize_t resource_from_user(unsigned int minor, const char __user *buf,
 }
 
 static ssize_t buffer_to_user(unsigned int minor, char __user *buf,
-	size_t count, loff_t *ppos)
+			      size_t count, loff_t *ppos)
 {
 	void *image_ptr;
 	ssize_t retval;
@@ -214,7 +214,7 @@ static ssize_t buffer_to_user(unsigned int minor, char __user *buf,
 }
 
 static ssize_t buffer_from_user(unsigned int minor, const char __user *buf,
-	size_t count, loff_t *ppos)
+				size_t count, loff_t *ppos)
 {
 	void *image_ptr;
 	size_t retval;
@@ -233,7 +233,7 @@ static ssize_t buffer_from_user(unsigned int minor, const char __user *buf,
 }
 
 static ssize_t vme_user_read(struct file *file, char __user *buf, size_t count,
-			loff_t *ppos)
+			     loff_t *ppos)
 {
 	unsigned int minor = MINOR(file_inode(file)->i_rdev);
 	ssize_t retval;
@@ -279,7 +279,7 @@ static ssize_t vme_user_read(struct file *file, char __user *buf, size_t count,
 }
 
 static ssize_t vme_user_write(struct file *file, const char __user *buf,
-			size_t count, loff_t *ppos)
+			      size_t count, loff_t *ppos)
 {
 	unsigned int minor = MINOR(file_inode(file)->i_rdev);
 	ssize_t retval;
@@ -354,7 +354,7 @@ static loff_t vme_user_llseek(struct file *file, loff_t off, int whence)
  * already been defined.
  */
 static int vme_user_ioctl(struct inode *inode, struct file *file,
-	unsigned int cmd, unsigned long arg)
+			  unsigned int cmd, unsigned long arg)
 {
 	struct vme_master master;
 	struct vme_slave slave;
@@ -390,12 +390,13 @@ static int vme_user_ioctl(struct inode *inode, struct file *file,
 			 *	to userspace as they are
 			 */
 			retval = vme_master_get(image[minor].resource,
-				&master.enable, &master.vme_addr,
-				&master.size, &master.aspace,
-				&master.cycle, &master.dwidth);
+						&master.enable,
+						&master.vme_addr,
+						&master.size, &master.aspace,
+						&master.cycle, &master.dwidth);
 
 			copied = copy_to_user(argp, &master,
-				sizeof(struct vme_master));
+					      sizeof(struct vme_master));
 			if (copied != 0) {
 				pr_warn("Partial copy to userspace\n");
 				return -EFAULT;
@@ -435,12 +436,12 @@ static int vme_user_ioctl(struct inode *inode, struct file *file,
 			 *	to userspace as they are
 			 */
 			retval = vme_slave_get(image[minor].resource,
-				&slave.enable, &slave.vme_addr,
-				&slave.size, &pci_addr, &slave.aspace,
-				&slave.cycle);
+					       &slave.enable, &slave.vme_addr,
+					       &slave.size, &pci_addr,
+					       &slave.aspace, &slave.cycle);
 
 			copied = copy_to_user(argp, &slave,
-				sizeof(struct vme_slave));
+					      sizeof(struct vme_slave));
 			if (copied != 0) {
 				pr_warn("Partial copy to userspace\n");
 				return -EFAULT;
@@ -606,7 +607,7 @@ static int vme_user_probe(struct vme_dev *vdev)
 
 	/* Assign major and minor numbers for the driver */
 	err = register_chrdev_region(MKDEV(VME_MAJOR, 0), VME_DEVS,
-		driver_name);
+				     driver_name);
 	if (err) {
 		dev_warn(&vdev->dev, "Error getting Major Number %d for driver.\n",
 			 VME_MAJOR);
-- 
1.8.3.1

[PATCH 2/9] staging: vme_user: fix blank lines

[Dmitry Kalinkin]

Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
---
 drivers/staging/vme/devices/vme_user.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
index 285e00e..1f00ad7 100644
--- a/drivers/staging/vme/devices/vme_user.c
+++ b/drivers/staging/vme/devices/vme_user.c
@@ -101,13 +101,13 @@ struct image_desc {
 	struct vme_resource *resource;	/* VME resource */
 	int mmap_count;		/* Number of current mmap's */
 };
+
 static struct image_desc image[VME_DEVS];
 
 static struct cdev *vme_user_cdev;		/* Character device */
 static struct class *vme_user_sysfs_class;	/* Sysfs class */
 static struct vme_dev *vme_user_bridge;		/* Pointer to user device */
 
-
 static const int type[VME_DEVS] = {	MASTER_MINOR,	MASTER_MINOR,
 					MASTER_MINOR,	MASTER_MINOR,
 					SLAVE_MINOR,	SLAVE_MINOR,
@@ -120,7 +120,6 @@ struct vme_user_vma_priv {
 	atomic_t refcnt;
 };
 
-
 /*
  * We are going ot alloc a page during init per window for small transfers.
  * Small transfers will go VME -> buffer -> user space. Larger (more than a
@@ -836,7 +835,6 @@ static void __exit vme_user_exit(void)
 	vme_unregister_driver(&vme_user_driver);
 }
 
-
 MODULE_PARM_DESC(bus, "Enumeration of VMEbus to which the driver is connected");
 module_param_array(bus, int, &bus_num, 0);
 
-- 
1.8.3.1

[PATCH 3/9] staging: vme_user: fix NULL comparison style

[Dmitry Kalinkin]

Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
---
 drivers/staging/vme/devices/vme_user.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
index 1f00ad7..b6d81e5 100644
--- a/drivers/staging/vme/devices/vme_user.c
+++ b/drivers/staging/vme/devices/vme_user.c
@@ -527,7 +527,7 @@ static int vme_user_master_mmap(unsigned int minor, struct vm_area_struct *vma)
 	}
 
 	vma_priv = kmalloc(sizeof(struct vme_user_vma_priv), GFP_KERNEL);
-	if (vma_priv == NULL) {
+	if (!vma_priv) {
 		mutex_unlock(&image[minor].mutex);
 		return -ENOMEM;
 	}
@@ -588,7 +588,7 @@ static int vme_user_probe(struct vme_dev *vdev)
 	char *name;
 
 	/* Save pointer to the bridge device */
-	if (vme_user_bridge != NULL) {
+	if (vme_user_bridge) {
 		dev_err(&vdev->dev, "Driver can only be loaded for 1 device\n");
 		err = -EINVAL;
 		goto err_dev;
@@ -636,7 +636,7 @@ static int vme_user_probe(struct vme_dev *vdev)
 		 */
 		image[i].resource = vme_slave_request(vme_user_bridge,
 			VME_A24, VME_SCT);
-		if (image[i].resource == NULL) {
+		if (!image[i].resource) {
 			dev_warn(&vdev->dev,
 				 "Unable to allocate slave resource\n");
 			err = -ENOMEM;
@@ -645,7 +645,7 @@ static int vme_user_probe(struct vme_dev *vdev)
 		image[i].size_buf = PCI_BUF_SIZE;
 		image[i].kern_buf = vme_alloc_consistent(image[i].resource,
 			image[i].size_buf, &image[i].pci_buf);
-		if (image[i].kern_buf == NULL) {
+		if (!image[i].kern_buf) {
 			dev_warn(&vdev->dev,
 				 "Unable to allocate memory for buffer\n");
 			image[i].pci_buf = 0;
@@ -663,7 +663,7 @@ static int vme_user_probe(struct vme_dev *vdev)
 		/* XXX Need to properly request attributes */
 		image[i].resource = vme_master_request(vme_user_bridge,
 			VME_A32, VME_SCT, VME_D32);
-		if (image[i].resource == NULL) {
+		if (!image[i].resource) {
 			dev_warn(&vdev->dev,
 				 "Unable to allocate master resource\n");
 			err = -ENOMEM;
@@ -671,7 +671,7 @@ static int vme_user_probe(struct vme_dev *vdev)
 		}
 		image[i].size_buf = PCI_BUF_SIZE;
 		image[i].kern_buf = kmalloc(image[i].size_buf, GFP_KERNEL);
-		if (image[i].kern_buf == NULL) {
+		if (!image[i].kern_buf) {
 			err = -ENOMEM;
 			vme_master_free(image[i].resource);
 			goto err_master;
-- 
1.8.3.1

[PATCH 4/9] staging: vme_user: fix kmalloc style

[Dmitry Kalinkin]

Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
---
 drivers/staging/vme/devices/vme_user.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
index b6d81e5..db5f890 100644
--- a/drivers/staging/vme/devices/vme_user.c
+++ b/drivers/staging/vme/devices/vme_user.c
@@ -526,7 +526,7 @@ static int vme_user_master_mmap(unsigned int minor, struct vm_area_struct *vma)
 		return err;
 	}
 
-	vma_priv = kmalloc(sizeof(struct vme_user_vma_priv), GFP_KERNEL);
+	vma_priv = kmalloc(sizeof(*vma_priv), GFP_KERNEL);
 	if (!vma_priv) {
 		mutex_unlock(&image[minor].mutex);
 		return -ENOMEM;
-- 
1.8.3.1

[PATCH 5/9] staging: vme_user: allow large read()/write()

[Dmitry Kalinkin]

This changes large master transfers to do shorter read/write rather than
return -EINVAL. User space will now be able to optimistically request a
large transfer and get at least some data.

This also removes comments suggesting on how to implement large
transfers. Current vme_master_* read and write implementations use CPU
copies that don't produce burst PCI accesses and subsequently no block
transfer on VME bus. In the end overall performance is quiet low and it
can't be fixed by doing direct copy to user space. Much easier solution
would be to just reuse kernel buffer.

Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
---
 drivers/staging/vme/devices/vme_user.c | 67 ++++++++++++----------------------
 1 file changed, 24 insertions(+), 43 deletions(-)

diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
index db5f890..52cd638 100644
--- a/drivers/staging/vme/devices/vme_user.c
+++ b/drivers/staging/vme/devices/vme_user.c
@@ -132,63 +132,44 @@ static ssize_t resource_to_user(int minor, char __user *buf, size_t count,
 	ssize_t retval;
 	ssize_t copied = 0;
 
-	if (count <= image[minor].size_buf) {
-		/* We copy to kernel buffer */
-		copied = vme_master_read(image[minor].resource,
-			image[minor].kern_buf, count, *ppos);
-		if (copied < 0)
-			return (int)copied;
-
-		retval = __copy_to_user(buf, image[minor].kern_buf,
-			(unsigned long)copied);
-		if (retval != 0) {
-			copied = (copied - retval);
-			pr_info("User copy failed\n");
-			return -EINVAL;
-		}
+	if (count > image[minor].size_buf)
+		count = image[minor].size_buf;
 
-	} else {
-		/* XXX Need to write this */
-		pr_info("Currently don't support large transfers\n");
-		/* Map in pages from userspace */
+	/* We copy to kernel buffer */
+	copied = vme_master_read(image[minor].resource, image[minor].kern_buf,
+				 count, *ppos);
+	if (copied < 0)
+		return (int)copied;
 
-		/* Call vme_master_read to do the transfer */
+	retval = __copy_to_user(buf, image[minor].kern_buf,
+				(unsigned long)copied);
+	if (retval != 0) {
+		copied = (copied - retval);
+		pr_info("User copy failed\n");
 		return -EINVAL;
 	}
 
 	return copied;
 }
 
-/*
- * We are going to alloc a page during init per window for small transfers.
- * Small transfers will go user space -> buffer -> VME. Larger (more than a
- * page) transfers will lock the user space buffer into memory and then
- * transfer the data directly from the user space buffers out to VME.
- */
 static ssize_t resource_from_user(unsigned int minor, const char __user *buf,
 				  size_t count, loff_t *ppos)
 {
 	ssize_t retval;
 	ssize_t copied = 0;
 
-	if (count <= image[minor].size_buf) {
-		retval = __copy_from_user(image[minor].kern_buf, buf,
-			(unsigned long)count);
-		if (retval != 0)
-			copied = (copied - retval);
-		else
-			copied = count;
-
-		copied = vme_master_write(image[minor].resource,
-			image[minor].kern_buf, copied, *ppos);
-	} else {
-		/* XXX Need to write this */
-		pr_info("Currently don't support large transfers\n");
-		/* Map in pages from userspace */
-
-		/* Call vme_master_write to do the transfer */
-		return -EINVAL;
-	}
+	if (count > image[minor].size_buf)
+		count = image[minor].size_buf;
+
+	retval = __copy_from_user(image[minor].kern_buf, buf,
+				  (unsigned long)count);
+	if (retval != 0)
+		copied = (copied - retval);
+	else
+		copied = count;
+
+	copied = vme_master_write(image[minor].resource, image[minor].kern_buf,
+				  copied, *ppos);
 
 	return copied;
 }
-- 
1.8.3.1

[PATCH 6/9] staging: vme_user: return -EFAULT on __copy_*_user errors

[Dmitry Kalinkin]

Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
---
 drivers/staging/vme/devices/vme_user.c | 47 ++++++++--------------------------
 1 file changed, 11 insertions(+), 36 deletions(-)

diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
index 52cd638..85eb6ee 100644
--- a/drivers/staging/vme/devices/vme_user.c
+++ b/drivers/staging/vme/devices/vme_user.c
@@ -129,7 +129,6 @@ struct vme_user_vma_priv {
 static ssize_t resource_to_user(int minor, char __user *buf, size_t count,
 				loff_t *ppos)
 {
-	ssize_t retval;
 	ssize_t copied = 0;
 
 	if (count > image[minor].size_buf)
@@ -141,13 +140,8 @@ static ssize_t resource_to_user(int minor, char __user *buf, size_t count,
 	if (copied < 0)
 		return (int)copied;
 
-	retval = __copy_to_user(buf, image[minor].kern_buf,
-				(unsigned long)copied);
-	if (retval != 0) {
-		copied = (copied - retval);
-		pr_info("User copy failed\n");
-		return -EINVAL;
-	}
+	if (__copy_to_user(buf, image[minor].kern_buf, (unsigned long)copied))
+		return -EFAULT;
 
 	return copied;
 }
@@ -155,21 +149,16 @@ static ssize_t resource_to_user(int minor, char __user *buf, size_t count,
 static ssize_t resource_from_user(unsigned int minor, const char __user *buf,
 				  size_t count, loff_t *ppos)
 {
-	ssize_t retval;
 	ssize_t copied = 0;
 
 	if (count > image[minor].size_buf)
 		count = image[minor].size_buf;
 
-	retval = __copy_from_user(image[minor].kern_buf, buf,
-				  (unsigned long)count);
-	if (retval != 0)
-		copied = (copied - retval);
-	else
-		copied = count;
+	if (__copy_from_user(image[minor].kern_buf, buf, (unsigned long)count))
+		return -EFAULT;
 
 	copied = vme_master_write(image[minor].resource, image[minor].kern_buf,
-				  copied, *ppos);
+				  count, *ppos);
 
 	return copied;
 }
@@ -178,38 +167,24 @@ static ssize_t buffer_to_user(unsigned int minor, char __user *buf,
 			      size_t count, loff_t *ppos)
 {
 	void *image_ptr;
-	ssize_t retval;
 
 	image_ptr = image[minor].kern_buf + *ppos;
+	if (__copy_to_user(buf, image_ptr, (unsigned long)count))
+		return -EINVAL;
 
-	retval = __copy_to_user(buf, image_ptr, (unsigned long)count);
-	if (retval != 0) {
-		retval = (count - retval);
-		pr_warn("Partial copy to userspace\n");
-	} else
-		retval = count;
-
-	/* Return number of bytes successfully read */
-	return retval;
+	return count;
 }
 
 static ssize_t buffer_from_user(unsigned int minor, const char __user *buf,
 				size_t count, loff_t *ppos)
 {
 	void *image_ptr;
-	size_t retval;
 
 	image_ptr = image[minor].kern_buf + *ppos;
+	if (__copy_from_user(image_ptr, buf, (unsigned long)count))
+		return -EINVAL;
 
-	retval = __copy_from_user(image_ptr, buf, (unsigned long)count);
-	if (retval != 0) {
-		retval = (count - retval);
-		pr_warn("Partial copy to userspace\n");
-	} else
-		retval = count;
-
-	/* Return number of bytes successfully read */
-	return retval;
+	return count;
 }
 
 static ssize_t vme_user_read(struct file *file, char __user *buf, size_t count,
-- 
1.8.3.1

[PATCH 7/9] staging: vme_user: remove unused variable

[Dmitry Kalinkin]

Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
---
 drivers/staging/vme/devices/vme_user.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
index 85eb6ee..28a70f4 100644
--- a/drivers/staging/vme/devices/vme_user.c
+++ b/drivers/staging/vme/devices/vme_user.c
@@ -149,18 +149,14 @@ static ssize_t resource_to_user(int minor, char __user *buf, size_t count,
 static ssize_t resource_from_user(unsigned int minor, const char __user *buf,
 				  size_t count, loff_t *ppos)
 {
-	ssize_t copied = 0;
-
 	if (count > image[minor].size_buf)
 		count = image[minor].size_buf;
 
 	if (__copy_from_user(image[minor].kern_buf, buf, (unsigned long)count))
 		return -EFAULT;
 
-	copied = vme_master_write(image[minor].resource, image[minor].kern_buf,
-				  count, *ppos);
-
-	return copied;
+	return vme_master_write(image[minor].resource, image[minor].kern_buf,
+				count, *ppos);
 }
 
 static ssize_t buffer_to_user(unsigned int minor, char __user *buf,
-- 
1.8.3.1

[PATCH 8/9] staging: vme_user: remove distracting comment

[Dmitry Kalinkin]

Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
---
 drivers/staging/vme/devices/vme_user.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
index 28a70f4..6f5bbc4 100644
--- a/drivers/staging/vme/devices/vme_user.c
+++ b/drivers/staging/vme/devices/vme_user.c
@@ -134,7 +134,6 @@ static ssize_t resource_to_user(int minor, char __user *buf, size_t count,
 	if (count > image[minor].size_buf)
 		count = image[minor].size_buf;
 
-	/* We copy to kernel buffer */
 	copied = vme_master_read(image[minor].resource, image[minor].kern_buf,
 				 count, *ppos);
 	if (copied < 0)
-- 
1.8.3.1

[PATCH 9/9] staging: vme_user: remove okcount variable

[Dmitry Kalinkin]

Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
---
 drivers/staging/vme/devices/vme_user.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
index 6f5bbc4..14f9554 100644
--- a/drivers/staging/vme/devices/vme_user.c
+++ b/drivers/staging/vme/devices/vme_user.c
@@ -188,7 +188,6 @@ static ssize_t vme_user_read(struct file *file, char __user *buf, size_t count,
 	unsigned int minor = MINOR(file_inode(file)->i_rdev);
 	ssize_t retval;
 	size_t image_size;
-	size_t okcount;
 
 	if (minor == CONTROL_MINOR)
 		return 0;
@@ -206,16 +205,14 @@ static ssize_t vme_user_read(struct file *file, char __user *buf, size_t count,
 
 	/* Ensure not reading past end of the image */
 	if (*ppos + count > image_size)
-		okcount = image_size - *ppos;
-	else
-		okcount = count;
+		count = image_size - *ppos;
 
 	switch (type[minor]) {
 	case MASTER_MINOR:
-		retval = resource_to_user(minor, buf, okcount, ppos);
+		retval = resource_to_user(minor, buf, count, ppos);
 		break;
 	case SLAVE_MINOR:
-		retval = buffer_to_user(minor, buf, okcount, ppos);
+		retval = buffer_to_user(minor, buf, count, ppos);
 		break;
 	default:
 		retval = -EINVAL;
@@ -234,7 +231,6 @@ static ssize_t vme_user_write(struct file *file, const char __user *buf,
 	unsigned int minor = MINOR(file_inode(file)->i_rdev);
 	ssize_t retval;
 	size_t image_size;
-	size_t okcount;
 
 	if (minor == CONTROL_MINOR)
 		return 0;
@@ -251,16 +247,14 @@ static ssize_t vme_user_write(struct file *file, const char __user *buf,
 
 	/* Ensure not reading past end of the image */
 	if (*ppos + count > image_size)
-		okcount = image_size - *ppos;
-	else
-		okcount = count;
+		count = image_size - *ppos;
 
 	switch (type[minor]) {
 	case MASTER_MINOR:
-		retval = resource_from_user(minor, buf, okcount, ppos);
+		retval = resource_from_user(minor, buf, count, ppos);
 		break;
 	case SLAVE_MINOR:
-		retval = buffer_from_user(minor, buf, okcount, ppos);
+		retval = buffer_from_user(minor, buf, count, ppos);
 		break;
 	default:
 		retval = -EINVAL;
-- 
1.8.3.1

Re: [PATCH 1/9] staging: vme_user: fix code alignment

[Frans Klaver]

On Tue, Jun 23, 2015 at 2:42 PM, Dmitry Kalinkin
<dmitry.kalinkin@gmail.com> wrote:
> Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>

You left one in the function declarations (vme_user_write).

> ---
>  drivers/staging/vme/devices/vme_user.c | 33 +++++++++++++++++----------------
>  1 file changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
> index 5ff44fb..285e00e 100644
> --- a/drivers/staging/vme/devices/vme_user.c
> +++ b/drivers/staging/vme/devices/vme_user.c
> @@ -128,7 +128,7 @@ struct vme_user_vma_priv {
>   * transfer the data directly into the user space buffers.
>   */
>  static ssize_t resource_to_user(int minor, char __user *buf, size_t count,
> -       loff_t *ppos)
> +                               loff_t *ppos)
>  {
>         ssize_t retval;
>         ssize_t copied = 0;
> @@ -167,7 +167,7 @@ static ssize_t resource_to_user(int minor, char __user *buf, size_t count,
>   * transfer the data directly from the user space buffers out to VME.
>   */
>  static ssize_t resource_from_user(unsigned int minor, const char __user *buf,
> -       size_t count, loff_t *ppos)
> +                                 size_t count, loff_t *ppos)
>  {
>         ssize_t retval;
>         ssize_t copied = 0;
> @@ -195,7 +195,7 @@ static ssize_t resource_from_user(unsigned int minor, const char __user *buf,
>  }
>
>  static ssize_t buffer_to_user(unsigned int minor, char __user *buf,
> -       size_t count, loff_t *ppos)
> +                             size_t count, loff_t *ppos)
>  {
>         void *image_ptr;
>         ssize_t retval;
> @@ -214,7 +214,7 @@ static ssize_t buffer_to_user(unsigned int minor, char __user *buf,
>  }
>
>  static ssize_t buffer_from_user(unsigned int minor, const char __user *buf,
> -       size_t count, loff_t *ppos)
> +                               size_t count, loff_t *ppos)
>  {
>         void *image_ptr;
>         size_t retval;
> @@ -233,7 +233,7 @@ static ssize_t buffer_from_user(unsigned int minor, const char __user *buf,
>  }
>
>  static ssize_t vme_user_read(struct file *file, char __user *buf, size_t count,
> -                       loff_t *ppos)
> +                            loff_t *ppos)
>  {
>         unsigned int minor = MINOR(file_inode(file)->i_rdev);
>         ssize_t retval;
> @@ -279,7 +279,7 @@ static ssize_t vme_user_read(struct file *file, char __user *buf, size_t count,
>  }
>
>  static ssize_t vme_user_write(struct file *file, const char __user *buf,
> -                       size_t count, loff_t *ppos)
> +                             size_t count, loff_t *ppos)
>  {
>         unsigned int minor = MINOR(file_inode(file)->i_rdev);
>         ssize_t retval;
> @@ -354,7 +354,7 @@ static loff_t vme_user_llseek(struct file *file, loff_t off, int whence)
>   * already been defined.
>   */
>  static int vme_user_ioctl(struct inode *inode, struct file *file,
> -       unsigned int cmd, unsigned long arg)
> +                         unsigned int cmd, unsigned long arg)
>  {
>         struct vme_master master;
>         struct vme_slave slave;
> @@ -390,12 +390,13 @@ static int vme_user_ioctl(struct inode *inode, struct file *file,
>                          *      to userspace as they are
>                          */
>                         retval = vme_master_get(image[minor].resource,
> -                               &master.enable, &master.vme_addr,
> -                               &master.size, &master.aspace,
> -                               &master.cycle, &master.dwidth);
> +                                               &master.enable,
> +                                               &master.vme_addr,
> +                                               &master.size, &master.aspace,
> +                                               &master.cycle, &master.dwidth);
>
>                         copied = copy_to_user(argp, &master,
> -                               sizeof(struct vme_master));
> +                                             sizeof(struct vme_master));
>                         if (copied != 0) {
>                                 pr_warn("Partial copy to userspace\n");
>                                 return -EFAULT;
> @@ -435,12 +436,12 @@ static int vme_user_ioctl(struct inode *inode, struct file *file,
>                          *      to userspace as they are
>                          */
>                         retval = vme_slave_get(image[minor].resource,
> -                               &slave.enable, &slave.vme_addr,
> -                               &slave.size, &pci_addr, &slave.aspace,
> -                               &slave.cycle);
> +                                              &slave.enable, &slave.vme_addr,
> +                                              &slave.size, &pci_addr,
> +                                              &slave.aspace, &slave.cycle);
>
>                         copied = copy_to_user(argp, &slave,
> -                               sizeof(struct vme_slave));
> +                                             sizeof(struct vme_slave));
>                         if (copied != 0) {
>                                 pr_warn("Partial copy to userspace\n");
>                                 return -EFAULT;
> @@ -606,7 +607,7 @@ static int vme_user_probe(struct vme_dev *vdev)
>
>         /* Assign major and minor numbers for the driver */
>         err = register_chrdev_region(MKDEV(VME_MAJOR, 0), VME_DEVS,
> -               driver_name);
> +                                    driver_name);
>         if (err) {
>                 dev_warn(&vdev->dev, "Error getting Major Number %d for driver.\n",
>                          VME_MAJOR);
> --
> 1.8.3.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/9] staging: vme_user: fix code alignment

[Frans Klaver]

On Tue, Jun 23, 2015 at 3:44 PM, Dmitry Kalinkin
<dmitry.kalinkin@gmail.com> wrote:
>
>> On 23 Jun 2015, at 16:21, Frans Klaver <fransklaver@gmail.com> wrote:
>>
>> You left one in the function declarations (vme_user_write).
>
> If you mean forward declarations, they are already gone in Greg’s tree:
> https://git.kernel.org/cgit/linux/kernel/git/gregkh/staging.git/commit/drivers/staging/vme/devices/vme_user.c?h=staging-testing&id=e4aea6aa03267b496c21abefe170bb0d77192882

Yea, I meant those. Never mind then :)

Frans

Re: [PATCH 1/9] staging: vme_user: fix code alignment

[Dmitry Kalinkin]

> On 23 Jun 2015, at 16:21, Frans Klaver <fransklaver@gmail.com> wrote:
> 
> You left one in the function declarations (vme_user_write).

If you mean forward declarations, they are already gone in Greg’s tree:
https://git.kernel.org/cgit/linux/kernel/git/gregkh/staging.git/commit/drivers/staging/vme/devices/vme_user.c?h=staging-testing&id=e4aea6aa03267b496c21abefe170bb0d77192882

Re: [PATCH 6/9] staging: vme_user: return -EFAULT on __copy_*_user errors

[Dan Carpenter]

On Tue, Jun 23, 2015 at 03:42:30PM +0300, Dmitry Kalinkin wrote:
> @@ -178,38 +167,24 @@ static ssize_t buffer_to_user(unsigned int minor, char __user *buf,
>  			      size_t count, loff_t *ppos)
>  {
>  	void *image_ptr;
> -	ssize_t retval;
>  
>  	image_ptr = image[minor].kern_buf + *ppos;
> +	if (__copy_to_user(buf, image_ptr, (unsigned long)count))
> +		return -EINVAL;

s/EINVAL/EFAULT/

>  
> -	retval = __copy_to_user(buf, image_ptr, (unsigned long)count);
> -	if (retval != 0) {
> -		retval = (count - retval);
> -		pr_warn("Partial copy to userspace\n");
> -	} else
> -		retval = count;
> -
> -	/* Return number of bytes successfully read */
> -	return retval;
> +	return count;
>  }
>  
>  static ssize_t buffer_from_user(unsigned int minor, const char __user *buf,
>  				size_t count, loff_t *ppos)
>  {
>  	void *image_ptr;
> -	size_t retval;
>  
>  	image_ptr = image[minor].kern_buf + *ppos;
> +	if (__copy_from_user(image_ptr, buf, (unsigned long)count))
> +		return -EINVAL;

s/EINVAL/EFAULT/

>  
> -	retval = __copy_from_user(image_ptr, buf, (unsigned long)count);
> -	if (retval != 0) {
> -		retval = (count - retval);
> -		pr_warn("Partial copy to userspace\n");
> -	} else
> -		retval = count;
> -
> -	/* Return number of bytes successfully read */
> -	return retval;
> +	return count;
>  }

regards,
dan carpenter

Re: [PATCH 6/9] staging: vme_user: return -EFAULT on __copy_*_user errors

[Dmitry Kalinkin]

> On 23 Jun 2015, at 16:51, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> On Tue, Jun 23, 2015 at 03:42:30PM +0300, Dmitry Kalinkin wrote:
>> @@ -178,38 +167,24 @@ static ssize_t buffer_to_user(unsigned int minor, char __user *buf,
>> 			      size_t count, loff_t *ppos)
>> {
>> 	void *image_ptr;
>> -	ssize_t retval;
>> 
>> 	image_ptr = image[minor].kern_buf + *ppos;
>> +	if (__copy_to_user(buf, image_ptr, (unsigned long)count))
>> +		return -EINVAL;
> 
> s/EINVAL/EFAULT/

Right. Will fix in v2.

[PATCHv2 0/9] vme_user checkpatch fixes and read()/write() rework

[Dmitry Kalinkin]

First four patches are fixes for various checpatch warnings.  Next there is a
change to drop large read()/write() stub followed by a change to rework user
copy error codes.  Last three changes are refactorings.

v2 fixes  ("vme_user: return -EFAULT on __copy_*_user errors") that had EINVAL
instead of EFAULT in a couple of places.

Dmitry Kalinkin (9):
  staging: vme_user: fix code alignment
  staging: vme_user: fix blank lines
  staging: vme_user: fix NULL comparison style
  staging: vme_user: fix kmalloc style
  staging: vme_user: allow large read()/write()
  staging: vme_user: return -EFAULT on __copy_*_user errors
  staging: vme_user: remove unused variable
  staging: vme_user: remove distracting comment
  staging: vme_user: remove okcount variable

 drivers/staging/vme/devices/vme_user.c | 158 +++++++++++----------------------
 1 file changed, 51 insertions(+), 107 deletions(-)

-- 
1.8.3.1

[PATCHv2 1/9] staging: vme_user: fix code alignment

[Dmitry Kalinkin]

Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
---
 drivers/staging/vme/devices/vme_user.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
index 9cca97a..ccf9602 100644
--- a/drivers/staging/vme/devices/vme_user.c
+++ b/drivers/staging/vme/devices/vme_user.c
@@ -128,7 +128,7 @@ struct vme_user_vma_priv {
  * transfer the data directly into the user space buffers.
  */
 static ssize_t resource_to_user(int minor, char __user *buf, size_t count,
-	loff_t *ppos)
+				loff_t *ppos)
 {
 	ssize_t retval;
 	ssize_t copied = 0;
@@ -167,7 +167,7 @@ static ssize_t resource_to_user(int minor, char __user *buf, size_t count,
  * transfer the data directly from the user space buffers out to VME.
  */
 static ssize_t resource_from_user(unsigned int minor, const char __user *buf,
-	size_t count, loff_t *ppos)
+				  size_t count, loff_t *ppos)
 {
 	ssize_t retval;
 	ssize_t copied = 0;
@@ -195,7 +195,7 @@ static ssize_t resource_from_user(unsigned int minor, const char __user *buf,
 }
 
 static ssize_t buffer_to_user(unsigned int minor, char __user *buf,
-	size_t count, loff_t *ppos)
+			      size_t count, loff_t *ppos)
 {
 	void *image_ptr;
 	ssize_t retval;
@@ -214,7 +214,7 @@ static ssize_t buffer_to_user(unsigned int minor, char __user *buf,
 }
 
 static ssize_t buffer_from_user(unsigned int minor, const char __user *buf,
-	size_t count, loff_t *ppos)
+				size_t count, loff_t *ppos)
 {
 	void *image_ptr;
 	size_t retval;
@@ -233,7 +233,7 @@ static ssize_t buffer_from_user(unsigned int minor, const char __user *buf,
 }
 
 static ssize_t vme_user_read(struct file *file, char __user *buf, size_t count,
-			loff_t *ppos)
+			     loff_t *ppos)
 {
 	unsigned int minor = MINOR(file_inode(file)->i_rdev);
 	ssize_t retval;
@@ -279,7 +279,7 @@ static ssize_t vme_user_read(struct file *file, char __user *buf, size_t count,
 }
 
 static ssize_t vme_user_write(struct file *file, const char __user *buf,
-			size_t count, loff_t *ppos)
+			      size_t count, loff_t *ppos)
 {
 	unsigned int minor = MINOR(file_inode(file)->i_rdev);
 	ssize_t retval;
@@ -354,7 +354,7 @@ static loff_t vme_user_llseek(struct file *file, loff_t off, int whence)
  * already been defined.
  */
 static int vme_user_ioctl(struct inode *inode, struct file *file,
-	unsigned int cmd, unsigned long arg)
+			  unsigned int cmd, unsigned long arg)
 {
 	struct vme_master master;
 	struct vme_slave slave;
@@ -390,12 +390,13 @@ static int vme_user_ioctl(struct inode *inode, struct file *file,
 			 *	to userspace as they are
 			 */
 			retval = vme_master_get(image[minor].resource,
-				&master.enable, &master.vme_addr,
-				&master.size, &master.aspace,
-				&master.cycle, &master.dwidth);
+						&master.enable,
+						&master.vme_addr,
+						&master.size, &master.aspace,
+						&master.cycle, &master.dwidth);
 
 			copied = copy_to_user(argp, &master,
-				sizeof(struct vme_master));
+					      sizeof(struct vme_master));
 			if (copied != 0) {
 				pr_warn("Partial copy to userspace\n");
 				return -EFAULT;
@@ -435,12 +436,12 @@ static int vme_user_ioctl(struct inode *inode, struct file *file,
 			 *	to userspace as they are
 			 */
 			retval = vme_slave_get(image[minor].resource,
-				&slave.enable, &slave.vme_addr,
-				&slave.size, &pci_addr, &slave.aspace,
-				&slave.cycle);
+					       &slave.enable, &slave.vme_addr,
+					       &slave.size, &pci_addr,
+					       &slave.aspace, &slave.cycle);
 
 			copied = copy_to_user(argp, &slave,
-				sizeof(struct vme_slave));
+					      sizeof(struct vme_slave));
 			if (copied != 0) {
 				pr_warn("Partial copy to userspace\n");
 				return -EFAULT;
@@ -606,7 +607,7 @@ static int vme_user_probe(struct vme_dev *vdev)
 
 	/* Assign major and minor numbers for the driver */
 	err = register_chrdev_region(MKDEV(VME_MAJOR, 0), VME_DEVS,
-		driver_name);
+				     driver_name);
 	if (err) {
 		dev_warn(&vdev->dev, "Error getting Major Number %d for driver.\n",
 			 VME_MAJOR);
-- 
1.8.3.1

[PATCHv2 2/9] staging: vme_user: fix blank lines

[Dmitry Kalinkin]

Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
---
 drivers/staging/vme/devices/vme_user.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
index ccf9602..494655a 100644
--- a/drivers/staging/vme/devices/vme_user.c
+++ b/drivers/staging/vme/devices/vme_user.c
@@ -101,13 +101,13 @@ struct image_desc {
 	struct vme_resource *resource;	/* VME resource */
 	int mmap_count;		/* Number of current mmap's */
 };
+
 static struct image_desc image[VME_DEVS];
 
 static struct cdev *vme_user_cdev;		/* Character device */
 static struct class *vme_user_sysfs_class;	/* Sysfs class */
 static struct vme_dev *vme_user_bridge;		/* Pointer to user device */
 
-
 static const int type[VME_DEVS] = {	MASTER_MINOR,	MASTER_MINOR,
 					MASTER_MINOR,	MASTER_MINOR,
 					SLAVE_MINOR,	SLAVE_MINOR,
@@ -120,7 +120,6 @@ struct vme_user_vma_priv {
 	atomic_t refcnt;
 };
 
-
 /*
  * We are going ot alloc a page during init per window for small transfers.
  * Small transfers will go VME -> buffer -> user space. Larger (more than a
@@ -836,7 +835,6 @@ static void __exit vme_user_exit(void)
 	vme_unregister_driver(&vme_user_driver);
 }
 
-
 MODULE_PARM_DESC(bus, "Enumeration of VMEbus to which the driver is connected");
 module_param_array(bus, int, &bus_num, 0);
 
-- 
1.8.3.1

[PATCHv2 3/9] staging: vme_user: fix NULL comparison style

[Dmitry Kalinkin]

Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
---
 drivers/staging/vme/devices/vme_user.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
index 494655a..2ff15f0 100644
--- a/drivers/staging/vme/devices/vme_user.c
+++ b/drivers/staging/vme/devices/vme_user.c
@@ -527,7 +527,7 @@ static int vme_user_master_mmap(unsigned int minor, struct vm_area_struct *vma)
 	}
 
 	vma_priv = kmalloc(sizeof(struct vme_user_vma_priv), GFP_KERNEL);
-	if (vma_priv == NULL) {
+	if (!vma_priv) {
 		mutex_unlock(&image[minor].mutex);
 		return -ENOMEM;
 	}
@@ -588,7 +588,7 @@ static int vme_user_probe(struct vme_dev *vdev)
 	char *name;
 
 	/* Save pointer to the bridge device */
-	if (vme_user_bridge != NULL) {
+	if (vme_user_bridge) {
 		dev_err(&vdev->dev, "Driver can only be loaded for 1 device\n");
 		err = -EINVAL;
 		goto err_dev;
@@ -636,7 +636,7 @@ static int vme_user_probe(struct vme_dev *vdev)
 		 */
 		image[i].resource = vme_slave_request(vme_user_bridge,
 			VME_A24, VME_SCT);
-		if (image[i].resource == NULL) {
+		if (!image[i].resource) {
 			dev_warn(&vdev->dev,
 				 "Unable to allocate slave resource\n");
 			err = -ENOMEM;
@@ -645,7 +645,7 @@ static int vme_user_probe(struct vme_dev *vdev)
 		image[i].size_buf = PCI_BUF_SIZE;
 		image[i].kern_buf = vme_alloc_consistent(image[i].resource,
 			image[i].size_buf, &image[i].pci_buf);
-		if (image[i].kern_buf == NULL) {
+		if (!image[i].kern_buf) {
 			dev_warn(&vdev->dev,
 				 "Unable to allocate memory for buffer\n");
 			image[i].pci_buf = 0;
@@ -663,7 +663,7 @@ static int vme_user_probe(struct vme_dev *vdev)
 		/* XXX Need to properly request attributes */
 		image[i].resource = vme_master_request(vme_user_bridge,
 			VME_A32, VME_SCT, VME_D32);
-		if (image[i].resource == NULL) {
+		if (!image[i].resource) {
 			dev_warn(&vdev->dev,
 				 "Unable to allocate master resource\n");
 			err = -ENOMEM;
@@ -671,7 +671,7 @@ static int vme_user_probe(struct vme_dev *vdev)
 		}
 		image[i].size_buf = PCI_BUF_SIZE;
 		image[i].kern_buf = kmalloc(image[i].size_buf, GFP_KERNEL);
-		if (image[i].kern_buf == NULL) {
+		if (!image[i].kern_buf) {
 			err = -ENOMEM;
 			vme_master_free(image[i].resource);
 			goto err_master;
-- 
1.8.3.1

[PATCHv2 4/9] staging: vme_user: fix kmalloc style

[Dmitry Kalinkin]

Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
---
 drivers/staging/vme/devices/vme_user.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
index 2ff15f0..3467cde 100644
--- a/drivers/staging/vme/devices/vme_user.c
+++ b/drivers/staging/vme/devices/vme_user.c
@@ -526,7 +526,7 @@ static int vme_user_master_mmap(unsigned int minor, struct vm_area_struct *vma)
 		return err;
 	}
 
-	vma_priv = kmalloc(sizeof(struct vme_user_vma_priv), GFP_KERNEL);
+	vma_priv = kmalloc(sizeof(*vma_priv), GFP_KERNEL);
 	if (!vma_priv) {
 		mutex_unlock(&image[minor].mutex);
 		return -ENOMEM;
-- 
1.8.3.1

[PATCHv2 5/9] staging: vme_user: allow large read()/write()

[Dmitry Kalinkin]

This changes large master transfers to do shorter read/write rather than
return -EINVAL. User space will now be able to optimistically request a
large transfer and get at least some data.

This also removes comments suggesting on how to implement large
transfers. Current vme_master_* read and write implementations use CPU
copies that don't produce burst PCI accesses and subsequently no block
transfer on VME bus. In the end overall performance is quiet low and it
can't be fixed by doing direct copy to user space. Much easier solution
would be to just reuse kernel buffer.

Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
---
 drivers/staging/vme/devices/vme_user.c | 67 ++++++++++++----------------------
 1 file changed, 24 insertions(+), 43 deletions(-)

diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
index 3467cde..101f7b9 100644
--- a/drivers/staging/vme/devices/vme_user.c
+++ b/drivers/staging/vme/devices/vme_user.c
@@ -132,63 +132,44 @@ static ssize_t resource_to_user(int minor, char __user *buf, size_t count,
 	ssize_t retval;
 	ssize_t copied = 0;
 
-	if (count <= image[minor].size_buf) {
-		/* We copy to kernel buffer */
-		copied = vme_master_read(image[minor].resource,
-			image[minor].kern_buf, count, *ppos);
-		if (copied < 0)
-			return (int)copied;
-
-		retval = __copy_to_user(buf, image[minor].kern_buf,
-			(unsigned long)copied);
-		if (retval != 0) {
-			copied = (copied - retval);
-			pr_info("User copy failed\n");
-			return -EINVAL;
-		}
+	if (count > image[minor].size_buf)
+		count = image[minor].size_buf;
 
-	} else {
-		/* XXX Need to write this */
-		pr_info("Currently don't support large transfers\n");
-		/* Map in pages from userspace */
+	/* We copy to kernel buffer */
+	copied = vme_master_read(image[minor].resource, image[minor].kern_buf,
+				 count, *ppos);
+	if (copied < 0)
+		return (int)copied;
 
-		/* Call vme_master_read to do the transfer */
+	retval = __copy_to_user(buf, image[minor].kern_buf,
+				(unsigned long)copied);
+	if (retval != 0) {
+		copied = (copied - retval);
+		pr_info("User copy failed\n");
 		return -EINVAL;
 	}
 
 	return copied;
 }
 
-/*
- * We are going to alloc a page during init per window for small transfers.
- * Small transfers will go user space -> buffer -> VME. Larger (more than a
- * page) transfers will lock the user space buffer into memory and then
- * transfer the data directly from the user space buffers out to VME.
- */
 static ssize_t resource_from_user(unsigned int minor, const char __user *buf,
 				  size_t count, loff_t *ppos)
 {
 	ssize_t retval;
 	ssize_t copied = 0;
 
-	if (count <= image[minor].size_buf) {
-		retval = __copy_from_user(image[minor].kern_buf, buf,
-			(unsigned long)count);
-		if (retval != 0)
-			copied = (copied - retval);
-		else
-			copied = count;
-
-		copied = vme_master_write(image[minor].resource,
-			image[minor].kern_buf, copied, *ppos);
-	} else {
-		/* XXX Need to write this */
-		pr_info("Currently don't support large transfers\n");
-		/* Map in pages from userspace */
-
-		/* Call vme_master_write to do the transfer */
-		return -EINVAL;
-	}
+	if (count > image[minor].size_buf)
+		count = image[minor].size_buf;
+
+	retval = __copy_from_user(image[minor].kern_buf, buf,
+				  (unsigned long)count);
+	if (retval != 0)
+		copied = (copied - retval);
+	else
+		copied = count;
+
+	copied = vme_master_write(image[minor].resource, image[minor].kern_buf,
+				  copied, *ppos);
 
 	return copied;
 }
-- 
1.8.3.1

[PATCHv2 6/9] staging: vme_user: return -EFAULT on __copy_*_user errors

[Dmitry Kalinkin]

Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
---
 drivers/staging/vme/devices/vme_user.c | 47 ++++++++--------------------------
 1 file changed, 11 insertions(+), 36 deletions(-)

diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
index 101f7b9..070e63f 100644
--- a/drivers/staging/vme/devices/vme_user.c
+++ b/drivers/staging/vme/devices/vme_user.c
@@ -129,7 +129,6 @@ struct vme_user_vma_priv {
 static ssize_t resource_to_user(int minor, char __user *buf, size_t count,
 				loff_t *ppos)
 {
-	ssize_t retval;
 	ssize_t copied = 0;
 
 	if (count > image[minor].size_buf)
@@ -141,13 +140,8 @@ static ssize_t resource_to_user(int minor, char __user *buf, size_t count,
 	if (copied < 0)
 		return (int)copied;
 
-	retval = __copy_to_user(buf, image[minor].kern_buf,
-				(unsigned long)copied);
-	if (retval != 0) {
-		copied = (copied - retval);
-		pr_info("User copy failed\n");
-		return -EINVAL;
-	}
+	if (__copy_to_user(buf, image[minor].kern_buf, (unsigned long)copied))
+		return -EFAULT;
 
 	return copied;
 }
@@ -155,21 +149,16 @@ static ssize_t resource_to_user(int minor, char __user *buf, size_t count,
 static ssize_t resource_from_user(unsigned int minor, const char __user *buf,
 				  size_t count, loff_t *ppos)
 {
-	ssize_t retval;
 	ssize_t copied = 0;
 
 	if (count > image[minor].size_buf)
 		count = image[minor].size_buf;
 
-	retval = __copy_from_user(image[minor].kern_buf, buf,
-				  (unsigned long)count);
-	if (retval != 0)
-		copied = (copied - retval);
-	else
-		copied = count;
+	if (__copy_from_user(image[minor].kern_buf, buf, (unsigned long)count))
+		return -EFAULT;
 
 	copied = vme_master_write(image[minor].resource, image[minor].kern_buf,
-				  copied, *ppos);
+				  count, *ppos);
 
 	return copied;
 }
@@ -178,38 +167,24 @@ static ssize_t buffer_to_user(unsigned int minor, char __user *buf,
 			      size_t count, loff_t *ppos)
 {
 	void *image_ptr;
-	ssize_t retval;
 
 	image_ptr = image[minor].kern_buf + *ppos;
+	if (__copy_to_user(buf, image_ptr, (unsigned long)count))
+		return -EFAULT;
 
-	retval = __copy_to_user(buf, image_ptr, (unsigned long)count);
-	if (retval != 0) {
-		retval = (count - retval);
-		pr_warn("Partial copy to userspace\n");
-	} else
-		retval = count;
-
-	/* Return number of bytes successfully read */
-	return retval;
+	return count;
 }
 
 static ssize_t buffer_from_user(unsigned int minor, const char __user *buf,
 				size_t count, loff_t *ppos)
 {
 	void *image_ptr;
-	size_t retval;
 
 	image_ptr = image[minor].kern_buf + *ppos;
+	if (__copy_from_user(image_ptr, buf, (unsigned long)count))
+		return -EFAULT;
 
-	retval = __copy_from_user(image_ptr, buf, (unsigned long)count);
-	if (retval != 0) {
-		retval = (count - retval);
-		pr_warn("Partial copy to userspace\n");
-	} else
-		retval = count;
-
-	/* Return number of bytes successfully read */
-	return retval;
+	return count;
 }
 
 static ssize_t vme_user_read(struct file *file, char __user *buf, size_t count,
-- 
1.8.3.1

[PATCHv2 7/9] staging: vme_user: remove unused variable

[Dmitry Kalinkin]

Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
---
 drivers/staging/vme/devices/vme_user.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
index 070e63f..cf47649 100644
--- a/drivers/staging/vme/devices/vme_user.c
+++ b/drivers/staging/vme/devices/vme_user.c
@@ -149,18 +149,14 @@ static ssize_t resource_to_user(int minor, char __user *buf, size_t count,
 static ssize_t resource_from_user(unsigned int minor, const char __user *buf,
 				  size_t count, loff_t *ppos)
 {
-	ssize_t copied = 0;
-
 	if (count > image[minor].size_buf)
 		count = image[minor].size_buf;
 
 	if (__copy_from_user(image[minor].kern_buf, buf, (unsigned long)count))
 		return -EFAULT;
 
-	copied = vme_master_write(image[minor].resource, image[minor].kern_buf,
-				  count, *ppos);
-
-	return copied;
+	return vme_master_write(image[minor].resource, image[minor].kern_buf,
+				count, *ppos);
 }
 
 static ssize_t buffer_to_user(unsigned int minor, char __user *buf,
-- 
1.8.3.1

[PATCHv2 8/9] staging: vme_user: remove distracting comment

[Dmitry Kalinkin]

Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
---
 drivers/staging/vme/devices/vme_user.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
index cf47649..de9eda5 100644
--- a/drivers/staging/vme/devices/vme_user.c
+++ b/drivers/staging/vme/devices/vme_user.c
@@ -134,7 +134,6 @@ static ssize_t resource_to_user(int minor, char __user *buf, size_t count,
 	if (count > image[minor].size_buf)
 		count = image[minor].size_buf;
 
-	/* We copy to kernel buffer */
 	copied = vme_master_read(image[minor].resource, image[minor].kern_buf,
 				 count, *ppos);
 	if (copied < 0)
-- 
1.8.3.1

[PATCHv2 9/9] staging: vme_user: remove okcount variable

[Dmitry Kalinkin]

Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
---
 drivers/staging/vme/devices/vme_user.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
index de9eda5..efed9c7 100644
--- a/drivers/staging/vme/devices/vme_user.c
+++ b/drivers/staging/vme/devices/vme_user.c
@@ -188,7 +188,6 @@ static ssize_t vme_user_read(struct file *file, char __user *buf, size_t count,
 	unsigned int minor = MINOR(file_inode(file)->i_rdev);
 	ssize_t retval;
 	size_t image_size;
-	size_t okcount;
 
 	if (minor == CONTROL_MINOR)
 		return 0;
@@ -206,16 +205,14 @@ static ssize_t vme_user_read(struct file *file, char __user *buf, size_t count,
 
 	/* Ensure not reading past end of the image */
 	if (*ppos + count > image_size)
-		okcount = image_size - *ppos;
-	else
-		okcount = count;
+		count = image_size - *ppos;
 
 	switch (type[minor]) {
 	case MASTER_MINOR:
-		retval = resource_to_user(minor, buf, okcount, ppos);
+		retval = resource_to_user(minor, buf, count, ppos);
 		break;
 	case SLAVE_MINOR:
-		retval = buffer_to_user(minor, buf, okcount, ppos);
+		retval = buffer_to_user(minor, buf, count, ppos);
 		break;
 	default:
 		retval = -EINVAL;
@@ -234,7 +231,6 @@ static ssize_t vme_user_write(struct file *file, const char __user *buf,
 	unsigned int minor = MINOR(file_inode(file)->i_rdev);
 	ssize_t retval;
 	size_t image_size;
-	size_t okcount;
 
 	if (minor == CONTROL_MINOR)
 		return 0;
@@ -251,16 +247,14 @@ static ssize_t vme_user_write(struct file *file, const char __user *buf,
 
 	/* Ensure not reading past end of the image */
 	if (*ppos + count > image_size)
-		okcount = image_size - *ppos;
-	else
-		okcount = count;
+		count = image_size - *ppos;
 
 	switch (type[minor]) {
 	case MASTER_MINOR:
-		retval = resource_from_user(minor, buf, okcount, ppos);
+		retval = resource_from_user(minor, buf, count, ppos);
 		break;
 	case SLAVE_MINOR:
-		retval = buffer_from_user(minor, buf, okcount, ppos);
+		retval = buffer_from_user(minor, buf, count, ppos);
 		break;
 	default:
 		retval = -EINVAL;
-- 
1.8.3.1

Re: [PATCHv2 6/9] staging: vme_user: return -EFAULT on __copy_*_user errors

[Sudip Mukherjee]

On Tue, Jun 23, 2015 at 07:03:36PM +0300, Dmitry Kalinkin wrote:
> Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
> ---
>  drivers/staging/vme/devices/vme_user.c | 47 ++++++++--------------------------
>  1 file changed, 11 insertions(+), 36 deletions(-)
> 
<snip>
> @@ -178,38 +167,24 @@ static ssize_t buffer_to_user(unsigned int minor, char __user *buf,
>  			      size_t count, loff_t *ppos)
>  {
>  	void *image_ptr;
> -	ssize_t retval;
>  
>  	image_ptr = image[minor].kern_buf + *ppos;
> +	if (__copy_to_user(buf, image_ptr, (unsigned long)count))
> +		return -EFAULT;
>  
> -	retval = __copy_to_user(buf, image_ptr, (unsigned long)count);
> -	if (retval != 0) {
> -		retval = (count - retval);
> -		pr_warn("Partial copy to userspace\n");
> -	} else
> -		retval = count;
> -
> -	/* Return number of bytes successfully read */
> -	return retval;
> +	return count;
will it not affect the userspace code?
previously number of bytes successfully read was returned, now incase of
partial read -EFAULT is being returned.

regards
sudip

Re: [PATCHv2 6/9] staging: vme_user: return -EFAULT on __copy_*_user errors

[Dmitry Kalinkin]

> On 25 Jun 2015, at 14:27, Sudip Mukherjee <sudipm.mukherjee@gmail.com> wrote:
> 
> On Tue, Jun 23, 2015 at 07:03:36PM +0300, Dmitry Kalinkin wrote:
>> Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
>> ---
>> drivers/staging/vme/devices/vme_user.c | 47 ++++++++--------------------------
>> 1 file changed, 11 insertions(+), 36 deletions(-)
>> 
> <snip>
>> @@ -178,38 +167,24 @@ static ssize_t buffer_to_user(unsigned int minor, char __user *buf,
>> 			      size_t count, loff_t *ppos)
>> {
>> 	void *image_ptr;
>> -	ssize_t retval;
>> 
>> 	image_ptr = image[minor].kern_buf + *ppos;
>> +	if (__copy_to_user(buf, image_ptr, (unsigned long)count))
>> +		return -EFAULT;
>> 
>> -	retval = __copy_to_user(buf, image_ptr, (unsigned long)count);
>> -	if (retval != 0) {
>> -		retval = (count - retval);
>> -		pr_warn("Partial copy to userspace\n");
>> -	} else
>> -		retval = count;
>> -
>> -	/* Return number of bytes successfully read */
>> -	return retval;
>> +	return count;
> will it not affect the userspace code?
> previously number of bytes successfully read was returned, now incase of
> partial read -EFAULT is being returned.
Exactly.

Practically there is an access_ok() call in vfs_read() and vfs_write() that
will catch this first.  I don’t know exactly what is the condition for
__copy_to_user to fail, but it is probably some rare arch-specific thing (and
we only care for x86/powerpc here). But when it happens it better be returning
proper error codes. This is why I think this is not a “we broke userspace”
situation.

Cheers,
Dmitry

Re: [PATCHv2 6/9] staging: vme_user: return -EFAULT on __copy_*_user errors

[Dmitry Kalinkin]

> On 25 Jun 2015, at 15:05, Dmitry Kalinkin <dmitry.kalinkin@gmail.com> wrote:
> 
> 
>> On 25 Jun 2015, at 14:27, Sudip Mukherjee <sudipm.mukherjee@gmail.com> wrote:
>> 
>> On Tue, Jun 23, 2015 at 07:03:36PM +0300, Dmitry Kalinkin wrote:
>>> Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
>>> ---
>>> drivers/staging/vme/devices/vme_user.c | 47 ++++++++--------------------------
>>> 1 file changed, 11 insertions(+), 36 deletions(-)
>>> 
>> <snip>
>>> @@ -178,38 +167,24 @@ static ssize_t buffer_to_user(unsigned int minor, char __user *buf,
>>> 			      size_t count, loff_t *ppos)
>>> {
>>> 	void *image_ptr;
>>> -	ssize_t retval;
>>> 
>>> 	image_ptr = image[minor].kern_buf + *ppos;
>>> +	if (__copy_to_user(buf, image_ptr, (unsigned long)count))
>>> +		return -EFAULT;
>>> 
>>> -	retval = __copy_to_user(buf, image_ptr, (unsigned long)count);
>>> -	if (retval != 0) {
>>> -		retval = (count - retval);
>>> -		pr_warn("Partial copy to userspace\n");
>>> -	} else
>>> -		retval = count;
>>> -
>>> -	/* Return number of bytes successfully read */
>>> -	return retval;
>>> +	return count;
>> will it not affect the userspace code?
>> previously number of bytes successfully read was returned, now incase of
>> partial read -EFAULT is being returned.
> Exactly.
> 
> Practically there is an access_ok() call in vfs_read() and vfs_write() that
> will catch this first.  I don’t know exactly what is the condition for
> __copy_to_user to fail, but it is probably some rare arch-specific thing (and
> we only care for x86/powerpc here). But when it happens it better be returning
> proper error codes. This is why I think this is not a “we broke userspace”
> situation.

It seems like what I wrote above is not correct. access_ok does only coarse
checks and __copy_to_user does fail. Anyway, only a rare userspace application
would depend on “succesfull” read that was interrupted by a segfault. Also, if
__copy_to_user fails completely, the original code would return zero, which in
POSIX should mean something like “everything is good, try again later” and
this may cause infinite loops (e.g. python).

[PATCHv3 0/9] vme_user checkpatch fixes and read()/write() rework

[Dmitry Kalinkin]

First four patches are fixes for various checpatch warnings.  Next there is a
change to drop large read()/write() stub followed by a change to rework user
copy error codes.  Last three changes are refactorings.

v2 fixes  ("vme_user: return -EFAULT on __copy_*_user errors") that had EINVAL
instead of EFAULT in a couple of places.

v3 fixes ("allow large read()/write()") to also remove the comment right above
resource_to_user()
v3 also renames ("vme_user: return -EFAULT on __copy_*_user errors") to
("switch to returning -EFAULT on __copy_*_user errors")

Dmitry Kalinkin (9):
  staging: vme_user: fix code alignment
  staging: vme_user: fix blank lines
  staging: vme_user: fix NULL comparison style
  staging: vme_user: fix kmalloc style
  staging: vme_user: allow large read()/write()
  staging: vme_user: switch to returning -EFAULT on __copy_*_user errors
  staging: vme_user: remove unused variable
  staging: vme_user: remove distracting comment
  staging: vme_user: remove okcount variable

 drivers/staging/vme/devices/vme_user.c | 164 ++++++++++-----------------------
 1 file changed, 51 insertions(+), 113 deletions(-)

-- 
1.8.3.1

[PATCHv3 1/9] staging: vme_user: fix code alignment

[Dmitry Kalinkin]

Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
---
 drivers/staging/vme/devices/vme_user.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
index 9cca97a..ccf9602 100644
--- a/drivers/staging/vme/devices/vme_user.c
+++ b/drivers/staging/vme/devices/vme_user.c
@@ -128,7 +128,7 @@ struct vme_user_vma_priv {
  * transfer the data directly into the user space buffers.
  */
 static ssize_t resource_to_user(int minor, char __user *buf, size_t count,
-	loff_t *ppos)
+				loff_t *ppos)
 {
 	ssize_t retval;
 	ssize_t copied = 0;
@@ -167,7 +167,7 @@ static ssize_t resource_to_user(int minor, char __user *buf, size_t count,
  * transfer the data directly from the user space buffers out to VME.
  */
 static ssize_t resource_from_user(unsigned int minor, const char __user *buf,
-	size_t count, loff_t *ppos)
+				  size_t count, loff_t *ppos)
 {
 	ssize_t retval;
 	ssize_t copied = 0;
@@ -195,7 +195,7 @@ static ssize_t resource_from_user(unsigned int minor, const char __user *buf,
 }
 
 static ssize_t buffer_to_user(unsigned int minor, char __user *buf,
-	size_t count, loff_t *ppos)
+			      size_t count, loff_t *ppos)
 {
 	void *image_ptr;
 	ssize_t retval;
@@ -214,7 +214,7 @@ static ssize_t buffer_to_user(unsigned int minor, char __user *buf,
 }
 
 static ssize_t buffer_from_user(unsigned int minor, const char __user *buf,
-	size_t count, loff_t *ppos)
+				size_t count, loff_t *ppos)
 {
 	void *image_ptr;
 	size_t retval;
@@ -233,7 +233,7 @@ static ssize_t buffer_from_user(unsigned int minor, const char __user *buf,
 }
 
 static ssize_t vme_user_read(struct file *file, char __user *buf, size_t count,
-			loff_t *ppos)
+			     loff_t *ppos)
 {
 	unsigned int minor = MINOR(file_inode(file)->i_rdev);
 	ssize_t retval;
@@ -279,7 +279,7 @@ static ssize_t vme_user_read(struct file *file, char __user *buf, size_t count,
 }
 
 static ssize_t vme_user_write(struct file *file, const char __user *buf,
-			size_t count, loff_t *ppos)
+			      size_t count, loff_t *ppos)
 {
 	unsigned int minor = MINOR(file_inode(file)->i_rdev);
 	ssize_t retval;
@@ -354,7 +354,7 @@ static loff_t vme_user_llseek(struct file *file, loff_t off, int whence)
  * already been defined.
  */
 static int vme_user_ioctl(struct inode *inode, struct file *file,
-	unsigned int cmd, unsigned long arg)
+			  unsigned int cmd, unsigned long arg)
 {
 	struct vme_master master;
 	struct vme_slave slave;
@@ -390,12 +390,13 @@ static int vme_user_ioctl(struct inode *inode, struct file *file,
 			 *	to userspace as they are
 			 */
 			retval = vme_master_get(image[minor].resource,
-				&master.enable, &master.vme_addr,
-				&master.size, &master.aspace,
-				&master.cycle, &master.dwidth);
+						&master.enable,
+						&master.vme_addr,
+						&master.size, &master.aspace,
+						&master.cycle, &master.dwidth);
 
 			copied = copy_to_user(argp, &master,
-				sizeof(struct vme_master));
+					      sizeof(struct vme_master));
 			if (copied != 0) {
 				pr_warn("Partial copy to userspace\n");
 				return -EFAULT;
@@ -435,12 +436,12 @@ static int vme_user_ioctl(struct inode *inode, struct file *file,
 			 *	to userspace as they are
 			 */
 			retval = vme_slave_get(image[minor].resource,
-				&slave.enable, &slave.vme_addr,
-				&slave.size, &pci_addr, &slave.aspace,
-				&slave.cycle);
+					       &slave.enable, &slave.vme_addr,
+					       &slave.size, &pci_addr,
+					       &slave.aspace, &slave.cycle);
 
 			copied = copy_to_user(argp, &slave,
-				sizeof(struct vme_slave));
+					      sizeof(struct vme_slave));
 			if (copied != 0) {
 				pr_warn("Partial copy to userspace\n");
 				return -EFAULT;
@@ -606,7 +607,7 @@ static int vme_user_probe(struct vme_dev *vdev)
 
 	/* Assign major and minor numbers for the driver */
 	err = register_chrdev_region(MKDEV(VME_MAJOR, 0), VME_DEVS,
-		driver_name);
+				     driver_name);
 	if (err) {
 		dev_warn(&vdev->dev, "Error getting Major Number %d for driver.\n",
 			 VME_MAJOR);
-- 
1.8.3.1

[PATCHv3 2/9] staging: vme_user: fix blank lines

[Dmitry Kalinkin]

Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
---
 drivers/staging/vme/devices/vme_user.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
index ccf9602..494655a 100644
--- a/drivers/staging/vme/devices/vme_user.c
+++ b/drivers/staging/vme/devices/vme_user.c
@@ -101,13 +101,13 @@ struct image_desc {
 	struct vme_resource *resource;	/* VME resource */
 	int mmap_count;		/* Number of current mmap's */
 };
+
 static struct image_desc image[VME_DEVS];
 
 static struct cdev *vme_user_cdev;		/* Character device */
 static struct class *vme_user_sysfs_class;	/* Sysfs class */
 static struct vme_dev *vme_user_bridge;		/* Pointer to user device */
 
-
 static const int type[VME_DEVS] = {	MASTER_MINOR,	MASTER_MINOR,
 					MASTER_MINOR,	MASTER_MINOR,
 					SLAVE_MINOR,	SLAVE_MINOR,
@@ -120,7 +120,6 @@ struct vme_user_vma_priv {
 	atomic_t refcnt;
 };
 
-
 /*
  * We are going ot alloc a page during init per window for small transfers.
  * Small transfers will go VME -> buffer -> user space. Larger (more than a
@@ -836,7 +835,6 @@ static void __exit vme_user_exit(void)
 	vme_unregister_driver(&vme_user_driver);
 }
 
-
 MODULE_PARM_DESC(bus, "Enumeration of VMEbus to which the driver is connected");
 module_param_array(bus, int, &bus_num, 0);
 
-- 
1.8.3.1

[PATCHv3 3/9] staging: vme_user: fix NULL comparison style

[Dmitry Kalinkin]

Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
---
 drivers/staging/vme/devices/vme_user.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
index 494655a..2ff15f0 100644
--- a/drivers/staging/vme/devices/vme_user.c
+++ b/drivers/staging/vme/devices/vme_user.c
@@ -527,7 +527,7 @@ static int vme_user_master_mmap(unsigned int minor, struct vm_area_struct *vma)
 	}
 
 	vma_priv = kmalloc(sizeof(struct vme_user_vma_priv), GFP_KERNEL);
-	if (vma_priv == NULL) {
+	if (!vma_priv) {
 		mutex_unlock(&image[minor].mutex);
 		return -ENOMEM;
 	}
@@ -588,7 +588,7 @@ static int vme_user_probe(struct vme_dev *vdev)
 	char *name;
 
 	/* Save pointer to the bridge device */
-	if (vme_user_bridge != NULL) {
+	if (vme_user_bridge) {
 		dev_err(&vdev->dev, "Driver can only be loaded for 1 device\n");
 		err = -EINVAL;
 		goto err_dev;
@@ -636,7 +636,7 @@ static int vme_user_probe(struct vme_dev *vdev)
 		 */
 		image[i].resource = vme_slave_request(vme_user_bridge,
 			VME_A24, VME_SCT);
-		if (image[i].resource == NULL) {
+		if (!image[i].resource) {
 			dev_warn(&vdev->dev,
 				 "Unable to allocate slave resource\n");
 			err = -ENOMEM;
@@ -645,7 +645,7 @@ static int vme_user_probe(struct vme_dev *vdev)
 		image[i].size_buf = PCI_BUF_SIZE;
 		image[i].kern_buf = vme_alloc_consistent(image[i].resource,
 			image[i].size_buf, &image[i].pci_buf);
-		if (image[i].kern_buf == NULL) {
+		if (!image[i].kern_buf) {
 			dev_warn(&vdev->dev,
 				 "Unable to allocate memory for buffer\n");
 			image[i].pci_buf = 0;
@@ -663,7 +663,7 @@ static int vme_user_probe(struct vme_dev *vdev)
 		/* XXX Need to properly request attributes */
 		image[i].resource = vme_master_request(vme_user_bridge,
 			VME_A32, VME_SCT, VME_D32);
-		if (image[i].resource == NULL) {
+		if (!image[i].resource) {
 			dev_warn(&vdev->dev,
 				 "Unable to allocate master resource\n");
 			err = -ENOMEM;
@@ -671,7 +671,7 @@ static int vme_user_probe(struct vme_dev *vdev)
 		}
 		image[i].size_buf = PCI_BUF_SIZE;
 		image[i].kern_buf = kmalloc(image[i].size_buf, GFP_KERNEL);
-		if (image[i].kern_buf == NULL) {
+		if (!image[i].kern_buf) {
 			err = -ENOMEM;
 			vme_master_free(image[i].resource);
 			goto err_master;
-- 
1.8.3.1

[PATCHv3 4/9] staging: vme_user: fix kmalloc style

[Dmitry Kalinkin]

Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
---
 drivers/staging/vme/devices/vme_user.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
index 2ff15f0..3467cde 100644
--- a/drivers/staging/vme/devices/vme_user.c
+++ b/drivers/staging/vme/devices/vme_user.c
@@ -526,7 +526,7 @@ static int vme_user_master_mmap(unsigned int minor, struct vm_area_struct *vma)
 		return err;
 	}
 
-	vma_priv = kmalloc(sizeof(struct vme_user_vma_priv), GFP_KERNEL);
+	vma_priv = kmalloc(sizeof(*vma_priv), GFP_KERNEL);
 	if (!vma_priv) {
 		mutex_unlock(&image[minor].mutex);
 		return -ENOMEM;
-- 
1.8.3.1

[PATCHv3 5/9] staging: vme_user: allow large read()/write()

[Dmitry Kalinkin]

This changes large master transfers to do shorter read/write rather than
return -EINVAL. User space will now be able to optimistically request a
large transfer and get at least some data.

This also removes comments suggesting on how to implement large
transfers. Current vme_master_* read and write implementations use CPU
copies that don't produce burst PCI accesses and subsequently no block
transfer on VME bus. In the end overall performance is quiet low and it
can't be fixed by doing direct copy to user space. Much easier solution
would be to just reuse kernel buffer.

Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
---
 drivers/staging/vme/devices/vme_user.c | 73 +++++++++++-----------------------
 1 file changed, 24 insertions(+), 49 deletions(-)

diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
index 3467cde..a2345db 100644
--- a/drivers/staging/vme/devices/vme_user.c
+++ b/drivers/staging/vme/devices/vme_user.c
@@ -120,75 +120,50 @@ struct vme_user_vma_priv {
 	atomic_t refcnt;
 };
 
-/*
- * We are going ot alloc a page during init per window for small transfers.
- * Small transfers will go VME -> buffer -> user space. Larger (more than a
- * page) transfers will lock the user space buffer into memory and then
- * transfer the data directly into the user space buffers.
- */
 static ssize_t resource_to_user(int minor, char __user *buf, size_t count,
 				loff_t *ppos)
 {
 	ssize_t retval;
 	ssize_t copied = 0;
 
-	if (count <= image[minor].size_buf) {
-		/* We copy to kernel buffer */
-		copied = vme_master_read(image[minor].resource,
-			image[minor].kern_buf, count, *ppos);
-		if (copied < 0)
-			return (int)copied;
-
-		retval = __copy_to_user(buf, image[minor].kern_buf,
-			(unsigned long)copied);
-		if (retval != 0) {
-			copied = (copied - retval);
-			pr_info("User copy failed\n");
-			return -EINVAL;
-		}
+	if (count > image[minor].size_buf)
+		count = image[minor].size_buf;
 
-	} else {
-		/* XXX Need to write this */
-		pr_info("Currently don't support large transfers\n");
-		/* Map in pages from userspace */
+	/* We copy to kernel buffer */
+	copied = vme_master_read(image[minor].resource, image[minor].kern_buf,
+				 count, *ppos);
+	if (copied < 0)
+		return (int)copied;
 
-		/* Call vme_master_read to do the transfer */
+	retval = __copy_to_user(buf, image[minor].kern_buf,
+				(unsigned long)copied);
+	if (retval != 0) {
+		copied = (copied - retval);
+		pr_info("User copy failed\n");
 		return -EINVAL;
 	}
 
 	return copied;
 }
 
-/*
- * We are going to alloc a page during init per window for small transfers.
- * Small transfers will go user space -> buffer -> VME. Larger (more than a
- * page) transfers will lock the user space buffer into memory and then
- * transfer the data directly from the user space buffers out to VME.
- */
 static ssize_t resource_from_user(unsigned int minor, const char __user *buf,
 				  size_t count, loff_t *ppos)
 {
 	ssize_t retval;
 	ssize_t copied = 0;
 
-	if (count <= image[minor].size_buf) {
-		retval = __copy_from_user(image[minor].kern_buf, buf,
-			(unsigned long)count);
-		if (retval != 0)
-			copied = (copied - retval);
-		else
-			copied = count;
-
-		copied = vme_master_write(image[minor].resource,
-			image[minor].kern_buf, copied, *ppos);
-	} else {
-		/* XXX Need to write this */
-		pr_info("Currently don't support large transfers\n");
-		/* Map in pages from userspace */
-
-		/* Call vme_master_write to do the transfer */
-		return -EINVAL;
-	}
+	if (count > image[minor].size_buf)
+		count = image[minor].size_buf;
+
+	retval = __copy_from_user(image[minor].kern_buf, buf,
+				  (unsigned long)count);
+	if (retval != 0)
+		copied = (copied - retval);
+	else
+		copied = count;
+
+	copied = vme_master_write(image[minor].resource, image[minor].kern_buf,
+				  copied, *ppos);
 
 	return copied;
 }
-- 
1.8.3.1

[PATCHv3 6/9] staging: vme_user: switch to returning -EFAULT on __copy_*_user errors

[Dmitry Kalinkin]

Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
---
 drivers/staging/vme/devices/vme_user.c | 47 ++++++++--------------------------
 1 file changed, 11 insertions(+), 36 deletions(-)

diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
index a2345db..ef876a4 100644
--- a/drivers/staging/vme/devices/vme_user.c
+++ b/drivers/staging/vme/devices/vme_user.c
@@ -123,7 +123,6 @@ struct vme_user_vma_priv {
 static ssize_t resource_to_user(int minor, char __user *buf, size_t count,
 				loff_t *ppos)
 {
-	ssize_t retval;
 	ssize_t copied = 0;
 
 	if (count > image[minor].size_buf)
@@ -135,13 +134,8 @@ static ssize_t resource_to_user(int minor, char __user *buf, size_t count,
 	if (copied < 0)
 		return (int)copied;
 
-	retval = __copy_to_user(buf, image[minor].kern_buf,
-				(unsigned long)copied);
-	if (retval != 0) {
-		copied = (copied - retval);
-		pr_info("User copy failed\n");
-		return -EINVAL;
-	}
+	if (__copy_to_user(buf, image[minor].kern_buf, (unsigned long)copied))
+		return -EFAULT;
 
 	return copied;
 }
@@ -149,21 +143,16 @@ static ssize_t resource_to_user(int minor, char __user *buf, size_t count,
 static ssize_t resource_from_user(unsigned int minor, const char __user *buf,
 				  size_t count, loff_t *ppos)
 {
-	ssize_t retval;
 	ssize_t copied = 0;
 
 	if (count > image[minor].size_buf)
 		count = image[minor].size_buf;
 
-	retval = __copy_from_user(image[minor].kern_buf, buf,
-				  (unsigned long)count);
-	if (retval != 0)
-		copied = (copied - retval);
-	else
-		copied = count;
+	if (__copy_from_user(image[minor].kern_buf, buf, (unsigned long)count))
+		return -EFAULT;
 
 	copied = vme_master_write(image[minor].resource, image[minor].kern_buf,
-				  copied, *ppos);
+				  count, *ppos);
 
 	return copied;
 }
@@ -172,38 +161,24 @@ static ssize_t buffer_to_user(unsigned int minor, char __user *buf,
 			      size_t count, loff_t *ppos)
 {
 	void *image_ptr;
-	ssize_t retval;
 
 	image_ptr = image[minor].kern_buf + *ppos;
+	if (__copy_to_user(buf, image_ptr, (unsigned long)count))
+		return -EFAULT;
 
-	retval = __copy_to_user(buf, image_ptr, (unsigned long)count);
-	if (retval != 0) {
-		retval = (count - retval);
-		pr_warn("Partial copy to userspace\n");
-	} else
-		retval = count;
-
-	/* Return number of bytes successfully read */
-	return retval;
+	return count;
 }
 
 static ssize_t buffer_from_user(unsigned int minor, const char __user *buf,
 				size_t count, loff_t *ppos)
 {
 	void *image_ptr;
-	size_t retval;
 
 	image_ptr = image[minor].kern_buf + *ppos;
+	if (__copy_from_user(image_ptr, buf, (unsigned long)count))
+		return -EFAULT;
 
-	retval = __copy_from_user(image_ptr, buf, (unsigned long)count);
-	if (retval != 0) {
-		retval = (count - retval);
-		pr_warn("Partial copy to userspace\n");
-	} else
-		retval = count;
-
-	/* Return number of bytes successfully read */
-	return retval;
+	return count;
 }
 
 static ssize_t vme_user_read(struct file *file, char __user *buf, size_t count,
-- 
1.8.3.1

[PATCHv3 7/9] staging: vme_user: remove unused variable

[Dmitry Kalinkin]

Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
---
 drivers/staging/vme/devices/vme_user.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
index ef876a4..947a38e 100644
--- a/drivers/staging/vme/devices/vme_user.c
+++ b/drivers/staging/vme/devices/vme_user.c
@@ -143,18 +143,14 @@ static ssize_t resource_to_user(int minor, char __user *buf, size_t count,
 static ssize_t resource_from_user(unsigned int minor, const char __user *buf,
 				  size_t count, loff_t *ppos)
 {
-	ssize_t copied = 0;
-
 	if (count > image[minor].size_buf)
 		count = image[minor].size_buf;
 
 	if (__copy_from_user(image[minor].kern_buf, buf, (unsigned long)count))
 		return -EFAULT;
 
-	copied = vme_master_write(image[minor].resource, image[minor].kern_buf,
-				  count, *ppos);
-
-	return copied;
+	return vme_master_write(image[minor].resource, image[minor].kern_buf,
+				count, *ppos);
 }
 
 static ssize_t buffer_to_user(unsigned int minor, char __user *buf,
-- 
1.8.3.1

[PATCHv3 8/9] staging: vme_user: remove distracting comment

[Dmitry Kalinkin]

Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
---
 drivers/staging/vme/devices/vme_user.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
index 947a38e..7ca943c 100644
--- a/drivers/staging/vme/devices/vme_user.c
+++ b/drivers/staging/vme/devices/vme_user.c
@@ -128,7 +128,6 @@ static ssize_t resource_to_user(int minor, char __user *buf, size_t count,
 	if (count > image[minor].size_buf)
 		count = image[minor].size_buf;
 
-	/* We copy to kernel buffer */
 	copied = vme_master_read(image[minor].resource, image[minor].kern_buf,
 				 count, *ppos);
 	if (copied < 0)
-- 
1.8.3.1

[PATCHv3 9/9] staging: vme_user: remove okcount variable

[Dmitry Kalinkin]

Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
---
 drivers/staging/vme/devices/vme_user.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
index 7ca943c..b3e3c2d 100644
--- a/drivers/staging/vme/devices/vme_user.c
+++ b/drivers/staging/vme/devices/vme_user.c
@@ -182,7 +182,6 @@ static ssize_t vme_user_read(struct file *file, char __user *buf, size_t count,
 	unsigned int minor = MINOR(file_inode(file)->i_rdev);
 	ssize_t retval;
 	size_t image_size;
-	size_t okcount;
 
 	if (minor == CONTROL_MINOR)
 		return 0;
@@ -200,16 +199,14 @@ static ssize_t vme_user_read(struct file *file, char __user *buf, size_t count,
 
 	/* Ensure not reading past end of the image */
 	if (*ppos + count > image_size)
-		okcount = image_size - *ppos;
-	else
-		okcount = count;
+		count = image_size - *ppos;
 
 	switch (type[minor]) {
 	case MASTER_MINOR:
-		retval = resource_to_user(minor, buf, okcount, ppos);
+		retval = resource_to_user(minor, buf, count, ppos);
 		break;
 	case SLAVE_MINOR:
-		retval = buffer_to_user(minor, buf, okcount, ppos);
+		retval = buffer_to_user(minor, buf, count, ppos);
 		break;
 	default:
 		retval = -EINVAL;
@@ -228,7 +225,6 @@ static ssize_t vme_user_write(struct file *file, const char __user *buf,
 	unsigned int minor = MINOR(file_inode(file)->i_rdev);
 	ssize_t retval;
 	size_t image_size;
-	size_t okcount;
 
 	if (minor == CONTROL_MINOR)
 		return 0;
@@ -245,16 +241,14 @@ static ssize_t vme_user_write(struct file *file, const char __user *buf,
 
 	/* Ensure not reading past end of the image */
 	if (*ppos + count > image_size)
-		okcount = image_size - *ppos;
-	else
-		okcount = count;
+		count = image_size - *ppos;
 
 	switch (type[minor]) {
 	case MASTER_MINOR:
-		retval = resource_from_user(minor, buf, okcount, ppos);
+		retval = resource_from_user(minor, buf, count, ppos);
 		break;
 	case SLAVE_MINOR:
-		retval = buffer_from_user(minor, buf, okcount, ppos);
+		retval = buffer_from_user(minor, buf, count, ppos);
 		break;
 	default:
 		retval = -EINVAL;
-- 
1.8.3.1

Re: [PATCHv2 6/9] staging: vme_user: return -EFAULT on __copy_*_user errors

[Martyn Welch]

On 25/06/15 13:05, Dmitry Kalinkin wrote:
>
> This is why I think this is not a “we broke userspace” situation.

The vme_user module is also in the staging tree and (almost) by 
definition the API shouldn't be considered as stable.

Martyn

-- 
Martyn Welch (Lead Software Engineer)  | Registered in England and Wales
GE Intelligent Platforms               | (3828642) at 100 Barbirolli Square
T +44(0)1327322748                     | Manchester, M2 3AB
E martyn.welch@ge.com                  | VAT:GB 927559189

Re: [PATCHv3 6/9] staging: vme_user: switch to returning -EFAULT on __copy_*_user errors

[Martyn Welch]

On 26/06/15 21:39, Dmitry Kalinkin wrote:
> Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
> ---
>   drivers/staging/vme/devices/vme_user.c | 47 ++++++++--------------------------
>   1 file changed, 11 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/staging/vme/devices/vme_user.c b/drivers/staging/vme/devices/vme_user.c
> index a2345db..ef876a4 100644
> --- a/drivers/staging/vme/devices/vme_user.c
> +++ b/drivers/staging/vme/devices/vme_user.c
> @@ -123,7 +123,6 @@ struct vme_user_vma_priv {
>   static ssize_t resource_to_user(int minor, char __user *buf, size_t count,
>   				loff_t *ppos)
>   {
> -	ssize_t retval;
>   	ssize_t copied = 0;
>
>   	if (count > image[minor].size_buf)
> @@ -135,13 +134,8 @@ static ssize_t resource_to_user(int minor, char __user *buf, size_t count,
>   	if (copied < 0)
>   		return (int)copied;
>
> -	retval = __copy_to_user(buf, image[minor].kern_buf,
> -				(unsigned long)copied);
> -	if (retval != 0) {
> -		copied = (copied - retval);
> -		pr_info("User copy failed\n");
> -		return -EINVAL;
> -	}
> +	if (__copy_to_user(buf, image[minor].kern_buf, (unsigned long)copied))
> +		return -EFAULT;
>
>   	return copied;

Does __copy_to_user() not return the number of bytes still to be copied? 
The above seems to add the assumption that __copy_to_user()
can't return part way through a copy.

>   }
> @@ -149,21 +143,16 @@ static ssize_t resource_to_user(int minor, char __user *buf, size_t count,
>   static ssize_t resource_from_user(unsigned int minor, const char __user *buf,
>   				  size_t count, loff_t *ppos)
>   {
> -	ssize_t retval;
>   	ssize_t copied = 0;
>
>   	if (count > image[minor].size_buf)
>   		count = image[minor].size_buf;
>
> -	retval = __copy_from_user(image[minor].kern_buf, buf,
> -				  (unsigned long)count);
> -	if (retval != 0)
> -		copied = (copied - retval);
> -	else
> -		copied = count;
> +	if (__copy_from_user(image[minor].kern_buf, buf, (unsigned long)count))
> +		return -EFAULT;
>

Same here.

>   	copied = vme_master_write(image[minor].resource, image[minor].kern_buf,
> -				  copied, *ppos);
> +				  count, *ppos);
>
>   	return copied;
>   }
> @@ -172,38 +161,24 @@ static ssize_t buffer_to_user(unsigned int minor, char __user *buf,
>   			      size_t count, loff_t *ppos)
>   {
>   	void *image_ptr;
> -	ssize_t retval;
>
>   	image_ptr = image[minor].kern_buf + *ppos;
> +	if (__copy_to_user(buf, image_ptr, (unsigned long)count))
> +		return -EFAULT;
>

Ditto.

> -	retval = __copy_to_user(buf, image_ptr, (unsigned long)count);
> -	if (retval != 0) {
> -		retval = (count - retval);
> -		pr_warn("Partial copy to userspace\n");
> -	} else
> -		retval = count;
> -
> -	/* Return number of bytes successfully read */
> -	return retval;
> +	return count;
>   }
>
>   static ssize_t buffer_from_user(unsigned int minor, const char __user *buf,
>   				size_t count, loff_t *ppos)
>   {
>   	void *image_ptr;
> -	size_t retval;
>
>   	image_ptr = image[minor].kern_buf + *ppos;
> +	if (__copy_from_user(image_ptr, buf, (unsigned long)count))
> +		return -EFAULT;
>

And here.

> -	retval = __copy_from_user(image_ptr, buf, (unsigned long)count);
> -	if (retval != 0) {
> -		retval = (count - retval);
> -		pr_warn("Partial copy to userspace\n");
> -	} else
> -		retval = count;
> -
> -	/* Return number of bytes successfully read */
> -	return retval;
> +	return count;
>   }
>
>   static ssize_t vme_user_read(struct file *file, char __user *buf, size_t count,
>

-- 
Martyn Welch (Lead Software Engineer)  | Registered in England and Wales
GE Intelligent Platforms               | (3828642) at 100 Barbirolli Square
T +44(0)1327322748                     | Manchester, M2 3AB
E martyn.welch@ge.com                  | VAT:GB 927559189

Re: [PATCHv3 6/9] staging: vme_user: switch to returning -EFAULT on __copy_*_user errors

[Dmitry Kalinkin]

On Mon, Jul 6, 2015 at 3:51 PM, Martyn Welch <martyn.welch@ge.com> wrote:
> On 26/06/15 21:39, Dmitry Kalinkin wrote:
>>
>> Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
>> ---
>>   drivers/staging/vme/devices/vme_user.c | 47
>> ++++++++--------------------------
>>   1 file changed, 11 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/staging/vme/devices/vme_user.c
>> b/drivers/staging/vme/devices/vme_user.c
>> index a2345db..ef876a4 100644
>> --- a/drivers/staging/vme/devices/vme_user.c
>> +++ b/drivers/staging/vme/devices/vme_user.c
>> @@ -123,7 +123,6 @@ struct vme_user_vma_priv {
>>   static ssize_t resource_to_user(int minor, char __user *buf, size_t
>> count,
>>                                 loff_t *ppos)
>>   {
>> -       ssize_t retval;
>>         ssize_t copied = 0;
>>
>>         if (count > image[minor].size_buf)
>> @@ -135,13 +134,8 @@ static ssize_t resource_to_user(int minor, char
>> __user *buf, size_t count,
>>         if (copied < 0)
>>                 return (int)copied;
>>
>> -       retval = __copy_to_user(buf, image[minor].kern_buf,
>> -                               (unsigned long)copied);
>> -       if (retval != 0) {
>> -               copied = (copied - retval);
>> -               pr_info("User copy failed\n");
>> -               return -EINVAL;
>> -       }
>> +       if (__copy_to_user(buf, image[minor].kern_buf, (unsigned
>> long)copied))
>> +               return -EFAULT;
>>
>>         return copied;
>
>
> Does __copy_to_user() not return the number of bytes still to be copied? The
> above seems to add the assumption that __copy_to_user()
> can't return part way through a copy.
Yes it does. And this information is useless in userspace.
Returning -EFAULT would be more informative in this case.
I couldn't find an example of kernel code doing this any other way:
http://lxr.free-electrons.com/ident?i=__copy_to_user
>
>>   }
>> @@ -149,21 +143,16 @@ static ssize_t resource_to_user(int minor, char
>> __user *buf, size_t count,
>>   static ssize_t resource_from_user(unsigned int minor, const char __user
>> *buf,
>>                                   size_t count, loff_t *ppos)
>>   {
>> -       ssize_t retval;
>>         ssize_t copied = 0;
>>
>>         if (count > image[minor].size_buf)
>>                 count = image[minor].size_buf;
>>
>> -       retval = __copy_from_user(image[minor].kern_buf, buf,
>> -                                 (unsigned long)count);
>> -       if (retval != 0)
>> -               copied = (copied - retval);
>> -       else
>> -               copied = count;
>> +       if (__copy_from_user(image[minor].kern_buf, buf, (unsigned
>> long)count))
>> +               return -EFAULT;
>>
>
> Same here.
>
>>         copied = vme_master_write(image[minor].resource,
>> image[minor].kern_buf,
>> -                                 copied, *ppos);
>> +                                 count, *ppos);
>>
>>         return copied;
>>   }
>> @@ -172,38 +161,24 @@ static ssize_t buffer_to_user(unsigned int minor,
>> char __user *buf,
>>                               size_t count, loff_t *ppos)
>>   {
>>         void *image_ptr;
>> -       ssize_t retval;
>>
>>         image_ptr = image[minor].kern_buf + *ppos;
>> +       if (__copy_to_user(buf, image_ptr, (unsigned long)count))
>> +               return -EFAULT;
>>
>
> Ditto.
>
>> -       retval = __copy_to_user(buf, image_ptr, (unsigned long)count);
>> -       if (retval != 0) {
>> -               retval = (count - retval);
>> -               pr_warn("Partial copy to userspace\n");
>> -       } else
>> -               retval = count;
>> -
>> -       /* Return number of bytes successfully read */
>> -       return retval;
>> +       return count;
>>   }
>>
>>   static ssize_t buffer_from_user(unsigned int minor, const char __user
>> *buf,
>>                                 size_t count, loff_t *ppos)
>>   {
>>         void *image_ptr;
>> -       size_t retval;
>>
>>         image_ptr = image[minor].kern_buf + *ppos;
>> +       if (__copy_from_user(image_ptr, buf, (unsigned long)count))
>> +               return -EFAULT;
>>
>
> And here.
>
>> -       retval = __copy_from_user(image_ptr, buf, (unsigned long)count);
>> -       if (retval != 0) {
>> -               retval = (count - retval);
>> -               pr_warn("Partial copy to userspace\n");
>> -       } else
>> -               retval = count;
>> -
>> -       /* Return number of bytes successfully read */
>> -       return retval;
>> +       return count;
>>   }
>>
>>   static ssize_t vme_user_read(struct file *file, char __user *buf, size_t
>> count,
>>
>
> --
> Martyn Welch (Lead Software Engineer)  | Registered in England and Wales
> GE Intelligent Platforms               | (3828642) at 100 Barbirolli Square
> T +44(0)1327322748                     | Manchester, M2 3AB
> E martyn.welch@ge.com                  | VAT:GB 927559189

Re: [PATCHv3 6/9] staging: vme_user: switch to returning -EFAULT on __copy_*_user errors

[Martyn Welch]

On 06/07/15 14:10, Dmitry Kalinkin wrote:
> On Mon, Jul 6, 2015 at 3:51 PM, Martyn Welch <martyn.welch@ge.com> wrote:
>> On 26/06/15 21:39, Dmitry Kalinkin wrote:
>>>
>>> Signed-off-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
>>> ---
>>>    drivers/staging/vme/devices/vme_user.c | 47
>>> ++++++++--------------------------
>>>    1 file changed, 11 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/drivers/staging/vme/devices/vme_user.c
>>> b/drivers/staging/vme/devices/vme_user.c
>>> index a2345db..ef876a4 100644
>>> --- a/drivers/staging/vme/devices/vme_user.c
>>> +++ b/drivers/staging/vme/devices/vme_user.c
>>> @@ -123,7 +123,6 @@ struct vme_user_vma_priv {
>>>    static ssize_t resource_to_user(int minor, char __user *buf, size_t
>>> count,
>>>                                  loff_t *ppos)
>>>    {
>>> -       ssize_t retval;
>>>          ssize_t copied = 0;
>>>
>>>          if (count > image[minor].size_buf)
>>> @@ -135,13 +134,8 @@ static ssize_t resource_to_user(int minor, char
>>> __user *buf, size_t count,
>>>          if (copied < 0)
>>>                  return (int)copied;
>>>
>>> -       retval = __copy_to_user(buf, image[minor].kern_buf,
>>> -                               (unsigned long)copied);
>>> -       if (retval != 0) {
>>> -               copied = (copied - retval);
>>> -               pr_info("User copy failed\n");
>>> -               return -EINVAL;
>>> -       }
>>> +       if (__copy_to_user(buf, image[minor].kern_buf, (unsigned
>>> long)copied))
>>> +               return -EFAULT;
>>>
>>>          return copied;
>>
>>
>> Does __copy_to_user() not return the number of bytes still to be copied? The
>> above seems to add the assumption that __copy_to_user()
>> can't return part way through a copy.
> Yes it does. And this information is useless in userspace.
> Returning -EFAULT would be more informative in this case.
> I couldn't find an example of kernel code doing this any other way:
> https://urldefense.proofpoint.com/v2/url?u=http-3A__lxr.free-2Delectrons.com_ident-3Fi-3D-5F-5Fcopy-5Fto-5Fuser&d=AwIBaQ&c=IV_clAzoPDE253xZdHuilRgztyh_RiV3wUrLrDQYWSI&r=o_jX-O9t9rDa5QmNKEzuslLxr8l2x1M2rHid0HdzmY4&m=YqOunx1QvC1jz8WjtHd_feWk5cdNWIVW0utSWesR8ag&s=BdRq0di7Bryi1LqA3LtL_Z9UeSHdpiYwghMElfWbR1A&e=

Fair enough. If that's the approach taken elsewhere, then I'm happy for 
this to become consistent.

>>
>>>    }
>>> @@ -149,21 +143,16 @@ static ssize_t resource_to_user(int minor, char
>>> __user *buf, size_t count,
>>>    static ssize_t resource_from_user(unsigned int minor, const char __user
>>> *buf,
>>>                                    size_t count, loff_t *ppos)
>>>    {
>>> -       ssize_t retval;
>>>          ssize_t copied = 0;
>>>
>>>          if (count > image[minor].size_buf)
>>>                  count = image[minor].size_buf;
>>>
>>> -       retval = __copy_from_user(image[minor].kern_buf, buf,
>>> -                                 (unsigned long)count);
>>> -       if (retval != 0)
>>> -               copied = (copied - retval);
>>> -       else
>>> -               copied = count;
>>> +       if (__copy_from_user(image[minor].kern_buf, buf, (unsigned
>>> long)count))
>>> +               return -EFAULT;
>>>
>>
>> Same here.
>>
>>>          copied = vme_master_write(image[minor].resource,
>>> image[minor].kern_buf,
>>> -                                 copied, *ppos);
>>> +                                 count, *ppos);
>>>
>>>          return copied;
>>>    }
>>> @@ -172,38 +161,24 @@ static ssize_t buffer_to_user(unsigned int minor,
>>> char __user *buf,
>>>                                size_t count, loff_t *ppos)
>>>    {
>>>          void *image_ptr;
>>> -       ssize_t retval;
>>>
>>>          image_ptr = image[minor].kern_buf + *ppos;
>>> +       if (__copy_to_user(buf, image_ptr, (unsigned long)count))
>>> +               return -EFAULT;
>>>
>>
>> Ditto.
>>
>>> -       retval = __copy_to_user(buf, image_ptr, (unsigned long)count);
>>> -       if (retval != 0) {
>>> -               retval = (count - retval);
>>> -               pr_warn("Partial copy to userspace\n");
>>> -       } else
>>> -               retval = count;
>>> -
>>> -       /* Return number of bytes successfully read */
>>> -       return retval;
>>> +       return count;
>>>    }
>>>
>>>    static ssize_t buffer_from_user(unsigned int minor, const char __user
>>> *buf,
>>>                                  size_t count, loff_t *ppos)
>>>    {
>>>          void *image_ptr;
>>> -       size_t retval;
>>>
>>>          image_ptr = image[minor].kern_buf + *ppos;
>>> +       if (__copy_from_user(image_ptr, buf, (unsigned long)count))
>>> +               return -EFAULT;
>>>
>>
>> And here.
>>
>>> -       retval = __copy_from_user(image_ptr, buf, (unsigned long)count);
>>> -       if (retval != 0) {
>>> -               retval = (count - retval);
>>> -               pr_warn("Partial copy to userspace\n");
>>> -       } else
>>> -               retval = count;
>>> -
>>> -       /* Return number of bytes successfully read */
>>> -       return retval;
>>> +       return count;
>>>    }
>>>
>>>    static ssize_t vme_user_read(struct file *file, char __user *buf, size_t
>>> count,
>>>
>>
>> --
>> Martyn Welch (Lead Software Engineer)  | Registered in England and Wales
>> GE Intelligent Platforms               | (3828642) at 100 Barbirolli Square
>> T +44(0)1327322748                     | Manchester, M2 3AB
>> E martyn.welch@ge.com                  | VAT:GB 927559189

-- 
Martyn Welch (Lead Software Engineer)  | Registered in England and Wales
GE Intelligent Platforms               | (3828642) at 100 Barbirolli Square
T +44(0)1327322748                     | Manchester, M2 3AB
E martyn.welch@ge.com                  | VAT:GB 927559189