Re: [PATCH v2 4/4] usb: hub: rename khubd to hub_wq in documentation and comments

["Petr Mládek" <pmladek@suse.cz>]

On Thu 18-09-14 10:24:23, Alan Stern wrote:
> On Thu, 18 Sep 2014, Tejun Heo wrote:
> 
> > Hello, Alan, Petr.
> > 
> > On Wed, Sep 17, 2014 at 01:36:26PM -0400, Alan Stern wrote:
> > > > -	/* If khubd ever becomes multithreaded, this will need a lock */
> > > > +	/* If hub_wq ever becomes multithreaded, this will need a lock */
> > > >  	if (udev->wusb) {
> > > >  		devnum = udev->portnum + 1;
> > > >  		BUG_ON(test_bit(devnum, bus->devmap.devicemap));
> > > 
> > > You probably didn't notice when changing this comment.  But in fact,
> > > workqueues _are_ multithreaded.  Therefore you need to add a lock to 
> > > this routine.
> 
> > Haven't read the code but if this function is called from a single
> > work_struct, workqueue guarantees that there's only single thread of
> > execution at any given time.  A work item is never executed
> > concurrently no matter what.
> 
> This routine can be called from multiple work_structs, because a USB 
> bus can have multiple hubs.

The easiest solution would be to allocate the work queue with
the flag WQ_UNBOUND and max_active = 1. It will force serialization
of all work items.

Alternatively, we might add the locking. But to be honest, I am not
sure that I am brave enough to do so.


First, I am not sure if this is the only location that might be
affected by the parallel execution of hub_event().

Second, there are used so many locks and the code is so complex that I
would need many days and maybe weeks to understand it.


Well, if we assume that this is the only problematic location, here
are the ideas how to prevent the parallel execution:

1. Use some brand new lock, e.g. call it hub_devnum_lock, and do:

   static void choose_devnum(struct usb_device *udev)
   {
	spin_lock_irq(&hub_devnum_lock);
	[...]
	spin_unlock_irq(&hub_event_lock);
   }

   This looks clean but it creates another lock.


2. Alternatively, we could use an existing global lock the same way,
   for example usb_bus_list_lock.

   But this looks like a hack and I do not like it much.


3. Alternatively, it seems the the function affects one
   "struct usb_device" and one "struct usb_bus". It might
   be enough to take the appropriate locks for these
   structures.

   This would mean to take two locks. It would be slower
   and we would need to make sure that it does not cause
   a dead lock.


Best Regards,
Petr