On Fri, 2019-06-21 at 15:28 -0400, Alan Stern wrote: > On Fri, 21 Jun 2019, Mayuresh Kulkarni wrote: > > However, I have been thinking about how to do all this in light of > Oliver's comments, and it seems like we should make some changes. > > First, let's change the ioctls' names. Instead of RESUME and SUSPEND, > I'll call them FORBID_SUSPEND and ALLOW_SUSPEND. The way they work > should be clear: ALLOW_SUSPEND will permit the device to be suspended > but might not cause a suspend to happen immediately. FORBID_SUSPEND > will cause an immediate resume if the device is currently suspended > and > will prevent the device from being suspended in the future. The new > names more accurately reflect what the ioctls actually do. > > Second, the WAIT_FOR_RESUME ioctl will wait only until a resume has > occurred more recently than the most recent ALLOW_SUSPEND ioctl. So > for example, if the program calls ALLOW_SUSPEND, and the device > suspends, and then the device resumes, and then the device suspends > again, and then the program calls WAIT_FOR_RESUME, the ioctl will > return immediately even though the device is currently suspended. > Thus you don't have to worry about missing a remote resume. This also > means that when WAIT_FOR_RESUME returns, the program should call > FORBID_SUSPEND to ensure that the device is active and doesn't go > back > into suspend. > > In practice, your program would use the ioctls as follows: > > When the device file is opened, the kernel will automatically > do the equivalent of FORBID_SUSPEND (to remain compatible > with the current behavior). > > When the program is ready for the device to suspend, it will > call the ALLOW_SUSPEND ioctl. But it won't cancel the > outstanding URBs; instead it will continue to interact > normally with the device, because the device might not be > suspended for some time. > > When the device does go into suspend, URBs will start failing > with an appropriate error code (EHOSTUNREACH, ESHUTDOWN, > EPROTO, or something similar). In this way the program will > realize the device is suspended. At that point the program > should call the WAIT_FOR_RESUME ioctl and stop trying to > communicate with the device. > > When WAIT_FOR_RESUME returns, the program should call the > FORBID_SUSPEND ioctl and resume normal communication with the > device. > > How does that sound? > > The proposed patch is below. > > Alan Stern > Hi Alan, With the 3-ioctl patch you had send, I checked the behaviour using my test program on our reference platform. AFAIU, the patch seems to work fine for our use-cases of: remote-wake and host-wake. For our typical use-cases, the user-space does not need to communicate with the device even after calling ALLOW_SUSPEND, so I am not in position to verify this point. The test does the below steps - ---------------- A. REMOTE-WAKE - ---------------- 1. Open the device file. 2. Find our interface and bind to it. 3. Send multiple commands to our interface. 4. Call ALLOW_SUSPEND. 5. Call WAIT_FOR_RESUME. 6. Wait for sometime (10-20 sec). 7. Do the activity on device which I know causes remote-wake to host. 8. Waiter in (5) above, calls FORBID_SUSPEND and reads the cause of resume. 9. Release the interface and close the device file. After (5) + auto-suspend time-out expires, I can see device, root-hub and host-controller (DWC-3) going into suspend. After (7), host-controller, root-hub and device resume are seen. -------------- B. HOST-WAKE - -------------- 1. Open the device file. 2. Find our interface and bind to it. 3. Send multiple commands to our interface. 4. Spawn a thread to cause host-wake. 5. Call ALLOW_SUSPEND. 6. Call WAIT_FOR_RESUME. 7. Signal thread after a delay (10-20 sec). 8. Thread calls FORBID_SUSPEND and sends a command to device. 9. Release the interface, wait for thread-join and close the device file. After (6) + auto-suspend time-out expires, device, root-hub and host controller goes into suspend. After (8), host-controller, root-hub and device resume are seen. Also the response to command is correct. With that said, at this point, the above observations are in-sync with what I had verified when I had send-out the old patch (with 2-ioctls and interface based suspend-resume). I am checking if I can verify a bit more complex use-cases. But not sure, if I can do that with current setup. As you had mentioned in one of the comment before, the only addition to the patch I have locally is - usbfs_notify_resume() has usbfs_mutex lock around list traversal. Could you please send the patch for review? Please note, I think I am not a part of linux-usb mailing-list, so probably need to be in cc to get the patch email. Do let me know if something else is needed from me. Thanks to You/Oliver/Greg KH for the all the comments and reviews.
