|

»

NEWBUS states (was Re: svn commit: r196779 - in head/sys: kern sys)

View: New views

16 Messages — Rating Filter: Alert me

	NEWBUS states (was Re: svn commit: r196779 - in head/sys: kern sys) by M. Warner Losh :: Rate this Message: Reply to Author \| View Threaded \| Show Only this Message [[ redirected to arch@ since too much of this discussion has been private ]] In message: <3bbf2fe10909041455u552b0dbdm1708ea0a26365149@...> Attilio Rao <attilio@...> writes: : 2009/9/4 M. Warner Losh <imp@...>: : > In message: <200909031340.n83Defkv034013@...> : > Attilio Rao <attilio@...> 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). : > 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? I think it is a minor concern. There's no locking now to prevent multiple threads doing stuff. I think it would be better to completely omit this stuff than to put it in 1/2 way. Also, it breaks the linear nature of device_state_t. : > 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. I explained about partial ordering already. Let me try again. DS_BUSY isn't compatible with this definition. It would be better to merge DS_ATTACHING and DS_DETACHING into one state, say DS_TRANSITION. Then we'd do the attach sequence as DS_ALIVE -> DS_TRANSITION -> DS_ATTACHED (and later DS_BUSY maybe). Then detach would go from DS_ATTACHED -> DS_TRANSITION to tear it down. There's a strong ordering expected in the code, and finding all the subtle places that this extra state beyond DS_BUSY causes problems will be hard. I'm not entirely sure how DS_DETACHING can cause problems here. However, I've not looked at all the possible code paths and such to make sure that there isn't a problem. Given the strong ordering of device_state_t that's present today, I'm not so glib that it would cause no problems. I want strong assurance that it won't. If we use DS_TRANSITION, then I know it won't. It would solve the problems with only one new state, and would preserve the ordering that's implicit in the code and hard to ferret out. This is only one example. : > 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. Well, the rp.c that I talked about is a minor breakage that can easily be fixed. It is unrelated to your changes, but my grep found the breakage... devinfo breakage, however is a bigger deal, as is the breaking of the linearness of device_state_t. Let's just use one new state. It won't make locking harder, and will also prevent a device from attaching while someone else is detaching it better. In summary: I support adding one new state (and only one new state) to device_state_t that is numerically smaller than DS_ATTACHED. I think we should take this opportunity to make the states sparser as well (since that would allow us to insert them in the future, should a proven need be found). I think that the extra checks that were added to subr_bus.c should be backed out. I think that only the new state enums and their new values should be MFC'd. I further thing that ALL* future discussions of newbus ABI/API changes go through arch@. 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... Warner _______________________________________________ freebsd-arch@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-arch To unsubscribe, send any mail to "freebsd-arch-unsubscribe@..."
	Re: NEWBUS states (was Re: svn commit: r196779 - in head/sys: kern sys) by Attilio Rao-2 :: Rate this Message: Reply to Author \| View Threaded \| Show Only this Message 2009/9/5 M. Warner Losh <imp@...>: > [[ redirected to arch@ since too much of this discussion has been private ]] > > In message: <3bbf2fe10909041455u552b0dbdm1708ea0a26365149@...> > Attilio Rao <attilio@...> writes: > : 2009/9/4 M. Warner Losh <imp@...>: > : > In message: <200909031340.n83Defkv034013@...> > : > Attilio Rao <attilio@...> 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. > : > 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? > > I think it is a minor concern. There's no locking now to prevent > multiple threads doing stuff. I think it would be better to > completely omit this stuff than to put it in 1/2 way. 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. > : > 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. > > I explained about partial ordering already. Let me try again. > > DS_BUSY isn't compatible with this definition. It would be better to > merge DS_ATTACHING and DS_DETACHING into one state, say DS_TRANSITION. > Then we'd do the attach sequence as DS_ALIVE -> DS_TRANSITION -> > DS_ATTACHED (and later DS_BUSY maybe). Then detach would go from > DS_ATTACHED -> DS_TRANSITION to tear it down. There's a strong > ordering expected in the code, and finding all the subtle places that > this extra state beyond DS_BUSY causes problems will be hard. 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. The right fix, as pointed out in other e-mails, is to not use 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 - 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. I'm sorry if these points weren't clear before. > 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 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. For the moment the only thing I still see to be fixed is devinfo. I will provide a patch before the end of the day. Attilio -- Peace can only be achieved by understanding - A. Einstein _______________________________________________ freebsd-arch@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-arch To unsubscribe, send any mail to "freebsd-arch-unsubscribe@..."
	Re: NEWBUS states by M. Warner Losh :: Rate this Message: Reply to Author \| View Threaded \| Show Only this Message In message: <3bbf2fe10909041546y2b5633e1ue063955568df1a06@...> Attilio Rao <attilio@...> writes: : 2009/9/5 M. Warner Losh <imp@...>: : > [[ redirected to arch@ since too much of this discussion has been private ]] : > : > In message: <3bbf2fe10909041455u552b0dbdm1708ea0a26365149@...> : > Attilio Rao <attilio@...> writes: : > : 2009/9/4 M. Warner Losh <imp@...>: : > : > In message: <200909031340.n83Defkv034013@...> : > : > Attilio Rao <attilio@...> 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... : > : > 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? : > : > I think it is a minor concern. There's no locking now to prevent : > multiple threads doing stuff. I think it would be better to : > completely omit this stuff than to put it in 1/2 way. : : 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. : > : > 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. : > : > I explained about partial ordering already. Let me try again. : > : > DS_BUSY isn't compatible with this definition. It would be better to : > merge DS_ATTACHING and DS_DETACHING into one state, say DS_TRANSITION. : > Then we'd do the attach sequence as DS_ALIVE -> DS_TRANSITION -> : > DS_ATTACHED (and later DS_BUSY maybe). Then detach would go from : > DS_ATTACHED -> DS_TRANSITION to tear it down. There's a strong : > ordering expected in the code, and finding all the subtle places that : > this extra state beyond DS_BUSY causes problems will be hard. : : 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... : 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... : 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... : - 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. : > 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... : 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. : For the moment the only thing I still see to be fixed is devinfo. I : will provide a patch before the end of the day. Well, and getting agreement on the model that will be used for the state machine... We're not there yet... There may also be some breakage from this change that's hidden, and I need to carefully audit all uses of the state field, as well as functions that expose its state to the rest of the kernel... Warner _______________________________________________ freebsd-arch@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-arch To unsubscribe, send any mail to "freebsd-arch-unsubscribe@..."
	Re: NEWBUS states by M. Warner Losh :: Rate this Message: Reply to Author \| View Threaded \| Show Only this Message In message: <20090904.172310.-1939841993.imp@...> "M. Warner Losh" <imp@...> writes: : 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. For a version of this hack, see http://people.freebsd.org/~imp/newbus-flags.diff This preserves the semantics of the state field as they exist today, while also providing protection against reentrent code. It also restores a check for GIANT_HELD to the attach routine, which seems to have disappeared along the way. While not needed when the locking does arrive, it is needed today, I believe. It also has the advantage that the following could easily be added to catch wayward drivers: int device_is_attached(device_t dev) { if (dev->flags & DF_TRANSITION) printf( "%s called %s while in attach/detach. This is no deprecated.", device_get_nameunit(dev), __func__); return (dev->state >= DS_ATTACHED); } Or make it a KASSERT later in the 9.x release cycle. Hope this make it clear what my proposed alternative would be... Warner P.S. Yes, I'd rate this code as speculative as the code it replaces (see my prior posts for this criticism). This is an example of how it could be done with less impact to the existing code base, a consideration only because we're in the high 50's of minutes in the 11th hour for the 8.0 release... _______________________________________________ freebsd-arch@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-arch To unsubscribe, send any mail to "freebsd-arch-unsubscribe@..."
	Re: NEWBUS states by Attilio Rao-2 :: Rate this Message: Reply to Author \| View Threaded \| Show Only this Message 2009/9/5 M. Warner Losh <imp@...>: > In message: <3bbf2fe10909041546y2b5633e1ue063955568df1a06@...> > Attilio Rao <attilio@...> writes: > : 2009/9/5 M. Warner Losh <imp@...>: > : > [[ redirected to arch@ since too much of this discussion has been private ]] > : > > : > In message: <3bbf2fe10909041455u552b0dbdm1708ea0a26365149@...> > : > Attilio Rao <attilio@...> writes: > : > : 2009/9/4 M. Warner Losh <imp@...>: > : > : > In message: <200909031340.n83Defkv034013@...> > : > : > Attilio Rao <attilio@...> 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 _______________________________________________ freebsd-arch@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-arch To unsubscribe, send any mail to "freebsd-arch-unsubscribe@..."
	Re: NEWBUS states by Attilio Rao-2 :: Rate this Message: Reply to Author \| View Threaded \| Show Only this Message 2009/9/5 M. Warner Losh <imp@...>: > In message: <20090904.172310.-1939841993.imp@...> > "M. Warner Losh" <imp@...> writes: > : 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. > > For a version of this hack, see > http://people.freebsd.org/~imp/newbus-flags.diff So you propose to offer the transition on the device flags instead than the device states? That is an interesting approach mostly because it won't require an ABI breakage, but let me think about locking implications with it as I want to review some code and came up with a patch/thoughts in some hours. Attilio -- Peace can only be achieved by understanding - A. Einstein _______________________________________________ freebsd-arch@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-arch To unsubscribe, send any mail to "freebsd-arch-unsubscribe@..."
	Re: NEWBUS states by M. Warner Losh :: Rate this Message: Reply to Author \| View Threaded \| Show Only this Message In message: <3bbf2fe10909050817w4e8da3adxd8e431749b432070@...> Attilio Rao <attilio@...> writes: : 2009/9/5 M. Warner Losh <imp@...>: : > In message: <20090904.172310.-1939841993.imp@...> : > "M. Warner Losh" <imp@...> writes: : > : 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. : > : > For a version of this hack, see : > http://people.freebsd.org/~imp/newbus-flags.diff : : So you propose to offer the transition on the device flags instead : than the device states? : That is an interesting approach mostly because it won't require an ABI : breakage, but let me think about locking implications with it as I : want to review some code and came up with a patch/thoughts in some : hours. Please do. I wanted this to provoke thought and experimentation. While not the most beautiful approach, it is one that we can use to get around the API/ABI issues. Please let me know how it works out... The assumption in the patch is that locking requirements for state and flags are identical... Warner _______________________________________________ freebsd-arch@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-arch To unsubscribe, send any mail to "freebsd-arch-unsubscribe@..."
	Re: NEWBUS states by M. Warner Losh :: Rate this Message: Reply to Author \| View Threaded \| Show Only this Message Attilio, [[ trimmed ]] Sounds like we're getting closer to closure... In message: <3bbf2fe10909050812l4340f679h6a4d7dae1daa3bf8@...> Attilio Rao <attilio@...> writes: : 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? [ disconnect between libdevinfo and kernel types ] I think we should have both for either 8.x or 9.x (depending on what the RE@ will permit), and then drop the DIS_ the next release after that. : Ok, if you are strongly against it, we can remove them, I just think : they will be harmless and a good reminder. [ Have the code there to document/enforce protocol wrt state ] I think I'd prefer not to have it, but could easily allow it if we know that it causes no harm and can be reasonably sure that it is close to what the final protocol will be. Warner _______________________________________________ freebsd-arch@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-arch To unsubscribe, send any mail to "freebsd-arch-unsubscribe@..."
	Re: NEWBUS states (was Re: svn commit: r196779 - in head/sys: kern sys) by John Baldwin :: Rate this Message: Reply to Author \| View Threaded \| Show Only this Message On Friday 04 September 2009 6:46:03 pm Attilio Rao wrote: > 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. > The right fix, as pointed out in other e-mails, is to not use > 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 > - 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. Wait, all you need to MFC is the change to the enum. Fixing the various detach routines does _not_ have to be in 8.0. That could be merged after the release. -- John Baldwin _______________________________________________ freebsd-arch@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-arch To unsubscribe, send any mail to "freebsd-arch-unsubscribe@..."
	Re: NEWBUS states (was Re: svn commit: r196779 - in head/sys: kern sys) by Attilio Rao-2 :: Rate this Message: Reply to Author \| View Threaded \| Show Only this Message 2009/9/8 John Baldwin <jhb@...>: > On Friday 04 September 2009 6:46:03 pm Attilio Rao wrote: >> 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. >> The right fix, as pointed out in other e-mails, is to not use >> 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 >> - 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. > > Wait, all you need to MFC is the change to the enum. Fixing the various > detach routines does _not_ have to be in 8.0. That could be merged after the > release. That's not what I mean. What I mean is that in order to have a perfect job right now (and have single-state transition usable right now by both STABLE_8 and HEAD) that what should happen, which is impractical. I was just explaining to Warner why we didn't go with the single-state in the end. Attilio -- Peace can only be achieved by understanding - A. Einstein _______________________________________________ freebsd-arch@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-arch To unsubscribe, send any mail to "freebsd-arch-unsubscribe@..."
	Re: NEWBUS states (was Re: svn commit: r196779 - in head/sys: kern sys) by John Baldwin :: Rate this Message: Reply to Author \| View Threaded \| Show Only this Message On Wednesday 09 September 2009 5:39:39 am Attilio Rao wrote: > 2009/9/8 John Baldwin <jhb@...>: > > On Friday 04 September 2009 6:46:03 pm Attilio Rao wrote: > >> 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. > >> The right fix, as pointed out in other e-mails, is to not use > >> 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 > >> - 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. > > > > Wait, all you need to MFC is the change to the enum. Fixing the various > > detach routines does _not_ have to be in 8.0. That could be merged after the > > release. > > That's not what I mean. > What I mean is that in order to have a perfect job right now (and have > single-state transition usable right now by both STABLE_8 and HEAD) > that what should happen, which is impractical. > I was just explaining to Warner why we didn't go with the single-state > in the end. But we don't need it usable right now. All you need for 8.0 is to reserve the slot in the enum so that the ABI of the enum values doesn't change. Making the state usable is something that can happen after the release and it can include all the changes to make the single state usable. -- John Baldwin _______________________________________________ freebsd-arch@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-arch To unsubscribe, send any mail to "freebsd-arch-unsubscribe@..."
	Re: NEWBUS states (was Re: svn commit: r196779 - in head/sys: kern sys) by Hans Petter Selasky :: Rate this Message: Reply to Author \| View Threaded \| Show Only this Message On Tuesday 08 September 2009 15:36:37 John Baldwin wrote: > On Friday 04 September 2009 6:46:03 pm Attilio Rao wrote: > > 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. > > The right fix, as pointed out in other e-mails, is to not use > > 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 > > - 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. > > Wait, all you need to MFC is the change to the enum. Fixing the various > detach routines does _not_ have to be in 8.0. That could be merged after > the release. Hi, http://svn.freebsd.org/viewvc/base/head/sys/kern/subr_bus.c?r1=196529&r2=196779 I'm sorry to say that the latest patches to subr_bus.c have broken USB. I've got several reports on memory used after free, due to bus_generic_detach() returning EBUSY when called from uhub_detach(). ... bus_generic_detach(device_t dev) { device_t child; int error; if (dev->state != DS_ATTACHED) return (EBUSY); TAILQ_FOREACH(child, &dev->children, link) { if ((error = device_detach(child)) != 0) return (error); } return (0); } A fix for USB is available here: http://perforce.freebsd.org/chv.cgi?CH=168387 --HPS _______________________________________________ freebsd-arch@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-arch To unsubscribe, send any mail to "freebsd-arch-unsubscribe@..."
	Re: NEWBUS states by Attilio Rao-2 :: Rate this Message: Reply to Author \| View Threaded \| Show Only this Message 2009/9/5 M. Warner Losh <imp@...>: > Attilio, > > [[ trimmed ]] > > Sounds like we're getting closer to closure... > So I reverted r196779 and now I would like to commit and MFC just this patch: http://www.freebsd.org/~attilio/newbus9-8.diff I still think the good way to go is to use the device flags, but this patch will allow us to be highly flexible in terms of adding new states, if we need it in the future. Let me know if you have objections against that. Attilio -- Peace can only be achieved by understanding - A. Einstein _______________________________________________ freebsd-arch@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-arch To unsubscribe, send any mail to "freebsd-arch-unsubscribe@..."
	Re: NEWBUS states (was Re: svn commit: r196779 - in head/sys: kern sys) by Attilio Rao-2 :: Rate this Message: Reply to Author \| View Threaded \| Show Only this Message 2009/9/12 Hans Petter Selasky <hselasky@...>: > On Tuesday 08 September 2009 15:36:37 John Baldwin wrote: >> On Friday 04 September 2009 6:46:03 pm Attilio Rao wrote: >> > 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. >> > The right fix, as pointed out in other e-mails, is to not use >> > 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 >> > - 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. >> >> Wait, all you need to MFC is the change to the enum. Fixing the various >> detach routines does _not_ have to be in 8.0. That could be merged after >> the release. > > Hi, > > http://svn.freebsd.org/viewvc/base/head/sys/kern/subr_bus.c?r1=196529&r2=196779 > > I'm sorry to say that the latest patches to subr_bus.c have broken USB. I've > got several reports on memory used after free, due to bus_generic_detach() > returning EBUSY when called from uhub_detach(). Yes, I think there was an error in the logic. However I reverted them because we decided to go with another approach. I'm not sure if your patch (I still didn't review them) are though necessary even in the case of a saner logic, so for the moment, try to not loose them. Attilio -- Peace can only be achieved by understanding - A. Einstein _______________________________________________ freebsd-arch@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-arch To unsubscribe, send any mail to "freebsd-arch-unsubscribe@..."
	Re: NEWBUS states (was Re: svn commit: r196779 - in head/sys: kern sys) by John Baldwin :: Rate this Message: Reply to Author \| View Threaded \| Show Only this Message On Saturday 12 September 2009 4:09:21 am Hans Petter Selasky wrote: > On Tuesday 08 September 2009 15:36:37 John Baldwin wrote: > > On Friday 04 September 2009 6:46:03 pm Attilio Rao wrote: > > > 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. > > > The right fix, as pointed out in other e-mails, is to not use > > > 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 > > > - 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. > > > > Wait, all you need to MFC is the change to the enum. Fixing the various > > detach routines does _not_ have to be in 8.0. That could be merged after > > the release. > > Hi, > > http://svn.freebsd.org/viewvc/base/head/sys/kern/subr_bus.c?r1=196529&r2=196779 > > I'm sorry to say that the latest patches to subr_bus.c have broken USB. I've > got several reports on memory used after free, due to bus_generic_detach() > returning EBUSY when called from uhub_detach(). > > ... > bus_generic_detach(device_t dev) > { > device_t child; > int error; > > if (dev->state != DS_ATTACHED) > return (EBUSY); > > TAILQ_FOREACH(child, &dev->children, link) { > if ((error = device_detach(child)) != 0) > return (error); > } > > return (0); > } > > A fix for USB is available here: > > http://perforce.freebsd.org/chv.cgi?CH=168387 I think bus_generic_detach() needs to work when called from a foo_detach() routine, so I think you don't need the USB changes. Even if the new states get reintroduced bus_generic_detach() will still have to work correctly the way you had used it. -- John Baldwin _______________________________________________ freebsd-arch@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-arch To unsubscribe, send any mail to "freebsd-arch-unsubscribe@..."
	Re: NEWBUS states (was Re: svn commit: r196779 - in head/sys: kern sys) by Hans Petter Selasky :: Rate this Message: Reply to Author \| View Threaded \| Show Only this Message On Monday 14 September 2009 14:52:48 John Baldwin wrote: > > I think bus_generic_detach() needs to work when called from a foo_detach() > routine, so I think you don't need the USB changes. Even if the new states > get reintroduced bus_generic_detach() will still have to work correctly the > way you had used it. Ok. --HPS _______________________________________________ freebsd-arch@... mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-arch To unsubscribe, send any mail to "freebsd-arch-unsubscribe@..."