From owner-freebsd-usb@FreeBSD.ORG Mon Nov 1 18:51:31 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 44D681065672; Mon, 1 Nov 2010 18:51:31 +0000 (UTC) (envelope-from weongyo.jeong@gmail.com) Received: from mail-fx0-f54.google.com (mail-fx0-f54.google.com [209.85.161.54]) by mx1.freebsd.org (Postfix) with ESMTP id 9AFAC8FC1B; Mon, 1 Nov 2010 18:51:29 +0000 (UTC) Received: by fxm17 with SMTP id 17so5391103fxm.13 for ; Mon, 01 Nov 2010 11:51:29 -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=kPGQleNt08cezlDRjX11DSh6BrAxX/4bxkEXuAS9cLE=; b=yGY5eYAcIFTZFeHqoA/Nkghrzy9E+eAXQTraTzBDyHbubAFDkvMxovIkq+d56tga4m zZ4ww+o2PX0XQ83tn4/URG0Lc97EnCgIIB8fZn7cCvMabZndprNx08Adt/BBPuwPYr+j T12sTmgCJ8BGiUEf1BGb1VEpa8G1+agc2G3WI= 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=RFpfvhqHAhfKNSvzLbRVBhiFqrx0yBA8Mf37l8CoLE0Dq7bXkWPADatCXcyCFbaVS/ BpeXvW+Wnfi9vDG+mg78z1vBCyZK3DxdiqRPowZ4F40zcwsNEjiaUSPf/v9NBllLlVOh siF8iSglgL8/yeJExcgynIl+SrzbAhOPb8ooc= Received: by 10.223.86.2 with SMTP id q2mr3903987fal.53.1288637489226; Mon, 01 Nov 2010 11:51:29 -0700 (PDT) Received: from weongyo ([174.35.1.224]) by mx.google.com with ESMTPS id a25sm2621769fab.37.2010.11.01.11.51.24 (version=SSLv3 cipher=RC4-MD5); Mon, 01 Nov 2010 11:51:27 -0700 (PDT) Received: by weongyo (sSMTP sendmail emulation); Mon, 01 Nov 2010 11:51:22 -0700 From: Weongyo Jeong Date: Mon, 1 Nov 2010 11:51:22 -0700 To: Hans Petter Selasky Message-ID: <20101101185122.GE3918@weongyo> References: <20101031224304.GB3918@weongyo> <201011010930.26038.hselasky@c2i.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201011010930.26038.hselasky@c2i.net> User-Agent: Mutt/1.4.2.3i Organization: CDNetworks. X-Operation-Sytem: FreeBSD Cc: freebsd-usb@freebsd.org Subject: Re: [CFR 2-3/n] removes uther dependency of axe(4) 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 18:51:31 -0000 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: > > +static void > > +axe_watchdog(void *arg) > > +{ > > + struct axe_softc *sc = arg; > > + struct ifnet *ifp = sc->sc_ifp; > > + > > + if ((ifp->if_drv_flags & IFF_DRV_RUNNING) == 0) > > + return; > > + > > 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, 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. > 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: - Please fill. 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. > 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. regards, Weongyo Jeong