skipping locks, mutex_owned, usb

[Warner Losh]

On Aug 31, 2011, at 10:13 AM, Andriy Gapon wrote:

> 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...

Today, I think we know these things.  In the past, as the code was written, there was a lot more special case code needed for giant cohabitation.

Warner