christos@zoulas.com (Christos Zoulas) writes:

> In article <200203252128.g2PLSGm00960@kaakeli.ssh.fi>,  <kivinen@ssh.fi> wrote:
> >
> >>Number:         16058
> >>Category:       kern
> >>Synopsis:       If pcmcia com card insertation fails, kernel panics
> >when card is removed.
> >>Confidential:   no
> >>Severity:       serious
> >>Priority:       medium
> >>Responsible:    kern-bug-people
> >>State:          open
> >>Class:          sw-bug
> >>Submitter-Id:   net
> >>Arrival-Date:   Mon Mar 25 13:30:00 PST 2002
> >>Closed-Date:
> >>Last-Modified:
> >>Originator:     Tero Kivinen
> >>Release:        NetBSD 1.5ZC
> >>Organization:
> >SSH Communications Security
> >>Environment:
> >System: NetBSD kaakeli.ssh.fi 1.5ZC NetBSD 1.5ZC (KAAKELI) #11: Mon Mar
> >25 22:50:11 EET 2002
> >root@kaakeli.ssh.fi:/usr/src/sys/arch/i386/compile/KAAKELI i386
> >Architecture: i386
> >Machine: i386
> >>Description:
> Can you try this one?

Yes, I can, but I am pretty sure it does not fix the problem. BTW it
would be easier to notice that someone wants me to test something if
the mail comes directly to me, instead to the netbsd-bugs@netbsd.org
mailing list.... I do not read netbsd-bugs@netbsd.org that often.

> It is much shorter and does not play with the mi
> drivers.

So the bug in the mi driver is still there... Note, that my patch
fixed two bugs, one in the mi-driver and one in the com_pcmcia.c. 

> Index: com_pcmcia.c
> ===================================================================
> RCS file: /cvsroot/syssrc/sys/dev/pcmcia/com_pcmcia.c,v
> retrieving revision 1.26
> diff -u -u -r1.26 com_pcmcia.c
> --- com_pcmcia.c	2002/03/10 19:20:50	1.26
> +++ com_pcmcia.c	2002/03/25 22:36:18
> @@ -217,6 +217,8 @@
>  
>  	psc->sc_pf = pa->pf;
>  
> +	psc->sc_io_window = -1;
> +
>  retry:
>  	/* find a cfe we can use */
>  
> @@ -308,11 +310,13 @@
>  	struct com_pcmcia_softc *psc = (struct com_pcmcia_softc *) self;
>  	int error;
>  
> -	if ((error = com_detach(self, flags)) != 0)
> -		return error;
> +	if (psc->sc_io_window != -1) {
> +		if ((error = com_detach(self, flags)) != 0)
> +			return error;
>  
> -	/* Unmap our i/o window. */
> -	pcmcia_io_unmap(psc->sc_pf, psc->sc_io_window);
> +		/* Unmap our i/o window. */
> +		pcmcia_io_unmap(psc->sc_pf, psc->sc_io_window);
> +	}
>  
>  	/* Free our i/o space. */
>  	pcmcia_io_free(psc->sc_pf, &psc->sc_pcioh);

Crashed in the pcmcia_io_free, because sc_pcioh is not yet set...
Note, that we return from the com_pcmcia_attach in the middle, before
calling pcmcia_io_map, com_attach_subr, or pcmcia_io_alloc, thus we
cannot do anything in the com_pcmcia_detach.

This patch fixes the problem:
----------------------------------------------------------------------
Index: com_pcmcia.c
===================================================================
RCS file: /cvsroot/syssrc/sys/dev/pcmcia/com_pcmcia.c,v
retrieving revision 1.27
diff -u -r1.27 com_pcmcia.c
--- com_pcmcia.c        2002/04/03 00:18:31     1.27
+++ com_pcmcia.c        2002/04/08 15:14:38
@@ -310,13 +310,19 @@
        struct com_pcmcia_softc *psc = (struct com_pcmcia_softc *) self;
        int error;
 
-       if (psc->sc_io_window != -1) {
-               if ((error = com_detach(self, flags)) != 0)
-                       return error;
-
-               /* Unmap our i/o window. */
-               pcmcia_io_unmap(psc->sc_pf, psc->sc_io_window);
+       /* Unmap our i/o window. */
+       if (psc->sc_io_window == -1) {
+               /* Pcmcia i/o not allocated, return. */
+               printf("%s: I/O window not allocated.",
+                      psc->sc_com.sc_dev.dv_xname);
+               return (0);
        }
+
+       if ((error = com_detach(self, flags)) != 0)
+               return error;
+
+       /* Unmap our i/o window. */
+       pcmcia_io_unmap(psc->sc_pf, psc->sc_io_window);
 
        /* Free our i/o space. */
        pcmcia_io_free(psc->sc_pf, &psc->sc_pcioh);
----------------------------------------------------------------------

The ic/com.c still has the similar bugs if the sc->sc_rbuf allocation
fails, as it returns from the middle of the function before calling
tty_attach, softintr_establish, rnd_attach_source etc. Thus the
com_detach needs to notice that if sc->sc_rbuf is NULL then it must
only do ttyfree and return, and the rest can only be done if the
sc->sc_rbuf is != NULL.

My previous fix was little to complicated, as it also worked in the
case the com_pcmcia_attach didn't call com_attach_subr at all, but the
com_pcmcia_detach still called com_detach. With the current code the
sc_tty checking is not needed and it is enough to simply call ttyfree
and return if sc_rbuf == NULL. Something like this should be enough:
(I cannot test this as this is only run if the malloc fails, and I
don't know easy way to make that specific malloc to fail).

Note, that the line numbers are off as I have already written a driver
for the Nokia pc card, and that causes line number of the com.c to be
different (it is almost complete, but there seems to be some problems
with the 60 byte fifo of the card, thus I need to still investigate it
more. It currently works fine when the fifos are disabled):

Index: com.c
===================================================================
RCS file: /cvsroot/syssrc/sys/dev/ic/com.c,v
retrieving revision 1.194
diff -u -r1.194 com.c
--- com.c       2002/03/17 19:40:57     1.194
+++ com.c       2002/04/08 15:28:24
@@ -660,6 +680,13 @@
        mn |= COMDIALOUT_MASK;
        vdevgone(maj, mn, mn, VCHR);
 
+       if (sc->sc_rbuf == NULL) {
+                       /* Ring buffer allocation failed in the com_attach_subr,
+                  only the tty is allocated, and nothing else. */
+               ttyfree(sc->sc_tty);
+               return 0;
+       }
+       
        /* Free the receive buffer. */
        free(sc->sc_rbuf, M_DEVBUF);
  
-- 
kivinen@ssh.fi
SSH Communications Security                  http://www.ssh.fi/
SSH IPSEC Toolkit                            http://www.ssh.fi/ipsec/