[PATCH 0/2] Fixes for two more musb regressions

[PATCH 0/2] Fixes for two more musb regressions

[Tony Lindgren]

Hi all,

Looks like we have two more regressions caused by my attempts
to make musb PM simpler. As it's very close to the merge window
opening, these are against current Linux next.

Once reviewed, tested and merged to the mainline kernel we can
request these to be included also into v4.8.y kernel. Maybe Bin
can tag these as cc stable v4.8 already when applying.

Regards,

Tony

Tony Lindgren (2):
  usb: musb: Fix hardirq-safe hardirq-unsafe lock order error
  usb: musb: Call pm_runtime from musb_gadget_queue

 drivers/usb/musb/musb_gadget.c | 4 ++++
 drivers/usb/musb/omap2430.c    | 7 ++-----
 2 files changed, 6 insertions(+), 5 deletions(-)

-- 
2.9.3
--
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

[PATCH 1/2] usb: musb: Fix hardirq-safe hardirq-unsafe lock order error

[Tony Lindgren]

If we configure musb with 2430 glue as a peripheral, and then rmmod
omap2430 module, we'll get the following error:

[ INFO: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected ]
...
rmmod/413 [HC0[0]:SC0[0]:HE0:SE1] is trying to acquire:
 (&phy->mutex){+.+.+.}, at: [<c04b9fd0>] phy_power_off+0x1c/0xb8
[  204.678710]
               and this task is already holding:
 (&(&musb->lock)->rlock){-.-...}, at: [<bf3a482c>]
 musb_gadget_stop+0x24/0xec [musb_hdrc]
which would create a new lock dependency:
 (&(&musb->lock)->rlock){-.-...} -> (&phy->mutex){+.+.+.}
...

This is because some glue layers expect musb_platform_enable/disable
to be called with spinlock held, and 2430 glue layer has USB PHY on
the I2C bus using a mutex.

We could fix the glue layers to take the spinlock, but we still have
a problem of musb_plaform_enable/disable being called in an unbalanced
manner. So that would still lead into USB PHY enable/disable related
problems for omap2430 glue layer.

While it makes sense to only enable USB PHY when needed from PM point
of view, in this case we just can't do it yet without breaking things.
So let's just revert phy_enable/disable related changes instead and
reconsider this after we have fixed musb_platform_enable/disable to
be balanced.

Fixes: a83e17d0f73b ("usb: musb: Improve PM runtime and phy handling
for 2430 glue layer")
Signed-off-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
---
 drivers/usb/musb/omap2430.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
--- a/drivers/usb/musb/omap2430.c
+++ b/drivers/usb/musb/omap2430.c
@@ -287,6 +287,7 @@ static int omap2430_musb_init(struct musb *musb)
 	}
 	musb->isr = omap2430_musb_interrupt;
 	phy_init(musb->phy);
+	phy_power_on(musb->phy);
 
 	l = musb_readl(musb->mregs, OTG_INTERFSEL);
 
@@ -323,8 +324,6 @@ static void omap2430_musb_enable(struct musb *musb)
 	struct musb_hdrc_platform_data *pdata = dev_get_platdata(dev);
 	struct omap_musb_board_data *data = pdata->board_data;
 
-	if (!WARN_ON(!musb->phy))
-		phy_power_on(musb->phy);
 
 	switch (glue->status) {
 
@@ -361,9 +360,6 @@ static void omap2430_musb_disable(struct musb *musb)
 	struct device *dev = musb->controller;
 	struct omap2430_glue *glue = dev_get_drvdata(dev->parent);
 
-	if (!WARN_ON(!musb->phy))
-		phy_power_off(musb->phy);

Re: [PATCH 1/2] usb: musb: Fix hardirq-safe hardirq-unsafe lock order error

[Laurent Pinchart]

Hi Tony,

Thank you for the patch.

On Friday 30 Sep 2016 11:10:09 Tony Lindgren wrote:
> If we configure musb with 2430 glue as a peripheral, and then rmmod
> omap2430 module, we'll get the following error:
> 
> [ INFO: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected ]
> ...
> rmmod/413 [HC0[0]:SC0[0]:HE0:SE1] is trying to acquire:
>  (&phy->mutex){+.+.+.}, at: [<c04b9fd0>] phy_power_off+0x1c/0xb8
> [  204.678710]
>                and this task is already holding:
>  (&(&musb->lock)->rlock){-.-...}, at: [<bf3a482c>]
>  musb_gadget_stop+0x24/0xec [musb_hdrc]
> which would create a new lock dependency:
>  (&(&musb->lock)->rlock){-.-...} -> (&phy->mutex){+.+.+.}
> ...
> 
> This is because some glue layers expect musb_platform_enable/disable
> to be called with spinlock held, and 2430 glue layer has USB PHY on
> the I2C bus using a mutex.
> 
> We could fix the glue layers to take the spinlock, but we still have
> a problem of musb_plaform_enable/disable being called in an unbalanced
> manner. So that would still lead into USB PHY enable/disable related
> problems for omap2430 glue layer.
> 
> While it makes sense to only enable USB PHY when needed from PM point
> of view, in this case we just can't do it yet without breaking things.
> So let's just revert phy_enable/disable related changes instead and
> reconsider this after we have fixed musb_platform_enable/disable to
> be balanced.
> 
> Fixes: a83e17d0f73b ("usb: musb: Improve PM runtime and phy handling
> for 2430 glue layer")
> Signed-off-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>

> ---
>  drivers/usb/musb/omap2430.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
> --- a/drivers/usb/musb/omap2430.c
> +++ b/drivers/usb/musb/omap2430.c
> @@ -287,6 +287,7 @@ static int omap2430_musb_init(struct musb *musb)
>  	}
>  	musb->isr = omap2430_musb_interrupt;
>  	phy_init(musb->phy);
> +	phy_power_on(musb->phy);
> 
>  	l = musb_readl(musb->mregs, OTG_INTERFSEL);
> 
> @@ -323,8 +324,6 @@ static void omap2430_musb_enable(struct musb *musb)
>  	struct musb_hdrc_platform_data *pdata = dev_get_platdata(dev);
>  	struct omap_musb_board_data *data = pdata->board_data;
> 
> -	if (!WARN_ON(!musb->phy))
> -		phy_power_on(musb->phy);
> 
>  	switch (glue->status) {
> 
> @@ -361,9 +360,6 @@ static void omap2430_musb_disable(struct musb *musb)
>  	struct device *dev = musb->controller;
>  	struct omap2430_glue *glue = dev_get_drvdata(dev->parent);
> 
> -	if (!WARN_ON(!musb->phy))
> -		phy_power_off(musb->phy);
> -
>  	if (glue->status != MUSB_UNKNOWN)
>  		omap_control_usb_set_mode(glue->control_otghs,
>  			USB_MODE_DISCONNECT);
> @@ -375,6 +371,7 @@ static int omap2430_musb_exit(struct musb *musb)
>  	struct omap2430_glue *glue = dev_get_drvdata(dev->parent);
> 
>  	omap2430_low_level_exit(musb);
> +	phy_power_off(musb->phy);
>  	phy_exit(musb->phy);
>  	musb->phy = NULL;
>  	cancel_work_sync(&glue->omap_musb_mailbox_work);

-- 
Regards,

Laurent Pinchart

--
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

[PATCH 2/2] usb: musb: Call pm_runtime from musb_gadget_queue

[Tony Lindgren]

If we're booting pandaboard using NFSroot over built-in g_ether, we
can get the following after booting once and doing a warm reset:

g_ether gadget: ecm_open
g_ether gadget: notify connect true
...
WARNING: CPU: 0 PID: 1 at drivers/bus/omap_l3_noc.c:147
l3_interrupt_handler+0x220/0x34c
44000000.ocp:L3 Custom Error: MASTER MPU TARGET L4CFG (Read):
Data Access in User mode du ring Functional access
...

Fix the issue by calling pm_runtime functions from
musb_gadget_queue.

Note that in the long run we should be able to queue the pending
transfers if pm_runtime is not active, and flush the queue from
pm_runtime_resume.

Reported-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
Tested-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
Signed-off-by: Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org>
---
 drivers/usb/musb/musb_gadget.c | 4 ++++
 1 file changed, 4 insertions(+)

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
@@ -1255,6 +1255,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);
 	spin_lock_irqsave(&musb->lock, lockflags);
 
 	/* don't queue if the ep is down */
@@ -1275,6 +1276,9 @@ static int musb_gadget_queue(struct usb_ep *ep, struct usb_request *req,
 
 unlock:
 	spin_unlock_irqrestore(&musb->lock, lockflags);
+	pm_runtime_mark_last_busy(musb->controller);
+	pm_runtime_put_autosuspend(musb->controller);
+
 	return status;
 }
 
-- 
2.9.3
--
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

Re: [PATCH 0/2] Fixes for two more musb regressions

[Bin Liu]

Hi,

On Fri, Sep 30, 2016 at 11:10:08AM -0700, Tony Lindgren wrote:
> Hi all,
> 
> Looks like we have two more regressions caused by my attempts
> to make musb PM simpler. As it's very close to the merge window
> opening, these are against current Linux next.
> 
> Once reviewed, tested and merged to the mainline kernel we can
> request these to be included also into v4.8.y kernel. Maybe Bin
> can tag these as cc stable v4.8 already when applying.
> 
> Regards,
> 
> Tony
> 
> Tony Lindgren (2):
>   usb: musb: Fix hardirq-safe hardirq-unsafe lock order error
>   usb: musb: Call pm_runtime from musb_gadget_queue

Applied.

Regards,
-Bin.

> 
>  drivers/usb/musb/musb_gadget.c | 4 ++++
>  drivers/usb/musb/omap2430.c    | 7 ++-----
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> -- 
> 2.9.3
--
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