Re: if_dc panics with 3Com OfficeConnect 10/100B PCI

On Monday 12 September 2005 10:55 pm, Giorgos Keramidas wrote:
> On 2005-08-22 00:56, Giorgos Keramidas <keramida_at_freebsd.org> wrote:
> > On 2005-08-16 13:48, John Baldwin <jhb_at_FreeBSD.org> wrote:
> > > Compile in DDB and get a stack trace over the serial console using 't'
> > > in DDB instead of worrying about getting a dump.
> >
> > [...]
> > I have no way to debug the crash now, since I only have one machine at
> > home.  When I get the chance I plan to test this again, and I'll report
> > back then, if this is still a problem.
>
> I think I've finally located the cause of this panic.  The panic happens
> early in dc_attach(), and the last message I see is the one printed by
> dc_attach() in src/sys/pci/if_dc.c:
>
> % 	if (sc->dc_res == NULL) {
> % 		device_printf(dev, "couldn't map ports/memory\n");
> % 		error = ENXIO;
> % 		goto fail;
> % 	}
> %
> % 	[ various busdma setup calls ]
> %
> % fail:
> % 	if (error)
> % 		dc_detach(dev);
> % 	return (error);
>
> The bug of dc_detach() that causes the panic is that it doesn't check if
> one of the busdma tags and some maps it destroys have been allocated.  In
> this case, because dc_attach() fails too early, before any busdma calls,
> they haven't been allocated and one of the busdma calls panics when it gets
> a NULL pointer.
>
> The following patch fixes the panic of loading if_dc.ko when the 3Com
> NIC of the subject is installed:
>
> %%%
> Index: if_dc.c
> ===================================================================
> RCS file: /home/ncvs/src/sys/pci/if_dc.c,v
> retrieving revision 1.166
> diff -u -r1.166 if_dc.c
> --- if_dc.c	29 Aug 2005 18:45:21 -0000	1.166
> +++ if_dc.c	13 Sep 2005 00:32:51 -0000
> _at__at_ -2364,11 +2364,17 _at__at_
>  		bus_dmamem_free(sc->dc_stag, sc->dc_cdata.dc_sbuf, sc->dc_smap);
>  	if (sc->dc_ldata != NULL)
>  		bus_dmamem_free(sc->dc_ltag, sc->dc_ldata, sc->dc_lmap);
> -	for (i = 0; i < DC_TX_LIST_CNT; i++)
> -		bus_dmamap_destroy(sc->dc_mtag, sc->dc_cdata.dc_tx_map[i]);
> -	for (i = 0; i < DC_RX_LIST_CNT; i++)
> -		bus_dmamap_destroy(sc->dc_mtag, sc->dc_cdata.dc_rx_map[i]);
> -	bus_dmamap_destroy(sc->dc_mtag, sc->dc_sparemap);
> +	if (sc->dc_mtag) {
> +		for (i = 0; i < DC_TX_LIST_CNT; i++)
> +			if (sc->dc_cdata.dc_tx_map[i] != NULL)
> +				bus_dmamap_destroy(sc->dc_mtag,
> +				    sc->dc_cdata.dc_tx_map[i]);
> +		for (i = 0; i < DC_RX_LIST_CNT; i++)
> +			if (sc->dc_cdata.dc_rx_map[i] != NULL)
> +				bus_dmamap_destroy(sc->dc_mtag,
> +				    sc->dc_cdata.dc_rx_map[i]);
> +		bus_dmamap_destroy(sc->dc_mtag, sc->dc_sparemap);
> +	}
>  	if (sc->dc_stag)
>  		bus_dma_tag_destroy(sc->dc_stag);
>  	if (sc->dc_mtag)
> %%%

Ah, looks good to me!

> With this patch, on CURRENT of Sep-12-2005, the attach of the NIC in
> question still fails, but at least it doesn't panic the system while
> doing so :)
>
> P.S.: Is there any way to guard against similar bugs in other drivers
> and/or bugs that may be introduced later on?  For instance, would adding
> KASSERT() macros in busdma_machdep.c slow things down too much?

Since KASSERT()'s are optional, you can go ahead and add them if you want, 
though it's just trading one panic for another.

-- 
John Baldwin <jhb_at_FreeBSD.org>  <><  http://www.FreeBSD.org/~jhb/
"Power Users Use the Power to Serve"  =  http://www.FreeBSD.org