On Sat, Aug 21, 2010 at 04:31:58PM -0700, Matthew Dempsky wrote:
> On Sat, Aug 21, 2010 at 08:25:23AM +0000, Jacob Meuser wrote:
> > +volatile int usb_run_tasks;
> > +volatile int discover_pending = 0;
>
> Do these really need to be volatile?
no
> +/* called by usbd_finish when lasst usb is detached */
>
> s/lasst/last/
oops
> +void
> +usb_end_tasks(void)
> +{
> + usb_run_tasks = 0;
> +}
>
> I think we need a wakeup(&usb_run_tasks) after the assignment.
yes
> > - s = splusb();
> > - for (;;) {
> > - task = TAILQ_FIRST(&usb_all_tasks);
> > - if (task == NULL) {
> > - tsleep(&usb_all_tasks, PWAIT, "usbtsk", 0);
> > - task = TAILQ_FIRST(&usb_all_tasks);
> > + while (usb_run_tasks) {
> > + s = splusb();
> > + while (usb_run_tasks &&
> > + (!TAILQ_EMPTY(&usb_hub_tasks) ||
> > + !TAILQ_EMPTY(&usb_device_tasks))) {
> > +
> > + task = TAILQ_FIRST(&usb_hub_tasks);
> > + while (task != NULL && usb_run_tasks) {
> > + TAILQ_REMOVE(&usb_hub_tasks, task, next);
> > + task->onqueue = 0;
> > + splx(s);
> > + task->fun(task->arg);
> > + s = splusb();
> > + task = TAILQ_FIRST(&usb_hub_tasks);
> > + }
> > +
> > + task = TAILQ_FIRST(&usb_device_tasks);
> > + while (task != NULL && usb_run_tasks) {
> > + TAILQ_REMOVE(&usb_device_tasks, task, next);
> > + task->onqueue = 0;
> > + splx(s);
> > + task->fun(task->arg);
> > + s = splusb();
> > + if (!TAILQ_EMPTY(&usb_hub_tasks))
> > + break;
> > + task = TAILQ_FIRST(&usb_device_tasks);
> > + }
> > }
> > - DPRINTFN(2,("usb_task_thread: woke up task=%p\n", task));
> > - if (task != NULL) {
> > - TAILQ_REMOVE(&usb_all_tasks, task, next);
> > - task->onqueue = 0;
> > - splx(s);
> > - task->fun(task->arg);
> > - s = splusb();
> > + splx(s);
> > +
> > + if (usb_run_tasks) {
> > + /* XXX Is not timing out too optimistic? */
> > + tsleep(&usb_run_tasks, PWAIT, "usbtsk", 0);
> > + DPRINTFN(2,("%s: woke up\n", __func__));
> > }
> > }
>
> If a bunch of device tasks get enqueued and then while we're running
> them, a discover task gets enqueued, the device tasks will finish
> running before we run the discover task.
this is in the device task loop to catch that:
> > + if (!TAILQ_EMPTY(&usb_hub_tasks))
> > + break;
> How about writing the task thread loop like this:
>
> s = splusb();
> for (usb_run_tasks) {
> if ((task = TAILQ_FIRST(&usb_hub_tasks)) != NULL)
> TAILQ_REMOVE(&usb_hub_tasks, task, next);
> else if ((task = TAILQ_FIRST(&usb_device_tasks)) != NULL)
> TAILQ_REMOVE(&usb_device_tasks, task, next);
> else {
> tsleep(&usb_run_tasks, PWAIT, "usbtsk", 0);
> continue;
> }
>
> task->onqueue = 0;
> splx(s);
> task->fun(task->arg);
> s = splusb();
> }
> splx(s);
that is much nicer :)
> Also, is there a chance that there are tasks still enqueued when
> usb_end_tasks() is called? Do they need any cleanup?
that's a good question. the current code doesn't deal with that.
in fact, the current code doesn't even try to stop the thread.
--
[email protected]
SDF Public Access UNIX System - http://sdf.lonestar.org
