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

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

The following reply was made to PR kern/50810; it has been noted by GNATS.

From: Nick Hudson <skrll%netbsd.org@localhost>
To: gnats-bugs%NetBSD.org@localhost, gnats-admin%netbsd.org@localhost, netbsd-bugs%netbsd.org@localhost,
 Andreas Gustafsson <gson%gson.org@localhost>
Cc: 
Subject: Re: kern/50810: Kernel page fault trap in ugenclose()
Date: Sun, 21 Feb 2016 10:05:36 +0000

 This is a multi-part message in MIME format.
 --------------050805080502070203020306
 Content-Type: text/plain; charset=windows-1252; format=flowed
 Content-Transfer-Encoding: 7bit
 
 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
 
 
 --------------050805080502070203020306
 Content-Type: text/x-patch;
  name="ugen.c.diff"
 Content-Transfer-Encoding: 7bit
 Content-Disposition: attachment;
  filename="ugen.c.diff"
 
 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);
 
 
 --------------050805080502070203020306--