Re: [PATCH 12/12] drivers: PMC MSP71xx security engine driver

Hi Marc.

On Thu, Jun 28, 2007 at 01:49:13PM -0600, Marc St-Jean (stjeanma@pmc-sierra.com) wrote:

+static int
+sec_init_queues(void)
+{
+	int i;
+	struct workq *wq;
+	struct compq *cq;
+
+	/*
+	 * Allocate uncached space for hw_ptr values.
+	 * NOTE: status ptr value is not currently used.
+	 */
+	status_ptr = dma_alloc_coherent(NULL, sizeof(int), &status_dma_addr,
+					GFP_KERNEL);
+	DBG_SEC("Allocated status ptr memory at 0x%p (0x%08x)\n",
+			status_ptr, status_dma_addr);
+	if (!status_ptr)
+		return -ENOMEM;
+
+	for (i = 0; i < HW_NR_COMP_QUEUES; i++) {
+		void *base; /* slowpath virtual address of base */
+		dma_addr_t base_dma_addr; /* DMA bus address of base */
+
+		base = dma_alloc_coherent(NULL, SEC_COMP_Q_SIZE,
+				&base_dma_addr, GFP_KERNEL);
+		DBG_SEC("Allocated CQ%d at 0x%p (0x%08x)\n",
+			i, base, base_dma_addr);
+		if (!base)
+			return -ENOMEM;

This leaks allocations.

+		cq = &sec_comp_queues[i];
+
+		cq->compq_lock = SPIN_LOCK_UNLOCKED;
+		cq->cq_regs = &sec2_regs->cq[i];
+		cq->base = base;
+		cq->base_dma_addr = base_dma_addr;
+		cq->out = 0;
+
+		cq->cq_regs->ofst_ptr = (unsigned int *)status_dma_addr;
+		cq->cq_regs->base = (unsigned char *)cq->base_dma_addr;
+		cq->cq_regs->size = SEC_COMP_Q_SIZE;
+		cq->cq_regs->in = 0;
+		cq->cq_regs->out = 0;
+	}
+
+	for (i = 0; i < HW_NR_WORK_QUEUES; i++) {
+		void *base; /* slowpath virtual address of base */
+		dma_addr_t base_dma_addr; /* DMA bus address of base */
+
+		base = dma_alloc_coherent(NULL, SEC_WORK_Q_SIZE,
+					&base_dma_addr, GFP_KERNEL);
+		DBG_SEC("Allocated WQ%d at 0x%p (0x%08x)\n",
+			i, base, base_dma_addr);
+		if (!base)
+			return -ENOMEM;

This too.

+		wq = &sec_work_queues[i];
+
+		init_waitqueue_head(&wq->space_wait);
+
+		wq->workq_lock = SPIN_LOCK_UNLOCKED;
+		wq->wq_regs = &sec2_regs->wq[i];
+		wq->base = base;
+		wq->base_dma_addr = base_dma_addr;
+		wq->in = 0;
+		wq->low_water = SEC_WORK_Q_SIZE >> 1; /* wake when half full */
+
+		wq->wq_regs->ofst_ptr = (unsigned int *)status_dma_addr;
+		wq->wq_regs->base = (unsigned char *)wq->base_dma_addr;
+		wq->wq_regs->size = SEC_WORK_Q_SIZE;
+		wq->wq_regs->in = 0;
+		wq->wq_regs->out = 0;
+	}
+	
+	debug_dump_sec_regs();
+
+	return 0;
+}
+
+static int __init
+msp_secv2_init(void)

Shouldn't this and other places be marked as __devinit?

...

+static irqreturn_t
+msp_secv2_interrupt(int irq, void *dev_id)
+{
+	/*
+	 * TODO: This clears all interrupts, and assumes
+	 * that the cause was a completion queue update.
+	 */
+	unsigned int status;
+
+	status = sec2_regs->sis;
+	sec2_regs->sis = /* ~status */ 0;
+
+	DBG_SEC("interrupt irq %d status was %x\n", irq, status);
+
+	poll_completion();
+
+	return IRQ_HANDLED;
+}

Irqs can not be shared?

...

+static int
+poll_completion(void)
+{
+	struct compq *cq;
+	int flags;
+	int work_ct = 0;
+	
+	/*
+	 * Check IPSEC engine register to see if at least one
+	 * completion element is in completion queue.
+	 */
+	cq = sec_comp_queues;
+	spin_lock_irqsave(&cq->compq_lock, flags);

This lock seems not to protect against desc_do_work() for example, but
there are register/mmio access under both - what is a locking rules
there?

-- 
	Evgeniy Polyakov