From owner-svn-src-head@FreeBSD.ORG Fri Sep 4 17:33:55 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 EB9911065769; Fri, 4 Sep 2009 17:33:55 +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 8E79D8FC1D; Fri, 4 Sep 2009 17:33:55 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by harmony.bsdimp.com (8.14.3/8.14.1) with ESMTP id n84HVsnp003335; Fri, 4 Sep 2009 11:31:54 -0600 (MDT) (envelope-from imp@bsdimp.com) Date: Fri, 04 Sep 2009 11:33:14 -0600 (MDT) Message-Id: <20090904.113314.1979406201.imp@bsdimp.com> To: attilio@FreeBSD.org From: "M. Warner Losh" In-Reply-To: <3bbf2fe10909030646o2dc30166r303ee646572c741b@mail.gmail.com> References: <200909031340.n83Defkv034013@svn.freebsd.org> <3bbf2fe10909030646o2dc30166r303ee646572c741b@mail.gmail.com> 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:33:56 -0000 In message: <3bbf2fe10909030646o2dc30166r303ee646572c741b@mail.gmail.com> Attilio Rao writes: : 2009/9/3 Attilio Rao : : > 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. : : About this commit, there are a few of things which worths nothing: : - This has been committed separately from the new-bus locking because : it is planned to be MFCed before 8.0 goes out in order to offer the : correct ABI compatibility for merging, while 8.1-REL, the newbus : locking I understand why the change is made here, but I don't think it is the right one. Or at least I don't think I adequately understand its motivation and need to understand that better since it doesn't make sense to me. : - Probabilly what we really wanted here is just a transitioning state : instead than both DS_ATTACHING and DS_DETACHING. Some consumers would : eventually understand what of the 2 is that and one of these consumers : is device_is_attached(). That function is used improperly by many : detach handlers in a construct like: I don't understand the point you are trying to make here... I'm not sure that piggybacking this information on the state is the right approach. : int : foo_attach(device_t dev) : { : ... : if (error != 0) : foo_detach(dev); : ... : } : : int : foo_detach(device_dev) : { : ... : if (device_is_attached(dev)) { : /* do some handover */ : } : ... : } : : That is an incorrect behaviour which needs to be discouraged and that : will be fixed during 9.0. Why is that bad? There are a lot of drivers that do that. I don't understand the point you are making here... The alternatives are likely worse. Please let me know what the plan here is. : In the while this semantic (used by many drivers) makes the : single-transition state impraticable and so we will need to stick with : that device states also in 9.0. A lot of drivers use that idiom today. Are you signing up to make sure they all work in the future? Again: I wasn't consulted on this change, don't like it, and want to talk more about it. It broke devinfo, and ignored code in cardbus and pccard that likely needs to change to cope. Warner