lkml.org 
[lkml]   [2018]   [Jan]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3] input: pxrc: new driver for PhoenixRC Flight Controller Adapter
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@gmail.com>
> > > > ---
> > > > 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
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2018-01-20 22:08    [W:0.127 / U:0.312 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site