[PATCH] usb: hub: Minor refactoring in usb_hub_init()

[PATCH] usb: hub: Minor refactoring in usb_hub_init()

[Austin Kim]

Normally when creation of workqueue fails, exception handling takes place
after the call to alloc_workqueue() is made.

But looking into usb_hub_init() function, 'return 0' statement is executed,
when alloc_workqueue() returns valid workqueue pointer.
       if (hub_wq)
               return 0;

This might make other Linux driver developers get confused 
because they could deduce that this is exceptional handling routine.

So perform minor refactoring by adding NULL pointer dereference check
routine right after the call to alloc_workqueue() is made.

Signed-off-by: Austin Kim <austindh.kim@gmail.com>
---
 drivers/usb/core/hub.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index e8ebacc..0ddbfe6 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -5530,9 +5530,12 @@ int usb_hub_init(void)
 	 * over to the companion full-speed controller.
 	 */
 	hub_wq = alloc_workqueue("usb_hub_wq", WQ_FREEZABLE, 0);
-	if (hub_wq)
-		return 0;
+	if (unlikely(!hub_wq))
+		goto err_workqueue;
+
+	return 0;
 
+err_workqueue:
 	/* Fall through if kernel_thread failed */
 	usb_deregister(&hub_driver);
 	pr_err("%s: can't allocate workqueue for usb hub\n", usbcore_name);
-- 
2.6.2

Re: [PATCH] usb: hub: Minor refactoring in usb_hub_init()

[Greg KH]

On Mon, Sep 23, 2019 at 03:19:21PM +0900, Austin Kim wrote:
> Normally when creation of workqueue fails, exception handling takes place
> after the call to alloc_workqueue() is made.
> 
> But looking into usb_hub_init() function, 'return 0' statement is executed,
> when alloc_workqueue() returns valid workqueue pointer.
>        if (hub_wq)
>                return 0;
> 
> This might make other Linux driver developers get confused 
> because they could deduce that this is exceptional handling routine.
> 
> So perform minor refactoring by adding NULL pointer dereference check
> routine right after the call to alloc_workqueue() is made.
> 
> Signed-off-by: Austin Kim <austindh.kim@gmail.com>
> ---
>  drivers/usb/core/hub.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index e8ebacc..0ddbfe6 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -5530,9 +5530,12 @@ int usb_hub_init(void)
>  	 * over to the companion full-speed controller.
>  	 */
>  	hub_wq = alloc_workqueue("usb_hub_wq", WQ_FREEZABLE, 0);
> -	if (hub_wq)
> -		return 0;
> +	if (unlikely(!hub_wq))

Only ever use likely/unlikely if you can measure the difference without
it.  Otherwise the compiler and cpu will almost always do this better
than you.

So please remove this.

> +		goto err_workqueue;
> +
> +	return 0;
>  
> +err_workqueue:
>  	/* Fall through if kernel_thread failed */

This comment is now incorrect.

But really, there is nothing wrong with the original code here.  It
works properly, and while it is not identical to normal code "style"
here, there's nothing wrong with it that I can see.

thanks,

greg k-h

Re: [PATCH] usb: hub: Minor refactoring in usb_hub_init()

[Alan Stern]

On Mon, 23 Sep 2019, Greg KH wrote:

> On Mon, Sep 23, 2019 at 03:19:21PM +0900, Austin Kim wrote:
> > Normally when creation of workqueue fails, exception handling takes place
> > after the call to alloc_workqueue() is made.
> > 
> > But looking into usb_hub_init() function, 'return 0' statement is executed,
> > when alloc_workqueue() returns valid workqueue pointer.
> >        if (hub_wq)
> >                return 0;
> > 
> > This might make other Linux driver developers get confused 
> > because they could deduce that this is exceptional handling routine.
> > 
> > So perform minor refactoring by adding NULL pointer dereference check
> > routine right after the call to alloc_workqueue() is made.
> > 
> > Signed-off-by: Austin Kim <austindh.kim@gmail.com>
> > ---
> >  drivers/usb/core/hub.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index e8ebacc..0ddbfe6 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -5530,9 +5530,12 @@ int usb_hub_init(void)
> >  	 * over to the companion full-speed controller.
> >  	 */
> >  	hub_wq = alloc_workqueue("usb_hub_wq", WQ_FREEZABLE, 0);
> > -	if (hub_wq)
> > -		return 0;
> > +	if (unlikely(!hub_wq))
> 
> Only ever use likely/unlikely if you can measure the difference without
> it.  Otherwise the compiler and cpu will almost always do this better
> than you.
> 
> So please remove this.
> 
> > +		goto err_workqueue;
> > +
> > +	return 0;
> >  
> > +err_workqueue:
> >  	/* Fall through if kernel_thread failed */
> 
> This comment is now incorrect.
> 
> But really, there is nothing wrong with the original code here.  It
> works properly, and while it is not identical to normal code "style"
> here, there's nothing wrong with it that I can see.

Indeed.  In fact, I suspect that this change would make the code less 
understandable, because the reader would wonder why anybody would go to 
the trouble of jumping over a return statement.  After all, this:

	if (!test)
		jump error;
	return 0;
 error:

just looks like a strange and inefficient way of writing:

	if (test)
		return 0;

Anyone reading it would wonder what the original author was thinking.

If you really want to fix up this subroutine, you could change the two 
"return -1" statements.  They should return an appropriate error code, 
not just -1.

Alan Stern

Re: [PATCH] usb: hub: Minor refactoring in usb_hub_init()

[Austin Kim]

> If you really want to fix up this subroutine, you could change the two
> "return -1" statements.  They should return an appropriate error code,
> not just -1.
>
> Alan Stern
>

Thanks for valuable feedback over the patch.
If I generate new patch later, let me make sure to contain an
appropriate error code.

Thanks,
Austin Kim