From owner-freebsd-usb@FreeBSD.ORG Sun Feb 1 17:50:26 2009 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 7347810656BC; Sun, 1 Feb 2009 17:50:26 +0000 (UTC) (envelope-from thompsa@FreeBSD.org) Received: from pele.citylink.co.nz (pele.citylink.co.nz [202.8.44.226]) by mx1.freebsd.org (Postfix) with ESMTP id 112978FC20; Sun, 1 Feb 2009 17:50:26 +0000 (UTC) (envelope-from thompsa@FreeBSD.org) Received: from localhost (localhost [127.0.0.1]) by pele.citylink.co.nz (Postfix) with ESMTP id 7714BFF79; Mon, 2 Feb 2009 06:50:25 +1300 (NZDT) X-Virus-Scanned: Debian amavisd-new at citylink.co.nz Received: from pele.citylink.co.nz ([127.0.0.1]) by localhost (pele.citylink.co.nz [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id B4BgOahUCSJk; Mon, 2 Feb 2009 06:50:21 +1300 (NZDT) Received: from citylink.fud.org.nz (unknown [202.8.44.45]) by pele.citylink.co.nz (Postfix) with ESMTP; Mon, 2 Feb 2009 06:50:21 +1300 (NZDT) Received: by citylink.fud.org.nz (Postfix, from userid 1001) id 7213A1142C; Mon, 2 Feb 2009 06:50:21 +1300 (NZDT) Date: Sun, 1 Feb 2009 09:50:21 -0800 From: Andrew Thompson To: Hans Petter Selasky Message-ID: <20090201175021.GA32503@citylink.fud.org.nz> References: <20090131231957.GB31825@citylink.fud.org.nz> <20090201030628.GE65558@elvis.mu.org> <200902011220.18745.hselasky@c2i.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <200902011220.18745.hselasky@c2i.net> User-Agent: Mutt/1.5.17 (2007-11-01) Cc: freebsd-usb@freebsd.org Subject: Re: USB2 patches X-BeenThere: freebsd-usb@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: FreeBSD support for USB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 01 Feb 2009 17:50:27 -0000 On Sun, Feb 01, 2009 at 12:20:17PM +0100, Hans Petter Selasky wrote: > Hi Andrew, > > We need to go through your patches together, to work out some issues. > > 1) A nit. > > - if (usb2_proc_setup(&ssc->sc_config_td, p_mtx, USB_PRI_MED)) { > - usb2_com_units_free(root_unit, sub_units); > - return (ENOMEM); > - } > + > + error = usb2_proc_create(&ssc->sc_tq, USB_PRI_MED, "ucom"); > + if (error) > > < missing "usb2_com_units_free()" > thanks. > + return (error); > + > > 2) Please explain why you think that "usb2_proc_is_gone()" can be removed. > > - if (usb2_proc_is_gone(&ssc->sc_config_td)) { > - DPRINTF("proc is gone\n"); > - return; /* nothing to do */ > - } > > If the taskqueue system does not provide a teardown mechanism so that we > inside the callback can know if the taskqueue is being drained, then the > taskqueue cannot replace the original "usb2_proc_xxx()" ! Maybe this means we > need to extend the taskqueue system? Possibly. Taskqueues will always complete the function callbacks before freeing so you dont need to worry about resources disappearing under you. The one reason would be to bail out of a long running task but, - Are there any tasks that actually run a long time? - If not, its easier just to wait on the drain > 3) > > In general avoid more than a few synchronous USB transfer inside > attach/detach. Always defer! Else there are corner cases where the device can > hang waiting for the transfer timeout 1-5 seconds for every USB transfer, and > that is unacceptable. I disagree. It hugely simplifies drivers to know all the resources are allocated after attach, you dont need to check for null pointers throughout the code. If programming commands fail/timeout in the attach routine then the driver needs to check this and abort the attach. You will wait one or two timeouts max. > > 4) > > int onoff = USB_TASK_ARG(task); > > This won't work! You need to be able to handle N-phase here, where N goes to > +infinity! If the DTR pin was toggled we should also see a toggle in the > programming of the DTR pin, not just re-program the last state! If there is a > USB_TASK_ARG() it must be constant and that is where I start seeing problems, > or features missing in the taskqueue system. > > It is also very important that you somehow freeze the configuration at the > moment a command is issued. For example in the WLAN drivers. If the command > is queued when the channel is 5 then we should program channel 5 and not > fetch the latest channel value from the softc! IMPORTANT! This isnt correct. Take your example of WLAN drivers, the net80211 layer will use ic_curchan for its logic so the driver needs to program the chip to this value at all times. You can _not_ queue channel changes as these will be out of sync. Tasks are very short lived and program the chipset to the current state where it otherwise could not have been done due to thread sleeping. Also, net80211 will soon have its own thread so you can do things like state/channel changes directly but in a sleepable context. > 5) Many of your other changes are good! Thanks! Andrew