Re: [PATCH] ohci1394: dma_pool_destroy while in_atomic() && irqs_disabled()

[Parag Warudkar <kernel-stuff@comcast.net>]

[-- Attachment #1: Type: text/plain, Size: 1210 bytes --]

Attached is the reworked patch to take care of Andrew's suggestions -

1) Allocate the work struct dynamically in struct ti_ohci during  device 
probe, free it during device remove
2) In ohci1394_pci_remove, ensure queued work, if any, is flushed before 
the device is removed (As I understand, this should work for both device 
removal cases as well as module removal - correct?)
3) Rework the free_dma_rcv_ctx  - It is called in multiple contexts by 
different callers - teach it to free the dma according to caller's wish 
- either direct free where caller determines the context is safe to 
sleep OR delayed via schedule_work when caller wants to call it from a 
context where it cannot sleep.  So it now takes 2 extra arguments - 
method to free - direct or delayed and if it is to be  freed delayed, an 
appropriate work_struct.

Andrew - flush_scheduled_work does not touch work which isn't shceduled 
- so I don't think INIT_WORK in setup is necessary. Let me know if I am 
missing something here. (I tried INIT_WORK in ochi1394_pci_probe and 
putting flush_scheduled_work in ohci1394_pci_remove - It did not result 
in calling the work function after I did rmmod, since it wasn't scheduled.)

Parag



[-- Attachment #2: patch-ohci1394 --]
[-- Type: text/plain, Size: 5918 bytes --]

--- drivers/ieee1394/ohci1394.c.orig	2004-12-24 16:35:25.000000000 -0500
+++ drivers/ieee1394/ohci1394.c	2005-01-30 20:00:42.000000000 -0500
@@ -99,6 +99,7 @@
 #include <asm/uaccess.h>
 #include <linux/delay.h>
 #include <linux/spinlock.h>
+#include <linux/workqueue.h>
 
 #include <asm/pgtable.h>
 #include <asm/page.h>
@@ -161,6 +162,10 @@
 #define PRINT(level, fmt, args...) \
 printk(level "%s: fw-host%d: " fmt "\n" , OHCI1394_DRIVER_NAME, ohci->host->id , ## args)
 
+/* Instruct free_dma_rcv_ctx how to free dma pools */
+#define OHCI_FREE_DMA_CTX_DIRECT 0
+#define OHCI_FREE_DMA_CTX_DELAYED 1
+
 static char version[] __devinitdata =
 	"$Rev: 1223 $ Ben Collins <bcollins@debian.org>";
 
@@ -176,7 +181,8 @@
 			     enum context_type type, int ctx, int num_desc,
 			     int buf_size, int split_buf_size, int context_base);
 static void stop_dma_rcv_ctx(struct dma_rcv_ctx *d);
-static void free_dma_rcv_ctx(struct dma_rcv_ctx *d);
+static void free_dma_rcv_ctx(struct dma_rcv_ctx *d , int method,
+			     struct work_struct* work);
 
 static int alloc_dma_trm_ctx(struct ti_ohci *ohci, struct dma_trm_ctx *d,
 			     enum context_type type, int ctx, int num_desc,
@@ -184,6 +190,8 @@
 
 static void ohci1394_pci_remove(struct pci_dev *pdev);
 
+static void ohci_free_irlc_pci_pool(void* data);
+
 #ifndef __LITTLE_ENDIAN
 static unsigned hdr_sizes[] =
 {
@@ -1117,7 +1125,8 @@
 
 		if (ohci->ir_legacy_channels == 0) {
 			stop_dma_rcv_ctx(&ohci->ir_legacy_context);
-			free_dma_rcv_ctx(&ohci->ir_legacy_context);
+			INIT_WORK(ohci->irlc_free_dma, ohci_free_irlc_pci_pool, ohci->ir_legacy_context.prg_pool);
+			free_dma_rcv_ctx(&ohci->ir_legacy_context, OHCI_FREE_DMA_CTX_DELAYED, ohci->irlc_free_dma);
 			DBGMSG("ISO receive legacy context deactivated");
 		}
                 break;
@@ -2876,7 +2885,8 @@
 }
 
 
-static void free_dma_rcv_ctx(struct dma_rcv_ctx *d)
+static void free_dma_rcv_ctx(struct dma_rcv_ctx *d, int method,
+		struct work_struct* work)
 {
 	int i;
 	struct ti_ohci *ohci = d->ohci;
@@ -2903,8 +2913,20 @@
 				pci_pool_free(d->prg_pool, d->prg_cpu[i], d->prg_bus[i]);
 				OHCI_DMA_FREE("consistent dma_rcv prg[%d]", i);
 			}
-		pci_pool_destroy(d->prg_pool);
-		OHCI_DMA_FREE("dma_rcv prg pool");
+		if(method == OHCI_FREE_DMA_CTX_DIRECT)
+		{
+			pci_pool_destroy(d->prg_pool);
+			OHCI_DMA_FREE("dma_rcv prg pool");
+		}
+		
+		else if(method == OHCI_FREE_DMA_CTX_DELAYED)
+		{
+			schedule_work(work);
+		}
+
+		else
+			PRINT(KERN_ERR, "Invalid method passed - %d", method);
+
 		kfree(d->prg_cpu);
 		kfree(d->prg_bus);
 	}
@@ -2938,7 +2960,7 @@
 
 	if (d->buf_cpu == NULL || d->buf_bus == NULL) {
 		PRINT(KERN_ERR, "Failed to allocate dma buffer");
-		free_dma_rcv_ctx(d);
+		free_dma_rcv_ctx(d, OHCI_FREE_DMA_CTX_DIRECT, NULL);
 		return -ENOMEM;
 	}
 	memset(d->buf_cpu, 0, d->num_desc * sizeof(quadlet_t*));
@@ -2950,7 +2972,7 @@
 
 	if (d->prg_cpu == NULL || d->prg_bus == NULL) {
 		PRINT(KERN_ERR, "Failed to allocate dma prg");
-		free_dma_rcv_ctx(d);
+		free_dma_rcv_ctx(d, OHCI_FREE_DMA_CTX_DIRECT, NULL);
 		return -ENOMEM;
 	}
 	memset(d->prg_cpu, 0, d->num_desc * sizeof(struct dma_cmd*));
@@ -2960,7 +2982,7 @@
 
 	if (d->spb == NULL) {
 		PRINT(KERN_ERR, "Failed to allocate split buffer");
-		free_dma_rcv_ctx(d);
+		free_dma_rcv_ctx(d, OHCI_FREE_DMA_CTX_DIRECT, NULL);
 		return -ENOMEM;
 	}
 
@@ -2979,7 +3001,7 @@
 		} else {
 			PRINT(KERN_ERR,
 			      "Failed to allocate dma buffer");
-			free_dma_rcv_ctx(d);
+			free_dma_rcv_ctx(d, OHCI_FREE_DMA_CTX_DIRECT, NULL);
 			return -ENOMEM;
 		}
 
@@ -2991,7 +3013,7 @@
 		} else {
 			PRINT(KERN_ERR,
 			      "Failed to allocate dma prg");
-			free_dma_rcv_ctx(d);
+			free_dma_rcv_ctx(d, OHCI_FREE_DMA_CTX_DIRECT, NULL);
 			return -ENOMEM;
 		}
 	}
@@ -3005,7 +3027,7 @@
 		if (ohci1394_register_iso_tasklet(ohci,
 						  &ohci->ir_legacy_tasklet) < 0) {
 			PRINT(KERN_ERR, "No IR DMA context available");
-			free_dma_rcv_ctx(d);
+			free_dma_rcv_ctx(d, OHCI_FREE_DMA_CTX_DIRECT, NULL);
 			return -EBUSY;
 		}
 
@@ -3355,6 +3377,7 @@
 
 	/* the IR DMA context is allocated on-demand; mark it inactive */
 	ohci->ir_legacy_context.ohci = NULL;
+	ohci->ir_legacy_context.prg_pool = NULL;
 
 	/* same for the IT DMA context */
 	ohci->it_legacy_context.ohci = NULL;
@@ -3377,16 +3400,32 @@
 	if (hpsb_add_host(host))
 		FAIL(-ENOMEM, "Failed to register host with highlevel");
 
+	ohci->irlc_free_dma = kmalloc(sizeof(struct work_struct), GFP_KERNEL);
+	if(ohci->irlc_free_dma == NULL)
+		FAIL(-ENOMEM, "Failed to allocate memory for ohci->irlc_free_dma");
+
 	ohci->init_state = OHCI_INIT_DONE;
 
 	return 0;
 #undef FAIL
 }
 
+static void ohci_free_irlc_pci_pool(void *data)
+{
+	if(data == NULL)
+		return;
+	pci_pool_destroy(data);
+	OHCI_DMA_FREE("dma_rcv prg pool");
+	return;
+}
+	
 static void ohci1394_pci_remove(struct pci_dev *pdev)
 {
 	struct ti_ohci *ohci;
 	struct device *dev;
+	
+	/* Ensure all dma ctx are freed */
+	flush_scheduled_work();
 
 	ohci = pci_get_drvdata(pdev);
 	if (!ohci)
@@ -3432,18 +3471,21 @@
 		/* The ohci_soft_reset() stops all DMA contexts, so we
 		 * dont need to do this.  */
 		/* Free AR dma */
-		free_dma_rcv_ctx(&ohci->ar_req_context);
-		free_dma_rcv_ctx(&ohci->ar_resp_context);
+		free_dma_rcv_ctx(&ohci->ar_req_context, OHCI_FREE_DMA_CTX_DIRECT, NULL);
+		free_dma_rcv_ctx(&ohci->ar_resp_context, OHCI_FREE_DMA_CTX_DIRECT, NULL);
 
 		/* Free AT dma */
 		free_dma_trm_ctx(&ohci->at_req_context);
 		free_dma_trm_ctx(&ohci->at_resp_context);
 
 		/* Free IR dma */
-		free_dma_rcv_ctx(&ohci->ir_legacy_context);
+		free_dma_rcv_ctx(&ohci->ir_legacy_context, OHCI_FREE_DMA_CTX_DIRECT, NULL);
 
 		/* Free IT dma */
 		free_dma_trm_ctx(&ohci->it_legacy_context);
+		
+		/* Free work struct for IR Legacy DMA */
+		kfree(ohci->irlc_free_dma);
 
 	case OHCI_INIT_HAVE_SELFID_BUFFER:
 		pci_free_consistent(ohci->dev, OHCI1394_SI_DMA_BUF_SIZE,