Re: panic: mutex Giant not owned at /usr/src/sys/kern/tty_ttydisc.c:1127

[...]

>> i've added taskqueue_drain() to detach() in my "stall" diff that i
>> posted few hours back. is that addresses some of your concerns?
>
> Yes, at detach you always need to drain by principle.

ok, fine :) taskqueue_drain() in detach is go :) move to the next item :)

>> also
>> keep in mind that while netgraph node is created and destroyed
>> primarily by the driver in response to hardware arrival and departure,
>> nothing protects it from the netgraph side. in other words there is
>> another way to manipulate netgraph node using netgraph api. for
>> example user can request shutdown of a netgraph node, etc. so all of
>> this has to be taking into consideration.
>
> Ok, I've fixed this: Destroy is now non-blocking USB-wise. See:
>
> http://perforce.freebsd.org/chv.cgi?CH=156586

i'm sorry, did i mention there is no sleeping in netgraph? :-)

again, sorry, but this is not going to work. you still doing
UBT_LOCK()/UBT_UNLOCK() in netgraph method. that is you are trying to
grab the same lock that is used for locking interfaces.

more importantly, like i said before, usb2_transfer_stop() does
USB_BUS_LOCK()/USB_BUS_UNLOCK(). is there a guarantee that another usb
device will not do something synchronously over the same usb bus?

i'm actually working on including some of your changes (except getting
rid of ubt_task), so, please, lets just stop for a second and talk
before we start commit war here :) please bear with me for just a
second, ok?

>> in fact, if i understand you correctly, with addition of
>> taskqueue_drain() to detach(), the later becomes completely
>> synchronous. that is
>>
>> 1) NG_NODE_REF()
>> 2) ng_rmnode_self (i.e. mark node as possibly dead, detach all the
>> hooks, etc. etc.)
>> 3) taskqueue_drain() (i.e. sleep until task has finished)
>> 4) usb2_transfer_unsetup() (which does "usb2_transfer_drain() and sleeps)
>> 5) NG_NODE_UNREF()
>> 6) free everything up
>>
>> in theory, items (1) and (5) above just an additional protection
>> because i was not sure about inner workings of usb2. i think, when we
>> sort everything out they can be removed.
>
> I have a question. What is the following doing at the middle of the
> ubt_detach():
>
>        if (node != NULL)
>                NG_NODE_UNREF(node);
>
> If you say that:
>
>                ng_rmnode_self(node);
>
> Cleans up the last reference?

the complete code is

node_p  node = sc->sc_node;

if (node != NULL) {
   sc->sc_node = NULL;  <-- clear sc_node

   NG_NODE_SET_PRIVATE(node, NULL);
   NG_NODE_REALLY_DIE(node);

   NG_NODE_REF(node); <--- grab +1 reference
   ng_rmnode_self(node); <--- mark node as "dead", but not ensure its
not free()d
}

/* bla, bla */

if (node != NULL)
   NG_NODE_UNREF(node); <--- drop 1 reference and possibly free() node

so, say we enter detach() with only one reference (from attach()).
when we see the sc_node pointer, we assume that there could be
multiple (> 1) references on it. so just for added protection we grab
one more (now reference count is 2). then we call ng_rmnode_self()
which will mark node as "dead" and drop one reference (now reference
count is 1). then we do "bla bla" stuff which could access node
pointer. the important thing is that node pointer still points to a
valid "dead" node, so NG_NODE_NOT_VALID() can be used to check if node
is "dead" or "alive". finally when "bla bla" stuff is done, we know we
are not going to access node pointer any mode and we drop our
reference (now reference count is 0 and node is destroyed).


>> i do not understand. if usb2_transfer_unsetup() calls
>> usb2_transfer_drain() and sleeps, then what difference does it make in
>> detach()?
>
> You are right that USB-wise it does not matter if you call
> usb2_transfer_drain() separately or if you call usb2_transfer_unsetup(). I'm
> just thinking with regard to the state inside bluetooth, because there are
> two contexts running in parallell. I.E. Callbacks might be executed in
> paralell to bluetooth tearing down.

do you mean transfer stop on hook disconnection? but those are
protected by interface locks, no? and like i said, i'm changing
ubt_task to call drain() instead of stop() to make stop completely
synchronous (which is actually a nice thing).

>> i just followed original code that did it this way, i.e.
>> drain and unsetup all transfers in one call.
>
> Yes, that's fine.

ok, fine. one more down :)

>> > In USB2 a "usb2_start_hardware()" call is always paired with a USB
>> > transfer callback, no matter what you do! This way of design has been
>> > chosen so that you can do refcounts inside the callback!
>>
>> ok, and that is exactly what code does. the only thing is that
>> refcounts are on netgraph node and not on softc structure. again the
>> reason for that is because netgraph node can be accessed from both
>> usb2 driver and netgraph subsystem directly.
>
> Isn't it enough to increment the refcount at attach and decrement it at
> detach ????

it would be if we could sleep in netgraph, and we can not. in theory,
we only need one extra reference which would tell us if there anything
in usb2 that still could access node pointer. in practice, instead of
checking every transfer and making sure its no pending before dropping
that extra reference i just add multiple references for each usb2
transfer and ubt_task (i.e. things that could access node pointer).

[...]

>> well, when i looked at the code, i saw that both functions do
>> USB_BUS_LOCK() and USB_BUS_UNLOCK().
>
> Well, this is the lock of the USB IRQ handler. One per USB controller. There
> are no more locks than this that will be locked. The IRQ handler is called
> from a high priority context and will not block very long. Same with USB
> callbacks.

right, and what if some other usb device attached to the same usb
controller is doing something synchronously? do you see that you could
potentially block netgraph for arbitrary time and that is really out
of ng_ubt2 driver control?

>> also, now that you mentioned it, i should call usb2_transfer_drain()
>> in ubt_task() instead of usb2_transfer_stop() to make transfer stop
>> completely synchronous as well.
>
> When I think about it, usb2_transfer_stop() is enough. See my latest Patch in
> P4.

please read my comment above.

>> please let me reiterate, sleeping in netgraph is NOT allowed. even if
>> its for a few milliseconds. please accept it as a fact. think of all
>> netgraph methods as fast interrupt handlers. it is very typical for
>> fast interrupt handlers to do minimal amount of work and schedule swi
>> to do more heavy processing. that is exactly what the code does here.
>
> Fixed!

i beg to differ :)

thanks,
max