[PATCH] usb: chipidea: host: Disable port power only if previously enabled

[PATCH] usb: chipidea: host: Disable port power only if previously enabled

[Guenter Roeck]

On shutdown, ehci_power_off() is called unconditionally to power off
each port, even if it was never called to power on the port.
For chipidea, this results in a call to ehci_ci_portpower() with a request
to power off ports even if the port was never powered on.
This results in the following warning from the regulator code.

WARNING: CPU: 0 PID: 182 at drivers/regulator/core.c:2596 _regulator_disable+0x1a8/0x210
unbalanced disables for usb_otg2_vbus
Modules linked in:
CPU: 0 PID: 182 Comm: init Not tainted 5.4.6 #1
Hardware name: Freescale i.MX7 Dual (Device Tree)
[<c0313658>] (unwind_backtrace) from [<c030d698>] (show_stack+0x10/0x14)
[<c030d698>] (show_stack) from [<c1133afc>] (dump_stack+0xe0/0x10c)
[<c1133afc>] (dump_stack) from [<c0349098>] (__warn+0xf4/0x10c)
[<c0349098>] (__warn) from [<c0349128>] (warn_slowpath_fmt+0x78/0xbc)
[<c0349128>] (warn_slowpath_fmt) from [<c09f36ac>] (_regulator_disable+0x1a8/0x210)
[<c09f36ac>] (_regulator_disable) from [<c09f374c>] (regulator_disable+0x38/0xe8)
[<c09f374c>] (regulator_disable) from [<c0df7bac>] (ehci_ci_portpower+0x38/0xdc)
[<c0df7bac>] (ehci_ci_portpower) from [<c0db4fa4>] (ehci_port_power+0x50/0xa4)
[<c0db4fa4>] (ehci_port_power) from [<c0db5420>] (ehci_silence_controller+0x5c/0xc4)
[<c0db5420>] (ehci_silence_controller) from [<c0db7644>] (ehci_stop+0x3c/0xcc)
[<c0db7644>] (ehci_stop) from [<c0d5bdc4>] (usb_remove_hcd+0xe0/0x19c)
[<c0d5bdc4>] (usb_remove_hcd) from [<c0df7638>] (host_stop+0x38/0xa8)
[<c0df7638>] (host_stop) from [<c0df2f34>] (ci_hdrc_remove+0x44/0xe4)
...

Keeping track of the power enable state avoids the warning and traceback.

Fixes: c8679a2fb8dec ("usb: chipidea: host: add portpower override")
Cc: Michael Grzeschik <m.grzeschik@pengutronix.de>
Cc: Peter Chen <peter.chen@freescale.com>
Cc: stable@vger.kernel.org
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/usb/chipidea/host.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
index b45ceb91c735..48e4a5ca1835 100644
--- a/drivers/usb/chipidea/host.c
+++ b/drivers/usb/chipidea/host.c
@@ -26,6 +26,7 @@ static int (*orig_bus_suspend)(struct usb_hcd *hcd);
 
 struct ehci_ci_priv {
 	struct regulator *reg_vbus;
+	bool enabled;
 };
 
 static int ehci_ci_portpower(struct usb_hcd *hcd, int portnum, bool enable)
@@ -37,7 +38,7 @@ static int ehci_ci_portpower(struct usb_hcd *hcd, int portnum, bool enable)
 	int ret = 0;
 	int port = HCS_N_PORTS(ehci->hcs_params);
 
-	if (priv->reg_vbus) {
+	if (priv->reg_vbus && enable != priv->enabled) {
 		if (port > 1) {
 			dev_warn(dev,
 				"Not support multi-port regulator control\n");
@@ -53,6 +54,7 @@ static int ehci_ci_portpower(struct usb_hcd *hcd, int portnum, bool enable)
 				enable ? "enable" : "disable", ret);
 			return ret;
 		}
+		priv->enabled = enable;
 	}
 
 	if (enable && (ci->platdata->phy_mode == USBPHY_INTERFACE_MODE_HSIC)) {
-- 
2.17.1

Re: [PATCH] usb: chipidea: host: Disable port power only if previously enabled

[Alan Stern]

On Thu, 26 Dec 2019, Guenter Roeck wrote:

> On shutdown, ehci_power_off() is called unconditionally to power off
> each port, even if it was never called to power on the port.
> For chipidea, this results in a call to ehci_ci_portpower() with a request
> to power off ports even if the port was never powered on.
> This results in the following warning from the regulator code.

That's weird -- we should always power-on every port during hub 
initialization.

It looks like there's a bug in hub.c:hub_activate(): The line under
HUB_INIT which calls hub_power_on() should call
usb_hub_set_port_power() instead.  In fact, the comment near the start
of hub_power_on() is wrong.  It says "Enable power on each port", but
in fact it only enables power for ports that had been powered-on
previously (i.e., the port's bit in hub->power_bits was set).  
Apparently this got messed up in commit ad493e5e5805 ("usb: add usb
port auto power off mechanism").

Now, the chipidea driver may still need to be updated, because 
ehci_turn_off_all_ports() will still be called during shutdown and it 
will try to power-down all ports, even those which are already powered 
down (for example, because the port is suspended).

Alan Stern

Re: [PATCH] usb: chipidea: host: Disable port power only if previously enabled

[Peter Chen]

On 19-12-26 14:46:15, Alan Stern wrote:
> On Thu, 26 Dec 2019, Guenter Roeck wrote:
> 
> > On shutdown, ehci_power_off() is called unconditionally to power off
> > each port, even if it was never called to power on the port.
> > For chipidea, this results in a call to ehci_ci_portpower() with a request
> > to power off ports even if the port was never powered on.
> > This results in the following warning from the regulator code.
> 
> That's weird -- we should always power-on every port during hub 
> initialization.
> 
> It looks like there's a bug in hub.c:hub_activate(): The line under
> HUB_INIT which calls hub_power_on() should call
> usb_hub_set_port_power() instead.  In fact, the comment near the start
> of hub_power_on() is wrong.  It says "Enable power on each port", but
> in fact it only enables power for ports that had been powered-on
> previously (i.e., the port's bit in hub->power_bits was set).  
> Apparently this got messed up in commit ad493e5e5805 ("usb: add usb
> port auto power off mechanism").
> 
> Now, the chipidea driver may still need to be updated, because 
> ehci_turn_off_all_ports() will still be called during shutdown and it 
> will try to power-down all ports, even those which are already powered 
> down (for example, because the port is suspended).
> 

Hi Alan,

When the port is suspended, why it was turned off? If it was turned
off, how could respond wakeup event?

-- 

Thanks,
Peter Chen

Re: [PATCH] usb: chipidea: host: Disable port power only if previously enabled

[Alan Stern]

On Fri, 27 Dec 2019, Peter Chen wrote:

> On 19-12-26 14:46:15, Alan Stern wrote:
> > On Thu, 26 Dec 2019, Guenter Roeck wrote:
> > 
> > > On shutdown, ehci_power_off() is called unconditionally to power off
> > > each port, even if it was never called to power on the port.
> > > For chipidea, this results in a call to ehci_ci_portpower() with a request
> > > to power off ports even if the port was never powered on.
> > > This results in the following warning from the regulator code.
> > 
> > That's weird -- we should always power-on every port during hub 
> > initialization.
> > 
> > It looks like there's a bug in hub.c:hub_activate(): The line under
> > HUB_INIT which calls hub_power_on() should call
> > usb_hub_set_port_power() instead.  In fact, the comment near the start
> > of hub_power_on() is wrong.  It says "Enable power on each port", but
> > in fact it only enables power for ports that had been powered-on
> > previously (i.e., the port's bit in hub->power_bits was set).  
> > Apparently this got messed up in commit ad493e5e5805 ("usb: add usb
> > port auto power off mechanism").
> > 
> > Now, the chipidea driver may still need to be updated, because 
> > ehci_turn_off_all_ports() will still be called during shutdown and it 
> > will try to power-down all ports, even those which are already powered 
> > down (for example, because the port is suspended).
> > 
> 
> Hi Alan,
> 
> When the port is suspended, why it was turned off? If it was turned
> off, how could respond wakeup event?

You're right; if a port is powered off then it can't respond to wakeup 
events.

But if the power/wakeup attribute is set to "disabled" then the port 
doesn't need to respond to wakeup events.  In that situation, usbcore 
may decide to power-down the port.

Alan Stern

Re: [PATCH] usb: chipidea: host: Disable port power only if previously enabled

[Guenter Roeck]

On Thu, Dec 26, 2019 at 02:46:15PM -0500, Alan Stern wrote:
> On Thu, 26 Dec 2019, Guenter Roeck wrote:
> 
> > On shutdown, ehci_power_off() is called unconditionally to power off
> > each port, even if it was never called to power on the port.
> > For chipidea, this results in a call to ehci_ci_portpower() with a request
> > to power off ports even if the port was never powered on.
> > This results in the following warning from the regulator code.
> 
> That's weird -- we should always power-on every port during hub 
> initialization.
> 
That is what I would have assumed, but test code shows that it doesn't
happen.

> It looks like there's a bug in hub.c:hub_activate(): The line under
> HUB_INIT which calls hub_power_on() should call
> usb_hub_set_port_power() instead.  In fact, the comment near the start

usb_hub_set_port_power() operates on a port of the hub. hub_activate()
operates on the hub itself, or at least I think it does. I don't know
how to convert the calls. Also, there are more calls to hub_power_on()
in the same function.  Can you provide more details on what to do,
or even better a patch for me to test ?

Thanks,
Guenter

> of hub_power_on() is wrong.  It says "Enable power on each port", but
> in fact it only enables power for ports that had been powered-on
> previously (i.e., the port's bit in hub->power_bits was set).  
> Apparently this got messed up in commit ad493e5e5805 ("usb: add usb
> port auto power off mechanism").
> 
> Now, the chipidea driver may still need to be updated, because 
> ehci_turn_off_all_ports() will still be called during shutdown and it 
> will try to power-down all ports, even those which are already powered 
> down (for example, because the port is suspended).
> 
> Alan Stern
> 

Re: [PATCH] usb: chipidea: host: Disable port power only if previously enabled

[Alan Stern]

On Fri, 27 Dec 2019, Guenter Roeck wrote:

> On Thu, Dec 26, 2019 at 02:46:15PM -0500, Alan Stern wrote:
> > On Thu, 26 Dec 2019, Guenter Roeck wrote:
> > 
> > > On shutdown, ehci_power_off() is called unconditionally to power off
> > > each port, even if it was never called to power on the port.
> > > For chipidea, this results in a call to ehci_ci_portpower() with a request
> > > to power off ports even if the port was never powered on.
> > > This results in the following warning from the regulator code.
> > 
> > That's weird -- we should always power-on every port during hub 
> > initialization.
> > 
> That is what I would have assumed, but test code shows that it doesn't
> happen.
> 
> > It looks like there's a bug in hub.c:hub_activate(): The line under
> > HUB_INIT which calls hub_power_on() should call
> > usb_hub_set_port_power() instead.  In fact, the comment near the start
> 
> usb_hub_set_port_power() operates on a port of the hub. hub_activate()
> operates on the hub itself, or at least I think it does. I don't know
> how to convert the calls. Also, there are more calls to hub_power_on()
> in the same function.  Can you provide more details on what to do,
> or even better a patch for me to test ?

Let's try a slightly different approach.  What happens with this patch?

Alan Stern


Index: usb-devel/drivers/usb/core/hub.c
===================================================================
--- usb-devel.orig/drivers/usb/core/hub.c
+++ usb-devel/drivers/usb/core/hub.c
@@ -1065,6 +1065,7 @@ static void hub_activate(struct usb_hub
 		if (type == HUB_INIT) {
 			delay = hub_power_on_good_delay(hub);
 
+			hub->power_bits[0] = ~0UL;	/* All ports on */
 			hub_power_on(hub, false);
 			INIT_DELAYED_WORK(&hub->init_work, hub_init_func2);
 			queue_delayed_work(system_power_efficient_wq,

Re: [PATCH] usb: chipidea: host: Disable port power only if previously enabled

[Guenter Roeck]

On Sat, Dec 28, 2019 at 02:33:01PM -0500, Alan Stern wrote:
> 
> Let's try a slightly different approach.  What happens with this patch?
> 
> Alan Stern
> 
> 
> Index: usb-devel/drivers/usb/core/hub.c
> ===================================================================
> --- usb-devel.orig/drivers/usb/core/hub.c
> +++ usb-devel/drivers/usb/core/hub.c
> @@ -1065,6 +1065,7 @@ static void hub_activate(struct usb_hub
>  		if (type == HUB_INIT) {
>  			delay = hub_power_on_good_delay(hub);
>  
> +			hub->power_bits[0] = ~0UL;	/* All ports on */
>  			hub_power_on(hub, false);
>  			INIT_DELAYED_WORK(&hub->init_work, hub_init_func2);
>  			queue_delayed_work(system_power_efficient_wq,
> 

That doesn't make a difference - the traceback is still seen with this patch
applied.

Guenter

Re: [PATCH] usb: chipidea: host: Disable port power only if previously enabled

[Alan Stern]

On Sun, 29 Dec 2019, Guenter Roeck wrote:

> On Sat, Dec 28, 2019 at 02:33:01PM -0500, Alan Stern wrote:
> > 
> > Let's try a slightly different approach.  What happens with this patch?
> > 
> > Alan Stern
> > 
> > 
> > Index: usb-devel/drivers/usb/core/hub.c
> > ===================================================================
> > --- usb-devel.orig/drivers/usb/core/hub.c
> > +++ usb-devel/drivers/usb/core/hub.c
> > @@ -1065,6 +1065,7 @@ static void hub_activate(struct usb_hub
> >  		if (type == HUB_INIT) {
> >  			delay = hub_power_on_good_delay(hub);
> >  
> > +			hub->power_bits[0] = ~0UL;	/* All ports on */
> >  			hub_power_on(hub, false);
> >  			INIT_DELAYED_WORK(&hub->init_work, hub_init_func2);
> >  			queue_delayed_work(system_power_efficient_wq,
> > 
> 
> That doesn't make a difference - the traceback is still seen with this patch
> applied.

Can you trace what's going on?  Does this code pathway now end up
calling ehci_port_power() for each root-hub port, and from there down
into the chipidea driver?  If not, can you find where it gets
sidetracked?

Alan Stern

Re: [PATCH] usb: chipidea: host: Disable port power only if previously enabled

[Guenter Roeck]

On Sun, Dec 29, 2019 at 11:40:52AM -0500, Alan Stern wrote:
> On Sun, 29 Dec 2019, Guenter Roeck wrote:
> 
> > On Sat, Dec 28, 2019 at 02:33:01PM -0500, Alan Stern wrote:
> > > 
> > > Let's try a slightly different approach.  What happens with this patch?
> > > 
> > > Alan Stern
> > > 
> > > 
> > > Index: usb-devel/drivers/usb/core/hub.c
> > > ===================================================================
> > > --- usb-devel.orig/drivers/usb/core/hub.c
> > > +++ usb-devel/drivers/usb/core/hub.c
> > > @@ -1065,6 +1065,7 @@ static void hub_activate(struct usb_hub
> > >  		if (type == HUB_INIT) {
> > >  			delay = hub_power_on_good_delay(hub);
> > >  
> > > +			hub->power_bits[0] = ~0UL;	/* All ports on */
> > >  			hub_power_on(hub, false);
> > >  			INIT_DELAYED_WORK(&hub->init_work, hub_init_func2);
> > >  			queue_delayed_work(system_power_efficient_wq,
> > > 
> > 
> > That doesn't make a difference - the traceback is still seen with this patch
> > applied.
> 
> Can you trace what's going on?  Does this code pathway now end up
> calling ehci_port_power() for each root-hub port, and from there down
> into the chipidea driver?  If not, can you find where it gets
> sidetracked?
> 
Sure, I'll do that. It will have to wait for the new year, though -
internet connectivity is terrible where I am right now,

Guenter

Re: [PATCH] usb: chipidea: host: Disable port power only if previously enabled

[Guenter Roeck]

On 12/29/19 8:40 AM, Alan Stern wrote:
> On Sun, 29 Dec 2019, Guenter Roeck wrote:
> 
>> On Sat, Dec 28, 2019 at 02:33:01PM -0500, Alan Stern wrote:
>>>
>>> Let's try a slightly different approach.  What happens with this patch?
>>>
>>> Alan Stern
>>>
>>>
>>> Index: usb-devel/drivers/usb/core/hub.c
>>> ===================================================================
>>> --- usb-devel.orig/drivers/usb/core/hub.c
>>> +++ usb-devel/drivers/usb/core/hub.c
>>> @@ -1065,6 +1065,7 @@ static void hub_activate(struct usb_hub
>>>   		if (type == HUB_INIT) {
>>>   			delay = hub_power_on_good_delay(hub);
>>>   
>>> +			hub->power_bits[0] = ~0UL;	/* All ports on */
>>>   			hub_power_on(hub, false);
>>>   			INIT_DELAYED_WORK(&hub->init_work, hub_init_func2);
>>>   			queue_delayed_work(system_power_efficient_wq,
>>>
>>
>> That doesn't make a difference - the traceback is still seen with this patch
>> applied.
> 
> Can you trace what's going on?  Does this code pathway now end up
> calling ehci_port_power() for each root-hub port, and from there down
> into the chipidea driver?  If not, can you find where it gets
> sidetracked?
> 

A complete traceback is attached below, so, yes, I think it is safe to say that
ehci_port_power() is called unconditionally for each root-hub port on shutdown.

The only mystery to me was why ehci_port_power() isn't called to enable port power
when the port comes up. As it turns out, HCS_PPC(ehci->hcs_params) is false on my
simulated hardware, and thus ehci_port_power(..., true) is not called from
ehci_hub_control().

Given that, it may well be that the problem is not seen on "real" hardware,
at least not with real mcimx7d-sabre hardware, if the hub on that hardware does
support power control. To test this idea, I modified qemu to claim hub power
control support by setting the "power control support" capability bit. With
that, the traceback is gone.

Any suggestion how to proceed ?

Thanks,
Guenter

---
[   31.916567] ------------[ cut here ]------------
[   31.917178] WARNING: CPU: 0 PID: 182 at drivers/regulator/core.c:2598 _regulator_disable+0x1a8/0x210
[   31.917331] unbalanced disables for usb_otg2_vbus
[   31.917468] Modules linked in:
[   31.917877] CPU: 0 PID: 182 Comm: init Not tainted 5.4.8-rc1-00003-gb8e36d27e314 #1
[   31.918032] Hardware name: Freescale i.MX7 Dual (Device Tree)
[   31.918246] [<c0313658>] (unwind_backtrace) from [<c030d698>] (show_stack+0x10/0x14)
[   31.918397] [<c030d698>] (show_stack) from [<c113439c>] (dump_stack+0xe0/0x10c)
[   31.918522] [<c113439c>] (dump_stack) from [<c0349098>] (__warn+0xf4/0x10c)
[   31.918641] [<c0349098>] (__warn) from [<c0349128>] (warn_slowpath_fmt+0x78/0xbc)
[   31.918768] [<c0349128>] (warn_slowpath_fmt) from [<c09f3c3c>] (_regulator_disable+0x1a8/0x210)
[   31.918900] [<c09f3c3c>] (_regulator_disable) from [<c09f3cdc>] (regulator_disable+0x38/0xe8)
[   31.919032] [<c09f3cdc>] (regulator_disable) from [<c0df837c>] (ehci_ci_portpower+0x38/0xdc)
	ehci_ci_portpower() calls regulator_enable() or regulator_disable()
	if priv->reg_vbus is not NULL
[   31.919166] [<c0df837c>] (ehci_ci_portpower) from [<c0db5724>] (ehci_port_power+0x50/0xa4)
	ehci_port_power() calls hcd->driver->port_power(), which is ehci_ci_portpower()
[   31.919296] [<c0db5724>] (ehci_port_power) from [<c0db5ba0>] (ehci_silence_controller+0x5c/0xc4)
	ehci_silence_controller() calls ehci_turn_off_all_ports(), which calls
	ehci_port_power() for each port
[   31.919430] [<c0db5ba0>] (ehci_silence_controller) from [<c0db7dc4>] (ehci_stop+0x3c/0xcc)
	ehci_silence_controller() called unconditionally from ehci_stop()
[   31.919560] [<c0db7dc4>] (ehci_stop) from [<c0d5c504>] (usb_remove_hcd+0xe0/0x19c)	
	ehci_stop() called unconditionally from usb_remove_hcd() with hcd->driver->stop(hcd);
[   31.919685] [<c0d5c504>] (usb_remove_hcd) from [<c0df7e08>] (host_stop+0x38/0xa8)
[   31.919809] [<c0df7e08>] (host_stop) from [<c0df3704>] (ci_hdrc_remove+0x44/0xe4)
[   31.919932] [<c0df3704>] (ci_hdrc_remove) from [<c0b1a5e4>] (platform_drv_remove+0x20/0x40)
[   31.920062] [<c0b1a5e4>] (platform_drv_remove) from [<c0b18a50>] (device_release_driver_internal+0xe8/0x1b8)
[   31.920210] [<c0b18a50>] (device_release_driver_internal) from [<c0b17464>] (bus_remove_device+0xd0/0xfc)
[   31.920351] [<c0b17464>] (bus_remove_device) from [<c0b1401c>] (device_del+0x140/0x374)
[   31.920477] [<c0b1401c>] (device_del) from [<c0b1aaf8>] (platform_device_del.part.3+0x10/0x74)
[   31.920608] [<c0b1aaf8>] (platform_device_del.part.3) from [<c0b1ab8c>] (platform_device_unregister+0x1c/0x28)
[   31.920758] [<c0b1ab8c>] (platform_device_unregister) from [<c0df22f4>] (ci_hdrc_remove_device+0xc/0x20)
[   31.920898] [<c0df22f4>] (ci_hdrc_remove_device) from [<c0df9ea4>] (ci_hdrc_imx_remove+0x28/0x110)
[   31.921033] [<c0df9ea4>] (ci_hdrc_imx_remove) from [<c0b15ac4>] (device_shutdown+0x180/0x224)
[   31.921166] [<c0b15ac4>] (device_shutdown) from [<c037652c>] (kernel_restart+0xc/0x50)
[   31.921292] [<c037652c>] (kernel_restart) from [<c03767d0>] (__do_sys_reboot+0x15c/0x1ec)
[   31.921444] [<c03767d0>] (__do_sys_reboot) from [<c0301000>] (ret_fast_syscall+0x0/0x28)
[   31.921595] Exception stack(0xc93c1fa8 to 0xc93c1ff0)

Re: [PATCH] usb: chipidea: host: Disable port power only if previously enabled

[Alan Stern]

On Wed, 1 Jan 2020, Guenter Roeck wrote:

> On 12/29/19 8:40 AM, Alan Stern wrote:
> > On Sun, 29 Dec 2019, Guenter Roeck wrote:
> > 
> >> On Sat, Dec 28, 2019 at 02:33:01PM -0500, Alan Stern wrote:
> >>>
> >>> Let's try a slightly different approach.  What happens with this patch?
> >>>
> >>> Alan Stern
> >>>
> >>>
> >>> Index: usb-devel/drivers/usb/core/hub.c
> >>> ===================================================================
> >>> --- usb-devel.orig/drivers/usb/core/hub.c
> >>> +++ usb-devel/drivers/usb/core/hub.c
> >>> @@ -1065,6 +1065,7 @@ static void hub_activate(struct usb_hub
> >>>   		if (type == HUB_INIT) {
> >>>   			delay = hub_power_on_good_delay(hub);
> >>>   
> >>> +			hub->power_bits[0] = ~0UL;	/* All ports on */
> >>>   			hub_power_on(hub, false);
> >>>   			INIT_DELAYED_WORK(&hub->init_work, hub_init_func2);
> >>>   			queue_delayed_work(system_power_efficient_wq,
> >>>
> >>
> >> That doesn't make a difference - the traceback is still seen with this patch
> >> applied.
> > 
> > Can you trace what's going on?  Does this code pathway now end up
> > calling ehci_port_power() for each root-hub port, and from there down
> > into the chipidea driver?  If not, can you find where it gets
> > sidetracked?
> > 
> 
> A complete traceback is attached below, so, yes, I think it is safe to say that
> ehci_port_power() is called unconditionally for each root-hub port on shutdown.

I was really asking about hub activation and powering-up, but you found 
the answer to that too, so okay.

> The only mystery to me was why ehci_port_power() isn't called to enable port power
> when the port comes up. As it turns out, HCS_PPC(ehci->hcs_params) is false on my
> simulated hardware, and thus ehci_port_power(..., true) is not called from
> ehci_hub_control().
> 
> Given that, it may well be that the problem is not seen on "real" hardware,
> at least not with real mcimx7d-sabre hardware, if the hub on that hardware does
> support power control. To test this idea, I modified qemu to claim hub power
> control support by setting the "power control support" capability bit. With
> that, the traceback is gone.
> 
> Any suggestion how to proceed ?

Given that HCS_PPC(ehci_hcs_params) is false, I would say that
ehci_turn_off_all_ports() shouldn't call ehci_port_power().  You should
add that test there.  (Although to tell the truth, I'm not really sure 
we need to test HCS_PPC anywhere...)

Did you check what happens without the patch I sent you?  I would like
to know if that patch really does make a difference.  If we don't send
the Set-Port-Feature(power) request during hub activation without the
patch then it does need to be merged.

Alan Stern

Re: [PATCH] usb: chipidea: host: Disable port power only if previously enabled

[Peter Chen]

On 19-12-26 07:57:54, Guenter Roeck wrote:
> On shutdown, ehci_power_off() is called unconditionally to power off
> each port, even if it was never called to power on the port.
> For chipidea, this results in a call to ehci_ci_portpower() with a request
> to power off ports even if the port was never powered on.
> This results in the following warning from the regulator code.
> 
> WARNING: CPU: 0 PID: 182 at drivers/regulator/core.c:2596 _regulator_disable+0x1a8/0x210
> unbalanced disables for usb_otg2_vbus
> Modules linked in:
> CPU: 0 PID: 182 Comm: init Not tainted 5.4.6 #1
> Hardware name: Freescale i.MX7 Dual (Device Tree)
> [<c0313658>] (unwind_backtrace) from [<c030d698>] (show_stack+0x10/0x14)
> [<c030d698>] (show_stack) from [<c1133afc>] (dump_stack+0xe0/0x10c)
> [<c1133afc>] (dump_stack) from [<c0349098>] (__warn+0xf4/0x10c)
> [<c0349098>] (__warn) from [<c0349128>] (warn_slowpath_fmt+0x78/0xbc)
> [<c0349128>] (warn_slowpath_fmt) from [<c09f36ac>] (_regulator_disable+0x1a8/0x210)
> [<c09f36ac>] (_regulator_disable) from [<c09f374c>] (regulator_disable+0x38/0xe8)
> [<c09f374c>] (regulator_disable) from [<c0df7bac>] (ehci_ci_portpower+0x38/0xdc)
> [<c0df7bac>] (ehci_ci_portpower) from [<c0db4fa4>] (ehci_port_power+0x50/0xa4)
> [<c0db4fa4>] (ehci_port_power) from [<c0db5420>] (ehci_silence_controller+0x5c/0xc4)
> [<c0db5420>] (ehci_silence_controller) from [<c0db7644>] (ehci_stop+0x3c/0xcc)
> [<c0db7644>] (ehci_stop) from [<c0d5bdc4>] (usb_remove_hcd+0xe0/0x19c)
> [<c0d5bdc4>] (usb_remove_hcd) from [<c0df7638>] (host_stop+0x38/0xa8)
> [<c0df7638>] (host_stop) from [<c0df2f34>] (ci_hdrc_remove+0x44/0xe4)
> ...
> 
> Keeping track of the power enable state avoids the warning and traceback.
> 
> Fixes: c8679a2fb8dec ("usb: chipidea: host: add portpower override")
> Cc: Michael Grzeschik <m.grzeschik@pengutronix.de>
> Cc: Peter Chen <peter.chen@freescale.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/usb/chipidea/host.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
> index b45ceb91c735..48e4a5ca1835 100644
> --- a/drivers/usb/chipidea/host.c
> +++ b/drivers/usb/chipidea/host.c
> @@ -26,6 +26,7 @@ static int (*orig_bus_suspend)(struct usb_hcd *hcd);
>  
>  struct ehci_ci_priv {
>  	struct regulator *reg_vbus;
> +	bool enabled;
>  };
>  
>  static int ehci_ci_portpower(struct usb_hcd *hcd, int portnum, bool enable)
> @@ -37,7 +38,7 @@ static int ehci_ci_portpower(struct usb_hcd *hcd, int portnum, bool enable)
>  	int ret = 0;
>  	int port = HCS_N_PORTS(ehci->hcs_params);
>  
> -	if (priv->reg_vbus) {
> +	if (priv->reg_vbus && enable != priv->enabled) {
>  		if (port > 1) {
>  			dev_warn(dev,
>  				"Not support multi-port regulator control\n");
> @@ -53,6 +54,7 @@ static int ehci_ci_portpower(struct usb_hcd *hcd, int portnum, bool enable)
>  				enable ? "enable" : "disable", ret);
>  			return ret;
>  		}
> +		priv->enabled = enable;
>  	}
>  
>  	if (enable && (ci->platdata->phy_mode == USBPHY_INTERFACE_MODE_HSIC)) {
> -- 

Acked-by: Peter Chen <peter.chen@nxp.com>

-- 

Thanks,
Peter Chen