On Tue, Feb 04, 2014 at 12:58:25PM +0100, Lars-Peter Clausen wrote:

On 02/04/2014 12:04 AM, Andy Gross wrote:
[...]

quoted

+static int bam_dma_remove(struct platform_device *pdev)
+{
+	struct bam_device *bdev = platform_get_drvdata(pdev);
+	u32 i;
+
+	dma_async_device_unregister(&bdev->common);
+	of_dma_controller_free(pdev->dev.of_node);

The controller should first be removed from the of lookup table,
then free the device.

Ah right, had this reversed.

quoted

+
+	/* mask all interrupts for this execution environment */
+	writel_relaxed(0, bdev->regs + BAM_IRQ_SRCS_MSK_EE(bdev->ee));

You still need to free the interrupt to make this race free,
especially on a multi-processor system. free_irq() acts as a
synchronization point that makes sure that interrupt handler has
finished running and that no new interrupt handlers are being run
after this point. Just masking the interrupt in the control register
does not provide these guarantees.

ok i'll just add in a devm_free_irq().  The only good thing then about using the
devm_request_irq is the cleanup on error paths in the probe.

quoted

+
+	for (i = 0; i < bdev->num_channels; i++) {
+		bam_dma_terminate_all(&bdev->channels[i]);
+		tasklet_kill(&bdev->channels[i].vc.task);
+	}
+
+	tasklet_kill(&bdev->task);
+
+	clk_disable_unprepare(bdev->bamclk);
+
+	return 0;
+}

[...]

-- 
sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation