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-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
