| Message ID | 1385408393-19707-3-git-send-email-zonque@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Nov 25, 2013 at 08:39:50PM +0100, Daniel Mack wrote: > Make musb_port_suspend() externally available, and call it when to host > goes into suspend. This allows the core to go into suspend while a > device is connected. > > Signed-off-by: Daniel Mack <zonque@gmail.com> > --- > drivers/usb/musb/musb_host.c | 2 ++ > drivers/usb/musb/musb_host.h | 2 ++ > drivers/usb/musb/musb_virthub.c | 2 +- > 3 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c > index 6582a20..81caf9f 100644 > --- a/drivers/usb/musb/musb_host.c > +++ b/drivers/usb/musb/musb_host.c > @@ -2433,6 +2433,8 @@ static int musb_bus_suspend(struct usb_hcd *hcd) > struct musb *musb = hcd_to_musb(hcd); > u8 devctl; > > + musb_port_suspend(musb, true); have you considered the fact that when musb looses context it'll cause a disconnect on the bus because soft_connect bit is lost ? What if you have a mounted file system on a pendrive ? Should we allow suspend in that case ? Alan, Greg, any hints ?
On 11/25/2013 08:46 PM, Felipe Balbi wrote: > On Mon, Nov 25, 2013 at 08:39:50PM +0100, Daniel Mack wrote: >> Make musb_port_suspend() externally available, and call it when to host >> goes into suspend. This allows the core to go into suspend while a >> device is connected. >> >> Signed-off-by: Daniel Mack <zonque@gmail.com> >> --- >> drivers/usb/musb/musb_host.c | 2 ++ >> drivers/usb/musb/musb_host.h | 2 ++ >> drivers/usb/musb/musb_virthub.c | 2 +- >> 3 files changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c >> index 6582a20..81caf9f 100644 >> --- a/drivers/usb/musb/musb_host.c >> +++ b/drivers/usb/musb/musb_host.c >> @@ -2433,6 +2433,8 @@ static int musb_bus_suspend(struct usb_hcd *hcd) >> struct musb *musb = hcd_to_musb(hcd); >> u8 devctl; >> >> + musb_port_suspend(musb, true); > > have you considered the fact that when musb looses context it'll cause a > disconnect on the bus because soft_connect bit is lost ? > > What if you have a mounted file system on a pendrive ? Should we allow > suspend in that case ? Well, I would have expected that, but in fact, the opposite is true. With 3.12, mounting a filesystem on a USB media and accessing it after resume was exactly my test case, and it worked just fine with that patch. In 3.13, something about the parition table reading seems to be broken currently, I'll have a closer look. Thanks, Daniel -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Mon, Nov 25, 2013 at 08:58:22PM +0100, Daniel Mack wrote: > On 11/25/2013 08:46 PM, Felipe Balbi wrote: > > On Mon, Nov 25, 2013 at 08:39:50PM +0100, Daniel Mack wrote: > >> Make musb_port_suspend() externally available, and call it when to host > >> goes into suspend. This allows the core to go into suspend while a > >> device is connected. > >> > >> Signed-off-by: Daniel Mack <zonque@gmail.com> > >> --- > >> drivers/usb/musb/musb_host.c | 2 ++ > >> drivers/usb/musb/musb_host.h | 2 ++ > >> drivers/usb/musb/musb_virthub.c | 2 +- > >> 3 files changed, 5 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c > >> index 6582a20..81caf9f 100644 > >> --- a/drivers/usb/musb/musb_host.c > >> +++ b/drivers/usb/musb/musb_host.c > >> @@ -2433,6 +2433,8 @@ static int musb_bus_suspend(struct usb_hcd *hcd) > >> struct musb *musb = hcd_to_musb(hcd); > >> u8 devctl; > >> > >> + musb_port_suspend(musb, true); > > > > have you considered the fact that when musb looses context it'll cause a > > disconnect on the bus because soft_connect bit is lost ? > > > > What if you have a mounted file system on a pendrive ? Should we allow > > suspend in that case ? > > Well, I would have expected that, but in fact, the opposite is true. > With 3.12, mounting a filesystem on a USB media and accessing it after > resume was exactly my test case, and it worked just fine with that patch. so you had a mounted file system, suspend, resume, it was still mounted? or did it reenumerate and you remounted it ? Try to do the same with transfers in flight, it's likely to corrupt your file system. > In 3.13, something about the parition table reading seems to be broken > currently, I'll have a closer look. cool
On 11/25/2013 09:01 PM, Felipe Balbi wrote: > Hi, > > On Mon, Nov 25, 2013 at 08:58:22PM +0100, Daniel Mack wrote: >> On 11/25/2013 08:46 PM, Felipe Balbi wrote: >>> On Mon, Nov 25, 2013 at 08:39:50PM +0100, Daniel Mack wrote: >>>> Make musb_port_suspend() externally available, and call it when to host >>>> goes into suspend. This allows the core to go into suspend while a >>>> device is connected. >>>> >>>> Signed-off-by: Daniel Mack <zonque@gmail.com> >>>> --- >>>> drivers/usb/musb/musb_host.c | 2 ++ >>>> drivers/usb/musb/musb_host.h | 2 ++ >>>> drivers/usb/musb/musb_virthub.c | 2 +- >>>> 3 files changed, 5 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c >>>> index 6582a20..81caf9f 100644 >>>> --- a/drivers/usb/musb/musb_host.c >>>> +++ b/drivers/usb/musb/musb_host.c >>>> @@ -2433,6 +2433,8 @@ static int musb_bus_suspend(struct usb_hcd *hcd) >>>> struct musb *musb = hcd_to_musb(hcd); >>>> u8 devctl; >>>> >>>> + musb_port_suspend(musb, true); >>> >>> have you considered the fact that when musb looses context it'll cause a >>> disconnect on the bus because soft_connect bit is lost ? >>> >>> What if you have a mounted file system on a pendrive ? Should we allow >>> suspend in that case ? >> >> Well, I would have expected that, but in fact, the opposite is true. >> With 3.12, mounting a filesystem on a USB media and accessing it after >> resume was exactly my test case, and it worked just fine with that patch. > > so you had a mounted file system, suspend, resume, it was still mounted? Yes, exactly. > or did it reenumerate and you remounted it ? No, it did not reenumerate, but the core did a reset on it. See the following log. # mount /dev/sda /mnt # md5sum /mnt/sine_left.wav bcd90548c1ebf293289072f1a6161f0f /mnt/sine_left.wav # # # echo mem > /sys/power/state [ 31.553425] PM: Syncing filesystems ... done. [ 31.575462] Freezing user space processes ... (elapsed 0.002 seconds) done. [ 31.585275] Freezing remaining freezable tasks ... (elapsed 0.002 seconds) done. [ 31.595953] PM: Sending message for entering DeepSleep mode [ 31.628887] PM: suspend of devices complete after 21.097 msecs [ 31.641082] PM: late suspend of devices complete after 5.906 msecs [ 31.653834] PM: noirq suspend of devices complete after 6.135 msecs [ 31.660558] PM: GFX domain entered low power state [ 31.660558] PM: Successfully put all powerdomains to target state [ 31.660558] PM: Wakeup source GPIO0 [ 31.679130] PM: noirq resume of devices complete after 17.941 msecs [ 31.693371] PM: early resume of devices complete after 4.388 msecs [ 31.702750] net eth0: initializing cpsw version 1.12 (0) [ 31.712084] net eth0: phy found : id is : 0x4dd076 [ 31.717317] libphy: PHY not found [ 31.722448] net eth0: phy not found on slave 1 [ 32.030792] usb 1-1: reset high-speed USB device number 2 using musb-hdrc <<<< ! [ 32.395192] PM: resume of devices complete after 695.279 msecs [ 32.407816] PM: Sending message for resetting M3 state machine [ 32.414683] Restarting tasks ... done. [ 36.701568] libphy: 4a101000.mdio:04 - Link is Up - 100/Full # # # md5sum /mnt/sine_left.wav bcd90548c1ebf293289072f1a6161f0f /mnt/sine_left.wav > Try to do the same with transfers in flight, it's likely to corrupt your > file system. I did that as well, and it appeared stable to me. After all, the USB device was properly put into suspend before VBUS went away, and the usb storage driver caught up after the core's reset. In my tests, this was the only approach that would work after resume. Daniel -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 25 Nov 2013, Felipe Balbi wrote: > On Mon, Nov 25, 2013 at 08:39:50PM +0100, Daniel Mack wrote: > > Make musb_port_suspend() externally available, and call it when to host > > goes into suspend. This allows the core to go into suspend while a > > device is connected. > have you considered the fact that when musb looses context it'll cause a > disconnect on the bus because soft_connect bit is lost ? > > What if you have a mounted file system on a pendrive ? Should we allow > suspend in that case ? > > Alan, Greg, any hints ? An HCD should strive to avoid losing power sessions over a suspend/resume. But if this is unavoidable, the USB Persist mechanism should be able to cope by issuing a reset-resume. (Unless userspace has turned Persist off, of course -- I believe some distributions do this. Chrome?) Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 25 Nov 2013, Daniel Mack wrote: > >>> What if you have a mounted file system on a pendrive ? Should we allow > >>> suspend in that case ? > >> > >> Well, I would have expected that, but in fact, the opposite is true. > >> With 3.12, mounting a filesystem on a USB media and accessing it after > >> resume was exactly my test case, and it worked just fine with that patch. > > > > so you had a mounted file system, suspend, resume, it was still mounted? > > Yes, exactly. > > > or did it reenumerate and you remounted it ? > > No, it did not reenumerate, but the core did a reset on it. See the > following log. ... > [ 32.030792] usb 1-1: reset high-speed USB device number 2 using > musb-hdrc <<<< ! Like I said, you get a reset-resume. > > Try to do the same with transfers in flight, it's likely to corrupt your > > file system. It's not so easy to suspend a system with USB mass-storage transfers in flight. For one thing, userspace gets frozen before the suspend starts, so there's nothing to initiate new I/O requests (except perhaps for dirty-block writebacks). Also, the child SCSI device gets suspended before the USB device, so its I/O queue stops running. In addition, the usb-storage suspend routine and the main I/O thread are mutually exclusive (they both grab the private mutex), so a suspend can't occur while a SCSI command is underway. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/25/2013 09:41 PM, Alan Stern wrote: > On Mon, 25 Nov 2013, Daniel Mack wrote: > >>>>> What if you have a mounted file system on a pendrive ? Should we allow >>>>> suspend in that case ? >>>> >>>> Well, I would have expected that, but in fact, the opposite is true. >>>> With 3.12, mounting a filesystem on a USB media and accessing it after >>>> resume was exactly my test case, and it worked just fine with that patch. >>> >>> so you had a mounted file system, suspend, resume, it was still mounted? >> >> Yes, exactly. >> >>> or did it reenumerate and you remounted it ? >> >> No, it did not reenumerate, but the core did a reset on it. See the >> following log. > > ... >> [ 32.030792] usb 1-1: reset high-speed USB device number 2 using >> musb-hdrc <<<< ! > > Like I said, you get a reset-resume. Yup. We can't avoid losing VBUS on this platform, and even if we could, we wouldn't want that, in order to save as much power as possible. >>> Try to do the same with transfers in flight, it's likely to corrupt your >>> file system. > > It's not so easy to suspend a system with USB mass-storage transfers in > flight. For one thing, userspace gets frozen before the suspend > starts, so there's nothing to initiate new I/O requests (except perhaps > for dirty-block writebacks). > > Also, the child SCSI device gets suspended before the USB device, so > its I/O queue stops running. In addition, the usb-storage suspend > routine and the main I/O thread are mutually exclusive (they both grab > the private mutex), so a suspend can't occur while a SCSI command is > underway. Thanks Alan for the explanation. That was my understanding as well. Best regards, Daniel -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Mon, Nov 25, 2013 at 03:41:14PM -0500, Alan Stern wrote: > On Mon, 25 Nov 2013, Daniel Mack wrote: > > > >>> What if you have a mounted file system on a pendrive ? Should we allow > > >>> suspend in that case ? > > >> > > >> Well, I would have expected that, but in fact, the opposite is true. > > >> With 3.12, mounting a filesystem on a USB media and accessing it after > > >> resume was exactly my test case, and it worked just fine with that patch. > > > > > > so you had a mounted file system, suspend, resume, it was still mounted? > > > > Yes, exactly. > > > > > or did it reenumerate and you remounted it ? > > > > No, it did not reenumerate, but the core did a reset on it. See the > > following log. > > ... > > [ 32.030792] usb 1-1: reset high-speed USB device number 2 using > > musb-hdrc <<<< ! > > Like I said, you get a reset-resume. > > > > Try to do the same with transfers in flight, it's likely to corrupt your > > > file system. > > It's not so easy to suspend a system with USB mass-storage transfers in > flight. For one thing, userspace gets frozen before the suspend > starts, so there's nothing to initiate new I/O requests (except perhaps > for dirty-block writebacks). > > Also, the child SCSI device gets suspended before the USB device, so > its I/O queue stops running. In addition, the usb-storage suspend > routine and the main I/O thread are mutually exclusive (they both grab > the private mutex), so a suspend can't occur while a SCSI command is > underway. awesome, thanks for the explanation Alan :-)
On 11/25/2013 09:01 PM, Felipe Balbi wrote: > On Mon, Nov 25, 2013 at 08:58:22PM +0100, Daniel Mack wrote: >> In 3.13, something about the parition table reading seems to be broken >> currently, I'll have a closer look. > > cool Ah, and forget about this one. I didn't look close enough. There's no such issue in 3.13. Sorry for the noise. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 25, 2013 at 09:47:16PM +0100, Daniel Mack wrote: > On 11/25/2013 09:01 PM, Felipe Balbi wrote: > > On Mon, Nov 25, 2013 at 08:58:22PM +0100, Daniel Mack wrote: > > >> In 3.13, something about the parition table reading seems to be broken > >> currently, I'll have a closer look. > > > > cool > > Ah, and forget about this one. I didn't look close enough. There's no > such issue in 3.13. Sorry for the noise. no problem :-)
diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c index 6582a20..81caf9f 100644 --- a/drivers/usb/musb/musb_host.c +++ b/drivers/usb/musb/musb_host.c @@ -2433,6 +2433,8 @@ static int musb_bus_suspend(struct usb_hcd *hcd) struct musb *musb = hcd_to_musb(hcd); u8 devctl; + musb_port_suspend(musb, true); + if (!is_host_active(musb)) return 0; diff --git a/drivers/usb/musb/musb_host.h b/drivers/usb/musb/musb_host.h index 960d735..e660af9 100644 --- a/drivers/usb/musb/musb_host.h +++ b/drivers/usb/musb/musb_host.h @@ -92,6 +92,7 @@ extern void musb_host_rx(struct musb *, u8); extern void musb_root_disconnect(struct musb *musb); extern void musb_host_resume_root_hub(struct musb *musb); extern void musb_host_poke_root_hub(struct musb *musb); +extern void musb_port_suspend(struct musb *musb, bool do_suspend); #else static inline struct musb *hcd_to_musb(struct usb_hcd *hcd) { @@ -121,6 +122,7 @@ static inline void musb_root_disconnect(struct musb *musb) {} static inline void musb_host_resume_root_hub(struct musb *musb) {} static inline void musb_host_poll_rh_status(struct musb *musb) {} static inline void musb_host_poke_root_hub(struct musb *musb) {} +static inline void musb_port_suspend(struct musb *musb, bool do_suspend) {} #endif struct usb_hcd; diff --git a/drivers/usb/musb/musb_virthub.c b/drivers/usb/musb/musb_virthub.c index 9af6bba..e977441 100644 --- a/drivers/usb/musb/musb_virthub.c +++ b/drivers/usb/musb/musb_virthub.c @@ -44,7 +44,7 @@ #include "musb_core.h" -static void musb_port_suspend(struct musb *musb, bool do_suspend) +void musb_port_suspend(struct musb *musb, bool do_suspend) { struct usb_otg *otg = musb->xceiv->otg; u8 power;
Make musb_port_suspend() externally available, and call it when to host goes into suspend. This allows the core to go into suspend while a device is connected. Signed-off-by: Daniel Mack <zonque@gmail.com> --- drivers/usb/musb/musb_host.c | 2 ++ drivers/usb/musb/musb_host.h | 2 ++ drivers/usb/musb/musb_virthub.c | 2 +- 3 files changed, 5 insertions(+), 1 deletion(-)