skipping locks, mutex_owned, usb

[Andriy Gapon]

on 23/08/2011 16:11 John Baldwin said the following:
> On Tuesday, August 23, 2011 8:09:15 am Andriy Gapon wrote:
>>
>> Yes, the subject sounds quite hairy, so please let me try to explain it.
>> First, let's consider one concrete function:
>>
>> static int
>> ukbd_poll(keyboard_t *kbd, int on)
>> {
>>         struct ukbd_softc *sc = kbd->kb_data;
>>
>>         if (!mtx_owned(&Giant)) {
>>                 /* XXX cludge */
>>                 int retval;
>>                 mtx_lock(&Giant);
>>                 retval = ukbd_poll(kbd, on);
>>                 mtx_unlock(&Giant);
>>                 return (retval);
>>         }
>>
>>         if (on) {
>>                 sc->sc_flags |= UKBD_FLAG_POLLING;
>>                 sc->sc_poll_thread = curthread;
>>         } else {
>>                 sc->sc_flags &= ~UKBD_FLAG_POLLING;
>>                 ukbd_start_timer(sc);   /* start timer */
>>         }
>>         return (0);
>> }
>>
>> 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 ...
>> }
>>
>> I have a few question directly and indirectly related to this pattern.
>>
>> I.  [Why] do we need this pattern?
>> Can the code be re-written in a smarter (if not to say proper) way?
> 
> Giant is recursive, it should just be always acquired.  Also, this recursive
> call pattern is very non-obvious.  A far more straightforward approach would
> be to just have:
> 
> static int
> ukbd_poll(keyboard_t *kbd, int on)
> {
>         struct ukbd_softc *sc = kbd->kb_data;
> 
>         mtx_lock(&Giant);
>         if (on) {
>                 sc->sc_flags |= UKBD_FLAG_POLLING;
>                 sc->sc_poll_thread = curthread;
>         } else {
>                 sc->sc_flags &= ~UKBD_FLAG_POLLING;
>                 ukbd_start_timer(sc);   /* start timer */
>         }
>         mtx_unlock(&Giant);
>         return (0);
> }
> 

Thank you for clarifying this, I agree this is simpler than the original code and
should work exactly the same.

There is more trouble with a few other functions that actually behave differently
(even if slightly) depending on what mtx_owned(&Giant) returns.  Not sure if
that's a legal use or an antipattern.

For example: ukbd_ioctl, ukbd_check, ukbd_check_char, ukbd_read, ukbd_read_char.


-- 
Andriy Gapon