Re: [RFC UPDATE PATCH] add wait_event_*_lock() functions and comments

[Nish Aravamudan <nish.aravamudan@gmail.com>]

On Sun, 13 Feb 2005 03:41:01 +0100, Arnd Bergmann <arnd@arndb.de> wrote:
> On Sünnavend 12 Februar 2005 14:28, Sergey Vlasov wrote:
> > On Sat, 12 Feb 2005 12:38:26 +0100 Arnd Bergmann wrote:
> > > #define __wait_event_lock(wq, condition, lock, flags)                  \
> > > do {                                                                   \
> > >        DEFINE_WAIT(__wait);                                            \
> > >                                                                        \
> > >        for (;;) {                                                      \
> > >                prepare_to_wait(&wq, &__wait, TASK_UNINTERRUPTIBLE);    \
> > >                spin_lock_irqsave(lock, flags);                         \
> > >                if (condition)                                          \
> > >                        break;                                          \
> > >                spin_unlock_irqrestore(lock, flags);                    \
> > >                schedule();                                             \
> > >        }                                                               \
> > >        spin_unlock_irqrestore(lock, flags);                            \
> > >        finish_wait(&wq, &__wait);                                      \
> > > } while (0)
> >
> > But in this case the result of testing the condition becomes useless
> > after spin_unlock_irqrestore - someone might grab the lock and change
> > things.   Therefore the calling code would need to add a loop around
> > wait_event_lock - and the wait_event_* macros were added precisely to
> > encapsulate such a loop and avoid the need to code it manually.
> 
> Ok, i understand now what the patch really wants to achieve. However,
> I'm not convinced it's a good idea. In the usb/gadget/serial.c driver,
> this appears to work only because an unconventional locking scheme is
> used, i.e. there is an extra flag (port->port_in_use) that is set to
> tell other functions about the state of the lock in case the lock holder
> wants to sleep.
> 
> Is there any place in the kernel that would benefit of the
> wait_event_lock() macro family while using locks without such
> special magic?

Sorry for replying from a different account, but it's the best I can
do right now. I know while I was scanning the whole kernel for other
wait_event*() replacements, I thought at least a handful of times,
"ugh, I could replace this whole block of code, except for that lock!"
I will try to get you a more concrete example on Monday. Thanks for
the feedback & patience!

-Nish