Re: Removing USB keyboard after filesystems synced causes panic with destroyed mutex twa(4)?

From: Garrett Cooper <yanefbsd_at_gmail.com>
Date: Tue, 13 Apr 2010 07:44:27 -0700
On Tue, Apr 13, 2010 at 3:50 AM, Attilio Rao <attilio_at_freebsd.org> wrote:
> 2010/4/13 Attilio Rao <attilio_at_freebsd.org>:
>> 2010/3/13 Garrett Cooper <yanefbsd_at_gmail.com>:
>>> On Wed, Mar 10, 2010 at 9:58 PM, Garrett Cooper <yanefbsd_at_gmail.com> wrote:
>>>> On Wed, Mar 10, 2010 at 2:07 PM, Tom Couch <tom.couch.storage_at_gmail.com> wrote:
>>>>> Hi FreeBSD-current,
>>>>>     My name is Tom Couch,
>>>>> I am part of the 3ware driver team recently acquired by LSI.
>>>>> I believe Giovanni's patch, below, is the correct fix for this bug.
>>>>>
>>>>> I am available to maintain the twa driver, now that I am on this list.
>>>>> Let me know how I can help,
>>>>>
>>>>> Tom
>>>>>
>>>>>
>>>>> On Wed, Mar 10, 2010 at 1:56 PM, Giovanni Trematerra <
>>>>> giovanni.trematerra_at_gmail.com> wrote:
>>>>>
>>>>>> On Sun, Mar 7, 2010 at 11:24 AM, Garrett Cooper <yanefbsd_at_gmail.com>
>>>>>> wrote:
>>>>>> > On Sun, Mar 7, 2010 at 2:07 AM, Garrett Cooper <yanefbsd_at_gmail.com>
>>>>>> wrote:
>>>>>> >> Hi Alexander and Hans,
>>>>>> >>    I recently did the following which generated a panic on a
>>>>>> >> 9-CURRENT kernel compiled on the 26th:
>>>>>> >>
>>>>>> >> 1. Executed reboot
>>>>>> >> 2. Removed keyboard.
>>>>>> >> 3. Some time after `All buffers synced\nUptime: ...' was displayed,
>>>>>> >> the keyboard was registered disconnected.
>>>>>> >> 4. The interrupt was delivered to my twa(4) enabled card and the
>>>>>> >> kernel panicked, like so:
>>>>>> >>
>>>>>> >> ugen2.2: <Mitsumi Electric> at usbus2 (disconnected)
>>>>>> >> uhub8: at uhub2, port 1, addr 2 (disconnected)
>>>>>> >> ugen2.3: <Mitsumi Electric> at usbus2 (disconnected)
>>>>>> >> ukbd0: at uhub8, port 3, addr 3 (disconnected)
>>>>>> >> uhid0: at uhub8, port 3, addr 3 (disconnected)
>>>>>> >> panic: mtx_lock_spin() of destroyed mutex _at_
>>>>>> /usr/src/sys/dev/twa/tw_cl_intr.c:88
>>>>>> >>
>>>>>> >> cpuid = 1
>>>>>> >> KDB: enter: panic
>>>>>> >> [thread pid 12 tid 100025 ]
>>>>>> >> Stopped at         kdb_enter+0x3d: movq     $0,0x40289c(%rip)
>>>>>> >> db>
>>>>>> >>
>>>>>> >>    I wish I could provide you with more details, but unfortunately I
>>>>>> >> the USB bus isn't registering the fact that I'm reattaching the
>>>>>> >> keyboard right now and the box won't reboot automatically :( (didn't
>>>>>> >> set the right sysctl beforehand to panic automatically). I'll try and
>>>>>> >> reproduce the issue again, but I was just wondering whether or not you
>>>>>> >> guys had seen this problem before.
>>>>>> >
>>>>>> >    Phew... it's reproducible with that kernel. Here's what I did
>>>>>> > exactly (because my original directions were incorrect):
>>>>>> >    1. Hit power button (for S5).
>>>>>> >    2. Disconnect keyboard RIGHT as `Uptime: ...' is displayed.
>>>>>> >    Kernel panicked on my system again. Now to figure out if it still
>>>>>> > exists with a kernel compiled today, and also how to debug it if it
>>>>>> > still does exist :/...
>>>>>> > Thanks,
>>>>>> > -Garrett
>>>>>>
>>>>>> Hi Garrett,
>>>>>> Could you please try the patch below and report back?
>>>>>>
>>>>>> Thank you
>>>>>>
>>>>>> diff -r cab6489de66d sys/dev/twa/tw_cl_intr.c
>>>>>> --- a/sys/dev/twa/tw_cl_intr.c        Wed Mar 03 04:51:13 2010 -0500
>>>>>> +++ b/sys/dev/twa/tw_cl_intr.c        Wed Mar 10 06:29:05 2010 -0500
>>>>>> _at__at_ -75,9 +75,12 _at__at_ tw_cl_interrupt(struct tw_cl_ctlr_handle
>>>>>>      if (ctlr == NULL)
>>>>>>               goto out;
>>>>>>
>>>>>> -     /* If we get an interrupt while resetting, it is a shared
>>>>>> -        one for another device, so just bail */
>>>>>> -     if (ctlr->state & TW_CLI_CTLR_STATE_RESET_IN_PROGRESS)
>>>>>> +     /*
>>>>>> +      *  If we get an interrupt while resetting or shutting down,
>>>>>> +      *  it is a shared one for another device, so just bail
>>>>>> +      */
>>>>>> +     if (ctlr->state & TW_CLI_CTLR_STATE_RESET_IN_PROGRESS ||
>>>>>> +                     (ctrl->state & TW_CLI_CTLR_STATE_ACTIVE) == 0)
>>>>>>              goto out;
>>>>>>
>>>>>>      /*
>>>
>>> Apart from the typo above (s/ctrl/ctlr/), things work appropriately
>>> now at reboot. The only problem is that bootup is really wonky now,
>>> because the RAID had a LOT of issues attaching to cam(4) (failed in
>>> 2/3 cold boot attempts); an additional branch condition may need to be
>>> added to the above if-statement if this change didn't take that into
>>> account. However, if the old behavior was incorrect and the new
>>> behavior is correct, s.t. the RAID controller demonstrating bus
>>> detection timeout issue that is occurring with a lot of USB devices
>>> and some RAID controllers today, this could be extremely problematic.
>>>
>>> So, while it looks better than before at reboot, it's not ready yet
>>> for prime time; I'd rather that the bug was filed with the patch you
>>> provided after the typo fixed, with the caveat mentioned and NOT
>>> committed, because the adverse affect(s) seem a bit more annoying than
>>> the previous panic issue described.
>>
>> I looked briefly at it and I think there are 2 bugs, one in
>> twa_detach() and another one in twa_shutdown().
>> Basically, locks get destroyed in tw_cl_shutdown_ctlr() which is
>> called by twa_shutdown() while interrupts are teared down in
>> tw_osli_free_resource(). twa_shutdown() is called in twa_detach()
>> before than tw_osli_free_resource().
>> tw_cl_shutdown_ctlr() also takes care to disable the interrupts for
>> twa but a problem can arise with shared IRQ. Infact, the handler will
>> remain on the IRQ until the bus_intr_teardown() takes place and it may
>> run, trying to acknowledge the interrupt, but with destroyed lock, if
>> an interrupt is sent by a shared source between twa_shutdown() and
>> tw_osli_free_resource() call in twa_detach() or just after a simple
>> call to twa_shutdown().
>>
>> Problems I see here:
>> - twa_shutdown() should not destroy the mutex at all, it is not
>> something our shutdown handlers generally do and it might be kept in
>> sync
>> - twa_detach() might do a first half of tw_cl_shutdown_ctlr(), do the
>> resource deallocation and just at the end destroy mutexes. That is how
>> generally our detach handler works.
>>
>> All these solutions would mean refactoring the tw_osli_free_resource()
>> and tw_cl_shutdown_ctlr(). I don't know very well the twa code, but it
>> seems to me that we want to keep the driver very compatible with any
>> vendor version or such? If yes this may be a problem because the
>> failing patterns are all located into the shared code and an ideal
>> solution could be more difficult to find out. Otherwise a fix might be
>> simple to hammer down.
>
> Forgot to tell: twe might have the same problem even if it doesn't
> expose just for luckiness.

    Hmmm... ok. I don't have a twe enabled card so I can't verify
whether or not this problem exists :/.
Thanks for the comments!
-Garrett
Received on Tue Apr 13 2010 - 12:44:32 UTC

This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:40:02 UTC