On Thu, Nov 10, 2016 at 12:40:03PM -0700, Tony Lindgren wrote:
> * Johan Hovold <johan@xxxxxxxxxx> [161110 11:43]:
> > On Thu, Nov 10, 2016 at 10:41:50AM -0700, Tony Lindgren wrote:
> > > * Johan Hovold <johan@xxxxxxxxxx> [161110 09:04]:
> > > > I'm afraid that won't work as pm_runtime_get() would still succeed (i.e.
> > > > even after musb_suspend()).
> > > >
> > > > See 6f3c77b040fc ("PM / Runtime: let rpm_resume() succeed if RPM_ACTIVE,
> > > > even when disabled, v2").
> > >
> > > But doesn't that assume that we have musb core as in musb->controller
> > > in RPM_ACTIVE state? While if it's been suspended that's not the
> > > case meaning rpm_resume would fail?
> >
> > Right, and it's still a good idea to check the return value of
> > pm_runtime_get(). It just won't be enough for when RPM_ACTIVE.
> >
> > > If we have a window for a race there with RPM_ACTIVE set, we could
> > > add musb->is_disabled flag that gets set in musb_suspend().
> >
> > Yes.
> >
> > > > > In the long run it would be nice to make whatever optional state polling
> > > > > musb generic with just a glue layer callback.
> > > >
> > > > Yes, and make sure to stop polling in musb_suspend(). Would it be
> > > > possible to use the enable and disable ops for this until then?
> > >
> > > Hmm care to explain a bit more? That is assuming that rpm_resume()
> > > won't fail above.. And that using musb->is_disabled flag won't
> > > work..
> >
> > By stopping the timer in musb->ops->disable which is called during
> > suspend, the race could perhaps also be avoided.
>
> Yes I think that's the way to go here :) Updated version below
> again.
>
> Regards,
>
> Tony
>
> 8< --------------------------
> From tony Mon Sep 17 00:00:00 2001
> From: Tony Lindgren <tony@xxxxxxxxxxx>
> Date: Wed, 2 Nov 2016 19:59:05 -0700
> Subject: [PATCH] usb: musb: Fix sleeping function called from invalid
> context for hdrc glue
>
> Commit 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS
> glue layer") wrongly added a call for pm_runtime_get_sync to otg_timer
> that runs in softirq context. That causes a "BUG: sleeping function called
> from invalid context" every time when polling the cable status:
>
> [<c015ebb4>] (__might_sleep) from [<c0413d60>] (__pm_runtime_resume+0x9c/0xa0)
> [<c0413d60>] (__pm_runtime_resume) from [<c04d0bc4>] (otg_timer+0x3c/0x254)
> [<c04d0bc4>] (otg_timer) from [<c0191180>] (call_timer_fn+0xfc/0x41c)
> [<c0191180>] (call_timer_fn) from [<c01915c0>] (expire_timers+0x120/0x210)
> [<c01915c0>] (expire_timers) from [<c0191acc>] (run_timer_softirq+0xa4/0xdc)
> [<c0191acc>] (run_timer_softirq) from [<c010168c>] (__do_softirq+0x12c/0x594)
>
> I did not notice that as I did not have CONFIG_DEBUG_ATOMIC_SLEEP enabled.
> And looks like also musb_gadget_queue() suffers from the same problem.
>
> Let's fix the issue by using a list of delayed work then call it on
> resume. Note that we want to do this only when musb core and it's
> parent devices are awake, and we need to make sure the DSPS glue
> timer is stopped as noted by Johan Hovold <johan@xxxxxxxxxx>.
>
> Later on we may be able to remove other delayed work in the musb driver
> and just do it from pending_resume_work. But this should be done only
> for delayed work that does not have other timing requirements beyond
> just being run on resume.
>
> Fixes: 65b3f50ed6fa ("usb: musb: Add PM runtime support for MUSB DSPS
> glue layer")
> Reported-by: Johan Hovold <johan@xxxxxxxxxx>
> Signed-off-by: Tony Lindgren <tony@xxxxxxxxxxx>
> ---
> drivers/usb/musb/musb_core.c | 110 +++++++++++++++++++++++++++++++++++++++--
> drivers/usb/musb/musb_core.h | 7 +++
> drivers/usb/musb/musb_dsps.c | 35 +++++++++----
> drivers/usb/musb/musb_gadget.c | 32 ++++++++++--
> 4 files changed, 166 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> --- a/drivers/usb/musb/musb_core.c
> +++ b/drivers/usb/musb/musb_core.c
> @@ -1969,6 +1969,7 @@ static struct musb *allocate_instance(struct device *dev,
> INIT_LIST_HEAD(&musb->control);
> INIT_LIST_HEAD(&musb->in_bulk);
> INIT_LIST_HEAD(&musb->out_bulk);
> + INIT_LIST_HEAD(&musb->pending_list);
>
> musb->vbuserr_retry = VBUSERR_RETRY_COUNT;
> musb->a_wait_bcon = OTG_TIME_A_WAIT_BCON;
> @@ -2018,6 +2019,85 @@ static void musb_free(struct musb *musb)
> musb_host_free(musb);
> }
>
> +struct musb_pending_work {
> + int (*callback)(struct musb *musb, void *data);
> + void *data;
> + struct list_head node;
> +};
> +
> +/*
> + * Called from musb_runtime_resume(), musb_resume(), and
> + * musb_queue_resume_work(). Callers must take musb->lock and must hold
> + * an RPM reference.
The RPM reference is needed for musb_queue_resume_work() below, not this
one.
> + */
> +static int musb_run_resume_work(struct musb *musb)
> +{
> + struct musb_pending_work *w, *_w;
> + unsigned long flags;
> + int error = 0;
> +
> + spin_lock_irqsave(&musb->list_lock, flags);
> + list_for_each_entry_safe(w, _w, &musb->pending_list, node) {
> + if (w->callback) {
> + error = w->callback(musb, w->data);
> + if (error < 0) {
> + dev_err(musb->controller,
> + "resume callback %p failed: %i\n",
> + w->callback, error);
> + }
> + }
> + list_del(&w->node);
> + devm_kfree(musb->controller, w);
> + }
> + spin_unlock_irqrestore(&musb->list_lock, flags);
> +
> + return error;
> +}
> +
> +/*
> + * Called to run work if device is active or else queue the work to happen
> + * on resume. Caller must take musb->lock.
> + *
> + * Note that we cowardly refuse queuing work after musb PM runtime
> + * resume is done calling musb_run_resume_work() and return -EINPROGRESS
> + * instead.
> + */
> +int musb_queue_resume_work(struct musb *musb,
> + int (*callback)(struct musb *musb, void *data),
> + void *data)
> +{
> + struct musb_pending_work *w;
> + unsigned long flags;
> + int error;
> +
> + if (WARN_ON(!callback))
> + return -EINVAL;
> +
> + if (pm_runtime_active(musb->controller))
> + return callback(musb, data);
> +
> + w = devm_kzalloc(musb->controller, sizeof(*w), GFP_ATOMIC);
> + if (!w)
> + return -ENOMEM;
> +
> + w->callback = callback;
> + w->data = data;
> + spin_lock_irqsave(&musb->list_lock, flags);
> + if (musb->is_runtime_suspended) {
> + list_add_tail(&w->node, &musb->pending_list);
> + error = 0;
> + } else {
> + dev_err(musb->controller, "could not add resume work %p\n",
> + callback);
> + devm_kfree(musb->controller, w);
> + error = -EINPROGRESS;
> + }
> + spin_unlock_irqrestore(&musb->list_lock, flags);
> +
> + return error;
> +}
> +EXPORT_SYMBOL_GPL(musb_queue_resume_work);
> +
> static void musb_deassert_reset(struct work_struct *work)
> {
> struct musb *musb;
> diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
> --- a/drivers/usb/musb/musb_dsps.c
> +++ b/drivers/usb/musb/musb_dsps.c
> @@ -185,24 +185,19 @@ static void dsps_musb_disable(struct musb *musb)
> musb_writel(reg_base, wrp->coreintr_clear, wrp->usb_bitmap);
> musb_writel(reg_base, wrp->epintr_clear,
> wrp->txep_bitmap | wrp->rxep_bitmap);
> + del_timer_sync(&glue->timer);
Don't you want to move starting the timer to dsps_musb_enable() as well?
> musb_writeb(musb->mregs, MUSB_DEVCTL, 0);
> }
>
> -static void otg_timer(unsigned long _musb)
> +/* Caller must take musb->lock */
> +static int dsps_check_status(struct musb *musb, void *unused)
> {
> - struct musb *musb = (void *)_musb;
> void __iomem *mregs = musb->mregs;
> struct device *dev = musb->controller;
> struct dsps_glue *glue = dev_get_drvdata(dev->parent);
> const struct dsps_musb_wrapper *wrp = glue->wrp;
> u8 devctl;
> - unsigned long flags;
> int skip_session = 0;
> - int err;
> -
> - err = pm_runtime_get_sync(dev);
> - if (err < 0)
> - dev_err(dev, "Poll could not pm_runtime_get: %i\n", err);
>
> /*
> * We poll because DSPS IP's won't expose several OTG-critical
> @@ -212,7 +207,6 @@ static void otg_timer(unsigned long _musb)
> dev_dbg(musb->controller, "Poll devctl %02x (%s)\n", devctl,
> usb_otg_state_string(musb->xceiv->otg->state));
>
> - spin_lock_irqsave(&musb->lock, flags);
> switch (musb->xceiv->otg->state) {
> case OTG_STATE_A_WAIT_VRISE:
> mod_timer(&glue->timer, jiffies +
> @@ -245,8 +239,29 @@ static void otg_timer(unsigned long _musb)
> default:
> break;
> }
> - spin_unlock_irqrestore(&musb->lock, flags);
>
> + return 0;
> +}
> +
> +static void otg_timer(unsigned long _musb)
> +{
> + struct musb *musb = (void *)_musb;
> + struct device *dev = musb->controller;
> + unsigned long flags;
> + int err;
> +
> + err = pm_runtime_get(dev);
> + if ((err != -EINPROGRESS) && err < 0) {
> + dev_err(dev, "Poll could not pm_runtime_get: %i\n", err);
> +
Add pm_runtime_put_noidle() to balance the counter also on errors.
> + return;
> + }
> +
> + spin_lock_irqsave(&musb->lock, flags);
> + err = musb_queue_resume_work(musb, dsps_check_status, NULL);
> + if (err < 0)
> + dev_err(dev, "%s resume work: %i\n", __func__, err);
> + spin_unlock_irqrestore(&musb->lock, flags);
> pm_runtime_mark_last_busy(dev);
> pm_runtime_put_autosuspend(dev);
> }
> 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,13 +1222,22 @@ void musb_ep_restart(struct musb *musb, struct musb_request *req)
> rxstate(musb, req);
> }
>
> +static int musb_ep_restart_resume_work(struct musb *musb, void *data)
> +{
> + struct musb_request *req = data;
> +
> + musb_ep_restart(musb, req);
> +
> + return 0;
> +}
> +
> static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
> gfp_t gfp_flags)
> {
> struct musb_ep *musb_ep;
> struct musb_request *request;
> struct musb *musb;
> - int status = 0;
> + int status;
> unsigned long lockflags;
>
> if (!ep || !req)
> @@ -1245,6 +1254,16 @@ static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
> if (request->ep != musb_ep)
> return -EINVAL;
>
> + status = pm_runtime_get(musb->controller);
> + if ((status != -EINPROGRESS) && status < 0) {
> + dev_err(musb->controller,
> + "pm runtime get failed in %s\n",
> + __func__);
Missing pm_runtime_put_noidle() here too.
> +
> + return status;
> + }
> + status = 0;
> +
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
