Re: panic in bus_dma_tag_destroy()

[Matthew Dillon <dillon@xxxxxxxxxxxxxxxxxxxx>]

:No, I'm not refering to the malloc()'s in the driver. I'm refering to
:the malloc that bus_dma_tag_create() does internally (i.e. the driver
:has no visibility to this malloc). Take a look at line 158 in
:
:src/sys/i386/i386/busdma_machdep.c
:
:That is that malloc at issue. This is involked in the mpt driver on
:line 504 in
:
:src/sys/dev/disk/mpt/mpt_pci.c
:..
:with the 4th to the last argument causing the problem. I'm pretty sure
:that mpt_pci.c will fail in the same way my driver did as I used
:mpt_pci.c as a template :)

    Hmm.  I think you are definitely on to something here.  It
    looks like the drivers such as the advansys are creating 
    a hierarchical bus dma structure and attempting to specify
    an unlimited number of segments for the 'parent' (e.g. at
    the EISA or PCI bus level), then specifying child tags
    with the actual number of segments.   The 'parent' bus_dma_tags
    are never actually used for I/O, the idea is that they 
    specify bus-governing restrictions.  In reality, however,
    the driver should not be making those decisions at all and
    the busdma architecture is only actually hierarchical when
    figuring out address restrictions.

    FreeBSD-5 moved the malloc from bus_dma_tag_create() to
    bus_dmamap_create() and bus_dmamem_alloc() to retain the
    API structure.  They didn't really fix the API issue, though,
    they just avoided mallocing the structures that weren't
    actually being used for DMA.

:That said, I changed the bus_dma_tag_create() in my driver to look
:like what isp does, and this problem goes away.
    
    Yes, that makes sense.  ISP is doing it a better way.  Practically
    speaking any driver you do should specify a sane value for
    nsegments and not even try to specify a parent, and certainly should
    not be making assumptions about what a particular bus architecture's
    limits are.  The design is seriously flawed.

:Would it be clearer if I submitted patches for the above 4 drivers? I
:know what to do for the drivers, but not how best to protect
:bus_dma_tag_create().
:
:-- 
:Chuck Tuffli
:Agilent Technologies

    I think I understand the issue.  This patch should 'fix' the
    problem in the same way FreeBSD-5 'fixed' it (that is, not really
    fixing it, just avoiding the malloc for bus_dma_tag structures
    that aren't actually used for DMA).

    To do this correctly we really have to go through and create 
    a master bus_dma_tag 'parent' structure for the various bus
    types (pci, eisa, isa, agp, etc) which the drivers use instead
    of rolling their own.

    I'd like people who are running some of these other disk drivers
    (not just ATA) to try this patch too.  I'll commit it in about a
    week.

					-Matt
					Matthew Dillon 
					<dillon@xxxxxxxxxxxxx>
Index: i386/busdma_machdep.c
===================================================================
RCS file: /cvs/src/sys/i386/i386/busdma_machdep.c,v
retrieving revision 1.9
diff -u -r1.9 busdma_machdep.c
--- i386/busdma_machdep.c	19 Apr 2004 13:37:43 -0000	1.9
+++ i386/busdma_machdep.c	11 Oct 2004 04:00:08 -0000
@@ -155,12 +155,7 @@
 	newtag->flags = flags;
 	newtag->ref_count = 1; /* Count ourself */
 	newtag->map_count = 0;
-	newtag->segments = malloc(sizeof(bus_dma_segment_t) * newtag->nsegments,
-				  M_DEVBUF, M_INTWAIT);
-	if (newtag->segments == NULL) {
-		free(newtag, M_DEVBUF);
-		return(ENOMEM);
-	}
+	newtag->segments = NULL;
 	
 	/* Take into account any restrictions imposed by our parent tag */
 	if (parent != NULL) {
@@ -259,6 +254,12 @@
 
 	error = 0;
 
+	if (dmat->segments == NULL) {
+		KKASSERT(dmat->nsegments < 16384);
+		dmat->segments = malloc(sizeof(bus_dma_segment_t) * 
+					dmat->nsegments, M_DEVBUF, M_INTWAIT);
+	}
+
 	if (dmat->lowaddr < ptoa(Maxmem)) {
 		/* Must bounce */
 		int maxpages;
@@ -339,6 +340,12 @@
 	/* If we succeed, no mapping/bouncing will be required */
 	*mapp = NULL;
 
+	if (dmat->segments == NULL) {
+		KKASSERT(dmat->nsegments < 16384);
+		dmat->segments = malloc(sizeof(bus_dma_segment_t) * 
+					dmat->nsegments, M_DEVBUF, M_INTWAIT);
+	}
+
 	if (flags & BUS_DMA_NOWAIT)
 		mflags = M_NOWAIT;
 	else