From owner-freebsd-arch@FreeBSD.ORG Sat Sep 5 15:12:19 2009 Return-Path: Delivered-To: arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 390E7106566B for ; Sat, 5 Sep 2009 15:12:19 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-fx0-f210.google.com (mail-fx0-f210.google.com [209.85.220.210]) by mx1.freebsd.org (Postfix) with ESMTP id 8EC558FC13 for ; Sat, 5 Sep 2009 15:12:18 +0000 (UTC) Received: by fxm6 with SMTP id 6so1175186fxm.43 for ; Sat, 05 Sep 2009 08:12:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:sender:received:in-reply-to :references:date:x-google-sender-auth:message-id:subject:from:to:cc :content-type; bh=2aCL9G56XoAHn6H/PMOCb1Gu/07VaNI3tDhg1REj3i0=; b=bGPgAG+RzTTZibhYXbUo8pFmINL6qqCXiJ7fsCnQZPMcSx17BYHA9wrLcW7EpnxUVj cl7sEMgkfy5ZBcxJRFBSO9Yx7xUXNjfKZFyWpIV8MISAUEYZUMo4kC3imjTTUr0Ru1kJ sggQ9uMbP2jD86GMx3RhKO9cGcIA/4kbDxGN0= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type; b=oTNR1JgcN9WTfkqQ9sj6FEvV6nLX4wmTE+g1nykKUI5jZTjWgrSS6k925RuFNx02+I Kl96ADxxr6obynfO/t1WVjHoDMkYMI6qQJvWZ4kEmFT9Cy17fkDvnUBubvkDy4xg2fAy wriR74vN2ClKIRGLSkEenp3KqpDEyCy4zd5rQ= MIME-Version: 1.0 Sender: asmrookie@gmail.com Received: by 10.223.54.15 with SMTP id o15mr4910571fag.96.1252163537532; Sat, 05 Sep 2009 08:12:17 -0700 (PDT) In-Reply-To: <20090904.172310.-1939841993.imp@bsdimp.com> References: <3bbf2fe10909041455u552b0dbdm1708ea0a26365149@mail.gmail.com> <20090904.161634.-217944108.imp@bsdimp.com> <3bbf2fe10909041546y2b5633e1ue063955568df1a06@mail.gmail.com> <20090904.172310.-1939841993.imp@bsdimp.com> Date: Sat, 5 Sep 2009 17:12:17 +0200 X-Google-Sender-Auth: 2c26e0f613746148 Message-ID: <3bbf2fe10909050812l4340f679h6a4d7dae1daa3bf8@mail.gmail.com> From: Attilio Rao To: "M. Warner Losh" Content-Type: text/plain; charset=UTF-8 Cc: arch@freebsd.org Subject: Re: NEWBUS states X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 05 Sep 2009 15:12:19 -0000 2009/9/5 M. Warner Losh : > In message: <3bbf2fe10909041546y2b5633e1ue063955568df1a06@mail.gmail.com> > Attilio Rao writes: > : 2009/9/5 M. Warner Losh : > : > [[ redirected to arch@ since too much of this discussion has been private ]] > : > > : > In message: <3bbf2fe10909041455u552b0dbdm1708ea0a26365149@mail.gmail.com> > : > Attilio Rao writes: > : > : 2009/9/4 M. Warner Losh : > : > : > In message: <200909031340.n83Defkv034013@svn.freebsd.org> > : > : > Attilio Rao writes: > : > : > : 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. > : > : > : > : I think the right fix here is to maintain in sync devinfo.h and bus.h > : > : definition by just having one. I see that the devices states are > : > : redefined in devinfo.h in order to avoid namespace pollution and this > : > : is a good point. What I propose is to add a new header (_bus.h, to be > : > : included in both bus.h and devinfo.h) which just containst the device > : > : states. > : > > : > There's a lot of possible fixes. It is broken right now. Your commit > : > broke it. > : > > : > The problem is that we have multiple names for the same things, and > : > that's a defined API/ABI today.... So having a _bus.h won't solve > : > this problem without introducing name space pollution (well, I suppose > : > you could have __DS_NOTPRESENT, __DS_ALIVE, etc and then have > : > devinfo.h and bus.h map those in, but that still wouldn't totally > : > solve the problem). > : > : What I would like to do is simply to typedef the enum I get from the > : _bus.h. I'm not sure if nothing else is still required. > > You can't do just that. There's the "DIS_foo vs DS_foo" issue as > well, which cannot be solved with typedefs. Some mapping is > required.. this is a sill API, one could argue, but it is the one we > have today... Bah, sorry, I kept reading it as DS_ rather than DIS_ . Anyways, what do you think about, for 9.0, just having one interface and remove the DIS_* bloat? > : > : The point for this change is to add now parts which could introduce, > : in the future, ABI compatibility breakage so that we can MFC the > : newbus locking to 8.1. Obviously that is not a finished patch, because > : it still misses of the full context. > > Yes. I understand that. I'm specifically suggesting that we only MFC > the changes to the ABI today, and MFC the changes to the code when it > is complete. Sure, and that's what I did. The small parts in subr_bus.c aren't that much important but they can be used as a reminder. If you feel strongly against it I can also review and eventually drop them. > : We all agreed the one-state was the better option but it can't be done > : in this way because of the device_is_attached() used in the detach > : virtual functions. Using just one transition state will break > : device_is_attached() in those parts. > > True. However, this device_is_attached stuff is fundamentally broken > as it is today. I took a closer look at the typical usage, and it > stands in as a proxy for 'did all the resources get allocated' and > even that's just the bus_resouce* stuff... Of course. Such paradigms are only sane (in particular from the standpoint of the locking) only when you can make some assumptions about a known context, and you should also know the state of your device there. > : The right fix, as pointed out in other e-mails, is to not use > > e-mails that I wasn't cc'd on since this discussion happened in > private. I hate to keep harping on this point, but there's been too > much of that lately... Sorry for that, but in this case I was referring to e-mail sent to public mailing list. My current english and expressive skillset is not that much developed so far though, so it is likely I did express the concept with cryptic/incorrect words as often happens, so I can't blame anyone else than me for that. > : device_is_attached() in detach virtual functions. The better fix, in > : my idea would involve: > : - replace the device_is_attached() usage in detach virtual functions, > : with a more functional support > > This would be transitioning to using a common set of code for > release_resources, ala the other most common driver idiom... I agree. There are various ways to fix this that we can discuss and put in place during 9.0 timelife. > : - use one-state transition > : > : But that is just too much job to push in before then 8.0-REL and if > : that would mean to not commit a patch and make impossible a future > : MFC, I prefer to go with a lesser-perfect-but-still-working-approach. > > Yes. The problem is that we're trying to guess what the right locking > approach will be at the 11th hour, and I'm worried we will guess wrong. > > : I'm sorry if these points weren't clear before. > > OK. Let me ponder based on that... It might be better for this round > of changes to leverage off the device 'flags' field to indicate that > we're attaching/detaching. This would not break the > device_is_attached() usage, and would solve the interlock problem > nicely. While it isn't as aesthetically pleasing as the new states, > it would allow us to easily MFC it without API/ABI breakage. This > field surely would be covered by the same set of locks as the state > field. > > I know that there's a good aesthetic argument to be made against this, > but on the other hand 'compatibility' hacks can violate one's > aesthetics. We can migrate to a more pleasing state-based model in 9 > and reduce the risk to other code from changing its semantics at this > late date. That was exactly the original point of my commit. > : > In short, I think that http://people.freebsd.org/~imp/newbus-20090904 > : > should be committed and MFC'd. I've not addressed the devinfo > : > breakage yet... > : > : 404 > > http://people.freebsd.org/~imp/newbus-20090904.diff > > sorry about that... So I see in this patch you are also implementing the idea to offer rooms in the enum intra-existing-states that kib proposed. That is a good idea to do now. However, the one-state transition can't be implemented in this patch as you accepted few lines above. > : Btw, I don't agree about removing the changes within subr_bus.c. They > : are harmless (KASSERT and further checks) > : and will help as a reminder. > > Why have them at all? We're just speculating about what the protocol > will be, rather than abstracting down from a known-to-be-good > implementation. This sort of thing has bit us in the past before. > Since at best we're speculating about what the best approach is, I'd > prefer to keep the leaps of faith as small as possible. Ok, if you are strongly against it, we can remove them, I just think they will be harmless and a good reminder. Thanks, Attilio -- Peace can only be achieved by understanding - A. Einstein