ohci1394: dma_pool_destroy while in_atomic() && irqs_disabled()

PATCH In review Parag Warudkar 4 participants 13 messages Jan 30, 2005 – Feb 19, 2005

Share & tools

Parag Warudkar

author

2005-01-30 20:54 UTC #0 lore raw

PATCH 1/1 ohci1394: dma_pool_destroy while in_atomic() && irqs_disabled()

Problem - ohci1394.c:ohci_devctl ends up calling dma_pool_destroy from invalid context. Below is the dmesg output when I exit Kino after video capture -

Debug: sleeping function called from invalid context at include/asm/semaphore.h:107 in_atomic():1, irqs_disabled():1

[<c0104c2e>] dump_stack+0x1e/0x20
[<c011f8a2>] __might_sleep+0xa2/0xc0
[<c028c660>] dma_pool_destroy+0x20/0x140
[<f0b7affe>] free_dma_rcv_ctx+0x8e/0x150 [ohci1394]
[<f0b774a4>] ohci_devctl+0x214/0x9b0 [ohci1394]
[<f0e7aa99>] handle_iso_listen+0x2d9/0x310 [raw1394]
[<f0e7f22b>] state_connected+0x29b/0x2b0 [raw1394]
[<f0e7f2de>] raw1394_write+0x9e/0xd0 [raw1394]
[<c0183ef2>] vfs_write+0xc2/0x170
[<c018406b>] sys_write+0x4b/0x80
[<c0103cb1>] sysenter_past_esp+0x52/0x75

Attached patch against 2.6.11-rc2 (tested to work normally with a camcorder device) enables ohci_devctl to defer the work of destroying the dma pool by using a work queue, so the dma pool is destroyed in a valid context and we no longer get an error in dmesg (duh :)

Note : There still exists one more similar problem with ohci1394 - that is with dma_pool_create being called from invalid context - this happens in response to ISO_LISTEN_CHANNEL. I get dmesg debug for this case which is similar to the above. My analysis is that this one is non-trivial to fix. More on this in a separate mail.

Patch is attached since T'Bird messes up with the inlined one. (Pls. suggest a good email client for kernel patch stuff :)

Signed-off-by Parag Warudkar ([email protected])

Parag

ieee1394/ohci1394.c renamed from ieee1394/ohci1394.c.orig +14 -2

@@ -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>

@@ -164,6 +165,7 @@
 static char version[] __devinitdata =
 	"$Rev: 1223 $ Ben Collins <[email protected]>";
 
+
 /* Module Parameters */
 static int phys_dma = 1;
 module_param(phys_dma, int, 0644);

@@ -184,6 +186,8 @@
 
 static void ohci1394_pci_remove(struct pci_dev *pdev);
 
+static void ohci_free_dma_work_fn(void* data);
+
 #ifndef __LITTLE_ENDIAN
 static unsigned hdr_sizes[] =
 {

@@ -1130,6 +1134,13 @@
 	return retval;
 }
 
+static void ohci_free_dma_work_fn(void* data)
+{
+	struct pci_pool* prg_pool = (struct pci_pool*) data;
+	pci_pool_destroy(prg_pool);
+	OHCI_DMA_FREE("dma_rcv prg pool");
+}
+
 /***********************************
  * rawiso ISO reception            *
  ***********************************/

@@ -2898,13 +2909,14 @@
 		kfree(d->buf_bus);
 	}
 	if (d->prg_cpu) {
+		struct work_struct ohci_free_dma_work;
 		for (i=0; i<d->num_desc; i++)
 			if (d->prg_cpu[i] && d->prg_bus[i]) {
 				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");
+		INIT_WORK(&ohci_free_dma_work, ohci_free_dma_work_fn, (void*) d->prg_pool);
+		schedule_work(&ohci_free_dma_work);
 		kfree(d->prg_cpu);
 		kfree(d->prg_bus);
 	}

Andrew Morton

2005-01-30 21:17 UTC #1 lore raw

Quoted 8 lines · Parag Warudkar

Problem - ohci1394.c:ohci_devctl ends up calling dma_pool_destroy from invalid context. Below is the dmesg output when I exit Kino after video capture -
Debug: sleeping function called from invalid context at include/asm/semaphore.h:107
in_atomic():1, irqs_disabled():1

Quoted 11 lines

 [<c0104c2e>] dump_stack+0x1e/0x20
 [<c011f8a2>] __might_sleep+0xa2/0xc0
 [<c028c660>] dma_pool_destroy+0x20/0x140
 [<f0b7affe>] free_dma_rcv_ctx+0x8e/0x150 [ohci1394]
 [<f0b774a4>] ohci_devctl+0x214/0x9b0 [ohci1394]
 [<f0e7aa99>] handle_iso_listen+0x2d9/0x310 [raw1394]
 [<f0e7f22b>] state_connected+0x29b/0x2b0 [raw1394]
 [<f0e7f2de>] raw1394_write+0x9e/0xd0 [raw1394]
 [<c0183ef2>] vfs_write+0xc2/0x170
 [<c018406b>] sys_write+0x4b/0x80
 [<c0103cb1>] sysenter_past_esp+0x52/0x75

Yes, that's certainly wrong.

Quoted 4 lines

Attached patch against 2.6.11-rc2 (tested to work normally with a camcorder device) enables ohci_devctl to defer the work of destroying the dma pool by using a work queue, so the dma pool is destroyed in a valid context and we no longer get an error in dmesg (duh :)

yup. But what happens if someone removes the module while ohci_free_dma_work_fn() is still pending?

Suggestions:

- The work_struct cannot be on the stack. The code as you have it will read gunk from the stack when the delayed work executes. The work_struct needs to be placed into some ohci data structure which has the appropriate lifetime. That might be struct ti_ohci. Or not.

- We'll need a flush_workqueue() in the teardown function for that data structure to ensure that any pending callbacks have completed before we free the storage.

Care needs to be taken to ensure that the work_struct is suitably initialised so that the flush_workqueue() will work OK even if the callback has never been scheduled.

- You have several typecasts between struct pci_pool* and void*. These defeat typechecking. It's better to leave these casts out.

Parag Warudkar

author

2005-01-30 22:49 UTC #2 lore raw

Quoted 11 lines · Andrew Morton

yup. But what happens if someone removes the module while ohci_free_dma_work_fn() is still pending?
Suggestions:
- The work_struct cannot be on the stack. The code as you have it will read gunk from the stack when the delayed work executes. The work_struct needs to be placed into some ohci data structure which has the appropriate lifetime. That might be struct ti_ohci. Or not.

Ok, got it.

Quoted 5 lines

- We'll need a flush_workqueue() in the teardown function for that data structure to ensure that any pending callbacks have completed before we free the storage.

By saying flush_workqueue did you intend to suggest using separate work queue for ohci1394? I think that might be the way to go since otherwise ohci1394 would have to wait on all other irrelevant work elements in the global queue to be finished. This will help in solving the other problem of dma_pool_create as well - we could then schedule_work for all create_pools and just do a flush_workqueue before starting to actually use the device. (Might help in reducing those sound skips when I attach my camcorder :) Error handling has to change a good amount (right now the init function returns ENOMEM if dma_pool_create fails and all is done. With this approach of asynch alloc , init function will not know if the allocation failed / succeeded. All other functions (like say resulting from sys_read) will have to explicitly check and return error to user if the pool create had failed.)

Quoted 5 lines

Care needs to be taken to ensure that the work_struct is suitably initialised so that the flush_workqueue() will work OK even if the callback has never been scheduled.

Didn't understand this one - Is this about properly NULL'ing elements in work queue so flush_workqueue doesn't touch them? Can you elaborate please?

Quoted 4 lines

- You have several typecasts between struct pci_pool* and void*. These defeat typechecking. It's better to leave these casts out.

Will leave them out.

Thanks

Parag

Andrew Morton

2005-01-30 23:02 UTC #3 lore raw

Quoted 11 lines · Parag Warudkar

Andrew Morton wrote:
...
Quoted 5 lines
- We'll need a flush_workqueue() in the teardown function for that data structure to ensure that any pending callbacks have completed before we free the storage.
By saying flush_workqueue did you intend to suggest using separate work queue for ohci1394?

No. Using keventd and flush_scheduled_work() should be OK in this application. rmmod isn't very common.

Quoted 8 lines

Quoted 5 lines
Care needs to be taken to ensure that the work_struct is suitably initialised so that the flush_workqueue() will work OK even if the callback has never been scheduled.
Didn't understand this one - Is this about properly NULL'ing elements in work queue so flush_workqueue doesn't touch them? Can you elaborate please?

Well, it's just that the shutdown code needs to run flush_scheduled_work() or flush_workqueue() on a work_struct which may or may not have been used. So we need to ensure that it has sane contents. Just run INIT_WORK() on it in the setup code.

Parag Warudkar

author

2005-01-31 01:19 UTC #4 lore raw

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

ieee1394/ohci1394.c renamed from ieee1394/ohci1394.c.orig +56 ~~-14~~

@@ -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 <[email protected]>";

@@ -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,

Parag Warudkar

author

2005-01-31 23:31 UTC #5 lore raw

Forgot to attach ohci.h diff which is affected by this change as well.

Meanwhile, David Brownell suggested it would be much simpler to move the dma allocations to _probe and deallocations to _remove. I am working on it and so far haven't got it to work.

Andrew - What do you think? If you too feel allocating pci_pool for the legacy contexts (only disadvantage I see is that the pool will be allocated even if it is not used, that is if ISO_LISTEN_CHANNEL isn't called any time) in _probe an removing it in _remove is a better thing to do, I will concentrate on getting that to work and we can forget about this patch. I feel there might be some reason why the current code treats the legacy IR and IT DMA ctxs differently (on-demand allocation).. Ben?

Parag

Quoted 208 lines · Parag Warudkar

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

Parag

------------------------------------------------------------------------

ieee1394/ohci1394.c renamed from ieee1394/ohci1394.c.orig +5 -0

@@ -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 <[email protected]>";

ieee1394/ohci1394.c renamed from ieee1394/ohci1394.c.orig +4 -1

@@ -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[] =
{

ieee1394/ohci1394.c renamed from ieee1394/ohci1394.c.orig +43 ~~-13~~

@@ -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);

ieee1394/ohci1394.c line 3491

Quoted 10 lines

ieee1394/ohci1394.c renamed from ieee1394/ohci1394.c.orig +4 -0

+		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,

ieee1394/ohci1394.h renamed from ieee1394/ohci1394.h.orig +2 -0

@@ -197,6 +197,8 @@
 				   it is safe to free the legacy API context */
 
 	struct dma_rcv_ctx ir_legacy_context;
+	struct work_struct* irlc_free_dma;
+	
 	struct ohci1394_iso_tasklet ir_legacy_tasklet;
 
         /* iso transmit */

Dan Dennedy

2005-02-11 15:43 UTC #6 lore raw

In reply to Parag Warudkar

Quoted 15 lines · Parag Warudkar

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.

Quoted 6 lines

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.)

I am testing this patch in the same manner as you: exiting Kino capture. I am getting a similar error in a different location. Can you look into it, please?

Debug: sleeping function called from invalid context at mm/slab.c:2082 in_atomic():1, irqs_disabled():1

 [<c0119eb1>] __might_sleep+0xa1/0xc0
 [<c0144914>] kmem_cache_alloc+0x64/0x80
 [<c037f07b>] dma_pool_create+0x7b/0x190
 [<e09ede32>] alloc_dma_rcv_ctx+0x1a2/0x400 [ohci1394]
 [<e09eb239>] ohci_devctl+0x3d9/0x640 [ohci1394]
 [<e0bc5d4e>] handle_iso_listen+0xee/0x160 [raw1394]
 [<e0bc878e>] state_connected+0x2de/0x2f0 [raw1394]
 [<e0bc884e>] raw1394_write+0xae/0xe0 [raw1394]
 [<c015c80c>] vfs_write+0x14c/0x160
 [<c015c8f1>] sys_write+0x51/0x80
 [<c0102a39>] sysenter_past_esp+0x52/0x75

Jody McIntyre

2005-02-11 18:44 UTC #7 lore raw

Quoted 6 lines · Dan Dennedy

I am testing this patch in the same manner as you: exiting Kino capture. I am getting a similar error in a different location. Can you look into it, please?
Debug: sleeping function called from invalid context at mm/slab.c:2082 in_atomic():1, irqs_disabled():1

Quoted 11 lines

 [<c0119eb1>] __might_sleep+0xa1/0xc0
 [<c0144914>] kmem_cache_alloc+0x64/0x80
 [<c037f07b>] dma_pool_create+0x7b/0x190
 [<e09ede32>] alloc_dma_rcv_ctx+0x1a2/0x400 [ohci1394]
 [<e09eb239>] ohci_devctl+0x3d9/0x640 [ohci1394]
 [<e0bc5d4e>] handle_iso_listen+0xee/0x160 [raw1394]
 [<e0bc878e>] state_connected+0x2de/0x2f0 [raw1394]
 [<e0bc884e>] raw1394_write+0xae/0xe0 [raw1394]
 [<c015c80c>] vfs_write+0x14c/0x160
 [<c015c8f1>] sys_write+0x51/0x80
 [<c0102a39>] sysenter_past_esp+0x52/0x75

Does this happen on exit or on startup? Looks like allocation problems, which will be harder to fix, since you can't return to userland until the allocation is complete. AFAICT the correct fix is to use finer-grained locks, hold them for less time, and not use _irq or _irqsave unless necessary. host_info_lock, in particular, is held for far too long.

Jody

Quoted 12 lines

-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users. Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click
_______________________________________________
mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/linux1394-devel

Parag Warudkar

author

2005-02-12 03:54 UTC #8 lore raw

Jody,
This happens every time you connect a device which ends up doing ISO_LISTEN_CHANNEL. We fixed the device disconnect case in -mm recently.

I had sent you and Andrew an alternative patch which fixes this dma_pool_create case as well as the dma_pool_destroy case, albeit with a disadvantage - The patch does pre-allocation of the IR Legacy DMA in _pci_probe and deallocates it in _pci_remove. However I am not truly happy with it since it possibly wastes 200K of memory for people who don't have devices which need it.

As I said earlier, I think the way to fix this is via schedule_work similar to the disconnect case, but it involves good amount of code change. I am working on it - any better ideas most welcome.

Dan - Can you try the attached patch - on top current -mm1? (It's pretty no brainer that it will fix both cases but two testing heads are better than one.. :)

Thanks

Parag

Quoted 42 lines · Jody McIntyre

Quoted 6 lines · Dan Dennedy
I am testing this patch in the same manner as you: exiting Kino capture. I am getting a similar error in a different location. Can you look into it, please?
Debug: sleeping function called from invalid context at mm/slab.c:2082 in_atomic():1, irqs_disabled():1
Quoted 11 lines
 [<c0119eb1>] __might_sleep+0xa1/0xc0
 [<c0144914>] kmem_cache_alloc+0x64/0x80
 [<c037f07b>] dma_pool_create+0x7b/0x190
 [<e09ede32>] alloc_dma_rcv_ctx+0x1a2/0x400 [ohci1394]
 [<e09eb239>] ohci_devctl+0x3d9/0x640 [ohci1394]
 [<e0bc5d4e>] handle_iso_listen+0xee/0x160 [raw1394]
 [<e0bc878e>] state_connected+0x2de/0x2f0 [raw1394]
 [<e0bc884e>] raw1394_write+0xae/0xe0 [raw1394]
 [<c015c80c>] vfs_write+0x14c/0x160
 [<c015c8f1>] sys_write+0x51/0x80
 [<c0102a39>] sysenter_past_esp+0x52/0x75
Does this happen on exit or on startup? Looks like allocation problems, which will be harder to fix, since you can't return to userland until the allocation is complete. AFAICT the correct fix is to use finer-grained locks, hold them for less time, and not use _irq or _irqsave unless necessary. host_info_lock, in particular, is held for far too long.
Jody
Quoted 12 lines
-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users. Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click
_______________________________________________
mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/linux1394-devel

Dan Dennedy

2005-02-18 15:35 UTC #9 lore raw

I have tested the patches (including for allocation), and it is working great, but should I only commit for now the deallocation patch? Hmm.. which is worse the debug or the 200K waste?

Quoted 19 lines · Parag Warudkar

Jody,
This happens every time you connect a device which ends up doing ISO_LISTEN_CHANNEL. We fixed the device disconnect case in -mm recently.
I had sent you and Andrew an alternative patch which fixes this dma_pool_create case as well as the dma_pool_destroy case, albeit with a disadvantage - The patch does pre-allocation of the IR Legacy DMA in _pci_probe and deallocates it in _pci_remove. However I am not truly happy with it since it possibly wastes 200K of memory for people who don't have devices which need it.
As I said earlier, I think the way to fix this is via schedule_work similar to the disconnect case, but it involves good amount of code change. I am working on it - any better ideas most welcome.
Dan - Can you try the attached patch - on top current -mm1? (It's pretty no brainer that it will fix both cases but two testing heads are better than one.. :)

Parag Warudkar

author

2005-02-18 15:43 UTC #10 lore raw

Quoted 3 lines · Dan Dennedy

I have tested the patches (including for allocation), and it is working great, but should I only commit for now the deallocation patch? Hmm.. which is worse the debug or the 200K waste?

Thanks for following it up.

IMHO, we should commit both patches for now since we don't have an alternative solution yet.

Jody - Is the 200K waste for sure or do you want me to verify it by some means? ( Reason I am asking is firstly, Dave Brownell was quite sure it wasn't that costly and secondly, I am hoping it isn't.. ;)

Parag

Jody McIntyre

2005-02-19 06:38 UTC #11 lore raw

Quoted 8 lines · Parag Warudkar

Quoted 3 lines · Dan Dennedy
I have tested the patches (including for allocation), and it is working great, but should I only commit for now the deallocation patch? Hmm.. which is worse the debug or the 200K waste?
Thanks for following it up.
IMHO, we should commit both patches for now since we don't have an alternative solution yet.

I disagree because the impact of this bug is small. How often do you start an ISO receive? If you think it needs to be fixed urgently, please explain why - maybe I'm just missing somethnig.

Quoted 3 lines

Jody - Is the 200K waste for sure or do you want me to verify it by some means? ( Reason I am asking is firstly, Dave Brownell was quite sure it wasn't that costly and secondly, I am hoping it isn't.. ;)

I'm not sure, but I looked through the code and it seems to allocate:
- 16 buffers of 2x PAGE_SIZE (= 131072 on i386)
- 16 buffers of PAGE_SIZE (= 65536 on i386)
- various other smaller structures.

I'm not sure how to actually _measure_ how much memory this is using. slabinfo isn't useful, at least on my system, because the 1394 allocations get lost in the noise of other activity.

If you really need this fixed quickly, I'll find some time this weekend to examine the locks. In particular, I'm not sure what host_info_lock protects or why it needs to be held in so many places with irqs disabled.

Jody

Quoted 2 lines

Parag

Parag Warudkar

author

2005-02-19 15:07 UTC #12 lore raw

Quoted 4 lines · Jody McIntyre

I disagree because the impact of this bug is small. How often do you start an ISO receive? If you think it needs to be fixed urgently, please explain why - maybe I'm just missing somethnig.

I have to agree that the impact is small even for the people using ISO recv - I happen to use it quite frequently and it hasn't locked up on me yet. So I certainly don't need it fixed atm. It's just the "dmesg annoyance" if you will, to deal with :) !

Quoted 4 lines

I'm not sure, but I looked through the code and it seems to allocate:
- 16 buffers of 2x PAGE_SIZE (= 131072 on i386)
- 16 buffers of PAGE_SIZE (= 65536 on i386)
- various other smaller structures.

OTOH, if it allocates so much of memory while irqs disabled and holding locks, isn't there a good chance for the allocator to sleep and things to go wrong?

Parag