Hello Dmitry, On Fri, Jan 19, 2018 at 03:24:32PM -0800, Dmitry Torokhov wrote: > On Wed, Jan 17, 2018 at 02:58:40PM +0100, Marcus Folkesson wrote: > > Hello Dmitry, > > > > On Tue, Jan 16, 2018 at 03:16:25PM -0800, Dmitry Torokhov wrote: > > > Hi Marcus, > > > > > > On Sat, Jan 13, 2018 at 09:15:32PM +0100, Marcus Folkesson wrote: > > > > This driver let you plug in your RC controller to the adapter and > > > > use it as input device in various RC simulators. > > > > > > > > Signed-off-by: Marcus Folkesson <marcus.folkesson@xxxxxxxxx> > > > > --- > > > > v3: > > > > - Use RUDDER and MISC instead of TILT_X and TILT_Y > > > > - Drop kref and anchor > > > > - Rework URB handling > > > > - Add PM support > > > > > > How did you test the PM support? By default the autopm is disabled on > > > USB devices; you need to enable it by writing to sysfs (I believe you > > > need to 'echo "auto" > /sys/bus/usb/<device>/power/control) and see if > > > it gets autosuspended when not in use and resumed after you start > > > interacting with it. > > > > The test I've done is simply reading from the input device and then call > > `pm-suspend`. > > It works, suspend is called and reset_resume() will submit the URB > > again. Without the PM code, the application did not read any events upon > > resume. > > We are talking about different things. You are testing system suspend, > whereas I was talking about runtime suspend (that's what > usb_autopm_get_interface() and friends does). It is disabled by default > and you need to enable it by writing into sysfs as I mentioned above. > Then, after a few seconds of not touching the device you should see the > USB interface going into low power state and the device shoudl correctly > implement remote wakeup signal to wake up the host controller/port when > user touches it. If the device does not implement this correctly, then > after suspending it will "die". > Ok, I have read more about the autosuspend feature and I will drop the support as the device does not seems to support remote wakeup signals. > > > > However, I found another tricky part. > > If I enable autosuspend (as you suggest) it will suspend when noone is > > using the device. Good. > > > > But when someone is opening the device, input_dev->users is counted up > > to 1 before resume() is called. > > Is this intended? > > > > This code (from resume()) will therefor allways submit the URB: > > > > if (input_dev->users && usb_submit_urb(pxrc->urb, GFP_NOIO) < 0) > > > > > > Then open() is called and fails because the urb is allready submitted. > > > > input_dev->users is only incremented in input.c:input_open_device() what > > I can tell? > > It is intended, but I guess we should not be using input_dev->users in > resume(), but rather have a local flag in your driver structure trhat > you update at the right time (i.e. after you submit USB in pxrc_open()). > > I suppose we need the same fix in synaptics_usb.c... > Will do. I fix the synaptics_usb driver as well. Also, I think we have a deadlock in the synaptics_usb driver. When the device is suspended and someone is open the device, the input subsystem will call input_open_device() which takes the input_dev->mutex and then call input_dev->open(). synusb_open() has a call to usb_autopm_get_interface() which will result in a call to the registered resume-function if the device is suspended. (see Documentation/driver-api/usb/power-manaement.rst). In the case of snaptics_usb, it will take the input_dev->mutex in the resume function. I have no synaptic mouse, but tested to put the same code into my driver just to confirm, and got the following dump: [ 9215.626476] INFO: task input-events:8590 blocked for more than 120 seconds. [ 9215.626495] Not tainted 4.15.0-rc8-ARCH+ #6 [ 9215.626500] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. [ 9215.626507] input-events D 0 8590 4394 0x00000004 [ 9215.626520] Call Trace: [ 9215.626546] ? __schedule+0x236/0x850 [ 9215.626559] schedule+0x2f/0x90 [ 9215.626569] schedule_preempt_disabled+0x11/0x20 [ 9215.626579] __mutex_lock.isra.0+0x1aa/0x520 [ 9215.626609] ? usb_runtime_suspend+0x70/0x70 [usbcore] [ 9215.626622] ? pxrc_resume+0x37/0x70 [pxrc] [ 9215.626632] pxrc_resume+0x37/0x70 [pxrc] [ 9215.626655] usb_resume_interface.isra.2+0x39/0xe0 [usbcore] [ 9215.626676] usb_resume_both+0xd2/0x120 [usbcore] [ 9215.626688] __rpm_callback+0xb6/0x1f0 [ 9215.626699] rpm_callback+0x1f/0x70 [ 9215.626718] ? usb_runtime_suspend+0x70/0x70 [usbcore] [ 9215.626726] rpm_resume+0x4e2/0x7f0 [ 9215.626737] rpm_resume+0x582/0x7f0 [ 9215.626749] __pm_runtime_resume+0x3a/0x50 [ 9215.626767] usb_autopm_get_interface+0x1d/0x50 [usbcore] [ 9215.626780] pxrc_open+0x17/0x8d [pxrc] [ 9215.626791] input_open_device+0x70/0xa0 [ 9215.626804] evdev_open+0x183/0x1c0 [evdev] [ 9215.626819] chrdev_open+0xa0/0x1b0 [ 9215.626830] ? cdev_put.part.1+0x20/0x20 [ 9215.626840] do_dentry_open+0x1ad/0x2c0 [ 9215.626855] path_openat+0x576/0x1300 [ 9215.626868] ? alloc_set_pte+0x22c/0x520 [ 9215.626883] ? filemap_map_pages+0x19b/0x340 [ 9215.626893] do_filp_open+0x9b/0x110 [ 9215.626908] ? __check_object_size+0x9d/0x190 [ 9215.626920] ? __alloc_fd+0xaf/0x160 [ 9215.626931] ? do_sys_open+0x1bd/0x250 [ 9215.626942] do_sys_open+0x1bd/0x250 [ 9215.626956] entry_SYSCALL_64_fastpath+0x20/0x83 [ 9215.626967] RIP: 0033:0x7fbf6358f7ae tablet/pegasus_notetaker.c and touchscreen/usbtouchscreen.c has the same construction (taking input_dev->mutex in resume/suspend and call usb_autopm_get_interface() in open()). I will create a separate "pm_mutex" to use instead of input_dev->mutex to get rid of the lockups in those drivers > > > > I will move the submitting code to reset_resume() instead. > > You need both resume() and reset_resume(), they are called in different > cases and you need to restart IO in both cases. > > Thanks. > > -- > Dmitry Best regards Marcus Folkesson
Attachment:
signature.asc
Description: PGP signature
