Re: [PATCH v3] input: pxrc: new driver for PhoenixRC Flight Controller Adapter

[Marcus Folkesson]

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