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

2014-02-22 Thread Tejun Heo 
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 t...@kernel.org
Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
Cc: Alan Stern st...@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 */
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-02-22 Thread Alan Stern 
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 t...@kernel.org
 Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
 Cc: Alan Stern st...@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.  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.

Alan Stern

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-02-22 Thread 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 t...@kernel.org
Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
Cc: Alan Stern st...@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.



--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-02-22 Thread Alan Stern 
On Sat, 22 Feb 2014, Tejun Heo wrote:

 On Sat, Feb 22, 2014 at 10:14:48AM -0500, Alan Stern wrote:
  Is the cancel_delayed_work_sync(hub-init_work) call in hub_quiesce()
  going to get confused by all this?
 
 Yeah, you can't cancel a work item which hasn't been initialzed.
 Maybe move init of the first work function there?  I don't think it
 really matters tho.
 
  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.  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.
 
 I don't really think that'd be necessary.  Just sleeping synchronously
 should be fine.  How many threads are we talking about?

One thread per hub (no more than 10 on a typical system).  The code in
question is part of the hub driver's probe sequence.


On Sat, 22 Feb 2014, Peter Hurley wrote:

 If a running hub init does not need to be single-threaded wrt
 a different running hub init,

I'm not quite sure what that means, but the hub init threads are indeed
independent of each other.

  then a single init work could be queued to
 the system_unbound_wq which doesn't care about running times.

This sort of thing sounds like the best approach.  Tejun, do you want
to rewrite the patch, getting rid of the hub_init_func3 and HUB_INIT3
business entirely?  Or would you like me to do it?

Alan Stern

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2014-02-22 Thread Tejun Heo 
Hey, Alan.

On Sat, Feb 22, 2014 at 06:03:04PM -0500, Alan Stern wrote:
   then a single init work could be queued to
  the system_unbound_wq which doesn't care about running times.
 
 This sort of thing sounds like the best approach.  Tejun, do you want
 to rewrite the patch, getting rid of the hub_init_func3 and HUB_INIT3
 business entirely?  Or would you like me to do it?

I'll doing the minimal patch in this series and then following up with
the update is probably better.  I can put the patches in a separate
branch so that it can be easily pulled.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html