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

From: Peter Chen
Date: Thu Feb 27 2020 - 21:56:05 EST

Next message: Anson Huang: "[PATCH V2 1/4] dt-bindings: thermal: imx8mm-thermal: Add binding doc for i.MX8MM"
Previous message: Saravana Kannan: "Re: [PATCH] power: supply: Allow charger manager can be built as a module"
In reply to: Alan Stern: "Re: [RFC PATCH v2] USB: hub: fix port suspend/resume"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

>
> > > > 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&data=02%7C01%7Cpeter.chen%40nxp.com%7Ce2
> 47403d3a8c420ef66d08d7bbbb63a5%7C686ea1d3bc2b4c6fa92cd99c5c301635%7
> C0%7C0%7C637184285813366300&sdata=MviQpc4vhyVu496wyNQ%2Bb3T
> hNE7Gh6hpenjzxn6%2FLwE%3D&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

Next message: Anson Huang: "[PATCH V2 1/4] dt-bindings: thermal: imx8mm-thermal: Add binding doc for i.MX8MM"
Previous message: Saravana Kannan: "Re: [PATCH] power: supply: Allow charger manager can be built as a module"
In reply to: Alan Stern: "Re: [RFC PATCH v2] USB: hub: fix port suspend/resume"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]