lkml.org 
[lkml]   [2014]   [Feb]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 5/9] usb: don't use PREPARE_DELAYED_WORK
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 <tj@kernel.org>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Alan Stern <stern@rowland.harvard.edu>
>> Cc: linux-usb@vger.kernel.org
>> ---
>> 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.




\
 
 \ /
  Last update: 2014-02-22 17:01    [W:0.377 / U:1.132 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site