From owner-svn-src-head@FreeBSD.ORG Fri Sep 4 17:31:05 2009 Return-Path: Delivered-To: svn-src-head@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 6BD271065700; Fri, 4 Sep 2009 17:31:05 +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 15F818FC21; Fri, 4 Sep 2009 17:31:05 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by harmony.bsdimp.com (8.14.3/8.14.1) with ESMTP id n84HS00l003294; Fri, 4 Sep 2009 11:28:00 -0600 (MDT) (envelope-from imp@bsdimp.com) Date: Fri, 04 Sep 2009 11:29:19 -0600 (MDT) Message-Id: <20090904.112919.1521002024.imp@bsdimp.com> To: attilio@FreeBSD.org From: "M. Warner Losh" In-Reply-To: <200909031340.n83Defkv034013@svn.freebsd.org> References: <200909031340.n83Defkv034013@svn.freebsd.org> X-Mailer: Mew version 5.2 on Emacs 21.3 / Mule 5.0 (SAKAKI) Mime-Version: 1.0 Content-Type: Text/Plain; charset=us-ascii Content-Transfer-Encoding: 7bit Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r196779 - in head/sys: kern sys X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 04 Sep 2009 17:31:05 -0000 In message: <200909031340.n83Defkv034013@svn.freebsd.org> Attilio Rao writes: : Author: attilio : Date: Thu Sep 3 13:40:41 2009 : New Revision: 196779 : URL: http://svn.freebsd.org/changeset/base/196779 : : Log: : Add intermediate states for attaching and detaching that will be : reused by the enhached newbus locking once it is checked in. : This change can be easilly MFCed to STABLE_8 at the appropriate moment. That appropriate moment better be ASAP. At least for the change I've highlighed below, since it changes the ABI. Btw, this change also breaks devinfo. : Modified: head/sys/sys/bus.h : ============================================================================== : --- head/sys/sys/bus.h Thu Sep 3 12:41:00 2009 (r196778) : +++ head/sys/sys/bus.h Thu Sep 3 13:40:41 2009 (r196779) : @@ -52,8 +52,11 @@ struct u_businfo { : typedef enum device_state { : DS_NOTPRESENT, /**< @brief not probed or probe failed */ : DS_ALIVE, /**< @brief probe succeeded */ : + DS_ATTACHING, /**< @brief attaching is in progress */ : DS_ATTACHED, /**< @brief attach method called */ : - DS_BUSY /**< @brief device is open */ : + DS_BUSY, /**< @brief device is open */ : + DS_DETACHING /**< @brief detaching is in progress */ : + : } device_state_t; device_state_t is exported to userland via devctl. Well, that's not entirely true... It isn't used EXACTLY, but there's this in devinfo.h: /* * State of the device. */ /* XXX not sure if I want a copy here, or expose sys/bus.h */ typedef enum devinfo_state { DIS_NOTPRESENT, /* not probed or probe failed */ DIS_ALIVE, /* probe succeeded */ DIS_ATTACHED, /* attach method called */ DIS_BUSY /* device is open */ } devinfo_state_t; which is why devinfo is broken. Also, DS_BUSY is used in many drivers to PREVENT detaching. So the change is bad from that point of view, since DS_DETACHING is now > DS_BUSY. There's really a partial ordering relationship now where before there was a total ordering (DS_BUSY is > DS_ATTACHED and DS_DETACHING is > DS_ATTACH, but DS_DETACHING isn't > DS_BUSY and DS_BUSY isn't > DS_DETACHING). I think that you've destroyed information here by unconditionally setting it: - if ((error = DEVICE_DETACH(dev)) != 0) + dev->state = DS_DETACHING; + if ((error = DEVICE_DETACH(dev)) != 0) { + KASSERT(dev->state == DS_DETACHING, + ("%s: %p device state must not been changing", __func__, + dev)); + dev->state = DS_ATTACHED; return (error); + } + KASSERT(dev->state == DS_DETACHING, + ("%s: %p device state must not been changing", __func__, dev)); And this looks racy between the check earlier and this setting. Properly locked, this wouldn't destroy information... At the very least cardbus/cardbus.c and pccard/pccard.c need to be looked at since they both have code that looks like: for (tmp = 0; tmp < numdevs; tmp++) { struct cardbus_devinfo *dinfo = device_get_ivars(devlist[tmp]); int status = device_get_state(devlist[tmp]); if (dinfo->pci.cfg.dev != devlist[tmp]) device_printf(cbdev, "devinfo dev mismatch\n"); if (status == DS_ATTACHED || status == DS_BUSY) device_detach(devlist[tmp]); cardbus_release_all_resources(cbdev, dinfo); cardbus_device_destroy(dinfo); device_delete_child(cbdev, devlist[tmp]); pci_freecfg((struct pci_devinfo *)dinfo); } which does ignore errors returned by device_detach for the DS_BUSY case because there's not currently a good way to tell device_detach that it *MUST* detach the device *NOW* without any possibility of veto by the driver. The above code also isn't DS_DETACHING aware, and may be wrong in the face of this new state. Of course, grepping the tree does show one or two places where DS_BUSY is used inappropriately: rp.c: static int rp_pcidetach(device_t dev) { CONTROLLER_t *ctlp; if (device_get_state(dev) == DS_BUSY) return (EBUSY); is one example. The above check should just be removed (ditto for its SHUTDOWN) routine. So I think we should fix rp.c, but we need to talk through this change a little more. I'm surprised I wasn't even pinged about it, since it hits code that I maintain and a simple grep would have found... I'm not yet asking for it to be backed out, but I don't like it on its surface and want to talk about it in more detail. Warner