* [PATCHv2 1/9] staging: vme_user: fix code alignment
2015-06-23 16:03 ` [PATCHv2 0/9] vme_user checkpatch fixes and read()/write() rework Dmitry Kalinkin
@ 2015-06-23 16:03 ` Dmitry Kalinkin
2015-06-23 16:03 ` [PATCHv2 2/9] staging: vme_user: fix blank lines Dmitry Kalinkin
` (8 subsequent siblings)
9 siblings, 0 replies; 42+ messages in thread
From: Dmitry Kalinkin @ 2015-06-23 16:03 UTC (permalink / raw)
To: linux-kernel, devel
Cc: Martyn Welch, Manohar Vanga, Greg Kroah-Hartman, 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
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCHv2 2/9] staging: vme_user: fix blank lines
2015-06-23 16:03 ` [PATCHv2 0/9] vme_user checkpatch fixes and read()/write() rework Dmitry Kalinkin
2015-06-23 16:03 ` [PATCHv2 1/9] staging: vme_user: fix code alignment Dmitry Kalinkin
@ 2015-06-23 16:03 ` Dmitry Kalinkin
2015-06-23 16:03 ` [PATCHv2 3/9] staging: vme_user: fix NULL comparison style Dmitry Kalinkin
` (7 subsequent siblings)
9 siblings, 0 replies; 42+ messages in thread
From: Dmitry Kalinkin @ 2015-06-23 16:03 UTC (permalink / raw)
To: linux-kernel, devel
Cc: Martyn Welch, Manohar Vanga, Greg Kroah-Hartman, 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
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCHv2 3/9] staging: vme_user: fix NULL comparison style
2015-06-23 16:03 ` [PATCHv2 0/9] vme_user checkpatch fixes and read()/write() rework Dmitry Kalinkin
2015-06-23 16:03 ` [PATCHv2 1/9] staging: vme_user: fix code alignment Dmitry Kalinkin
2015-06-23 16:03 ` [PATCHv2 2/9] staging: vme_user: fix blank lines Dmitry Kalinkin
@ 2015-06-23 16:03 ` Dmitry Kalinkin
2015-06-23 16:03 ` [PATCHv2 4/9] staging: vme_user: fix kmalloc style Dmitry Kalinkin
` (6 subsequent siblings)
9 siblings, 0 replies; 42+ messages in thread
From: Dmitry Kalinkin @ 2015-06-23 16:03 UTC (permalink / raw)
To: linux-kernel, devel
Cc: Martyn Welch, Manohar Vanga, Greg Kroah-Hartman, 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
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCHv2 4/9] staging: vme_user: fix kmalloc style
2015-06-23 16:03 ` [PATCHv2 0/9] vme_user checkpatch fixes and read()/write() rework Dmitry Kalinkin
` (2 preceding siblings ...)
2015-06-23 16:03 ` [PATCHv2 3/9] staging: vme_user: fix NULL comparison style Dmitry Kalinkin
@ 2015-06-23 16:03 ` Dmitry Kalinkin
2015-06-23 16:03 ` [PATCHv2 5/9] staging: vme_user: allow large read()/write() Dmitry Kalinkin
` (5 subsequent siblings)
9 siblings, 0 replies; 42+ messages in thread
From: Dmitry Kalinkin @ 2015-06-23 16:03 UTC (permalink / raw)
To: linux-kernel, devel
Cc: Martyn Welch, Manohar Vanga, Greg Kroah-Hartman, 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
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCHv2 5/9] staging: vme_user: allow large read()/write()
2015-06-23 16:03 ` [PATCHv2 0/9] vme_user checkpatch fixes and read()/write() rework Dmitry Kalinkin
` (3 preceding siblings ...)
2015-06-23 16:03 ` [PATCHv2 4/9] staging: vme_user: fix kmalloc style Dmitry Kalinkin
@ 2015-06-23 16:03 ` Dmitry Kalinkin
2015-06-23 16:03 ` [PATCHv2 6/9] staging: vme_user: return -EFAULT on __copy_*_user errors Dmitry Kalinkin
` (4 subsequent siblings)
9 siblings, 0 replies; 42+ messages in thread
From: Dmitry Kalinkin @ 2015-06-23 16:03 UTC (permalink / raw)
To: linux-kernel, devel
Cc: Martyn Welch, Manohar Vanga, Greg Kroah-Hartman, 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
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCHv2 6/9] staging: vme_user: return -EFAULT on __copy_*_user errors
2015-06-23 16:03 ` [PATCHv2 0/9] vme_user checkpatch fixes and read()/write() rework Dmitry Kalinkin
` (4 preceding siblings ...)
2015-06-23 16:03 ` [PATCHv2 5/9] staging: vme_user: allow large read()/write() Dmitry Kalinkin
@ 2015-06-23 16:03 ` Dmitry Kalinkin
2015-06-25 11:27 ` Sudip Mukherjee
2015-06-23 16:03 ` [PATCHv2 7/9] staging: vme_user: remove unused variable Dmitry Kalinkin
` (3 subsequent siblings)
9 siblings, 1 reply; 42+ messages in thread
From: Dmitry Kalinkin @ 2015-06-23 16:03 UTC (permalink / raw)
To: linux-kernel, devel
Cc: Martyn Welch, Manohar Vanga, Greg Kroah-Hartman, 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
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCHv2 6/9] staging: vme_user: return -EFAULT on __copy_*_user errors
2015-06-23 16:03 ` [PATCHv2 6/9] staging: vme_user: return -EFAULT on __copy_*_user errors Dmitry Kalinkin
@ 2015-06-25 11:27 ` Sudip Mukherjee
2015-06-25 12:05 ` Dmitry Kalinkin
0 siblings, 1 reply; 42+ messages in thread
From: Sudip Mukherjee @ 2015-06-25 11:27 UTC (permalink / raw)
To: Dmitry Kalinkin
Cc: linux-kernel, devel, Martyn Welch, Greg Kroah-Hartman, Manohar Vanga
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
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCHv2 6/9] staging: vme_user: return -EFAULT on __copy_*_user errors
2015-06-25 11:27 ` Sudip Mukherjee
@ 2015-06-25 12:05 ` Dmitry Kalinkin
2015-06-25 17:05 ` Dmitry Kalinkin
2015-07-06 12:42 ` Martyn Welch
0 siblings, 2 replies; 42+ messages in thread
From: Dmitry Kalinkin @ 2015-06-25 12:05 UTC (permalink / raw)
To: Sudip Mukherjee
Cc: linux-kernel, devel, Martyn Welch, Greg Kroah-Hartman, Manohar Vanga
> 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
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCHv2 6/9] staging: vme_user: return -EFAULT on __copy_*_user errors
2015-06-25 12:05 ` Dmitry Kalinkin
@ 2015-06-25 17:05 ` Dmitry Kalinkin
2015-07-06 12:42 ` Martyn Welch
1 sibling, 0 replies; 42+ messages in thread
From: Dmitry Kalinkin @ 2015-06-25 17:05 UTC (permalink / raw)
To: Sudip Mukherjee
Cc: linux-kernel, devel, Martyn Welch, Greg Kroah-Hartman, Manohar Vanga
> 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).
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCHv2 6/9] staging: vme_user: return -EFAULT on __copy_*_user errors
2015-06-25 12:05 ` Dmitry Kalinkin
2015-06-25 17:05 ` Dmitry Kalinkin
@ 2015-07-06 12:42 ` Martyn Welch
1 sibling, 0 replies; 42+ messages in thread
From: Martyn Welch @ 2015-07-06 12:42 UTC (permalink / raw)
To: Dmitry Kalinkin, Sudip Mukherjee
Cc: linux-kernel, devel, Greg Kroah-Hartman, Manohar Vanga
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
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCHv2 7/9] staging: vme_user: remove unused variable
2015-06-23 16:03 ` [PATCHv2 0/9] vme_user checkpatch fixes and read()/write() rework Dmitry Kalinkin
` (5 preceding siblings ...)
2015-06-23 16:03 ` [PATCHv2 6/9] staging: vme_user: return -EFAULT on __copy_*_user errors Dmitry Kalinkin
@ 2015-06-23 16:03 ` Dmitry Kalinkin
2015-06-23 16:03 ` [PATCHv2 8/9] staging: vme_user: remove distracting comment Dmitry Kalinkin
` (2 subsequent siblings)
9 siblings, 0 replies; 42+ messages in thread
From: Dmitry Kalinkin @ 2015-06-23 16:03 UTC (permalink / raw)
To: linux-kernel, devel
Cc: Martyn Welch, Manohar Vanga, Greg Kroah-Hartman, 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
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCHv2 8/9] staging: vme_user: remove distracting comment
2015-06-23 16:03 ` [PATCHv2 0/9] vme_user checkpatch fixes and read()/write() rework Dmitry Kalinkin
` (6 preceding siblings ...)
2015-06-23 16:03 ` [PATCHv2 7/9] staging: vme_user: remove unused variable Dmitry Kalinkin
@ 2015-06-23 16:03 ` Dmitry Kalinkin
2015-06-23 16:03 ` [PATCHv2 9/9] staging: vme_user: remove okcount variable Dmitry Kalinkin
2015-06-26 20:39 ` [PATCHv3 0/9] vme_user checkpatch fixes and read()/write() rework Dmitry Kalinkin
9 siblings, 0 replies; 42+ messages in thread
From: Dmitry Kalinkin @ 2015-06-23 16:03 UTC (permalink / raw)
To: linux-kernel, devel
Cc: Martyn Welch, Manohar Vanga, Greg Kroah-Hartman, 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
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCHv2 9/9] staging: vme_user: remove okcount variable
2015-06-23 16:03 ` [PATCHv2 0/9] vme_user checkpatch fixes and read()/write() rework Dmitry Kalinkin
` (7 preceding siblings ...)
2015-06-23 16:03 ` [PATCHv2 8/9] staging: vme_user: remove distracting comment Dmitry Kalinkin
@ 2015-06-23 16:03 ` Dmitry Kalinkin
2015-06-26 20:39 ` [PATCHv3 0/9] vme_user checkpatch fixes and read()/write() rework Dmitry Kalinkin
9 siblings, 0 replies; 42+ messages in thread
From: Dmitry Kalinkin @ 2015-06-23 16:03 UTC (permalink / raw)
To: linux-kernel, devel
Cc: Martyn Welch, Manohar Vanga, Greg Kroah-Hartman, 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
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCHv3 0/9] vme_user checkpatch fixes and read()/write() rework
2015-06-23 16:03 ` [PATCHv2 0/9] vme_user checkpatch fixes and read()/write() rework Dmitry Kalinkin
` (8 preceding siblings ...)
2015-06-23 16:03 ` [PATCHv2 9/9] staging: vme_user: remove okcount variable Dmitry Kalinkin
@ 2015-06-26 20:39 ` Dmitry Kalinkin
2015-06-26 20:39 ` [PATCHv3 1/9] staging: vme_user: fix code alignment Dmitry Kalinkin
` (8 more replies)
9 siblings, 9 replies; 42+ messages in thread
From: Dmitry Kalinkin @ 2015-06-26 20:39 UTC (permalink / raw)
To: linux-kernel, devel
Cc: Martyn Welch, Manohar Vanga, Greg Kroah-Hartman, 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
^ permalink raw reply [flat|nested] 42+ messages in thread* [PATCHv3 1/9] staging: vme_user: fix code alignment
2015-06-26 20:39 ` [PATCHv3 0/9] vme_user checkpatch fixes and read()/write() rework Dmitry Kalinkin
@ 2015-06-26 20:39 ` Dmitry Kalinkin
2015-06-26 20:39 ` [PATCHv3 2/9] staging: vme_user: fix blank lines Dmitry Kalinkin
` (7 subsequent siblings)
8 siblings, 0 replies; 42+ messages in thread
From: Dmitry Kalinkin @ 2015-06-26 20:39 UTC (permalink / raw)
To: linux-kernel, devel
Cc: Martyn Welch, Manohar Vanga, Greg Kroah-Hartman, 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
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCHv3 2/9] staging: vme_user: fix blank lines
2015-06-26 20:39 ` [PATCHv3 0/9] vme_user checkpatch fixes and read()/write() rework Dmitry Kalinkin
2015-06-26 20:39 ` [PATCHv3 1/9] staging: vme_user: fix code alignment Dmitry Kalinkin
@ 2015-06-26 20:39 ` Dmitry Kalinkin
2015-06-26 20:39 ` [PATCHv3 3/9] staging: vme_user: fix NULL comparison style Dmitry Kalinkin
` (6 subsequent siblings)
8 siblings, 0 replies; 42+ messages in thread
From: Dmitry Kalinkin @ 2015-06-26 20:39 UTC (permalink / raw)
To: linux-kernel, devel
Cc: Martyn Welch, Manohar Vanga, Greg Kroah-Hartman, 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
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCHv3 3/9] staging: vme_user: fix NULL comparison style
2015-06-26 20:39 ` [PATCHv3 0/9] vme_user checkpatch fixes and read()/write() rework Dmitry Kalinkin
2015-06-26 20:39 ` [PATCHv3 1/9] staging: vme_user: fix code alignment Dmitry Kalinkin
2015-06-26 20:39 ` [PATCHv3 2/9] staging: vme_user: fix blank lines Dmitry Kalinkin
@ 2015-06-26 20:39 ` Dmitry Kalinkin
2015-06-26 20:39 ` [PATCHv3 4/9] staging: vme_user: fix kmalloc style Dmitry Kalinkin
` (5 subsequent siblings)
8 siblings, 0 replies; 42+ messages in thread
From: Dmitry Kalinkin @ 2015-06-26 20:39 UTC (permalink / raw)
To: linux-kernel, devel
Cc: Martyn Welch, Manohar Vanga, Greg Kroah-Hartman, 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
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCHv3 4/9] staging: vme_user: fix kmalloc style
2015-06-26 20:39 ` [PATCHv3 0/9] vme_user checkpatch fixes and read()/write() rework Dmitry Kalinkin
` (2 preceding siblings ...)
2015-06-26 20:39 ` [PATCHv3 3/9] staging: vme_user: fix NULL comparison style Dmitry Kalinkin
@ 2015-06-26 20:39 ` Dmitry Kalinkin
2015-06-26 20:39 ` [PATCHv3 5/9] staging: vme_user: allow large read()/write() Dmitry Kalinkin
` (4 subsequent siblings)
8 siblings, 0 replies; 42+ messages in thread
From: Dmitry Kalinkin @ 2015-06-26 20:39 UTC (permalink / raw)
To: linux-kernel, devel
Cc: Martyn Welch, Manohar Vanga, Greg Kroah-Hartman, 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
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCHv3 5/9] staging: vme_user: allow large read()/write()
2015-06-26 20:39 ` [PATCHv3 0/9] vme_user checkpatch fixes and read()/write() rework Dmitry Kalinkin
` (3 preceding siblings ...)
2015-06-26 20:39 ` [PATCHv3 4/9] staging: vme_user: fix kmalloc style Dmitry Kalinkin
@ 2015-06-26 20:39 ` Dmitry Kalinkin
2015-06-26 20:39 ` [PATCHv3 6/9] staging: vme_user: switch to returning -EFAULT on __copy_*_user errors Dmitry Kalinkin
` (3 subsequent siblings)
8 siblings, 0 replies; 42+ messages in thread
From: Dmitry Kalinkin @ 2015-06-26 20:39 UTC (permalink / raw)
To: linux-kernel, devel
Cc: Martyn Welch, Manohar Vanga, Greg Kroah-Hartman, 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
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCHv3 6/9] staging: vme_user: switch to returning -EFAULT on __copy_*_user errors
2015-06-26 20:39 ` [PATCHv3 0/9] vme_user checkpatch fixes and read()/write() rework Dmitry Kalinkin
` (4 preceding siblings ...)
2015-06-26 20:39 ` [PATCHv3 5/9] staging: vme_user: allow large read()/write() Dmitry Kalinkin
@ 2015-06-26 20:39 ` Dmitry Kalinkin
2015-07-06 12:51 ` Martyn Welch
2015-06-26 20:39 ` [PATCHv3 7/9] staging: vme_user: remove unused variable Dmitry Kalinkin
` (2 subsequent siblings)
8 siblings, 1 reply; 42+ messages in thread
From: Dmitry Kalinkin @ 2015-06-26 20:39 UTC (permalink / raw)
To: linux-kernel, devel
Cc: Martyn Welch, Manohar Vanga, Greg Kroah-Hartman, 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
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCHv3 6/9] staging: vme_user: switch to returning -EFAULT on __copy_*_user errors
2015-06-26 20:39 ` [PATCHv3 6/9] staging: vme_user: switch to returning -EFAULT on __copy_*_user errors Dmitry Kalinkin
@ 2015-07-06 12:51 ` Martyn Welch
2015-07-06 13:10 ` Dmitry Kalinkin
0 siblings, 1 reply; 42+ messages in thread
From: Martyn Welch @ 2015-07-06 12:51 UTC (permalink / raw)
To: Dmitry Kalinkin, linux-kernel, devel; +Cc: Manohar Vanga, Greg Kroah-Hartman
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
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCHv3 6/9] staging: vme_user: switch to returning -EFAULT on __copy_*_user errors
2015-07-06 12:51 ` Martyn Welch
@ 2015-07-06 13:10 ` Dmitry Kalinkin
2015-07-06 13:51 ` Martyn Welch
0 siblings, 1 reply; 42+ messages in thread
From: Dmitry Kalinkin @ 2015-07-06 13:10 UTC (permalink / raw)
To: Martyn Welch; +Cc: linux-kernel, devel, Manohar Vanga, Greg Kroah-Hartman
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
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCHv3 6/9] staging: vme_user: switch to returning -EFAULT on __copy_*_user errors
2015-07-06 13:10 ` Dmitry Kalinkin
@ 2015-07-06 13:51 ` Martyn Welch
0 siblings, 0 replies; 42+ messages in thread
From: Martyn Welch @ 2015-07-06 13:51 UTC (permalink / raw)
To: Dmitry Kalinkin; +Cc: linux-kernel, devel, Manohar Vanga, Greg Kroah-Hartman
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
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCHv3 7/9] staging: vme_user: remove unused variable
2015-06-26 20:39 ` [PATCHv3 0/9] vme_user checkpatch fixes and read()/write() rework Dmitry Kalinkin
` (5 preceding siblings ...)
2015-06-26 20:39 ` [PATCHv3 6/9] staging: vme_user: switch to returning -EFAULT on __copy_*_user errors Dmitry Kalinkin
@ 2015-06-26 20:39 ` Dmitry Kalinkin
2015-06-26 20:39 ` [PATCHv3 8/9] staging: vme_user: remove distracting comment Dmitry Kalinkin
2015-06-26 20:39 ` [PATCHv3 9/9] staging: vme_user: remove okcount variable Dmitry Kalinkin
8 siblings, 0 replies; 42+ messages in thread
From: Dmitry Kalinkin @ 2015-06-26 20:39 UTC (permalink / raw)
To: linux-kernel, devel
Cc: Martyn Welch, Manohar Vanga, Greg Kroah-Hartman, 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
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCHv3 8/9] staging: vme_user: remove distracting comment
2015-06-26 20:39 ` [PATCHv3 0/9] vme_user checkpatch fixes and read()/write() rework Dmitry Kalinkin
` (6 preceding siblings ...)
2015-06-26 20:39 ` [PATCHv3 7/9] staging: vme_user: remove unused variable Dmitry Kalinkin
@ 2015-06-26 20:39 ` Dmitry Kalinkin
2015-06-26 20:39 ` [PATCHv3 9/9] staging: vme_user: remove okcount variable Dmitry Kalinkin
8 siblings, 0 replies; 42+ messages in thread
From: Dmitry Kalinkin @ 2015-06-26 20:39 UTC (permalink / raw)
To: linux-kernel, devel
Cc: Martyn Welch, Manohar Vanga, Greg Kroah-Hartman, 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
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCHv3 9/9] staging: vme_user: remove okcount variable
2015-06-26 20:39 ` [PATCHv3 0/9] vme_user checkpatch fixes and read()/write() rework Dmitry Kalinkin
` (7 preceding siblings ...)
2015-06-26 20:39 ` [PATCHv3 8/9] staging: vme_user: remove distracting comment Dmitry Kalinkin
@ 2015-06-26 20:39 ` Dmitry Kalinkin
8 siblings, 0 replies; 42+ messages in thread
From: Dmitry Kalinkin @ 2015-06-26 20:39 UTC (permalink / raw)
To: linux-kernel, devel
Cc: Martyn Welch, Manohar Vanga, Greg Kroah-Hartman, 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
^ permalink raw reply related [flat|nested] 42+ messages in thread