From owner-freebsd-new-bus@FreeBSD.ORG Mon Feb 27 18:10:30 2012 Return-Path: Delivered-To: new-bus@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id BAE1D1065677; Mon, 27 Feb 2012 18:10:30 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id 78DA08FC18; Mon, 27 Feb 2012 18:10:27 +0000 (UTC) Received: from bigwig.baldwin.cx (bigwig.baldwin.cx [96.47.65.170]) by cyrus.watson.org (Postfix) with ESMTPSA id 32AA646B3F; Mon, 27 Feb 2012 13:10:27 -0500 (EST) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 83130B990; Mon, 27 Feb 2012 13:10:26 -0500 (EST) From: John Baldwin To: Warner Losh Date: Mon, 27 Feb 2012 12:08:15 -0500 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p10; KDE/4.5.5; amd64; ; ) MIME-Version: 1.0 Content-Type: Text/Plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Message-Id: <201202271208.15664.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Mon, 27 Feb 2012 13:10:26 -0500 (EST) Cc: new-bus@freebsd.org, des@freebsd.org Subject: device_detach() never clears devclasses X-BeenThere: freebsd-new-bus@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: FreeBSD's new-bus architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 27 Feb 2012 18:10:30 -0000 I haven't reproduced this, I just have a hunch based on code-reading while fixing another bug. Ah, looking in SVN history it looks like this was broken in 129711 when device_attach() was split out of device_probe_and_attach(). Specifically, if DEVICE_ATTACH() fails then this: /* Unset the class; set in device_probe_child */ if (dev->devclass == NULL) (void)device_set_devclass(dev, NULL); is wrong. dev->devclass is always non-NULL at this point. However, we should be clearing the devclass for any device that doesn't have a fixed device class. And in fact that is what device_detach() does: if (!(dev->flags & DF_FIXEDCLASS)) devclass_delete_device(dev->devclass, dev); The patch below fixes this: Index: subr_bus.c =================================================================== --- subr_bus.c (revision 232218) +++ subr_bus.c (working copy) @@ -2732,9 +2732,8 @@ device_attach(device_t dev) if ((error = DEVICE_ATTACH(dev)) != 0) { printf("device_attach: %s%d attach returned %d\n", dev->driver->name, dev->unit, error); - /* Unset the class; set in device_probe_child */ - if (dev->devclass == NULL) - (void)device_set_devclass(dev, NULL); + if (!(dev->flags & DF_FIXEDCLASS)) + devclass_delete_device(dev->devclass, dev); (void)device_set_driver(dev, NULL); device_sysctl_fini(dev); dev->state = DS_NOTPRESENT; -- John Baldwin From owner-freebsd-new-bus@FreeBSD.ORG Mon Feb 27 19:37:50 2012 Return-Path: Delivered-To: new-bus@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id BA98A1065670; Mon, 27 Feb 2012 19:37:50 +0000 (UTC) (envelope-from imp@bsdimp.com) Received: from harmony.bsdimp.com (bsdimp.com [199.45.160.85]) by mx1.freebsd.org (Postfix) with ESMTP id 75CD68FC16; Mon, 27 Feb 2012 19:37:50 +0000 (UTC) Received: from [10.30.101.53] ([209.117.142.2]) (authenticated bits=0) by harmony.bsdimp.com (8.14.4/8.14.3) with ESMTP id q1RJVdiA001458 (version=TLSv1/SSLv3 cipher=DHE-DSS-AES128-SHA bits=128 verify=NO); Mon, 27 Feb 2012 12:31:41 -0700 (MST) (envelope-from imp@bsdimp.com) Mime-Version: 1.0 (Apple Message framework v1084) Content-Type: text/plain; charset=us-ascii From: Warner Losh In-Reply-To: <201202271208.15664.jhb@freebsd.org> Date: Mon, 27 Feb 2012 12:31:33 -0700 Content-Transfer-Encoding: quoted-printable Message-Id: References: <201202271208.15664.jhb@freebsd.org> To: John Baldwin X-Mailer: Apple Mail (2.1084) X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.0.1 (harmony.bsdimp.com [10.0.0.6]); Mon, 27 Feb 2012 12:31:42 -0700 (MST) Cc: Warner Losh , new-bus@freebsd.org, des@freebsd.org Subject: Re: device_detach() never clears devclasses X-BeenThere: freebsd-new-bus@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: FreeBSD's new-bus architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 27 Feb 2012 19:37:50 -0000 This looks good to me. I think I should have had !=3D here anyway. :) Warner On Feb 27, 2012, at 10:08 AM, John Baldwin wrote: > I haven't reproduced this, I just have a hunch based on code-reading = while=20 > fixing another bug. Ah, looking in SVN history it looks like this was = broken=20 > in 129711 when device_attach() was split out of = device_probe_and_attach(). =20 > Specifically, if DEVICE_ATTACH() fails then this: >=20 > /* Unset the class; set in device_probe_child */ > if (dev->devclass =3D=3D NULL) > (void)device_set_devclass(dev, NULL); >=20 > is wrong. dev->devclass is always non-NULL at this point. However, = we should=20 > be clearing the devclass for any device that doesn't have a fixed = device=20 > class. And in fact that is what device_detach() does: >=20 > if (!(dev->flags & DF_FIXEDCLASS)) > devclass_delete_device(dev->devclass, dev); >=20 > The patch below fixes this: >=20 > Index: subr_bus.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- subr_bus.c (revision 232218) > +++ subr_bus.c (working copy) > @@ -2732,9 +2732,8 @@ device_attach(device_t dev) > if ((error =3D DEVICE_ATTACH(dev)) !=3D 0) { > printf("device_attach: %s%d attach returned %d\n", > dev->driver->name, dev->unit, error); > - /* Unset the class; set in device_probe_child */ > - if (dev->devclass =3D=3D NULL) > - (void)device_set_devclass(dev, NULL); > + if (!(dev->flags & DF_FIXEDCLASS)) > + devclass_delete_device(dev->devclass, dev); > (void)device_set_driver(dev, NULL); > device_sysctl_fini(dev); > dev->state =3D DS_NOTPRESENT; >=20 >=20 > --=20 > John Baldwin >=20 >=20