Re: Panic with ugen

Bruce Cran wrote:

> I've just had the same panic on -CURRENT:
> 
> ugen0: at uhub1 port 1 (addr 2) disconnected^M
> WARNING: Driver mistake: destroy_dev on 114/1^M
> panic: don't do that^M
> Debugger("panic")^M
> db> tr^M
> Debugger(c05eb534) at Debugger+0x45^M
> panic(c05e8a79,c05e8aac,72,1,1) at panic+0xbb^M
> destroy_dev(c47f3b00,2,c4852860,e9b25c84,c47ff53d) at destroy_dev+0x32^M
> ugen_destroy_devnodes(c4851000,c47f3900,c4851000,c484f100,c484f100) at
> ugen_destroy_devnodes+0x5b^M
> ugen_detach(c484f100) at ugen_detach+0xc1^M

I think I might have a fix for this, and for Martin's problem as well. It 
appears my original patch didn't quite cover all the cases where this could 
happen in ugen.

Could one (or both) of you try the attached patch, to see if it alleviates the 
problem? I guess it might not be easily reproducable for you, Bruce, but I 
think Martin could fairly consistently panic the system by running a program.

-- 
Cheers,
Jay

http://www.evilrealms.net/ - Systems Administrator & Developer
http://www.imperial.ac.uk/ - 3rd year CS student

Index: ugen.c
===================================================================
RCS file: /home/ncvs/src/sys/dev/usb/ugen.c,v
retrieving revision 1.81
diff -u -3 -p -r1.81 ugen.c
--- ugen.c	9 Nov 2003 09:17:22 -0000	1.81
+++ ugen.c	12 Dec 2003 18:04:58 -0000
_at__at_ -313,8 +313,8 _at__at_ ugen_set_config(struct ugen_softc *sc, i
 	usbd_device_handle dev = sc->sc_udev;
 	usbd_interface_handle iface;
 	usb_endpoint_descriptor_t *ed;
-	struct ugen_endpoint *sce;
-	u_int8_t niface, nendpt;
+	struct ugen_endpoint *sce, **sce_cache, ***sce_cache_arr;
+	u_int8_t niface, niface_cache, nendpt, *nendpt_cache;
 	int ifaceno, endptno, endpt;
 	usbd_status err;
 	int dir;
_at__at_ -322,10 +322,6 _at__at_ ugen_set_config(struct ugen_softc *sc, i
 	DPRINTFN(1,("ugen_set_config: %s to configno %d, sc=%p\n",
 		    USBDEVNAME(sc->sc_dev), configno, sc));
 
-#if defined(__FreeBSD__)
-	ugen_destroy_devnodes(sc);
-#endif
-
 	/* We start at 1, not 0, because we don't care whether the
 	 * control endpoint is open or not. It is always present.
 	 */
_at__at_ -337,25 +333,88 _at__at_ ugen_set_config(struct ugen_softc *sc, i
 			return (USBD_IN_USE);
 		}
 
+	err = usbd_interface_count(dev, &niface);
+	if (err)
+		return (err);
+
+	/* store an array of endpoint descriptors to clear if the configuration
+	 * change succeeds - these aren't available afterwards */
+	nendpt_cache = malloc(sizeof(u_int8_t) * niface, M_TEMP, M_WAITOK);
+	sce_cache_arr = malloc(sizeof(struct ugen_endpoint **) * niface, M_TEMP,
+		M_WAITOK);
+	niface_cache = niface;
+
+	for (ifaceno = 0; ifaceno < niface; ifaceno++) {
+		DPRINTFN(1,("ugen_set_config: ifaceno %d\n", ifaceno));
+		err = usbd_device2interface_handle(dev, ifaceno, &iface);
+		if (err)
+			panic("ugen_set_config: can't obtain interface handle");
+		err = usbd_endpoint_count(iface, &nendpt);
+		if (err)
+			panic("ugen_set_config: endpoint count failed");
+
+		/* store endpoint descriptors for each interface */
+		nendpt_cache[ifaceno] = nendpt;
+		sce_cache = malloc(sizeof(struct ugen_endpoint *) * nendpt, M_TEMP,
+			M_WAITOK);
+		sce_cache_arr[ifaceno] = sce_cache;
+
+		for (endptno = 0; endptno < nendpt; endptno++) {
+			ed = usbd_interface2endpoint_descriptor(iface,endptno);
+			endpt = ed->bEndpointAddress;
+			dir = UE_GET_DIR(endpt) == UE_DIR_IN ? IN : OUT;
+			sce_cache[endptno] = &sc->sc_endpoints[UE_GET_ADDR(endpt)][dir];
+		}
+	}
+
 	/* Avoid setting the current value. */
 	if (usbd_get_config_descriptor(dev)->bConfigurationValue != configno) {
+		/* attempt to perform the configuration change */
 		err = usbd_set_config_no(dev, configno, 1);
-		if (err)
+		if (err) {
+			for(ifaceno = 0; ifaceno < niface_cache; ifaceno++)
+				free(sce_cache_arr[ifaceno], M_TEMP);
+			free(sce_cache_arr, M_TEMP);
+			free(nendpt_cache, M_TEMP);
 			return (err);
+		}
 	}
 
+#if defined(__FreeBSD__)
+	ugen_destroy_devnodes(sc);
+#endif
+
+	/* now we can clear the old interface's ugen_endpoints */
+	for(ifaceno = 0; ifaceno < niface_cache; ifaceno++) {
+		sce_cache = sce_cache_arr[ifaceno];
+		for(endptno = 0; endptno < nendpt_cache[ifaceno]; endptno++) {
+			sce = sce_cache[endptno];
+			sce->sc = 0;
+			sce->edesc = 0;
+			sce->iface = 0;
+		}
+	}
+
+	/* and free the cache storing them */
+	for(ifaceno = 0; ifaceno < niface_cache; ifaceno++)
+		free(sce_cache_arr[ifaceno], M_TEMP);
+	free(sce_cache_arr, M_TEMP);
+	free(nendpt_cache, M_TEMP);
+
+	/* set the new configuration's ugen_endpoints */
 	err = usbd_interface_count(dev, &niface);
 	if (err)
-		return (err);
+		panic("ugen_set_config: interface count failed");
+
 	memset(sc->sc_endpoints, 0, sizeof sc->sc_endpoints);
 	for (ifaceno = 0; ifaceno < niface; ifaceno++) {
 		DPRINTFN(1,("ugen_set_config: ifaceno %d\n", ifaceno));
 		err = usbd_device2interface_handle(dev, ifaceno, &iface);
 		if (err)
-			return (err);
+			panic("ugen_set_config: can't obtain interface handle");
 		err = usbd_endpoint_count(iface, &nendpt);
 		if (err)
-			return (err);
+			panic("ugen_set_config: endpoint count failed");
 		for (endptno = 0; endptno < nendpt; endptno++) {
 			ed = usbd_interface2endpoint_descriptor(iface,endptno);
 			endpt = ed->bEndpointAddress;
_at__at_ -1014,8 +1073,8 _at__at_ ugen_set_interface(struct ugen_softc *sc
 	usbd_interface_handle iface;
 	usb_endpoint_descriptor_t *ed;
 	usbd_status err;
-	struct ugen_endpoint *sce;
-	u_int8_t niface, nendpt, endptno, endpt;
+	struct ugen_endpoint *sce, **sce_cache;
+	u_int8_t niface, nendpt, nendpt_cache, endptno, endpt;
 	int dir;
 
 	DPRINTFN(15, ("ugen_set_interface %d %d\n", ifaceidx, altno));
_at__at_ -1033,30 +1092,45 _at__at_ ugen_set_interface(struct ugen_softc *sc
 	if (err)
 		return (err);
 
-#if defined(__FreeBSD__)
-	/* destroy the existing devices, we remake the new ones in a moment */
-	ugen_destroy_devnodes(sc);
-#endif
+	/* store an array of endpoint descriptors to clear if the interface
+	 * change succeeds - these aren't available afterwards */
+	sce_cache = malloc(sizeof(struct ugen_endpoint *) * nendpt, M_TEMP,
+		M_WAITOK);
+	nendpt_cache = nendpt;
 
-	/* XXX should only do this after setting new altno has succeeded */
 	for (endptno = 0; endptno < nendpt; endptno++) {
 		ed = usbd_interface2endpoint_descriptor(iface,endptno);
 		endpt = ed->bEndpointAddress;
 		dir = UE_GET_DIR(endpt) == UE_DIR_IN ? IN : OUT;
-		sce = &sc->sc_endpoints[UE_GET_ADDR(endpt)][dir];
-		sce->sc = 0;
-		sce->edesc = 0;
-		sce->iface = 0;
+		sce_cache[endptno] = &sc->sc_endpoints[UE_GET_ADDR(endpt)][dir];
 	}
 
 	/* change setting */
 	err = usbd_set_interface(iface, altno);
-	if (err)
+	if (err) {
+		free(sce_cache, M_TEMP);
 		return (err);
+	}
 
 	err = usbd_endpoint_count(iface, &nendpt);
 	if (err)
-		return (err);
+		panic("ugen_set_interface: endpoint count failed");
+
+#if defined(__FreeBSD__)
+	/* destroy the existing devices, we remake the new ones in a moment */
+	ugen_destroy_devnodes(sc);
+#endif
+
+	/* now we can clear the old interface's ugen_endpoints */
+	for (endptno = 0; endptno < nendpt_cache; endptno++) {
+		sce = sce_cache[endptno];
+		sce->sc = 0;
+		sce->edesc = 0;
+		sce->iface = 0;
+	}
+	free(sce_cache, M_TEMP);
+
+	/* set the new interface's ugen_endpoints */
 	for (endptno = 0; endptno < nendpt; endptno++) {
 		ed = usbd_interface2endpoint_descriptor(iface,endptno);
 		endpt = ed->bEndpointAddress;