Re: kern/50810: Kernel page fault trap in ugenclose()

[Nick Hudson <skrll%netbsd.org@localhost>]

On 02/20/16 20:55, Andreas Gustafsson wrote:
> The following reply was made to PR kern/50810; it has been noted by GNATS.
>
> From: Andreas Gustafsson<gson%gson.org@localhost>
> To:skrll%NetBSD.org@localhost
> Cc:gnats-bugs%netbsd.org@localhost
> Subject: Re: kern/50810: Kernel page fault trap in ugenclose()
> Date: Sat, 20 Feb 2016 22:53:52 +0200
>
>   Nick,
>   
>   You wrote:
>   >  > Also, there are several places checking for sce == NULL, for example:
>   >  >
>   >  >     sce = &sc->sc_endpoints[UGENENDPOINT(dev)][IN];
>   >  >     if (sce == NULL)
>   >  >
>   >  > None of these make any sense - sce is a pointer into the middle of a
>   >  > the ugen_softc struct, so it can *never* be NULL by definition.
>   >  > Should they all say "sce->sc == NULL"?

I got rid of these pointless conditionals

>   >  > What is the canonical way
>   >  > of distinguishing a valid endpoint from an invalid one?
>   >
>   >  I guess edesc and/or iface get populated with something after
>   >  ugen_set_interface/ugen_set_config
>   
>   I'm sorry, but I fail to see the connection between my question and
>   your answer.

I was really answering your second question and not the first. Both 
edesc and iface will
be non-null if the endpoint is valid.

>   --
>   Andreas Gustafsson,gson%gson.org@localhost
>   

Does this diff help? I'm not sure of retaining selinfo and the cv is 
correct for ugen_set_interface.


Nick

Index: sys/dev/usb/ugen.c
===================================================================
RCS file: /cvsroot/src/sys/dev/usb/ugen.c,v
retrieving revision 1.129
diff -u -p -r1.129 ugen.c
--- sys/dev/usb/ugen.c	21 Feb 2016 09:50:10 -0000	1.129
+++ sys/dev/usb/ugen.c	21 Feb 2016 10:04:19 -0000
@@ -270,6 +270,19 @@ ugen_attach(device_t parent, device_t se
 	return;
 }
 
+Static void
+ugen_clear_endpoints(struct ugen_softc *sc)
+{
+
+	/* Clear out the old info, but leave the selinfo and cv initialised. */
+	for (int i = 0; i < USB_MAX_ENDPOINTS; i++) {
+		for (int dir = OUT; dir <= IN; dir++) {
+			struct ugen_endpoint *sce = &sc->sc_endpoints[i][dir];
+			memset(sce, 0, UGEN_ENDPOINT_NONZERO_CRUFT);
+		}
+	}
+}
+
 Static int
 ugen_set_config(struct ugen_softc *sc, int configno)
 {
@@ -281,7 +294,7 @@ ugen_set_config(struct ugen_softc *sc, i
 	u_int8_t niface, nendpt;
 	int ifaceno, endptno, endpt;
 	usbd_status err;
-	int dir, i;
+	int dir;
 
 	DPRINTFN(1,("ugen_set_config: %s to configno %d, sc=%p\n",
 		    device_xname(sc->sc_dev), configno, sc));
@@ -310,13 +323,7 @@ ugen_set_config(struct ugen_softc *sc, i
 	if (err)
 		return (err);
 
-	/* Clear out the old info, but leave the selinfo and cv initialised. */
-	for (i = 0; i < USB_MAX_ENDPOINTS; i++) {
-		for (dir = OUT; dir <= IN; dir++) {
-			sce = &sc->sc_endpoints[i][dir];
-			memset(sce, 0, UGEN_ENDPOINT_NONZERO_CRUFT);
-		}
-	}
+	ugen_clear_endpoints(sc);
 
 	for (ifaceno = 0; ifaceno < niface; ifaceno++) {
 		DPRINTFN(1,("ugen_set_config: ifaceno %d\n", ifaceno));
@@ -1336,16 +1343,6 @@ ugen_set_interface(struct ugen_softc *sc
 	err = usbd_endpoint_count(iface, &nendpt);
 	if (err)
 		return (err);
-	/* 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 = NULL;
-		sce->edesc = NULL;
-		sce->iface = NULL;
-	}
 
 	/* change setting */
 	err = usbd_set_interface(iface, altno);
@@ -1355,6 +1352,9 @@ ugen_set_interface(struct ugen_softc *sc
 	err = usbd_endpoint_count(iface, &nendpt);
 	if (err)
 		return (err);
+
+	ugen_clear_endpoints(sc);
+
 	for (endptno = 0; endptno < nendpt; endptno++) {
 		ed = usbd_interface2endpoint_descriptor(iface,endptno);
 		KASSERT(ed != NULL);