Re: [PATCH v2 1/4] usb: hub: convert khubd into workqueue

[Alan Stern]

On Wed, 17 Sep 2014, Petr Mladek wrote:

> There is no need to have separate kthread for handling USB hub events.
> It is more elegant to use the workqueue framework.
> 
> The workqueue is allocated as freezable because the original thread was
> freezable as well.

I've got a few comments about style.

> -static void kick_khubd(struct usb_hub *hub)
> +static void kick_hub_wq(struct usb_hub *hub)
>  {
> -	unsigned long	flags;
> +	struct usb_interface *intf;
>  
> -	spin_lock_irqsave(&hub_event_lock, flags);
> -	if (!hub->disconnected && list_empty(&hub->event_list)) {
> -		list_add_tail(&hub->event_list, &hub_event_list);
> +	if (hub->disconnected || work_pending(&hub->events))
> +		return;
>  
> -		/* Suppress autosuspend until khubd runs */
> -		usb_autopm_get_interface_no_resume(
> -				to_usb_interface(hub->intfdev));
> -		wake_up(&khubd_wait);
> -	}
> -	spin_unlock_irqrestore(&hub_event_lock, flags);
> +	intf = to_usb_interface(hub->intfdev);
> +	/*
> +	 * Suppress autosuspend until the event is proceed.
> +	 *
> +	 * Be careful and make sure that the symmetric operation is
> +	 * always called. We are here only when there is no pending
> +	 * work for this hub. Therefore put the interface either when
> +	 * the new work is called or when it is canceled.
> +	 */
> +	kref_get(&hub->kref);
> +	usb_autopm_get_interface_no_resume(intf);

This looks a little weird.  The assignment to intf and the call to
usb_autopm_get_interface_no_resume naturally go together.  So why
put the big comment and the kref_get call in between them?  Put the
comment first, then the assigment and the autopm call, and then the
kref_get.

> @@ -5130,40 +5115,15 @@ static void hub_events(void)
>  			}
>  		}
>  
> - loop_autopm:
> + out_autopm:
>  		/* Balance the usb_autopm_get_interface() above */
>  		usb_autopm_put_interface_no_suspend(intf);
> - loop:
> -		/* Balance the usb_autopm_get_interface_no_resume() in
> -		 * kick_khubd() and allow autosuspend.
> -		 */
> -		usb_autopm_put_interface(intf);
> - loop_disconnected:
> + out_hdev_lock:
>  		usb_unlock_device(hdev);
> -		usb_put_dev(hdev);
> +		/* Ballance the stuff in kick_hub_wq() and allow autosuspend */
> +		usb_autopm_put_interface(intf);

You misspelled "Balance".  Also, there should be a blank line before
this comment.

> @@ -5203,21 +5163,26 @@ int usb_hub_init(void)
>  		return -1;
>  	}
>  
> -	khubd_task = kthread_run(hub_thread, NULL, "khubd");
> -	if (!IS_ERR(khubd_task))
> +	/*
> +	 * The workqueue needs to be freezable to avoid interfering with
> +	 * USB-PERSIST port handover. Otherwise it might see that a full-speed
> +	 * device was gone before the EHCI controller had handed its port
> +	 * over to the companion full-speed controller.
> +	 */
> +	hub_wq = alloc_workqueue("hub", WQ_FREEZABLE, 0);

Is "hub" really a good name for the workqueue?  If somebody reads it,
will they know what sort of hub it refers to?  Would "usb_hub" or even
"usb_hub_wq" be better?

>  void usb_hub_cleanup(void)
>  {
> -       kthread_stop(khubd_task);
> -
> +       destroy_workqueue(hub_wq);
>         /*

Don't get rid of this blank line.

More to come...

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/