From owner-freebsd-usb@FreeBSD.ORG Mon Nov 1 02:03:54 2010 Return-Path: Delivered-To: freebsd-usb@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id CB7C81065693; Mon, 1 Nov 2010 02:03:54 +0000 (UTC) (envelope-from weongyo.jeong@gmail.com) Received: from mail-gx0-f182.google.com (mail-gx0-f182.google.com [209.85.161.182]) by mx1.freebsd.org (Postfix) with ESMTP id 6AB7D8FC29; Mon, 1 Nov 2010 02:03:53 +0000 (UTC) Received: by gxk9 with SMTP id 9so3133603gxk.13 for ; Sun, 31 Oct 2010 19:03:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:received:from:date:to:cc :subject:message-id:reply-to:references:mime-version:content-type :content-disposition:in-reply-to:user-agent:organization :x-operation-sytem; bh=dGWfNCNewvAMxcsed2FK4dqGodVcTEo3VaQOeRWAUkU=; b=KY9vwycs08AoX+jlJrAdSyasAlsXTa+nsg6B1f4z3nlqxqMLTlceBeWE6TCVBa3Uzn EoPGN5gR85IIB8MKLf3F0yFuFxw2VxY/s74STB/3ynZBD4rzvZhxfVwECmQA8ja58SLW ve7nZT/SEIPuz/sR9TcaltzTlQqbcr2qx/2e0= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=from:date:to:cc:subject:message-id:reply-to:references:mime-version :content-type:content-disposition:in-reply-to:user-agent :organization:x-operation-sytem; b=qx/eDD+K+Z5uCzQ1xScR3yk8AuY553nJnIM5wRT+k0Ob5V2GxnehG+YWoePbWwdwuo 2smFLdOapdDmnIKc4Skj2SG8teU2Fi7PLFABtKa7BAxRE2XYKKaDnFUjQK6N9deSSdsc kiaUEav9AS6xnO4ZmVW8fNUXsrsZTK7UFp6rI= Received: by 10.229.191.85 with SMTP id dl21mr4110776qcb.260.1288577033267; Sun, 31 Oct 2010 19:03:53 -0700 (PDT) Received: from weongyo ([174.35.1.224]) by mx.google.com with ESMTPS id k15sm4560429qcu.11.2010.10.31.19.03.50 (version=SSLv3 cipher=RC4-MD5); Sun, 31 Oct 2010 19:03:52 -0700 (PDT) Received: by weongyo (sSMTP sendmail emulation); Sun, 31 Oct 2010 19:03:48 -0700 From: Weongyo Jeong Date: Sun, 31 Oct 2010 19:03:48 -0700 To: Hans Petter Selasky Message-ID: <20101101020348.GD3918@weongyo> References: <20101030231901.GA83161@weongyo> <201010311509.49138.hselasky@c2i.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201010311509.49138.hselasky@c2i.net> User-Agent: Mutt/1.4.2.3i Organization: CDNetworks. X-Operation-Sytem: FreeBSD Cc: freebsd-usb@freebsd.org Subject: Re: [CFR] add usb_sleepout.[ch] X-BeenThere: freebsd-usb@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: Weongyo Jeong List-Id: FreeBSD support for USB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 01 Nov 2010 02:03:55 -0000 On Sun, Oct 31, 2010 at 03:09:49PM +0100, Hans Petter Selasky wrote: > On Sunday 31 October 2010 01:19:01 Weongyo Jeong wrote: > > Hello USB guys, > > > > The following patch is to add a implementation, called `sleepout'. > > Please reviews. I'd like to commit it into HEAD if no objections. > > > > Adds `sleepout' prototype which is a comic combination of callout(9) and > > taskqueue(8) only for USB drivers to implement one step timer. In > > current USB drivers using callout(9) interface they all have two step > > execution flow as follows: > > > > 1. callout callback is fired by the interrupt context. Then it needs > > to pass it to USB process context because it could sleep(!) while > > callout(9) don't allow it. > > 2. In the USB process context it operates USB commands that most of > > times it'd be blocked at least 125 us (it'd be always true for USB) > > > > In a view of driver developer it'd be more convenient if USB stack has a > > feature like this (timer supporting blocking). > > > > Hi, > > I think this is a good idea too. > > However, there are some atomic issues I think you need to fix first. > > 1) All the sleepout_xxx() functions need mutex asserts. It looks it don't need it because callout(9) does it instead of sleepout routines. Moreover sleepout don't create any mutexes in itself. > 2) Is it allowed to call callout_stop() / callout_reset() unlocked at all? Yes as long as it doesn't have side effects. It seems no drivers hold a lock to call callout_stop() / callout_reset(). > What are the concequences if the mutex associated with the sleepout is NULL ? I added KASSERT macro to check this case at below. However the sleepout pointer normally never be NULL because the intention of usage was as follows: struct _softc { struct sleepout sleepout; struct sleepout_task sleepout_task; }; sleepout_create(&sc->sleepout, "blah"); Only it could be happen if `struct sleepout' is allocated by dynamically though it's not my first purpose. > 3) As per the current implementation it can happen that the task'ed timeout is > called after that sleepout_stop() is used. The code should have a check in the > task function to see if the sleepout() has been cancelled or not, when the > mutex associated with the sleepout is locked. Yes it's true but it'd better to implement first taskqueue_stop() than adding it sleepout routines directly. However no plans yet because I couldn't imagine a side effect due to lack of this feature. Please let me know if I missed the something important. > 4) Should the functions be prefixed by usb_ ? I don't have a preference for this. I think it's no problem to change it. It could happen soon. > 5) In drain you should drain the callout first, then drain the tasqueue. Else > the timer can trigger after the taskqueue is drained. It's fixed. Thank you for your review and the updated version is embedded at this email. regards, Weongyo Jeong Index: usb_sleepout.c =================================================================== --- usb_sleepout.c (revision 0) +++ usb_sleepout.c (revision 0) @@ -0,0 +1,149 @@ +/*- + * Permission to use, copy, modify, and distribute this software for any + * purpose with or without fee is hereby granted, provided that the above + * copyright notice and this permission notice appear in all copies. + * + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. + */ + +#include +__FBSDID("$FreeBSD$"); +#include +#include +#include +#include +#include +#include + +#include + +void +sleepout_create(struct sleepout *s, const char *name) +{ + + KASSERT(s != NULL, ("sleepout ptr is NULL")); + + s->s_taskqueue = taskqueue_create(name, M_WAITOK, + taskqueue_thread_enqueue, &s->s_taskqueue); + /* XXX adjusts the priority. */ + taskqueue_start_threads(&s->s_taskqueue, 1, PI_NET, "%s sleepout", + name); +} + +void +sleepout_free(struct sleepout *s) +{ + + KASSERT(s != NULL, ("sleepout ptr is NULL")); + + taskqueue_free(s->s_taskqueue); +} + +static void +_sleepout_taskqueue_callback(void *arg, int pending) +{ + struct sleepout_task *st = arg; + + (void)pending; + + if (st->st_mtx != NULL) + mtx_lock(st->st_mtx); + + st->st_func(st->st_arg); + + if (st->st_mtx != NULL) + mtx_unlock(st->st_mtx); +} + +void +sleepout_init(struct sleepout *s, struct sleepout_task *st, int mpsafe) +{ + + KASSERT(s != NULL, ("sleepout ptr is NULL")); + + st->st_sleepout = s; + callout_init(&st->st_callout, mpsafe); + TASK_INIT(&st->st_task, 0, _sleepout_taskqueue_callback, st); + st->st_mtx = NULL; +} + +void +sleepout_init_mtx(struct sleepout *s, struct sleepout_task *st, struct mtx *mtx, + int flags) +{ + + KASSERT(s != NULL, ("sleepout ptr is NULL")); + + st->st_sleepout = s; + callout_init_mtx(&st->st_callout, mtx, flags); + TASK_INIT(&st->st_task, 0, _sleepout_taskqueue_callback, st); + st->st_mtx = mtx; +} + +static void +_sleepout_callout_callback(void *arg) +{ + struct sleepout_task *st = arg; + struct sleepout *s = st->st_sleepout; + + KASSERT(s != NULL, ("sleepout ptr is NULL")); + + taskqueue_enqueue(s->s_taskqueue, &st->st_task); +} + +int +sleepout_reset(struct sleepout_task *st, int to_ticks, sleepout_func_t ftn, + void *arg) +{ + + st->st_func = ftn; + st->st_arg = arg; + return (callout_reset(&st->st_callout, to_ticks, + _sleepout_callout_callback, st)); +} + +int +sleepout_pending(struct sleepout_task *st) +{ + + KASSERT(s != NULL, ("sleepout ptr is NULL")); + + return (callout_pending(&st->st_callout)); +} + +/* + * NB: please notice that even if callout_stop(9) is called successfully, + * the task queue which enqueued for this sleepout_task could be fired + * by races. + */ +int +sleepout_stop(struct sleepout_task *st) +{ + + KASSERT(s != NULL, ("sleepout ptr is NULL")); + + return (callout_stop(&st->st_callout)); +} + +int +sleepout_drain(struct sleepout_task *st) +{ + struct sleepout *s = st->st_sleepout; + int ret; + + KASSERT(s != NULL, ("sleepout ptr is NULL")); + + /* + * Drains the callout first before draining the taskqueue that + * The reversed order make a callout trigger during taskqueue draining. + */ + ret = callout_drain(&st->st_callout); + taskqueue_drain(s->s_taskqueue, &st->st_task); + return (ret); +} Property changes on: usb_sleepout.c ___________________________________________________________________ Added: svn:mime-type + text/plain Added: svn:keywords + Id Added: svn:eol-style + native Index: usb_sleepout.h =================================================================== --- usb_sleepout.h (revision 0) +++ usb_sleepout.h (revision 0) @@ -0,0 +1,55 @@ +/*- + * Permission to use, copy, modify, and distribute this software for any + * purpose with or without fee is hereby granted, provided that the above + * copyright notice and this permission notice appear in all copies. + * + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. + * + * $FreeBSD$ + */ + +#ifndef _USB_SLEEPOUT_H_ +#define _USB_SLEEPOUT_H_ + +#include +#include + +struct sleepout { + struct taskqueue *s_taskqueue; +}; + +typedef void (*sleepout_func_t)(void *); + +struct sleepout_task { + struct sleepout *st_sleepout; + struct callout st_callout; + struct task st_task; + struct mtx *st_mtx; + int st_flags; +#define SLEEPOUT_CANCELLED (1 << 0) + sleepout_func_t st_func; + void *st_arg; +}; + +#define SLEEPOUT_RUNTASK(_so, _task) \ + taskqueue_enqueue((_so)->s_taskqueue, (_task)) +#define SLEEPOUT_DRAINTASK(_so, _task) \ + taskqueue_drain((_so)->s_taskqueue, (_task)) + +void sleepout_create(struct sleepout *, const char *); +void sleepout_free(struct sleepout *); +void sleepout_init(struct sleepout *, struct sleepout_task *, int); +void sleepout_init_mtx(struct sleepout *, struct sleepout_task *, + struct mtx *, int); +int sleepout_reset(struct sleepout_task *, int, sleepout_func_t, void *); +int sleepout_pending(struct sleepout_task *); +int sleepout_stop(struct sleepout_task *); +int sleepout_drain(struct sleepout_task *); + +#endif Property changes on: usb_sleepout.h ___________________________________________________________________ Added: svn:mime-type + text/plain Added: svn:keywords + Id Added: svn:eol-style + native