Date: Sun, 31 Oct 2010 19:03:48 -0700 From: Weongyo Jeong <weongyo.jeong@gmail.com> To: Hans Petter Selasky <hselasky@c2i.net> Cc: freebsd-usb@freebsd.org Subject: Re: [CFR] add usb_sleepout.[ch] Message-ID: <20101101020348.GD3918@weongyo> In-Reply-To: <201010311509.49138.hselasky@c2i.net> References: <20101030231901.GA83161@weongyo> <201010311509.49138.hselasky@c2i.net>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <driver>_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 <sys/cdefs.h>
+__FBSDID("$FreeBSD$");
+#include <sys/param.h>
+#include <sys/systm.h>
+#include <sys/kernel.h>
+#include <sys/lock.h>
+#include <sys/malloc.h>
+#include <sys/mutex.h>
+
+#include <dev/usb/usb_sleepout.h>
+
+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 <sys/callout.h>
+#include <sys/taskqueue.h>
+
+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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20101101020348.GD3918>