From owner-p4-projects@FreeBSD.ORG Fri Sep 15 18:39:19 2006 Return-Path: X-Original-To: p4-projects@freebsd.org Delivered-To: p4-projects@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 32767) id D648C16A417; Fri, 15 Sep 2006 18:39:18 +0000 (UTC) X-Original-To: perforce@freebsd.org Delivered-To: perforce@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 7F28D16A40F; Fri, 15 Sep 2006 18:39:18 +0000 (UTC) (envelope-from hselasky@c2i.net) Received: from swip.net (mailfe04.swip.net [212.247.154.97]) by mx1.FreeBSD.org (Postfix) with ESMTP id 66AED43D45; Fri, 15 Sep 2006 18:39:17 +0000 (GMT) (envelope-from hselasky@c2i.net) X-T2-Posting-ID: gvlK0tOCzrqh9CPROFOFPw== X-Cloudmark-Score: 0.000000 [] Received: from [193.216.121.196] (HELO [10.0.0.249]) by mailfe04.swip.net (CommuniGate Pro SMTP 5.0.8) with ESMTP id 279126304; Fri, 15 Sep 2006 20:39:13 +0200 From: Hans Petter Selasky To: John Baldwin Date: Fri, 15 Sep 2006 20:39:28 +0200 User-Agent: KMail/1.7 References: <200609151417.k8FEHSFx098273@repoman.freebsd.org> <200609151710.17572.hselasky@c2i.net> <200609151243.50388.jhb@freebsd.org> In-Reply-To: <200609151243.50388.jhb@freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200609152039.28868.hselasky@c2i.net> Cc: Perforce Change Reviews Subject: Re: PERFORCE change 106148 for review X-BeenThere: p4-projects@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: p4 projects tree changes List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 15 Sep 2006 18:39:19 -0000 On Friday 15 September 2006 18:43, John Baldwin wrote: > On Friday 15 September 2006 11:10, Hans Petter Selasky wrote: > > On Friday 15 September 2006 16:39, you wrote: > > > On Friday 15 September 2006 10:17, Hans Petter Selasky wrote: > > > > http://perforce.freebsd.org/chv.cgi?CH=106148 > > > > > > > > Change 106148 by hselasky@hselasky_mini_itx on 2006/09/15 14:17:12 > > > > > > > > Initialize all driver_t structures by record. Bugfix: Some of the > > > > modem drivers did not use the "ucom_devclass". Make sure that all > > > > modem drivers use this devclass. > > > > > My implementation for NetBSD works like this: > > This is FreeBSD I'm talking about. It is based off FreeBSD. > > > It then saves a pointer to > > > that device class object in the devclass_t pointer specified in > > > DRIVER_MODULE(). So, by making them all share the same devclass_t, they > > are > > > > all just going to overwrite the same pointer when the driver module > > > loads. Ok, right. But the old USB code did already do that. It shared the devclass among multiple devices. > > > > No, wrong. The devclass_t pointer is not overwritten if it is already > > initialized! > > Umm. It's overwritten, but to the same value. Go look at the actual code. ... > int > driver_module_handler(module_t mod, int what, void *arg) > { > ... > dmd = (struct driver_module_data *)arg; > ... > switch (what) { > case MOD_LOAD: > ... > if (driver->baseclasses) { > const char *parentname; > parentname = driver->baseclasses[0]->name; Maybe a missing "if (*dmd->dmd_devclass == NULL)" here. It is not a big problem. All the USB probe / attach code is currently executed under Giant, so there should not be any race condition. "devclass_find_internal()" allocates memory with M_NOWAIT, and will not sleep and cause problems. > *dmd->dmd_devclass = > devclass_find_internal(driver->name, > parentname, TRUE); > } else { > *dmd->dmd_devclass = > devclass_find_internal(driver->name, 0, TRUE); > } > break; > ... > } > > The writes to dmd_devclass are writing to the 'static devclass_t' you > specify in your DRIVER_MODULE() line. As I mentioned earlier, devclass_t > isn't a struct, it's a pointer to a struct: I know. But then my NetBSD implementation is slightly different. I only allocate a devclass when there are devices present, and not before, to save memory. I thought that was why devclass_t was a pointer and not a structure. > typedef struct devclass *devclass_t; > > Basically, passing a devclass_t into DRIVER_MODULE() just gives you a > pre-intialized pointer to your devclass. The devclass's aren't bound to > that devclass_t though, they are bound to the name. Thus if you have: > > static devclass_t foo_class, bar_class; > > static driver_t foo_driver { > "foo", ... > }; > > static driver_t bar_driver { > "foo", ... > }; > > DRIVER_MODULE(..., foo_driver, foo_devclass, ...); > DRIVER_MODULE(..., bar_driver, bar_devclass, ...); > Yes, but I cannot set the "devclass" to "NULL" ? That will panic according to the "driver_module_handler()" function ?? > foo_devclass and bar_devclass will both point to the same device class > object. > > > > To be honest, most drivers don't even use the devclass pointer, and I'd > > > actually like to axe it and make the few drivers that do care use > > > 'devclass_find("foo")' when they need the devclass pointer instead. > > > > I need the devclass to get unique unit numbers. > > Again, go look at the actual code, it can still call > devclass_find_internal(), but it is not required to save the pointer to do > so. Ok. --HPS