On Fri, 15 May 2015, Petr Mladek wrote:
> On Wed 2015-04-29 13:33:59, Alan Stern wrote:
> > On Wed, 29 Apr 2015, Petr Mladek wrote:
> >
> > > On Wed 2015-04-29 09:18:50, Chris Bainbridge wrote:
> > > > Regression in commit 638139eb95d2d241781330a321e88c8dafe46078
> > > > Author: Petr Mladek <pmladek@xxxxxxx>
> > > > Date: Fri Sep 19 17:32:24 2014 +0200
> > > >
> > > > usb: hub: allow to process more usb hub events in parallel
> > >
> > > Anyway, I would suggest to revert this commit. I wonder where I had my
> > > head when I sent it.
> > >
> > > It seems that the other operations with udev->portnum and
> > > bus->devnum_next are not guared by bus->usb_address0_mutex.
> > > And it seems that this is not the only operation that need to
> > > be serialized.
> >
> > What other operations are you thinking of? In general, drivers should
> > not depend on hub operations being serialized. If they do, it's a bug
> > and it should be fixed.
>
> I am sorry for the late reply. I have been sick last two weeks.
>
> I think that my primary concern was about
>
> + release_devnum()
> + update_devnum()
>
> They are called from
>
> + usb_disconnect()
> + hub_port_connect()
> + hub_port_finish_reset()
> + hub_set_address()
> + hub_port_init()
>
> They modify udev->devnum but they do not take the
> bus->usb_address0_mutex. Therefore I think that my commit
> 638139eb95d2d2417813 ("usb: hub: allow to process more usb hub events
> in parallel") is not sufficient because it protects only
> choose_devnum() calls by that bus->usb_address0_mutex.
udev->devnum doesn't need to be guarded by bus->usb_address0_mutex. In
every place where it is changed, either udev is private to the current
thread (the pointer hasn't been stored in any globally accessible
location yet) or else the caller holds udev->dev.mutex. See the
comment just above the hub_port_init() routine.
The reason choose_devnum() locks bus->usb_address0_mutex is because
that is where bus->devnum_next gets altered. In other words, the
usb_address0_mutex guards bus->devnum_next. Not udev->devnum.
Remember, locks protect pieces of data -- not regions of code.
> Sigh, the more I stare into the code the less I am sure about a proper
> fix.
I agree that the code and the locking are a bit of a mess. But I think
they are pretty much correct at this point.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
