On 02/22/2014 10:14 AM, Alan Stern wrote:
On Sat, 22 Feb 2014, Tejun Heo wrote:Hello, If this is actually safe, let's do it from the get-go. Thanks! ------- 8< ------- PREPARE_[DELAYED_]WORK() are being phased out. They have few users and a nasty surprise in terms of reentrancy guarantee as workqueue considers work items to be different if they don't have the same work function. usb_hub->init_work is multiplexed with multiple work functions; however, the work item is never queued while in-flight, so we can simply use INIT_DELAYED_WORK() before each queueing. It would probably be best to route this with other related updates through the workqueue tree. Lightly tested. v2: Greg and Alan confirm that the work item is never queued while in-flight. Simply use INIT_DELAYED_WORK(). Signed-off-by: Tejun Heo <[email protected]> Cc: Greg Kroah-Hartman <[email protected]> Cc: Alan Stern <[email protected]> Cc: [email protected] --- drivers/usb/core/hub.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -1040,7 +1040,7 @@ static void hub_activate(struct usb_hub */ if (type == HUB_INIT) { delay = hub_power_on(hub, false); - PREPARE_DELAYED_WORK(&hub->init_work, hub_init_func2); + INIT_DELAYED_WORK(&hub->init_work, hub_init_func2); schedule_delayed_work(&hub->init_work, msecs_to_jiffies(delay)); @@ -1194,7 +1194,7 @@ static void hub_activate(struct usb_hub /* Don't do a long sleep inside a workqueue routine */ if (type == HUB_INIT2) { - PREPARE_DELAYED_WORK(&hub->init_work, hub_init_func3); + INIT_DELAYED_WORK(&hub->init_work, hub_init_func3); schedule_delayed_work(&hub->init_work, msecs_to_jiffies(delay)); return; /* Continues at init3: below */This should work okay. But while you're making these changes, you should remove the INIT_DELAYED_WORK(&hub->init_work, NULL) call in hub_probe(). It is now unnecessary. Is the cancel_delayed_work_sync(&hub->init_work) call in hub_quiesce() going to get confused by all this? It's worth mentioning that the only reason for the hub_init_func3 stuff is, as the comment says, to avoid a long sleep (100 ms) inside a work routine.
If a running hub init does not need to be single-threaded wrt a different running hub init, then a single init work could be queued to the system_unbound_wq which doesn't care about running times.
With all the changes to the work queue infrastructure, maybe this doesn't matter so much any more. If we got rid of it then there wouldn't be any multiplexing, and this whole issue would become moot.
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
