From owner-svn-src-head@FreeBSD.ORG Fri Sep 4 21:55:54 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 69F901065672; Fri, 4 Sep 2009 21:55:54 +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 998EB8FC19; Fri, 4 Sep 2009 21:55:53 +0000 (UTC) Received: by fxm6 with SMTP id 6so933161fxm.43 for ; Fri, 04 Sep 2009 14:55:52 -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=8pyxS5VfajkBTt49OFFUQaiayMXJs3qtRVTm08HZ2pI=; b=DwAC00fWirsTSxZ4QFmX4QWYMSwIL8WTx2G7HhVWqhk+/tC3hWFcs56lbz6dtqcKrF 6nPsRSnZbb2QsEPa+8AvIo7uo1WVJx6egFBEJpDS+a98qwq+s1NbsNtZZKOR38iQO5vc h2CHAzpBBQRCuh/mpSjVFDuU8Q3jlhnLzU8qg= 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=CFNkYJuWVgkXdTtlyz2uSNj4c/t1Y1L3xNqnjKVlPm6GIDOpX46iTrRlmBLbdcnoZm 2ZLpDnNMtsXyTDcCUoyhEokZNRIdE1XJBFkrmmdwAZzn7vaNzH/UDhOmP7qA0V1AflIL uTudoxqkMFHkN9x8aG2IPJuqIKLwnrgqh1sBg= MIME-Version: 1.0 Sender: asmrookie@gmail.com Received: by 10.223.143.15 with SMTP id s15mr4780435fau.77.1252101352549; Fri, 04 Sep 2009 14:55:52 -0700 (PDT) In-Reply-To: <20090904.112919.1521002024.imp@bsdimp.com> References: <200909031340.n83Defkv034013@svn.freebsd.org> <20090904.112919.1521002024.imp@bsdimp.com> Date: Fri, 4 Sep 2009 23:55:52 +0200 X-Google-Sender-Auth: 34e1473445759915 Message-ID: <3bbf2fe10909041455u552b0dbdm1708ea0a26365149@mail.gmail.com> From: Attilio Rao To: "M. Warner Losh" Content-Type: text/plain; charset=UTF-8 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 21:55:54 -0000 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. > 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... Sorry, I really don't understand what point are you making here, and what the scaring words of "destroying", "racy", etc. means. Can you explain better that part? > 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. How DS_DETACHING can cause problems here? device_detach() simply won't run if the state is DS_DETACHING as expected (another thread is alredy detaching and there is no need for it to detach). Also, please note that in this case, for the state == DS_BUSY he device_detach() won't do anything. You can't simply skip the return value and anything else, but the reality is still the operation won't happen. > 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... Still, I don't see a problem with the codes you mentioned (if not in the consumers, which were alredy "broken" before this change and which situation is not worse now), unless the devinfo breakage. Attilio -- Peace can only be achieved by understanding - A. Einstein