Re: [PATCH v2 5/9] usb: don't use PREPARE_DELAYED_WORK

[Peter Hurley]

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@xxxxxxxxxx>
> > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> > Cc: linux-usb@xxxxxxxxxxxxxxx
> > ---
> >   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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/