| Message ID | 20200227135631.13983-1-m.felsch@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,v2] USB: hub: fix port suspend/resume | expand |
On Thu, 27 Feb 2020, Marco Felsch wrote: > At the momemnt the usb-port driver has only runime_pm hooks. > Suspending the port and turn off the VBUS supply should be triggered by > the hub device suspend callback usb_port_suspend() which calls the Strictly speaking it's just a routine, not a callback. That is, it doesn't get invoked through a function pointer. > pm_runtime_put_sync() if all pre-conditions are meet. This mechanism > don't work correctly due to the global PM behaviour, for more information > see [1]. According [1] I added the suspend/resume callbacks for the port > device to fix this. > > [1] https://www.spinics.net/lists/linux-usb/msg190537.html Please put at least a short description of the problem here; don't force people to go look up some random web page just to find out what's really going on. > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > --- > Hi, > > this v2 contains the fixes > > Reported-by: kbuild test robot <lkp@intel.com> Everything below the "---" line, except the patch itself, gets ignored. You need to move this Reported-by: up higher. > Regards, > Marco > > Changes: > - init retval to zero > - keep CONFIG_PM due to do_remote_wakeup availability > - adapt commit message > > drivers/usb/core/hub.c | 13 ------------- > drivers/usb/core/port.c | 35 ++++++++++++++++++++++++++++++----- > 2 files changed, 30 insertions(+), 18 deletions(-) > > 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? > usb_lock_port(port_dev); > > /* Skip the initial Clear-Suspend step for a remote wakeup */ > diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c > index bbbb35fa639f..13f130b67efe 100644 > --- a/drivers/usb/core/port.c > +++ b/drivers/usb/core/port.c > @@ -283,7 +283,34 @@ static int usb_port_runtime_suspend(struct device *dev) > > return retval; > } > -#endif > + > +static int __maybe_unused _usb_port_suspend(struct device *dev) Don't say _maybe_unused. Instead, protect these two routines with #ifdef CONFIG_PM_SLEEP. That way they won't be compiled on systems that can't use them. Also, try to find better names. Maybe usb_port_sleep and usb_port_wake, or usb_port_system_suspend and usb_port_system_resume. > +{ > + struct usb_port *port_dev = to_usb_port(dev); > + struct usb_device *udev = port_dev->child; > + int retval = 0; > + > + if (!udev->do_remote_wakeup && udev->persist_enabled) > + retval = usb_port_runtime_suspend(dev); > + > + /* Do not force the user to enable the power-off feature */ > + if (retval && retval != -EAGAIN) > + return retval; > + > + return 0; IMO it would be a lot more understandable if you wrote if (retval == -EAGAIN) retval = 0; Also, the relation between this code and the preceding comment is not obvious. The comment should say something more like: If the PM_QOS_FLAG setting prevents us from powering off the port, it's not an error. Alan Stern > +} > + > +static int __maybe_unused _usb_port_resume(struct device *dev) > +{ > + struct usb_port *port_dev = to_usb_port(dev); > + struct usb_device *udev = port_dev->child; > + > + if (!udev->do_remote_wakeup && udev->persist_enabled) > + return usb_port_runtime_resume(dev); > + > + return 0; > +} > +#endif /* CONFIG_PM */ > > static void usb_port_shutdown(struct device *dev) > { > @@ -294,10 +321,8 @@ static void usb_port_shutdown(struct device *dev) > } > > static const struct dev_pm_ops usb_port_pm_ops = { > -#ifdef CONFIG_PM > - .runtime_suspend = usb_port_runtime_suspend, > - .runtime_resume = usb_port_runtime_resume, > -#endif > + SET_SYSTEM_SLEEP_PM_OPS(_usb_port_suspend, _usb_port_resume) > + SET_RUNTIME_PM_OPS(usb_port_runtime_suspend, usb_port_runtime_resume, NULL) > }; > > struct device_type usb_port_device_type = { >
On 20-02-27 11:18, Alan Stern wrote: > On Thu, 27 Feb 2020, Marco Felsch wrote: > > > At the momemnt the usb-port driver has only runime_pm hooks. > > Suspending the port and turn off the VBUS supply should be triggered by > > the hub device suspend callback usb_port_suspend() which calls the > > Strictly speaking it's just a routine, not a callback. That is, it > doesn't get invoked through a function pointer. Damn, you right it gets called from the generic_suspend callback. > > pm_runtime_put_sync() if all pre-conditions are meet. This mechanism > > don't work correctly due to the global PM behaviour, for more information > > see [1]. According [1] I added the suspend/resume callbacks for the port > > device to fix this. > > > > [1] https://www.spinics.net/lists/linux-usb/msg190537.html > > Please put at least a short description of the problem here; don't > force people to go look up some random web page just to find out what's > really going on. Okay, I will do that. > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > --- > > Hi, > > > > this v2 contains the fixes > > > > Reported-by: kbuild test robot <lkp@intel.com> > > 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. > > Regards, > > Marco > > > > Changes: > > - init retval to zero > > - keep CONFIG_PM due to do_remote_wakeup availability > > - adapt commit message > > > > drivers/usb/core/hub.c | 13 ------------- > > drivers/usb/core/port.c | 35 ++++++++++++++++++++++++++++++----- > > 2 files changed, 30 insertions(+), 18 deletions(-) > > > > 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. 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. Therefore I came up with my question: https://www.spinics.net/lists/linux-usb/msg190537.html. > > usb_lock_port(port_dev); > > > > /* Skip the initial Clear-Suspend step for a remote wakeup */ > > diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c > > index bbbb35fa639f..13f130b67efe 100644 > > --- a/drivers/usb/core/port.c > > +++ b/drivers/usb/core/port.c > > @@ -283,7 +283,34 @@ static int usb_port_runtime_suspend(struct device *dev) > > > > return retval; > > } > > -#endif > > + > > +static int __maybe_unused _usb_port_suspend(struct device *dev) > > Don't say _maybe_unused. Instead, protect these two routines with > #ifdef CONFIG_PM_SLEEP. That way they won't be compiled on systems > that can't use them. Okay, as you like. I find the __maybe_unused better than #ifdefs. > 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 ;) > > +{ > > + struct usb_port *port_dev = to_usb_port(dev); > > + struct usb_device *udev = port_dev->child; > > + int retval = 0; > > + > > + if (!udev->do_remote_wakeup && udev->persist_enabled) > > + retval = usb_port_runtime_suspend(dev); > > + > > + /* Do not force the user to enable the power-off feature */ > > + if (retval && retval != -EAGAIN) > > + return retval; > > + > > + return 0; > > IMO it would be a lot more understandable if you wrote > > if (retval == -EAGAIN) > retval = 0; As you like. > Also, the relation between this code and the preceding comment is not > obvious. The comment should say something more like: If the > PM_QOS_FLAG setting prevents us from powering off the port, it's not an > error. Okay I will change that. Regards, Marco > Alan Stern > > > +} > > + > > +static int __maybe_unused _usb_port_resume(struct device *dev) > > +{ > > + struct usb_port *port_dev = to_usb_port(dev); > > + struct usb_device *udev = port_dev->child; > > + > > + if (!udev->do_remote_wakeup && udev->persist_enabled) > > + return usb_port_runtime_resume(dev); > > + > > + return 0; > > +} > > +#endif /* CONFIG_PM */ > > > > static void usb_port_shutdown(struct device *dev) > > { > > @@ -294,10 +321,8 @@ static void usb_port_shutdown(struct device *dev) > > } > > > > static const struct dev_pm_ops usb_port_pm_ops = { > > -#ifdef CONFIG_PM > > - .runtime_suspend = usb_port_runtime_suspend, > > - .runtime_resume = usb_port_runtime_resume, > > -#endif > > + SET_SYSTEM_SLEEP_PM_OPS(_usb_port_suspend, _usb_port_resume) > > + SET_RUNTIME_PM_OPS(usb_port_runtime_suspend, usb_port_runtime_resume, NULL) > > }; > > > > struct device_type usb_port_device_type = { > > > >
On Thu, 27 Feb 2020, Marco Felsch wrote: > On 20-02-27 11:18, Alan Stern wrote: > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > > --- > > > Hi, > > > > > > this v2 contains the fixes > > > > > > Reported-by: kbuild test robot <lkp@intel.com> > > > > 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://www.spinics.net/lists/linux-usb/msg190537.html. > > 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.
> > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > > > --- > > > > Hi, > > > > > > > > this v2 contains the fixes > > > > > > > > Reported-by: kbuild test robot <lkp@intel.com> > > > > > > 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
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; - } - } - usb_lock_port(port_dev); /* Skip the initial Clear-Suspend step for a remote wakeup */ diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c index bbbb35fa639f..13f130b67efe 100644 --- a/drivers/usb/core/port.c +++ b/drivers/usb/core/port.c @@ -283,7 +283,34 @@ static int usb_port_runtime_suspend(struct device *dev) return retval; } -#endif + +static int __maybe_unused _usb_port_suspend(struct device *dev) +{ + struct usb_port *port_dev = to_usb_port(dev); + struct usb_device *udev = port_dev->child; + int retval = 0; + + if (!udev->do_remote_wakeup && udev->persist_enabled) + retval = usb_port_runtime_suspend(dev); + + /* Do not force the user to enable the power-off feature */ + if (retval && retval != -EAGAIN) + return retval; + + return 0; +} + +static int __maybe_unused _usb_port_resume(struct device *dev) +{ + struct usb_port *port_dev = to_usb_port(dev); + struct usb_device *udev = port_dev->child; + + if (!udev->do_remote_wakeup && udev->persist_enabled) + return usb_port_runtime_resume(dev); + + return 0; +} +#endif /* CONFIG_PM */ static void usb_port_shutdown(struct device *dev) { @@ -294,10 +321,8 @@ static void usb_port_shutdown(struct device *dev) } static const struct dev_pm_ops usb_port_pm_ops = { -#ifdef CONFIG_PM - .runtime_suspend = usb_port_runtime_suspend, - .runtime_resume = usb_port_runtime_resume, -#endif + SET_SYSTEM_SLEEP_PM_OPS(_usb_port_suspend, _usb_port_resume) + SET_RUNTIME_PM_OPS(usb_port_runtime_suspend, usb_port_runtime_resume, NULL) }; struct device_type usb_port_device_type = {
At the momemnt the usb-port driver has only runime_pm hooks. Suspending the port and turn off the VBUS supply should be triggered by the hub device suspend callback usb_port_suspend() which calls the pm_runtime_put_sync() if all pre-conditions are meet. This mechanism don't work correctly due to the global PM behaviour, for more information see [1]. According [1] I added the suspend/resume callbacks for the port device to fix this. [1] https://www.spinics.net/lists/linux-usb/msg190537.html Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> --- Hi, this v2 contains the fixes Reported-by: kbuild test robot <lkp@intel.com> Regards, Marco Changes: - init retval to zero - keep CONFIG_PM due to do_remote_wakeup availability - adapt commit message drivers/usb/core/hub.c | 13 ------------- drivers/usb/core/port.c | 35 ++++++++++++++++++++++++++++++----- 2 files changed, 30 insertions(+), 18 deletions(-)