From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bruno =?UTF-8?B?UHLDqW1vbnQ=?= Subject: Re: [PATCH v2 1/6] hid: new driver for PicoLCD device Date: Mon, 22 Mar 2010 12:38:40 +0100 Message-ID: <20100322123840.7351cc82@neptune.home> References: <20100320170014.440959a8@neptune.home> <20100320170241.55258b0d@neptune.home> <20100321034600.GE29360@core.coreip.homeip.net> <20100321173737.5fcf3580@neptune.home> <20100322043508.GC31621@core.coreip.homeip.net> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: <20100322043508.GC31621-WlK9ik9hQGAhIp7JRqBPierSzoNAToWh@public.gmane.org> Sender: linux-usb-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Dmitry Torokhov Cc: Jiri Kosina , linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, "Rick L. Vinyard Jr." , Nicu Pavel , Oliver Neukum , Jaya Kumar List-Id: linux-input@vger.kernel.org On Sun, 21 March 2010 Dmitry Torokhov wrote= : > On Sun, Mar 21, 2010 at 05:37:37PM +0100, Bruno Pr=C3=A9mont wrote: > > On Sat, 20 March 2010 Dmitry Torokhov w= rote: > > > > + usbhid_submit_report(data->hdev, report, USB_DIR_OUT); > > > > + complete_all(&data->ready); > > > > + INIT_COMPLETION(data->ready); > > >=20 > > > Umm, what does this do, exactly? > >=20 > > It wakes up anyone waiting on the completion and then resets the co= mpletion > > as otherwise any future attempt to wait on it would succeed immedia= tely. > >=20 >=20 > You realize that if you re-initialize the completion right after > signalling it there is a big chance the waiters will miss it (they do > check completion->done flags that you reset right away. >=20 > In general completions are suited for something that happens once (a > single request - allocated - processed - signalled) but not for > repeating use. Would below approach be more correct? - move the completion to struct picolcd_pending so it meets the "happens once" requirement - protect data->pending with a mutex (though also use spinlock to prevent race between event which signals the completion and picolcd_send_and_wait() around timeout) - use the data->lock spinlock to protect multi-report requests from interleaving In speudo-code this would be something like: picolcd_send_and_wait(...) { struct picolcd_pending pending; ... init_completion(&pending->ready); aquire_mutex(data->mutex); spin_lock_irqsave(&data->lock, flags); ... // prepare report data->pending =3D &pending; usbhid_submit_report(data->hdev, report, USB_DIR_OUT); spin_unlock_irqrestore(&data->lock, flags); wait_for_completion_interruptible_timeout(&pending->ready, HZ*2); spin_lock_irqsave(&data->lock, flags); if (data->pending =3D=3D &pending) data->pending =3D NULL; spin_unlock_irqrestore(&data->lock, flags); release_mutext(data->mutex); ... } picolcd_raw_event(...) { ... spin_lock_irqsave(&data->lock, flags); if (data->pending) { // copy event data to pending ... complete(&pending->ready); } spin_unlock_irqrestore(&data->lock, flags); ... } In picolcd_remove() and picolcd_reset() I could then do something similar to picolcd_raw_event() to trigger completion with no data/error in order to skip timeout on the requester side. Though how should I prevent races on hot-unplug? Thanks, Bruno -- 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