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

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.

Attilio


-- 
Peace can only be achieved by understanding - A. Einstein