On Monday 01 November 2010 19:51:22 Weongyo Jeong wrote:
> On Mon, Nov 01, 2010 at 09:30:25AM +0100, Hans Petter Selasky wrote:
> > On Sunday 31 October 2010 23:43:04 Weongyo Jeong wrote:
> > 
> > Hi,
> > 
> > Please explain what is wrong with the existing code regarding code
> > synchronisation. Your patch is very big and is likely to introduce
> > problems.
> Hello Hans,

Hi Weongyo,

> I think your approach to synchronize the multiple tasks isn't bad though
> the implementation of approach isn't familiar with me comparing the
> other task queue implementations.  It seems to me that it's a customized
> task queue implementation only for USB subsystem.

Yes. I'm going to change that now, so that we get all the benefits of the 
taskqueue system. Please see other e-mail posted today.

> > I oppose the introduction of SX-locks. Please explain why you think
> > SX-locks are better than the USB process taskqueue.
> I'd like to say that I also don't like SX locks that the reason is
> mentioned at CONTEXT section of sx(9) man page.
> I think using SX lock and the USB process taskqueue are both bad that if
> I could avoid using it, I'll willing to avoid it to use it.  But the
> problem is that it's inevitable.  I think we could choice one of two
> approaches that one is using SX lock other is using USB process
> taskqueue.  The result of both would same, serialization.
> Regarding to the approach there'll be pros and cons:
>   Pros of using SX lock:
>     - Don't need to create extra thread.
>     - Simple to read.  Other developers could catch writer's intention
>       easily.
>   Cons of using SX lock:
>     - Unexpecting sleep with holding mutex according to CONTEXT section
>       of sx(9) man page.  It should be used carefully.
>     - Adds another lock to the driver.
>   Pros of using USB process taksqueue:

The USB process taskqueue is a temporary solution until the taskqueue system 
is updated to support the "USB requirements".

Are kernel taskqueues any better than USB process'es?

>   Cons of using USB process taskqueue:
>     - No atomic operation that it need to be drained explicitly with
>       sleeping that adds extra quirk code.
>     - Tasks would be merged into one if the task is enqueued multiple
>       times during pending that it means the writer takes care extra
>       exceptions.  It could lead to unexpected device behavior depending
>       on device characteristics.
>     - At least two threads are involved.  One is caller thread other
>       would be worker thread (USB process).  It makes weak to race
>       conditions.  It always make code complex.
> > Are you absolutely sure that all the IOCTL's that are called are allowed
> > to block in the way you have programmed?
> Of course not because I'm a human.  Human always do a mistake.  :-)  But
> there are few cases which need to be atomic in USB devices.  For
> example, device up and down.  It means sx(9) lock would be used very
> limited places only.

Ok. My intention was not to completely tear apart your patches :-) What I can 
say is that I've tried to keep all corner cases tight, and maybe it's not 
directly obvious to everyone how everything is working inside the USB stack 
like you write. Sometimes I even forget all of the small things I've fixed, 
and then it takes some time until I remember and can point out to you exactly 

If a USB task/process gets stuck (starts having USB timeouts), then it's 
easier to recover than if we are using a SX lock. Because if we are using an 
SX lock, the caller can get stuck for a substantial amount of time, whilst if 
we do it via task queue, we have the possibility to recover from the USB 
explore thread which runs the detach of the USB device.

> > The checks in xxx_watchdog() are not good enough. axe_tick() will execute
> > synchronous USB functions, which sleep for many hundreds of microseconds.
> > You should add this check before the sleepout_reset() too, and is this
> > code called with any lock locked? I.E. Are you doing the clearing of
> > IFF_DRV_RUNNING atomic to testing this flag? Else the result can be
> > random?
> Maybe xxx_watchdog() would be called with holding the driver lock so
> at least ifp->if_drv_flags would be protected.

That's also a solution.

freebsd-usb@freebsd.org mailing list
To unsubscribe, send any mail to "freebsd-usb-unsubscr...@freebsd.org"

Reply via email to