From: Thomas Gleixner <tglx@linutronix.de> To: Michal Nazarewicz <m.nazarewicz@samsung.com> Cc: linux-usb@vger.kernel.org, Michal Nazarewicz <mina86@mina86.com>, Davide Libenzi <davidel@xmailserver.org>, Greg KH <greg@kroah.com>, Kyungmin Park <kyungmin.park@samsung.com>, Marek Szyprowski <m.szyprowski@samsung.com>, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCHv2 1/8] wait_event_interruptible_locked() interface Date: Sun, 11 Apr 2010 17:02:24 +0200 (CEST) [thread overview] Message-ID: <alpine.LFD.2.00.1004111638240.32352@localhost.localdomain> (raw) In-Reply-To: <3baf39971e1f49e98498f5d00e21df4302396252.1270835924.git.mina86@mina86.com> On Fri, 9 Apr 2010, Michal Nazarewicz wrote: > New wait_event_interruptible{,_exclusive}_locked{,_irq,_irqsave} > macros added. They work just like versions without _locked* suffix The _irqsave variant is really not necessary. It's actively wrong. If you go to wait then the state _before_ acquiring the waitqueue head lock must be irqs enabled. Otherwise you would schedule with interrupts disabled after the unlock_irqrestore which is a BUG. So if there is code which uses spin_lock_irqsave() in the wait path then this code is wrong and needs to be fixed to spin_(un)lock_irq() first instead of adding a bogus interface. > + > +#define __wait_event_interruptible_locked(wq, condition, ret, exclusive, lock, unlock, lock_args) \ That will also simplify this to (wq, condition, exclusive, lockmode) > +do { \ > + DEFINE_WAIT(__wait); \ > + \ > + if (exclusive) \ > + __wait.flags |= WQ_FLAG_EXCLUSIVE; \ > + else \ > + __wait.flags &= ~WQ_FLAG_EXCLUSIVE; \ > + __add_wait_queue_tail(&(wq), &__wait); \ > + \ > + do { \ > + set_current_state(TASK_INTERRUPTIBLE); \ > + if (signal_pending(current)) { \ > + ret = -ERESTARTSYS; \ > + break; \ > + } \ > + spin_unlock ##unlock lock_args; \ > + schedule(); \ > + spin_lock ##lock lock_args; \ > + } while (!(condition)); \ > + __remove_wait_queue(&(wq), &__wait); \ > + __set_current_state(TASK_RUNNING); \ > +} while (0) > + > + > +/** > + * wait_event_interruptible_locked - sleep until a condition gets true > + * @wq: the waitqueue to wait on > + * @condition: a C expression for the event to wait for > + * > + * The process is put to sleep (TASK_INTERRUPTIBLE) until the > + * @condition evaluates to true or a signal is received. > + * The @condition is checked each time the waitqueue @wq is woken up. > + * > + * It must be called with wq.lock being held. This spinlock is > + * unlocked while sleeping but @condition testing is done while lock > + * is held and when this macro exits the lock is held. > + * > + * The lock is locked/unlocked using spin_lock()/spin_unlock() > + * functions which must match the way they are locked/unlocked outside > + * of this macro. > + * > + * wake_up_locked() has to be called after changing any variable that could > + * change the result of the wait condition. > + * > + * The function will return -ERESTARTSYS if it was interrupted by a > + * signal and 0 if @condition evaluated to true. > + */ > +#define wait_event_interruptible_locked(wq, condition) \ > +({ \ > + int __ret = 0; \ > + if (!(condition)) \ > + __wait_event_interruptible_locked(wq, condition, __ret, 0, , , (&(wq).lock)); \ I had to look more than once to figure out how that code might return anything else than 0. Can we please change that to if (!(condition)) __ret = __wait_.....(); to make that less confusing ? > + __ret; \ > +}) Thanks, tglx
WARNING: multiple messages have this Message-ID (diff)
From: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org> To: Michal Nazarewicz <m.nazarewicz-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> Cc: linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Michal Nazarewicz <mina86-deATy8a+UHjQT0dZR+AlfA@public.gmane.org>, Davide Libenzi <davidel-AhlLAIvw+VEjIGhXcJzhZg@public.gmane.org>, Greg KH <greg-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>, Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>, Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Subject: Re: [PATCHv2 1/8] wait_event_interruptible_locked() interface Date: Sun, 11 Apr 2010 17:02:24 +0200 (CEST) [thread overview] Message-ID: <alpine.LFD.2.00.1004111638240.32352@localhost.localdomain> (raw) In-Reply-To: <3baf39971e1f49e98498f5d00e21df4302396252.1270835924.git.mina86-deATy8a+UHjQT0dZR+AlfA@public.gmane.org> On Fri, 9 Apr 2010, Michal Nazarewicz wrote: > New wait_event_interruptible{,_exclusive}_locked{,_irq,_irqsave} > macros added. They work just like versions without _locked* suffix The _irqsave variant is really not necessary. It's actively wrong. If you go to wait then the state _before_ acquiring the waitqueue head lock must be irqs enabled. Otherwise you would schedule with interrupts disabled after the unlock_irqrestore which is a BUG. So if there is code which uses spin_lock_irqsave() in the wait path then this code is wrong and needs to be fixed to spin_(un)lock_irq() first instead of adding a bogus interface. > + > +#define __wait_event_interruptible_locked(wq, condition, ret, exclusive, lock, unlock, lock_args) \ That will also simplify this to (wq, condition, exclusive, lockmode) > +do { \ > + DEFINE_WAIT(__wait); \ > + \ > + if (exclusive) \ > + __wait.flags |= WQ_FLAG_EXCLUSIVE; \ > + else \ > + __wait.flags &= ~WQ_FLAG_EXCLUSIVE; \ > + __add_wait_queue_tail(&(wq), &__wait); \ > + \ > + do { \ > + set_current_state(TASK_INTERRUPTIBLE); \ > + if (signal_pending(current)) { \ > + ret = -ERESTARTSYS; \ > + break; \ > + } \ > + spin_unlock ##unlock lock_args; \ > + schedule(); \ > + spin_lock ##lock lock_args; \ > + } while (!(condition)); \ > + __remove_wait_queue(&(wq), &__wait); \ > + __set_current_state(TASK_RUNNING); \ > +} while (0) > + > + > +/** > + * wait_event_interruptible_locked - sleep until a condition gets true > + * @wq: the waitqueue to wait on > + * @condition: a C expression for the event to wait for > + * > + * The process is put to sleep (TASK_INTERRUPTIBLE) until the > + * @condition evaluates to true or a signal is received. > + * The @condition is checked each time the waitqueue @wq is woken up. > + * > + * It must be called with wq.lock being held. This spinlock is > + * unlocked while sleeping but @condition testing is done while lock > + * is held and when this macro exits the lock is held. > + * > + * The lock is locked/unlocked using spin_lock()/spin_unlock() > + * functions which must match the way they are locked/unlocked outside > + * of this macro. > + * > + * wake_up_locked() has to be called after changing any variable that could > + * change the result of the wait condition. > + * > + * The function will return -ERESTARTSYS if it was interrupted by a > + * signal and 0 if @condition evaluated to true. > + */ > +#define wait_event_interruptible_locked(wq, condition) \ > +({ \ > + int __ret = 0; \ > + if (!(condition)) \ > + __wait_event_interruptible_locked(wq, condition, __ret, 0, , , (&(wq).lock)); \ I had to look more than once to figure out how that code might return anything else than 0. Can we please change that to if (!(condition)) __ret = __wait_.....(); to make that less confusing ? > + __ret; \ > +}) Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2010-04-11 15:02 UTC|newest] Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top 2010-04-09 19:21 [PATCH 0/7] The FunctionFS composite function Michal Nazarewicz 2010-04-09 19:21 ` [PATCHv2 1/8] wait_event_interruptible_locked() interface Michal Nazarewicz 2010-04-09 19:21 ` [PATCHv2 2/8] fs/timerfd.c: make use of wait_event_interruptible_locked_irq() Michal Nazarewicz 2010-04-09 19:21 ` [PATCHv2 3/8] USB: gadget: __init and __exit tags removed Michal Nazarewicz 2010-04-09 19:21 ` [PATCHv2 4/8] USB: f_fs: the FunctionFS driver Michal Nazarewicz 2010-04-09 19:21 ` [PATCHv2 5/8] USB: g_ffs: the FunctionFS gadget driver Michal Nazarewicz 2010-04-09 19:21 ` [PATCHv2 6/8] USB: ffs-test: FunctionFS testing program Michal Nazarewicz 2010-04-29 22:15 ` [PATCHv2 3/8] USB: gadget: __init and __exit tags removed Greg KH 2010-04-29 23:02 ` Michal Nazarewicz 2010-04-29 23:22 ` Greg KH 2010-04-30 5:41 ` Michal Nazarewicz 2010-04-11 14:31 ` [PATCHv2 2/8] fs/timerfd.c: make use of wait_event_interruptible_locked_irq() Thomas Gleixner 2010-04-11 19:16 ` Michal Nazarewicz 2010-04-11 19:16 ` Michal Nazarewicz 2010-04-11 15:02 ` Thomas Gleixner [this message] 2010-04-11 15:02 ` [PATCHv2 1/8] wait_event_interruptible_locked() interface Thomas Gleixner 2010-04-11 19:27 ` Michal Nazarewicz -- strict thread matches above, loose matches on Subject: below -- 2010-04-07 13:41 [PATCH 0/7] The FunctionFS composite function Michal Nazarewicz 2010-04-07 13:41 ` [PATCH 1/8] USB: composite: allow optional removal of __init and __exit tags Michal Nazarewicz 2010-04-07 13:41 ` [PATCH 2/8] sched: __wake_up_locked() exported Michal Nazarewicz 2010-04-07 13:41 ` [PATCH 3/8] USB: f_fs: the FunctionFS driver Michal Nazarewicz 2010-04-07 13:41 ` [PATCH 4/8] USB: Ethernet: allow optional removal of __init and __init_data tags Michal Nazarewicz 2010-04-07 13:41 ` [PATCH 5/8] USB: g_ffs: the FunctionFS gadget driver Michal Nazarewicz 2010-04-07 13:41 ` [PATCH 6/8] USB: ffs-test: FunctionFS testing program Michal Nazarewicz 2010-04-07 13:41 ` [PATCH 7/8] USB: testusb: imported David Brownell's USB testing application Michal Nazarewicz 2010-04-07 13:41 ` [PATCH 8/8] USB: testusb: testusb compatibility with FunctionFS gadget Michal Nazarewicz 2010-04-08 0:30 ` Greg KH 2010-04-08 0:29 ` [PATCH 7/8] USB: testusb: imported David Brownell's USB testing application Greg KH 2010-04-09 19:21 ` [PATCHv2 7/8] USB: testusb: an " Michal Nazarewicz 2010-04-09 19:21 ` [PATCHv2 8/8] USB: testusb: testusb compatibility with FunctionFS gadget Michal Nazarewicz 2010-04-10 22:51 ` [PATCHv2 7/8] USB: testusb: an USB testing application David Brownell 2010-04-14 9:30 ` [PATCH 7/8] USB: testusb: imported David Brownell's " David Brownell 2010-04-14 16:46 ` Greg KH 2010-04-14 16:47 ` Greg KH 2010-04-08 6:10 ` Heikki Krogerus 2010-04-08 6:18 ` Greg KH 2010-04-08 6:34 ` Heikki Krogerus 2010-04-14 9:41 ` David Brownell 2010-04-14 12:50 ` Michał Nazarewicz 2010-04-14 16:47 ` Greg KH 2010-04-07 17:11 ` [PATCH 3/8] USB: f_fs: the FunctionFS driver Michał Nazarewicz 2010-04-07 15:29 ` [PATCH 2/8] sched: __wake_up_locked() exported Greg KH 2010-04-07 17:11 ` Michał Nazarewicz 2010-04-08 0:28 ` Greg KH 2010-04-07 15:28 ` [PATCH 1/8] USB: composite: allow optional removal of __init and __exit tags Greg KH 2010-04-07 15:39 ` Michał Nazarewicz 2010-04-08 0:26 ` Greg KH
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=alpine.LFD.2.00.1004111638240.32352@localhost.localdomain \ --to=tglx@linutronix.de \ --cc=davidel@xmailserver.org \ --cc=greg@kroah.com \ --cc=kyungmin.park@samsung.com \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-usb@vger.kernel.org \ --cc=m.nazarewicz@samsung.com \ --cc=m.szyprowski@samsung.com \ --cc=mina86@mina86.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.