Fixed a code styling issue
Signed-off-by: Tom Todd <[email protected]>
---
drivers/usb/core/devio.c | 57 ++++++++++++++++++++--------------------
1 file changed, 29 insertions(+), 28 deletions(-)
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 476dcc5f2da3..118a29349b1d 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -2139,38 +2139,39 @@ static int proc_ioctl(struct usb_dev_state *ps, struct usbdevfs_ioctl *ctl)
retval = -EHOSTUNREACH;
else if (!(intf = usb_ifnum_to_if(ps->dev, ctl->ifno)))
retval = -EINVAL;
- else switch (ctl->ioctl_code) {
-
- /* disconnect kernel driver from interface */
- case USBDEVFS_DISCONNECT:
- if (intf->dev.driver) {
- driver = to_usb_driver(intf->dev.driver);
- dev_dbg(&intf->dev, "disconnect by usbfs\n");
- usb_driver_release_interface(driver, intf);
- } else
- retval = -ENODATA;
- break;
+ else
+ switch (ctl->ioctl_code) {
+
+ /* disconnect kernel driver from interface */
+ case USBDEVFS_DISCONNECT:
+ if (intf->dev.driver) {
+ driver = to_usb_driver(intf->dev.driver);
+ dev_dbg(&intf->dev, "disconnect by usbfs\n");
+ usb_driver_release_interface(driver, intf);
+ } else
+ retval = -ENODATA;
+ break;
- /* let kernel drivers try to (re)bind to the interface */
- case USBDEVFS_CONNECT:
- if (!intf->dev.driver)
- retval = device_attach(&intf->dev);
- else
- retval = -EBUSY;
- break;
+ /* let kernel drivers try to (re)bind to the interface */
+ case USBDEVFS_CONNECT:
+ if (!intf->dev.driver)
+ retval = device_attach(&intf->dev);
+ else
+ retval = -EBUSY;
+ break;
- /* talk directly to the interface's driver */
- default:
- if (intf->dev.driver)
- driver = to_usb_driver(intf->dev.driver);
- if (driver == NULL || driver->unlocked_ioctl == NULL) {
- retval = -ENOTTY;
- } else {
- retval = driver->unlocked_ioctl(intf, ctl->ioctl_code, buf);
- if (retval == -ENOIOCTLCMD)
+ /* talk directly to the interface's driver */
+ default:
+ if (intf->dev.driver)
+ driver = to_usb_driver(intf->dev.driver);
+ if (driver == NULL || driver->unlocked_ioctl == NULL) {
retval = -ENOTTY;
+ } else {
+ retval = driver->unlocked_ioctl(intf, ctl->ioctl_code, buf);
+ if (retval == -ENOIOCTLCMD)
+ retval = -ENOTTY;
+ }
}
- }
/* cleanup and return */
if (retval >= 0
--
2.18.0
On Sun, 2018-08-12 at 19:06 +0100, Tom Todd wrote:
> Fixed a code styling issue
while it's OK to fix style only issues, it's much better
to reorganize the code for reader clarity.
For this code, something like:
o use memdup_user
o use an exit label to free allocated memory
o invert tests to return on unexpected results
o move logical continuations to EOL
o don't use successive code like:
if (foo)
return -ERRNO;
else if (bar)
return -ERRNO;
else
baz;
simpler and less indentation is:
if (foo)
return -ERRNO;
if (bar)
return -ERRNO;
baz;
---
drivers/usb/core/devio.c | 68 ++++++++++++++++++++++++++----------------------
1 file changed, 37 insertions(+), 31 deletions(-)
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 6ce77b33da61..74ae4e052357 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -2118,68 +2118,74 @@ static int proc_ioctl(struct usb_dev_state *ps, struct usbdevfs_ioctl *ctl)
/* alloc buffer */
size = _IOC_SIZE(ctl->ioctl_code);
if (size > 0) {
- buf = kmalloc(size, GFP_KERNEL);
- if (buf == NULL)
- return -ENOMEM;
- if ((_IOC_DIR(ctl->ioctl_code) & _IOC_WRITE)) {
- if (copy_from_user(buf, ctl->data, size)) {
- kfree(buf);
+ if (_IOC_DIR(ctl->ioctl_code) & _IOC_WRITE) {
+ buf = memdup_user(ctl->data, size);
+ if (!buf)
return -EFAULT;
- }
} else {
- memset(buf, 0, size);
+ buf = kzalloc(size, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
}
}
if (!connected(ps)) {
- kfree(buf);
- return -ENODEV;
+ retval = -ENODEV;
+ goto exit;
}
-
- if (ps->dev->state != USB_STATE_CONFIGURED)
+ if (ps->dev->state != USB_STATE_CONFIGURED) {
retval = -EHOSTUNREACH;
- else if (!(intf = usb_ifnum_to_if(ps->dev, ctl->ifno)))
+ goto exit;
+ }
+ intf = usb_ifnum_to_if(ps->dev, ctl->ifno);
+ if (!intf) {
retval = -EINVAL;
- else switch (ctl->ioctl_code) {
+ goto exit;
+ }
+ switch (ctl->ioctl_code) {
/* disconnect kernel driver from interface */
case USBDEVFS_DISCONNECT:
- if (intf->dev.driver) {
- driver = to_usb_driver(intf->dev.driver);
- dev_dbg(&intf->dev, "disconnect by usbfs\n");
- usb_driver_release_interface(driver, intf);
- } else
+ if (!intf->dev.driver) {
retval = -ENODATA;
+ goto exit;
+ }
+ driver = to_usb_driver(intf->dev.driver);
+ dev_dbg(&intf->dev, "disconnect by usbfs\n");
+ usb_driver_release_interface(driver, intf);
break;
/* let kernel drivers try to (re)bind to the interface */
case USBDEVFS_CONNECT:
- if (!intf->dev.driver)
- retval = device_attach(&intf->dev);
- else
+ if (!intf->dev.driver) {
retval = -EBUSY;
+ goto exit;
+ }
+ retval = device_attach(&intf->dev);
break;
/* talk directly to the interface's driver */
default:
if (intf->dev.driver)
driver = to_usb_driver(intf->dev.driver);
- if (driver == NULL || driver->unlocked_ioctl == NULL) {
+ if (!driver || !driver->unlocked_ioctl) {
retval = -ENOTTY;
- } else {
- retval = driver->unlocked_ioctl(intf, ctl->ioctl_code, buf);
- if (retval == -ENOIOCTLCMD)
- retval = -ENOTTY;
+ goto exit;
+ }
+ retval = driver->unlocked_ioctl(intf, ctl->ioctl_code, buf);
+ if (retval == -ENOIOCTLCMD) {
+ retval = -ENOTTY;
+ goto exit;
}
+ break;
}
/* cleanup and return */
- if (retval >= 0
- && (_IOC_DIR(ctl->ioctl_code) & _IOC_READ) != 0
- && size > 0
- && copy_to_user(ctl->data, buf, size) != 0)
+ if (retval >= 0 && (_IOC_DIR(ctl->ioctl_code) & _IOC_READ) != 0 &&
+ size > 0 && copy_to_user(ctl->data, buf, size) != 0)
retval = -EFAULT;
+exit:
kfree(buf);
return retval;
}
On Sun, Aug 12, 2018 at 07:06:56PM +0100, Tom Todd wrote:
> Fixed a code styling issue
>
Just ignore checkpatch on this one. The code is slightly unusual so
checkpatch got confused.
regards,
dan carpenter
On, Sun, 12 Aug 2018 11:34:59 -0700 Joe Perches wrote:
>On Sun, 2018-08-12 at 19:06 +0100, Tom Todd wrote:
>> Fixed a code styling issue
>while it's OK to fix style only issues, it's much better
>to reorganize the code for reader clarity.
Ok, thank you, I've taken your suggestions and created the a new version
Restructured method proc_ioctl for readability and fixed code style
errors.
Signed-off-by: Tom Todd <[email protected]>
---
drivers/usb/core/devio.c | 67 ++++++++++++++++++++++------------------
1 file changed, 37 insertions(+), 30 deletions(-)
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 476dcc5f2da3..740e60e086e2 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -2117,46 +2117,52 @@ static int proc_ioctl(struct usb_dev_state *ps, struct usbdevfs_ioctl *ctl)
/* alloc buffer */
size = _IOC_SIZE(ctl->ioctl_code);
if (size > 0) {
- buf = kmalloc(size, GFP_KERNEL);
- if (buf == NULL)
- return -ENOMEM;
- if ((_IOC_DIR(ctl->ioctl_code) & _IOC_WRITE)) {
- if (copy_from_user(buf, ctl->data, size)) {
- kfree(buf);
+ if (_IOC_DIR(ctl->ioctl_code) & _IOC_WRITE) {
+ buf = memdup_user(ctl->data, size);
+ if (!buf)
return -EFAULT;
- }
} else {
- memset(buf, 0, size);
+ buf = kmalloc(size, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
}
}
if (!connected(ps)) {
- kfree(buf);
- return -ENODEV;
+ retval = -ENODEV;
+ goto exit;
}
- if (ps->dev->state != USB_STATE_CONFIGURED)
+ if (ps->dev->state != USB_STATE_CONFIGURED) {
retval = -EHOSTUNREACH;
- else if (!(intf = usb_ifnum_to_if(ps->dev, ctl->ifno)))
+ goto exit;
+ }
+
+ intf = usb_ifnum_to_if(ps->dev, ctl->ifno);
+ if (!intf) {
retval = -EINVAL;
- else switch (ctl->ioctl_code) {
+ goto exit;
+ }
+ switch (ctl->ioctl_code) {
/* disconnect kernel driver from interface */
case USBDEVFS_DISCONNECT:
- if (intf->dev.driver) {
- driver = to_usb_driver(intf->dev.driver);
- dev_dbg(&intf->dev, "disconnect by usbfs\n");
- usb_driver_release_interface(driver, intf);
- } else
+ if (!intf->dev.driver) {
retval = -ENODATA;
+ goto exit;
+ }
+ driver = to_usb_driver(intf->dev.driver);
+ dev_dbg(&intf->dev, "disconnect by usbfs\n");
+ usb_driver_release_interface(driver, intf);
break;
/* let kernel drivers try to (re)bind to the interface */
case USBDEVFS_CONNECT:
- if (!intf->dev.driver)
- retval = device_attach(&intf->dev);
- else
+ if (!intf->dev.driver) {
retval = -EBUSY;
+ goto exit;
+ }
+ retval = device_attach(&intf->dev);
break;
/* talk directly to the interface's driver */
@@ -2165,20 +2171,21 @@ static int proc_ioctl(struct usb_dev_state *ps, struct usbdevfs_ioctl *ctl)
driver = to_usb_driver(intf->dev.driver);
if (driver == NULL || driver->unlocked_ioctl == NULL) {
retval = -ENOTTY;
- } else {
- retval = driver->unlocked_ioctl(intf, ctl->ioctl_code, buf);
- if (retval == -ENOIOCTLCMD)
- retval = -ENOTTY;
+ goto exit;
}
+ retval = driver->unlocked_ioctl(intf, ctl->ioctl_code, buf);
+ if (retval == -ENOIOCTLCMD) {
+ retval = -ENOTTY;
+ goto exit;
+ }
+ break;
}
/* cleanup and return */
- if (retval >= 0
- && (_IOC_DIR(ctl->ioctl_code) & _IOC_READ) != 0
- && size > 0
- && copy_to_user(ctl->data, buf, size) != 0)
+ if (retval >= 0 && (_IOC_DIR(ctl->ioctl_code) & _IOC_READ) != 0 &&
+ size > 0 && copy_to_user(ctl->data, buf, size) != 0)
retval = -EFAULT;
-
+exit:
kfree(buf);
return retval;
}
--
2.18.0
On Sun, 12 Aug 2018, Tom Todd wrote:
> On, Sun, 12 Aug 2018 11:34:59 -0700 Joe Perches wrote:
> >On Sun, 2018-08-12 at 19:06 +0100, Tom Todd wrote:
> >> Fixed a code styling issue
> >while it's OK to fix style only issues, it's much better
> >to reorganize the code for reader clarity.
>
> Ok, thank you, I've taken your suggestions and created the a new version
>
> Restructured method proc_ioctl for readability and fixed code style
> errors.
>
> Signed-off-by: Tom Todd <[email protected]>
"Style fixes" that introduce bugs are quite annoying...
> ---
> drivers/usb/core/devio.c | 67 ++++++++++++++++++++++------------------
> 1 file changed, 37 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index 476dcc5f2da3..740e60e086e2 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -2117,46 +2117,52 @@ static int proc_ioctl(struct usb_dev_state *ps, struct usbdevfs_ioctl *ctl)
> /* alloc buffer */
> size = _IOC_SIZE(ctl->ioctl_code);
> if (size > 0) {
> - buf = kmalloc(size, GFP_KERNEL);
> - if (buf == NULL)
> - return -ENOMEM;
> - if ((_IOC_DIR(ctl->ioctl_code) & _IOC_WRITE)) {
> - if (copy_from_user(buf, ctl->data, size)) {
> - kfree(buf);
> + if (_IOC_DIR(ctl->ioctl_code) & _IOC_WRITE) {
> + buf = memdup_user(ctl->data, size);
> + if (!buf)
> return -EFAULT;
> - }
> } else {
> - memset(buf, 0, size);
> + buf = kmalloc(size, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
Where do the contents of buf get set to 0 now?
> }
> }
>
> if (!connected(ps)) {
> - kfree(buf);
> - return -ENODEV;
> + retval = -ENODEV;
> + goto exit;
> }
>
> - if (ps->dev->state != USB_STATE_CONFIGURED)
> + if (ps->dev->state != USB_STATE_CONFIGURED) {
> retval = -EHOSTUNREACH;
> - else if (!(intf = usb_ifnum_to_if(ps->dev, ctl->ifno)))
> + goto exit;
> + }
> +
> + intf = usb_ifnum_to_if(ps->dev, ctl->ifno);
> + if (!intf) {
> retval = -EINVAL;
> - else switch (ctl->ioctl_code) {
> + goto exit;
> + }
>
> + switch (ctl->ioctl_code) {
> /* disconnect kernel driver from interface */
> case USBDEVFS_DISCONNECT:
> - if (intf->dev.driver) {
> - driver = to_usb_driver(intf->dev.driver);
> - dev_dbg(&intf->dev, "disconnect by usbfs\n");
> - usb_driver_release_interface(driver, intf);
> - } else
> + if (!intf->dev.driver) {
> retval = -ENODATA;
> + goto exit;
> + }
> + driver = to_usb_driver(intf->dev.driver);
> + dev_dbg(&intf->dev, "disconnect by usbfs\n");
> + usb_driver_release_interface(driver, intf);
> break;
>
> /* let kernel drivers try to (re)bind to the interface */
> case USBDEVFS_CONNECT:
> - if (!intf->dev.driver)
> - retval = device_attach(&intf->dev);
> - else
> + if (!intf->dev.driver) {
Now !intf->dev.driver generates an error instead of working?
> retval = -EBUSY;
> + goto exit;
> + }
> + retval = device_attach(&intf->dev);
> break;
>
> /* talk directly to the interface's driver */
> @@ -2165,20 +2171,21 @@ static int proc_ioctl(struct usb_dev_state *ps, struct usbdevfs_ioctl *ctl)
> driver = to_usb_driver(intf->dev.driver);
> if (driver == NULL || driver->unlocked_ioctl == NULL) {
> retval = -ENOTTY;
> - } else {
> - retval = driver->unlocked_ioctl(intf, ctl->ioctl_code, buf);
> - if (retval == -ENOIOCTLCMD)
> - retval = -ENOTTY;
> + goto exit;
> }
> + retval = driver->unlocked_ioctl(intf, ctl->ioctl_code, buf);
> + if (retval == -ENOIOCTLCMD) {
> + retval = -ENOTTY;
> + goto exit;
> + }
> + break;
> }
>
> /* cleanup and return */
> - if (retval >= 0
> - && (_IOC_DIR(ctl->ioctl_code) & _IOC_READ) != 0
> - && size > 0
> - && copy_to_user(ctl->data, buf, size) != 0)
> + if (retval >= 0 && (_IOC_DIR(ctl->ioctl_code) & _IOC_READ) != 0 &&
> + size > 0 && copy_to_user(ctl->data, buf, size) != 0)
> retval = -EFAULT;
Isn't the style in this file to indent continuation lines by two tab
stops?
> -
> +exit:
> kfree(buf);
> return retval;
> }
Alan Stern
On Sun, Aug 12, 2018 at 11:26:54PM +0100, Tom Todd wrote:
> On, Sun, 12 Aug 2018 11:34:59 -0700 Joe Perches wrote:
> >On Sun, 2018-08-12 at 19:06 +0100, Tom Todd wrote:
> >> Fixed a code styling issue
> >while it's OK to fix style only issues, it's much better
> >to reorganize the code for reader clarity.
>
> Ok, thank you, I've taken your suggestions and created the a new version
>
> Restructured method proc_ioctl for readability and fixed code style
> errors.
Please always be very specific as to exactly what code style issues you
are fixing. And never do more than one type of fix at a time, otherwise
it is hard to review, and you can cause errors to be added, like you did
here :(
Please do not start working on code cleanups in any other area of the
kernel other than drivers/staging/ as that is what that is for. To do
so in other areas of the kernel causes more work for maintainers.
thanks,
greg k-h
On Mon, Aug 13, 2018 at 08:34:55AM +0200, Greg KH wrote:
> On Sun, Aug 12, 2018 at 11:26:54PM +0100, Tom Todd wrote:
> > On, Sun, 12 Aug 2018 11:34:59 -0700 Joe Perches wrote:
> > >On Sun, 2018-08-12 at 19:06 +0100, Tom Todd wrote:
> > >> Fixed a code styling issue
> > >while it's OK to fix style only issues, it's much better
> > >to reorganize the code for reader clarity.
> >
> > Ok, thank you, I've taken your suggestions and created the a new version
> >
> > Restructured method proc_ioctl for readability and fixed code style
> > errors.
>
> Please always be very specific as to exactly what code style issues you
> are fixing. And never do more than one type of fix at a time, otherwise
> it is hard to review, and you can cause errors to be added, like you did
> here :(
>
> Please do not start working on code cleanups in any other area of the
> kernel other than drivers/staging/ as that is what that is for. To do
> so in other areas of the kernel causes more work for maintainers.
>
> thanks,
>
> greg k-h
Understood, sorry for the bother caused.
Regards,
Tom Todd