----- Original Message -----
> From: "Martyn Welch" <martyn@xxxxxxxxxxxx>
> Subject: Re: [PATCH 1/3] staging: vme_user: return retval in vme_user_ioctl
> On Fri, Sep 02, 2016 at 04:16:48PM -0500, Aaron Sierra wrote:
>> Update each case to set retval and return that value at the end of the
>> function. This also replaces most case statement returns with breaks
>> and collapses some whitespace.
>>
>
> Sorry if I'm being dense, but is there an advantage to doing it this way?
>
> This seems to be adding churn for no discernible gain.
>
>> Signed-off-by: Aaron Sierra <asierra@xxxxxxxxxxx>
>> ---
>> drivers/staging/vme/devices/vme_user.c | 22 ++++++++++------------
>> 1 file changed, 10 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/staging/vme/devices/vme_user.c
>> b/drivers/staging/vme/devices/vme_user.c
>> index fc660bd..5aa53c4 100644
>> --- a/drivers/staging/vme/devices/vme_user.c
>> +++ b/drivers/staging/vme/devices/vme_user.c
>> @@ -299,7 +299,7 @@ static int vme_user_ioctl(struct inode *inode, struct file
>> *file,
>> struct vme_irq_id irq_req;
>> unsigned long copied;
>> unsigned int minor = MINOR(inode->i_rdev);
>> - int retval;
>> + int retval = -EINVAL;
>> dma_addr_t pci_addr;
>> void __user *argp = (void __user *)arg;
>>
>> @@ -314,9 +314,10 @@ static int vme_user_ioctl(struct inode *inode, struct file
>> *file,
>> return -EFAULT;
>> }
>>
>> - return vme_irq_generate(vme_user_bridge,
>> + retval = vme_irq_generate(vme_user_bridge,
>> irq_req.level,
>> irq_req.statid);
>> + break;
>> }
>> break;
>> case MASTER_MINOR:
>> @@ -337,13 +338,11 @@ static int vme_user_ioctl(struct inode *inode, struct file
>> *file,
>> sizeof(master));
>> if (copied) {
>> pr_warn("Partial copy to userspace\n");
>> - return -EFAULT;
>> + retval = -EFAULT;
>> }
>>
>> - return retval;
>> -
>> + break;
>> case VME_SET_MASTER:
>> -
>> if (image[minor].mmap_count != 0) {
>> pr_warn("Can't adjust mapped window\n");
>> return -EPERM;
>> @@ -358,7 +357,7 @@ static int vme_user_ioctl(struct inode *inode, struct file
>> *file,
>> /* XXX We do not want to push aspace, cycle and width
>> * to userspace as they are
>> */
>> - return vme_master_set(image[minor].resource,
>> + retval = vme_master_set(image[minor].resource,
>> master.enable, master.vme_addr, master.size,
>> master.aspace, master.cycle, master.dwidth);
>>
>> @@ -382,11 +381,10 @@ static int vme_user_ioctl(struct inode *inode, struct file
>> *file,
>> sizeof(slave));
>> if (copied) {
>> pr_warn("Partial copy to userspace\n");
>> - return -EFAULT;
>> + retval = -EFAULT;
>> }
>>
>> - return retval;
>> -
>> + break;
>> case VME_SET_SLAVE:
>>
>> copied = copy_from_user(&slave, argp, sizeof(slave));
>> @@ -398,7 +396,7 @@ static int vme_user_ioctl(struct inode *inode, struct file
>> *file,
>> /* XXX We do not want to push aspace, cycle and width
>> * to userspace as they are
>> */
>> - return vme_slave_set(image[minor].resource,
>> + retval = vme_slave_set(image[minor].resource,
>> slave.enable, slave.vme_addr, slave.size,
>> image[minor].pci_buf, slave.aspace,
>> slave.cycle);
>> @@ -408,7 +406,7 @@ static int vme_user_ioctl(struct inode *inode, struct file
>> *file,
>> break;
>> }
>>
>> - return -EINVAL;
>> + return retval;
Martyn,
This last change is where the slippery slope began. With this change, the
additions I make to VME_SET_SLAVE in my "3/3" patch can use a break instead
of adding another return statement, which feels cleaner to me.
Also, this patch makes the breaks in VME_SET_MASTER and VME_SET_SLAVE do
something meaningful.
-Aaron S.
>> }
>>
>> static long
>> --
> > 1.9.1
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
