RE: [RFC PATCH v2] USB: hub: fix port suspend/resume

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



 
> 
> > > > Signed-off-by: Marco Felsch <m.felsch@xxxxxxxxxxxxxx>
> > > > ---
> > > > Hi,
> > > >
> > > > this v2 contains the fixes
> > > >
> > > > Reported-by: kbuild test robot <lkp@xxxxxxxxx>
> > >
> > > Everything below the "---" line, except the patch itself, gets ignored.
> > > You need to move this Reported-by: up higher.
> >
> > I know, I put it here because the patch isn't part of the kernel. IMHO
> > a
> >
> > Signed-off-by:
> > Reported-by:
> >
> > looks a bit strange.
> 
> Not at all.  That sort of thing occurs all the time; just look at a few commits in the
> kernel or patches on the mailing lists.  Especially ones that are bug fixes.
> 
> > > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index
> > > > 3405b146edc9..c294484e478d 100644
> > > > --- a/drivers/usb/core/hub.c
> > > > +++ b/drivers/usb/core/hub.c
> > > > @@ -3323,10 +3323,6 @@ int usb_port_suspend(struct usb_device *udev,
> pm_message_t msg)
> > > >  		usb_set_device_state(udev, USB_STATE_SUSPENDED);
> > > >  	}
> > > >
> > > > -	if (status == 0 && !udev->do_remote_wakeup && udev->persist_enabled
> > > > -			&& test_and_clear_bit(port1, hub->child_usage_bits))
> > > > -		pm_runtime_put_sync(&port_dev->dev);
> > > > -
> > > >  	usb_mark_last_busy(hub->hdev);
> > > >
> > > >  	usb_unlock_port(port_dev);
> > > > @@ -3514,15 +3510,6 @@ int usb_port_resume(struct usb_device *udev,
> pm_message_t msg)
> > > >  	int		status;
> > > >  	u16		portchange, portstatus;
> > > >
> > > > -	if (!test_and_set_bit(port1, hub->child_usage_bits)) {
> > > > -		status = pm_runtime_get_sync(&port_dev->dev);
> > > > -		if (status < 0) {
> > > > -			dev_dbg(&udev->dev, "can't resume usb port, status %d\n",
> > > > -					status);
> > > > -			return status;
> > > > -		}
> > > > -	}
> > > > -
> > >
> > > Why do you get rid of these two sections of code?  Won't that cause
> > > runtime PM to stop working properly?
> >
> > Both runtime_pm calls are part of the suspend/resume logic so this
> > code isn't called during runtime PM.
> 
> I'm not quite sure what you mean by that.  In any case, it would be completely
> wrong to think that usb_port_suspend isn't involved in runtime PM.
> 
> In fact, usb_port_suspend is _more_ important for runtime suspend than for system
> sleep.  The reason is simple: If you want to put a USB device into runtime suspend,
> you have to tell its upstream hub's port to enable the suspend feature (i.e., call
> usb_port_suspend).  But if you want to put an entire bus of USB devices to sleep
> for a system suspend, all you have to do is tell the host controller to stop sending
> packets; the ports don't need any notification.
> 
> (Actually the situation is more complicated for USB 3.  But you get the
> idea.)
> 
> > As far as I understood it correctly the purpose of those section was
> > to trigger port poweroff if the device supports it upon a
> > system-suspend.
> 
> No, the purpose of the sections you removed is to trigger port poweroff when the
> device goes into any type of suspend, either system or runtime.  Of course, as you
> discovered, during system sleep the code doesn't actually turn off the port power --
> that's a bug.  But during runtime PM it does.
> 
> > Therefore I came up with my question:
> >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.spinics.
> net%2Flists%2Flinux-
> usb%2Fmsg190537.html&amp;data=02%7C01%7Cpeter.chen%40nxp.com%7Ce2
> 47403d3a8c420ef66d08d7bbbb63a5%7C686ea1d3bc2b4c6fa92cd99c5c301635%7
> C0%7C0%7C637184285813366300&amp;sdata=MviQpc4vhyVu496wyNQ%2Bb3T
> hNE7Gh6hpenjzxn6%2FLwE%3D&amp;reserved=0.
> 
> > > Also, try to find better names.  Maybe usb_port_sleep and
> > > usb_port_wake, or usb_port_system_suspend and usb_port_system_resume.
> >
> > IMHO usb_port_suspend/resume should be the best ;)
> 
> Okay, so long as they are static and won't conflict with the functions in hub.c.
> 
> Alan Stern
> 
> PS: There's one more thing you need to know -- I completely forgot about it until
> just now.  During system sleep, we have to make sure that the child device gets
> suspended _before_ and resumed _after_ the port.  If it happened the other way,
> we'd be in trouble.
> 
> (The proper ordering would be automatic if the child USB device was registered
> under the port device, but for historical reasons it isn't; it gets registered directly
> under the parent hub.)
> 
> This means you'll have to call device_pm_wait_for_dev() at the appropriate places
> in the suspend and resume pathways.

Hi Alan & Marco,

I tried this VBUS off feature at one chipidea controller board with USB mouse, it works
well, at least with my expectation. See below log:

// There is a USB mouse at USB2 port

root@imx6ul7d:~# ./uclk.sh 
    pll_usb_main_clk                  1        1        0   480000000          0     0  50000
       usb_phy2_clk                   1        1        0   480000000          0     0  50000
    pll_usb1_main_clk                 0        0        0   480000000          0     0  50000
       usb_phy1_clk                   0        0        0   480000000          0     0  50000
             usb_hsic_src             0        0        0   480000000          0     0  50000
                usb_hsic_cg           0        0        0   480000000          0     0  50000
                   usb_hsic_pre_div       0        0        0   480000000          0     0  50000
                      usb_hsic_post_div       0        0        0   480000000          0     0  50000
                         usb_hsic_root_clk       0        0        0   480000000          0     0  50000
                            usb_ctrl_clk       1        1        0   135000000          0     0  50000
// clock is on

root@imx6ul7d:~# echo auto > /sys/bus/usb/devices/1-1/power/control 
root@imx6ul7d:~# ./uclk.sh 
    pll_usb_main_clk                  0        0        0   480000000          0     0  50000
       usb_phy2_clk                   0        0        0   480000000          0     0  50000
    pll_usb1_main_clk                 0        0        0   480000000          0     0  50000
       usb_phy1_clk                   0        0        0   480000000          0     0  50000
             usb_hsic_src             0        0        0   480000000          0     0  50000
                usb_hsic_cg           0        0        0   480000000          0     0  50000
                   usb_hsic_pre_div       0        0        0   480000000          0     0  50000
                      usb_hsic_post_div       0        0        0   480000000          0     0  50000
                         usb_hsic_root_clk       0        0        0   480000000          0     0  50000
                            usb_ctrl_clk       0        0        0   135000000          0     0  50000

//clock is off after enable mouse's autosuspend

root@imx6ul7d:~# echo 0 > //sys/bus/usb/devices/1-0\:1.0/usb1-port1/power/
autosuspend_delay_ms    pm_qos_no_power_off     runtime_status
control                 runtime_active_time     runtime_suspended_time
_no_power_off ~# echo 0 > //sys/bus/usb/devices/1-0\:1.0/usb1-port1/power/pm_qos_
root@imx6ul7d:~# [  250.865933] ci_hdrc ci_hdrc.1: at: ehci_ci_portpower, disable vbus regulator

//VBUS is off after set pm_qos_no_power_off 0. It stands for pm_qos_no_power_off works
well for runtime suspend.

root@imx6ul7d:~# echo enabled > /sys/class/tty/ttymxc0/power/wakeup
root@imx6ul7d:~# echo mem > /sys/power/state

// Enable tty wakeup, and suspend system.

[  292.530680] PM: suspend entry (s2idle)
[  292.547108] Filesystems sync: 0.012 seconds
[  292.575311] Freezing user space processes ... (elapsed 0.003 seconds) done.
[  292.586275] OOM killer disabled.
[  292.589726] Freezing remaining freezable tasks ... (elapsed 0.002 seconds) done.
[  292.599891] printk: Suspending console(s) (use no_console_suspend to debug)

[  292.674257] fec 30be0000.ethernet eth0: Link is Down
[  292.677564] PM: suspend devices took 0.070 seconds
[  295.690990] imx6q-pcie 33800000.pcie: Phy link never came up
[  295.691012] imx6q-pcie 33800000.pcie: pcie link is down after resume.
[  295.715262] ci_hdrc ci_hdrc.1: at: ehci_ci_portpower, enable vbus regulator

// The VBUS is only on after resume, but not during the system suspend routine.

[  296.321299] usb 1-1: reset low-speed USB device number 2 using ci_hdrc
[  296.680700] PM: resume devices took 0.990 seconds
[  296.721685] OOM killer enabled.
[  296.724847] Restarting tasks ... done.
[  296.750136] PM: suspend exit
root@imx6ul7d:~# 
root@imx6ul7d:~# [  298.811004] fec 30be0000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
[  299.169794] ci_hdrc ci_hdrc.1: at: ehci_ci_portpower, disable vbus regulator

root@imx6ul7d:~# ./uclk.sh 
    pll_usb_main_clk                  0        0        0   480000000          0     0  50000
       usb_phy2_clk                   0        0        0   480000000          0     0  50000
    pll_usb1_main_clk                 0        0        0   480000000          0     0  50000
       usb_phy1_clk                   0        0        0   480000000          0     0  50000
             usb_hsic_src             0        0        0   480000000          0     0  50000
                usb_hsic_cg           0        0        0   480000000          0     0  50000
                   usb_hsic_pre_div       0        0        0   480000000          0     0  50000
                      usb_hsic_post_div       0        0        0   480000000          0     0  50000
                         usb_hsic_root_clk       0        0        0   480000000          0     0  50000
                            usb_ctrl_clk       0        0        0   135000000          0     0  50000

// As USB mouse is auto-suspend, and pm_qos_no_power_off is 0. The clock is off, and VBUS is off after autosuspend
timeout.

Linux version 5.6.0-rc1-00027-g2671f46409d5-dirty (b29397@b29397-desktop) (gcc version 6.2.0 (GCC)) #7 SMP Fri Feb 28 
10:20:18 CST 2020

// I use the usb-next tree, it is the new upstream kernel

Peter





[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux