skipping locks, mutex_owned, usb
Andriy Gapon
avg at FreeBSD.org
Wed Aug 31 16:13:25 UTC 2011
on 31/08/2011 16:43 John Baldwin said the following:
> On Sunday, August 28, 2011 5:27:44 am Hans Petter Selasky wrote:
>> On Sunday 28 August 2011 11:25:51 Andriy Gapon wrote:
>>> on 23/08/2011 15:09 Andriy Gapon said the following:
>>>> This "XXX cludge" [sic] pattern is scattered around a few functions in
>>>> the ukbd code and perhaps other usb code:
>>>> func()
>>>> {
>>>>
>>>> if (!mtx_owned(&Giant)) {
>>>>
>>>> mtx_lock(&Giant);
>>>>
>>>> func();
>>>> mtx_unlock(&Giant);
>>>>
>>>> return;
>>>>
>>>> }
>>>>
>>>> // etc ...
>>>>
>>>> }
>>>
>>> Ohhh, nothing seems simple with the USB code:
>>>
>>> /* make sure that the BUS mutex is not locked */
>>> drop_bus = 0;
>>> while (mtx_owned(&xroot->udev->bus->bus_mtx)) {
>>> mtx_unlock(&xroot->udev->bus->bus_mtx);
>>> drop_bus++;
>>> }
>>>
>>> /* make sure that the transfer mutex is not locked */
>>> drop_xfer = 0;
>>> while (mtx_owned(xroot->xfer_mtx)) {
>>> mtx_unlock(xroot->xfer_mtx);
>>> drop_xfer++;
>>> }
>>>
>>> So many unconventional tricks.
>>
>> Similar code is used in the DROP_GIANT and PICKUP_GIANT macros. You might
> want
>> to check all references to mtx_owned() in the kernel, and make a set of
>> exceptions for post-panic code execution.
>
> Giant is special because it is a hack to let us run non-MPSAFE code. Other
> locks should not follow its model.
>
Hans,
actually this makes me wonder... It seems that usbd_transfer_poll() function that
utilizes the above code is called from a number of xxx_poll routines in various
usb drivers (ukbd, umass and a bunch of serial drivers):
$ glimpse -l usbd_transfer_poll | fgrep .c
/usr/src/sys/dev/usb/input/ukbd.c
/usr/src/sys/dev/usb/usb_transfer.c
/usr/src/sys/dev/usb/storage/umass.c
/usr/src/sys/dev/usb/serial/ucycom.c
/usr/src/sys/dev/usb/serial/umoscom.c
/usr/src/sys/dev/usb/serial/umcs.c
/usr/src/sys/dev/usb/serial/umodem.c
/usr/src/sys/dev/usb/serial/uchcom.c
/usr/src/sys/dev/usb/serial/uipaq.c
/usr/src/sys/dev/usb/serial/ugensa.c
/usr/src/sys/dev/usb/serial/uark.c
/usr/src/sys/dev/usb/serial/ufoma.c
/usr/src/sys/dev/usb/serial/umct.c
/usr/src/sys/dev/usb/serial/ubsa.c
/usr/src/sys/dev/usb/serial/uslcom.c
/usr/src/sys/dev/usb/serial/uplcom.c
/usr/src/sys/dev/usb/serial/uvscom.c
/usr/src/sys/dev/usb/serial/uftdi.c
/usr/src/sys/dev/usb/serial/ubser.c
So why the mutex unwinding/rewinding code is present at all?
What kind of situations is it supposed to prevent?
Personally I think that we either know that those drivers should not hold the
locks in question (bus_mtx and xfer_mtx) or we know that they hold them.
I do not see why we have to be conditional here or have a loop even...
--
Andriy Gapon
More information about the freebsd-arch
mailing list