Re: [PATCH] hid: usbhid: hid-core: fix recursive deadlock

Re: [PATCH] hid: usbhid: hid-core: fix recursive deadlock

[Ioan-Adrian Ratiu]

Here are some images with more information on this deadlock, might be helpful.

First part of lockdep report:
http://imgur.com/clLsCWe

Second part:
http://imgur.com/Wa2PzRl

Here are some printk's of mine while reproducing + debugging the issue:
http://imgur.com/SETOHT7

Re: [PATCH] hid: usbhid: hid-core: fix recursive deadlock

[Ioan-Adrian Ratiu]

On Thu, 19 Nov 2015 22:34:18 +0100 (CET)
Jiri Kosina <jikos@kernel.org> wrote:
> Could you please reformulate the changelog in this respect and resubmit?

Yes, of course, I tried to reformulate the problem and solution as clear and
succint as I could in v2, which I'll send shortly.

Thank you very much for your patience and feedback.

Re: [PATCH] hid: usbhid: hid-core: fix recursive deadlock

[Jiri Kosina]

On Thu, 19 Nov 2015, Ioan-Adrian Ratiu wrote:

> But please understand further my reasoning for submitting this patch. 
> Consider if this is a bug in the wacom driver or in the usbhid core? IMO 
> this is a usbhid bug: the critical region in hid_ctrl() is too big, 
> there is no reason for the call to hid_input_report() to be protected by 
> usbhid->lock.

Hmm, it's actually true that we might not need usbhid->lock during 
hid_input_report() at the end of the day, as we shouldn't be doing any 
URB-related operations there, neither iofl are being manipulated.

If you have already done the full analysis that shows that usbhid->lock is 
indeed not needed, this absolutely needs to go into changelog as proper 
justification.

Could you please reformulate the changelog in this respect and resubmit?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] hid: usbhid: hid-core: fix recursive deadlock

[Ioan-Adrian Ratiu]

On Thu, 19 Nov 2015 10:10:19 +0100 (CET)
Jiri Kosina <jikos@kernel.org> wrote:

> On Thu, 19 Nov 2015, Ioan-Adrian Ratiu wrote:
> 
> > First part of lockdep report:
> > http://imgur.com/clLsCWe
> > 
> > Second part:
> > http://imgur.com/Wa2PzRl
> > 
> > Here are some printk's of mine while reproducing + debugging the issue:
> > http://imgur.com/SETOHT7  
> 
> So the real problem is that Intuos driver is calling hid_hw_request() 
> (which tries to grab the lock in usbhid_submit_report()) while handling 
> the CTRL IRQ (lock gets acquired there).
> 
> So the proper way to fix seems to be delaying the scheduling of the 
> proximity read event in wacom_intuos_inout() to workqueue.
> 
> > I'll continue to research this more in depth, but progress is slow 
> > because I don't have much time, I'm doing this in my spare time because 
> > it's my girlfriend's tablet.  
> 
> Oh, now I understand the level of severity of this bug! :-)
> 
> Thanks,
> 

Yes, exactly, you are beginning to understand! :)  When I've put my 2 variants
above to solve this deadlock, by "removing the call from wacom" at 1) I was
trying to say exactly this, removing it from the irq to a workqueue.

But please understand further my reasoning for submitting this patch. Consider
if this is a bug in the wacom driver or in the usbhid core? IMO
this is a usbhid bug: the critical region in hid_ctrl() is too big, there
is no reason for the call to hid_input_report() to be protected by
usbhid->lock.

The correct way to fix this deadlock is to fix the critical section in
usbhid, not remove the call from the wacom irq. If wacom wants to
reschedule in the irq, it should not deadlock on usbhid. "Fixing" the wacom call
would just work around the critical region bug inside usbhid.

I hope I've made myself clear this time; I really needed to explain this
patch better :( sorry.

Re: [PATCH] hid: usbhid: hid-core: fix recursive deadlock

[Jiri Kosina]

On Thu, 19 Nov 2015, Ioan-Adrian Ratiu wrote:

> First part of lockdep report:
> http://imgur.com/clLsCWe
> 
> Second part:
> http://imgur.com/Wa2PzRl
> 
> Here are some printk's of mine while reproducing + debugging the issue:
> http://imgur.com/SETOHT7

So the real problem is that Intuos driver is calling hid_hw_request() 
(which tries to grab the lock in usbhid_submit_report()) while handling 
the CTRL IRQ (lock gets acquired there).

So the proper way to fix seems to be delaying the scheduling of the 
proximity read event in wacom_intuos_inout() to workqueue.

> I'll continue to research this more in depth, but progress is slow 
> because I don't have much time, I'm doing this in my spare time because 
> it's my girlfriend's tablet.

Oh, now I understand the level of severity of this bug! :-)

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] hid: usbhid: hid-core: fix recursive deadlock

[Jiri Kosina]

On Wed, 18 Nov 2015, Ioan-Adrian Ratiu wrote:

> > > The critical section protected by usbhid->lock in hid_ctrl() is too
> > > big and in rare cases causes a recursive deadlock because of its call
> > > to hid_input_report().
> > > 
> > > This deadlock reproduces on newer wacom tablets like 056a:033c because
> > > the wacom driver in its irq handler ends up calling hid_hw_request()
> > > from wacom_intuos_schedule_prox_event() in wacom_wac.c. What this means
> > > is that it submits a report to reschedule a proximity read through a
> > > sync ctrl call which grabs the lock in hid_ctrl(struct urb *urb)
> > > before calling hid_input_report(). When the irq kicks in on the same
> > > cpu, it also tries to grab the lock resulting in a recursive deadlock.
> > > 
> > > The proper fix is to shrink the critical section in hid_ctrl() to
> > > protect only the instructions which modify usbhid, thus move the lock
> > > after the hid_input_report() call and the deadlock dissapears.  
> > 
> > I think the proper fix actually is to spin_lock_irqsave() in hid_ctrl(), 
> > isn't it?
> 
> That was my first attempt, yes, but the deadlock still happens with interrupts
> disabled. 

That unfortunately however directly implies that your explanation above 
isn't actually correct description of the real problem.

So we'd better first understand the problem rather than papering it over 
with more or less random fixes.

First, have you tried to run your usecase on your system with lockdep 
enabled?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] hid: usbhid: hid-core: fix recursive deadlock

[Ioan-Adrian Ratiu]

On Wed, 18 Nov 2015 17:58:56 -0600
Josh Cartwright <joshc@ni.com> wrote:

> On Wed, Nov 18, 2015 at 11:05:44PM +0200, Ioan-Adrian Ratiu wrote:
> > On Wed, 18 Nov 2015 21:37:42 +0100 (CET)
> > Jiri Kosina <jikos@kernel.org> wrote:
> >   
> > > On Wed, 18 Nov 2015, Ioan-Adrian Ratiu wrote:
> > >   
> > > > The critical section protected by usbhid->lock in hid_ctrl() is too
> > > > big and in rare cases causes a recursive deadlock because of its call
> > > > to hid_input_report().
> > > > 
> > > > This deadlock reproduces on newer wacom tablets like 056a:033c because
> > > > the wacom driver in its irq handler ends up calling hid_hw_request()
> > > > from wacom_intuos_schedule_prox_event() in wacom_wac.c. What this means
> > > > is that it submits a report to reschedule a proximity read through a
> > > > sync ctrl call which grabs the lock in hid_ctrl(struct urb *urb)
> > > > before calling hid_input_report(). When the irq kicks in on the same
> > > > cpu, it also tries to grab the lock resulting in a recursive deadlock.
> > > > 
> > > > The proper fix is to shrink the critical section in hid_ctrl() to
> > > > protect only the instructions which modify usbhid, thus move the lock
> > > > after the hid_input_report() call and the deadlock dissapears.    
> > > 
> > > I think the proper fix actually is to spin_lock_irqsave() in hid_ctrl(), 
> > > isn't it?
> > >   
> > 
> > That was my first attempt, yes, but the deadlock still happens with
> > interrupts disabled. It is very weird, I know.  
> 
> I think your best course of action is to figure out why this is the
> case, instead of continuing with trying to solve the symptoms.  Do you
> have actual callstacks showing the cases where you hit?  That might be
> useful to share (your lockdep picture cuts out the callstacks).
> 
> Also, have you tried without the PREEMPT_RT patch in the picture at all?
> 
>   Josh

Yes, of course I tried it without PREEMPT_RT_FULL :) This happens on vanilla
mainline kernels (only after 4.4-rc1 which introduced support for this kind of
tablets).

I also backported all the wacom patches to 4.1 non-RT and the same deadlock
happens.

I've sent another email with some lockdep traces and printk's on a running
vanilla linux-next, maybe it didn't get through, here are the links again:

First part of lockdep report:
http://imgur.com/clLsCWe

Second part:
http://imgur.com/Wa2PzRl

Here are some printk's of mine while reproducing + debugging the issue:
http://imgur.com/SETOHT7

I'll continue to research this more in depth, but progress is slow because I
don't have much time, I'm doing this in my spare time because it's my
girlfriend's tablet.

Re: [PATCH] hid: usbhid: hid-core: fix recursive deadlock

[Josh Cartwright]

[-- Attachment #1: Type: text/plain, Size: 1766 bytes --]

On Wed, Nov 18, 2015 at 11:05:44PM +0200, Ioan-Adrian Ratiu wrote:
> On Wed, 18 Nov 2015 21:37:42 +0100 (CET)
> Jiri Kosina <jikos@kernel.org> wrote:
> 
> > On Wed, 18 Nov 2015, Ioan-Adrian Ratiu wrote:
> > 
> > > The critical section protected by usbhid->lock in hid_ctrl() is too
> > > big and in rare cases causes a recursive deadlock because of its call
> > > to hid_input_report().
> > > 
> > > This deadlock reproduces on newer wacom tablets like 056a:033c because
> > > the wacom driver in its irq handler ends up calling hid_hw_request()
> > > from wacom_intuos_schedule_prox_event() in wacom_wac.c. What this means
> > > is that it submits a report to reschedule a proximity read through a
> > > sync ctrl call which grabs the lock in hid_ctrl(struct urb *urb)
> > > before calling hid_input_report(). When the irq kicks in on the same
> > > cpu, it also tries to grab the lock resulting in a recursive deadlock.
> > > 
> > > The proper fix is to shrink the critical section in hid_ctrl() to
> > > protect only the instructions which modify usbhid, thus move the lock
> > > after the hid_input_report() call and the deadlock dissapears.  
> > 
> > I think the proper fix actually is to spin_lock_irqsave() in hid_ctrl(), 
> > isn't it?
> > 
> 
> That was my first attempt, yes, but the deadlock still happens with interrupts
> disabled. It is very weird, I know.

I think your best course of action is to figure out why this is the
case, instead of continuing with trying to solve the symptoms.  Do you
have actual callstacks showing the cases where you hit?  That might be
useful to share (your lockdep picture cuts out the callstacks).

Also, have you tried without the PREEMPT_RT patch in the picture at all?

  Josh

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] hid: usbhid: hid-core: fix recursive deadlock

[Josh Cartwright]

[-- Attachment #1: Type: text/plain, Size: 1795 bytes --]

On Wed, Nov 18, 2015 at 11:05:44PM +0200, Ioan-Adrian Ratiu wrote:
> On Wed, 18 Nov 2015 21:37:42 +0100 (CET)
> Jiri Kosina <jikos-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
> > On Wed, 18 Nov 2015, Ioan-Adrian Ratiu wrote:
> > 
> > > The critical section protected by usbhid->lock in hid_ctrl() is too
> > > big and in rare cases causes a recursive deadlock because of its call
> > > to hid_input_report().
> > > 
> > > This deadlock reproduces on newer wacom tablets like 056a:033c because
> > > the wacom driver in its irq handler ends up calling hid_hw_request()
> > > from wacom_intuos_schedule_prox_event() in wacom_wac.c. What this means
> > > is that it submits a report to reschedule a proximity read through a
> > > sync ctrl call which grabs the lock in hid_ctrl(struct urb *urb)
> > > before calling hid_input_report(). When the irq kicks in on the same
> > > cpu, it also tries to grab the lock resulting in a recursive deadlock.
> > > 
> > > The proper fix is to shrink the critical section in hid_ctrl() to
> > > protect only the instructions which modify usbhid, thus move the lock
> > > after the hid_input_report() call and the deadlock dissapears.  
> > 
> > I think the proper fix actually is to spin_lock_irqsave() in hid_ctrl(), 
> > isn't it?
> > 
> 
> That was my first attempt, yes, but the deadlock still happens with interrupts
> disabled. It is very weird, I know.

I think your best course of action is to figure out why this is the
case, instead of continuing with trying to solve the symptoms.  Do you
have actual callstacks showing the cases where you hit?  That might be
useful to share (your lockdep picture cuts out the callstacks).

Also, have you tried without the PREEMPT_RT patch in the picture at all?

  Josh

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] hid: usbhid: hid-core: fix recursive deadlock

[Ioan-Adrian Ratiu]

On Wed, 18 Nov 2015 21:37:42 +0100 (CET)
Jiri Kosina <jikos@kernel.org> wrote:

> On Wed, 18 Nov 2015, Ioan-Adrian Ratiu wrote:
> 
> > The critical section protected by usbhid->lock in hid_ctrl() is too
> > big and in rare cases causes a recursive deadlock because of its call
> > to hid_input_report().
> > 
> > This deadlock reproduces on newer wacom tablets like 056a:033c because
> > the wacom driver in its irq handler ends up calling hid_hw_request()
> > from wacom_intuos_schedule_prox_event() in wacom_wac.c. What this means
> > is that it submits a report to reschedule a proximity read through a
> > sync ctrl call which grabs the lock in hid_ctrl(struct urb *urb)
> > before calling hid_input_report(). When the irq kicks in on the same
> > cpu, it also tries to grab the lock resulting in a recursive deadlock.
> > 
> > The proper fix is to shrink the critical section in hid_ctrl() to
> > protect only the instructions which modify usbhid, thus move the lock
> > after the hid_input_report() call and the deadlock dissapears.  
> 
> I think the proper fix actually is to spin_lock_irqsave() in hid_ctrl(), 
> isn't it?
> 

That was my first attempt, yes, but the deadlock still happens with interrupts
disabled. It is very weird, I know. I tried many configurations, like disabling
PREEMPT_RT and other stuff which might affect the call stack in this case, but
the only two methods which actually avoid the deadlock are:

1. don't call wacom_intuos_schedule_prox_event() / hid_hw_request() from the
wacom driver

2. shrink the critical region to not cover hid_input_report() inside hid_ctrl()

I am very open to any ideas on how to better fix this, just to be able to use a
mainline kernel with my device without out of tree patching :)

Re: [PATCH] hid: usbhid: hid-core: fix recursive deadlock

[Ioan-Adrian Ratiu]

On Wed, 18 Nov 2015 21:37:42 +0100 (CET)
Jiri Kosina <jikos-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> On Wed, 18 Nov 2015, Ioan-Adrian Ratiu wrote:
> 
> > The critical section protected by usbhid->lock in hid_ctrl() is too
> > big and in rare cases causes a recursive deadlock because of its call
> > to hid_input_report().
> > 
> > This deadlock reproduces on newer wacom tablets like 056a:033c because
> > the wacom driver in its irq handler ends up calling hid_hw_request()
> > from wacom_intuos_schedule_prox_event() in wacom_wac.c. What this means
> > is that it submits a report to reschedule a proximity read through a
> > sync ctrl call which grabs the lock in hid_ctrl(struct urb *urb)
> > before calling hid_input_report(). When the irq kicks in on the same
> > cpu, it also tries to grab the lock resulting in a recursive deadlock.
> > 
> > The proper fix is to shrink the critical section in hid_ctrl() to
> > protect only the instructions which modify usbhid, thus move the lock
> > after the hid_input_report() call and the deadlock dissapears.  
> 
> I think the proper fix actually is to spin_lock_irqsave() in hid_ctrl(), 
> isn't it?
> 

That was my first attempt, yes, but the deadlock still happens with interrupts
disabled. It is very weird, I know. I tried many configurations, like disabling
PREEMPT_RT and other stuff which might affect the call stack in this case, but
the only two methods which actually avoid the deadlock are:

1. don't call wacom_intuos_schedule_prox_event() / hid_hw_request() from the
wacom driver

2. shrink the critical region to not cover hid_input_report() inside hid_ctrl()

I am very open to any ideas on how to better fix this, just to be able to use a
mainline kernel with my device without out of tree patching :)
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] hid: usbhid: hid-core: fix recursive deadlock

[Jiri Kosina]

On Wed, 18 Nov 2015, Ioan-Adrian Ratiu wrote:

> The critical section protected by usbhid->lock in hid_ctrl() is too
> big and in rare cases causes a recursive deadlock because of its call
> to hid_input_report().
> 
> This deadlock reproduces on newer wacom tablets like 056a:033c because
> the wacom driver in its irq handler ends up calling hid_hw_request()
> from wacom_intuos_schedule_prox_event() in wacom_wac.c. What this means
> is that it submits a report to reschedule a proximity read through a
> sync ctrl call which grabs the lock in hid_ctrl(struct urb *urb)
> before calling hid_input_report(). When the irq kicks in on the same
> cpu, it also tries to grab the lock resulting in a recursive deadlock.
> 
> The proper fix is to shrink the critical section in hid_ctrl() to
> protect only the instructions which modify usbhid, thus move the lock
> after the hid_input_report() call and the deadlock dissapears.

I think the proper fix actually is to spin_lock_irqsave() in hid_ctrl(), 
isn't it?

-- 
Jiri Kosina
SUSE Labs

[PATCH] hid: usbhid: hid-core: fix recursive deadlock

[Ioan-Adrian Ratiu]

The critical section protected by usbhid->lock in hid_ctrl() is too
big and in rare cases causes a recursive deadlock because of its call
to hid_input_report().

This deadlock reproduces on newer wacom tablets like 056a:033c because
the wacom driver in its irq handler ends up calling hid_hw_request()
from wacom_intuos_schedule_prox_event() in wacom_wac.c. What this means
is that it submits a report to reschedule a proximity read through a
sync ctrl call which grabs the lock in hid_ctrl(struct urb *urb)
before calling hid_input_report(). When the irq kicks in on the same
cpu, it also tries to grab the lock resulting in a recursive deadlock.

The proper fix is to shrink the critical section in hid_ctrl() to
protect only the instructions which modify usbhid, thus move the lock
after the hid_input_report() call and the deadlock dissapears.

Signed-off-by: Ioan-Adrian Ratiu <adi@adirat.com>
---
 drivers/hid/usbhid/hid-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 36712e9..5dd426f 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -477,8 +477,6 @@ static void hid_ctrl(struct urb *urb)
 	struct usbhid_device *usbhid = hid->driver_data;
 	int unplug = 0, status = urb->status;
 
-	spin_lock(&usbhid->lock);
-
 	switch (status) {
 	case 0:			/* success */
 		if (usbhid->ctrl[usbhid->ctrltail].dir == USB_DIR_IN)
@@ -498,6 +496,8 @@ static void hid_ctrl(struct urb *urb)
 		hid_warn(urb->dev, "ctrl urb status %d received\n", status);
 	}
 
+	spin_lock(&usbhid->lock);
+
 	if (unplug) {
 		usbhid->ctrltail = usbhid->ctrlhead;
 	} else {
-- 
2.6.3