[PATCH] ugen_destroy_devnodes panic

[Julian Elischer]

Peter Pentchev wrote:

>Hi,
>
>I am getting reproducible panics on RELENG_5 with ugen and my camera,
>Canon PowerShot A80.  The problem seems to be that ugen_set_config()
>calls ugen_destroy_devnodes() twice, and ugen_destroy_devnodes() may get
>to operate on the same sc's and the same devs, thus calling
>destroy_dev() on an already destroyed endpoint device.  My first hunch
>was to comment out the first ugen_destroy_devnodes() call in
>ugen_set_config(), but then I realized that it might be necessary to
>make sure no one else tries to open a ugen endpoint while it is being
>marked for destruction.:
>
how does this achieve that goal?

how about seeing if you can use the version of ugen.c from -current.
this has all been reworked there.


>
>What do you think about the following patch which makes
>ugen_destroy_devnodes() zero out the dev pointer for each destroyed
>device, so that it never attempts to destroy the same device twice?  Am
>I right to think that after this routine is called, no one should use
>the device endpoint that it freed?
>
>G'luck,
>Peter
>
>Index: src/sys/dev/usb/ugen.c
>===================================================================
>RCS file: /home/ncvs/src/sys/dev/usb/ugen.c,v
>retrieving revision 1.88.2.2
>diff -u -r1.88.2.2 ugen.c
>--- src/sys/dev/usb/ugen.c	31 Dec 2004 08:01:48 -0000	1.88.2.2
>+++ src/sys/dev/usb/ugen.c	4 Jan 2005 18:20:39 -0000
>@@ -283,7 +283,7 @@
> ugen_destroy_devnodes(struct ugen_softc *sc)
> {
> 	int endptno;
>-	struct cdev *dev;
>+	struct cdev **pdev;
> 
> 	/* destroy all devices for the other (existing) endpoints as well */
> 	for (endptno = 1; endptno < USB_MAX_ENDPOINTS; endptno++) {
>@@ -298,10 +298,13 @@
> 			 * of the structs is populated.
> 			 */
> 			if (sc->sc_endpoints[endptno][IN].sc != NULL)
>-				dev = sc->sc_endpoints[endptno][IN].dev;
>+				pdev = &sc->sc_endpoints[endptno][IN].dev;
> 			else
>-				dev = sc->sc_endpoints[endptno][OUT].dev;
>-			destroy_dev(dev);
>+				pdev = &sc->sc_endpoints[endptno][OUT].dev;
>+			if (*pdev == NULL)
>+				continue;
>+			destroy_dev(*pdev);
>+			*pdev = NULL;
> 		}
> 	}
> }
>
>  
>