Re: musb RPM sleep-while-atomic in 4.9-rc1

[Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>]

* Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> [161027 11:46]:
> On Thu, Oct 27, 2016 at 10:40:17AM -0700, Tony Lindgren wrote:
> > And that one I must have hosed when cleaning up, thanks for noticing
> > these. Updated patch below.
> 
> I had a couple of inline comments to the previous version about locking
> in the gadget code as well (hidden after too much context). Looks like
> there's a lock missing for the deferred work, and something that seems
> like a possible ABBA deadlock.

Oh sorry, I totally missed those.

> > diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
> > --- a/drivers/usb/musb/musb_gadget.c
> > +++ b/drivers/usb/musb/musb_gadget.c
> > @@ -1222,6 +1222,13 @@ void musb_ep_restart(struct musb *musb, struct musb_request *req)
> >  		rxstate(musb, req);
> >  }
> >  
> > +void musb_ep_restart_resume_work(struct musb *musb, void *data)
> > +{
> > +	struct musb_request *req = data;
> > +
> > +	musb_ep_restart(musb, req);
> 
> This one is supposed to be called with musb->lock held (according to the
> function header anyway).

Good point, yeah that calls the monster functions txstate and rxstate.

> >  static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
> >  			gfp_t gfp_flags)
> >  {
> > @@ -1255,7 +1262,7 @@ static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
> >  
> >  	map_dma_buffer(request, musb, musb_ep);
> >  
> > -	pm_runtime_get_sync(musb->controller);
> > +	pm_runtime_get(musb->controller);
> >  	spin_lock_irqsave(&musb->lock, lockflags);
> >  
> >  	/* don't queue if the ep is down */
> > @@ -1271,8 +1278,13 @@ static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
> >  	list_add_tail(&request->list, &musb_ep->req_list);
> >  
> >  	/* it this is the head of the queue, start i/o ... */
> > -	if (!musb_ep->busy && &request->list == musb_ep->req_list.next)
> > -		musb_ep_restart(musb, request);
> > +	if (!musb_ep->busy && &request->list == musb_ep->req_list.next) {
> > +		if (pm_runtime_active(musb->controller))
> > +			musb_ep_restart(musb, request);
> > +		else
> > +			musb_queue_on_resume(musb, musb_ep_restart_resume_work,
> > +					     request);
> > +	}
> 
> But then this looks like it could trigger an ABBA deadlock as musb->lock
> is held while queue_on_resume() takes musb->list_lock, and
> musb_run_pending() would take the same locks in the reverse order.

It seems we can avoid that by locking only list_add_tail() and list_del():

list_for_each_entry_safe(w, _w, &musb->resume_work, node) {
	spin_lock_irqsave(&musb->list_lock, flags);
	list_del(&w->node);
	spin_unlock_irqrestore(&musb->list_lock, flags);
	if (w->callback)
		w->callback(musb, w->data);
	devm_kfree(musb->controller, w);
}

Or do you have some better ideas?

Regards,

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