Sorry, the last email was a reply to Christoph (thank you all for reviewing)

On 2017-03-27 01:04 PM, Helen Koike wrote:

Hi Keith,

Thanks for your review. Please, see my comments below

On 2017-03-27 06:49 AM, Christoph Hellwig wrote:

quoted

quoted

+#define SQ_IDX(qid, stride)    ((qid) * 2 * (stride))
+#define CQ_IDX(qid, stride)    (((qid) * 2 + 1) * (stride))

Please use inline functions for these.

quoted

+    struct {
+        u32 *dbs;
+        u32 *eis;
+        dma_addr_t dbs_dma_addr;
+        dma_addr_t eis_dma_addr;
+    } dbbuf;

No need for a struct here, also please keep the field and its dma_addr_t
together.

quoted

+    struct {
+        u32 *sq_db;
+        u32 *cq_db;
+        u32 *sq_ei;
+        u32 *cq_ei;
+    } dbbuf;

No need for the struct here either.

quoted

+
+static int nvme_dbbuf_dma_alloc(struct nvme_dev *dev)
+{
+    unsigned int mem_size = nvme_dbbuf_size(dev->db_stride);
+
+    dev->dbbuf.dbs = dma_alloc_coherent(dev->dev, mem_size,
+                        &dev->dbbuf.dbs_dma_addr,
+                        GFP_KERNEL);
+    if (!dev->dbbuf.dbs)
+        return -ENOMEM;
+    dev->dbbuf.eis = dma_alloc_coherent(dev->dev, mem_size,
+                        &dev->dbbuf.eis_dma_addr,
+                        GFP_KERNEL);
+    if (!dev->dbbuf.eis) {
+        dma_free_coherent(dev->dev, mem_size,
+                  dev->dbbuf.dbs, dev->dbbuf.dbs_dma_addr);
+        dev->dbbuf.dbs = NULL;
+        return -ENOMEM;
+    }
+
+    return 0;

Please use normal kernel-style goto unwinding.

I don't think it is necessary as there is only a single item to unwind.
From my experience in other parts of the kernel code we usually use goto
when there are several other steps to unwind. But I don't mind changing
it if you still prefer a goto here.

quoted

quoted

+static void nvme_dbbuf_set(struct nvme_dev *dev)
+{
+    struct nvme_command c;
+
+    if (!dev->dbbuf.dbs)
+        return;
+
+    memset(&c, 0, sizeof(c));
+    c.dbbuf.opcode = nvme_admin_dbbuf;
+    c.dbbuf.prp1 = cpu_to_le64(dev->dbbuf.dbs_dma_addr);
+    c.dbbuf.prp2 = cpu_to_le64(dev->dbbuf.eis_dma_addr);
+
+    if (nvme_submit_sync_cmd(dev->ctrl.admin_q, &c, NULL, 0))
+        /* Free memory and continue on */
+        nvme_dbbuf_dma_free(dev);

At least log a warning.

quoted

+static inline int nvme_dbbuf_need_event(u16 event_idx, u16 new_idx,
u16 old)
+{
+    /* Borrowed from vring_need_event */

I don't think this comment matters.

quoted

+static void nvme_write_doorbell(u16 value,
+                u32 __iomem *db,
+                u32 *dbbuf_db,
+                volatile u32 *dbbuf_ei)
+{

Very odd formatting.  Why not:

static void nvme_write_doorbell(u16 value, u32 __iomem *db, u32
*dbbuf_db,
        volatile u32 *dbbuf_ei)

?

quoted

+    u16 old_value;
+
+    if (!dbbuf_db) {
+        writel(value, db);
+        return;
+    }

I'd prefer to keep this in the ultimate callers to make the flow
easier to read.

quoted

+static inline void nvme_write_doorbell_cq(struct nvme_queue *nvmeq,
u16 value)
+{
+    nvme_write_doorbell(value, nvmeq->q_db + nvmeq->dev->db_stride,
+                nvmeq->dbbuf.cq_db, nvmeq->dbbuf.cq_ei);
+}
+
+static inline void nvme_write_doorbell_sq(struct nvme_queue *nvmeq,
u16 value)
+{
+    nvme_write_doorbell(value, nvmeq->q_db,
+                nvmeq->dbbuf.sq_db, nvmeq->dbbuf.sq_ei);
 }

I'd skip these wrappers entirely.

I added them to avoid future mistakes as mixing nvmeq->dbbuf.sq_db with
nvmeq->dbbuf.sq_ei. But I don't mind to remove them either.

quoted

quoted

+    if (dev->ctrl.oacs & NVME_CTRL_OACS_DBBUF_SUPP) {
+        result = nvme_dbbuf_dma_alloc(dev);
+        if (result)
+            goto out;
+    }

Should we really fail the init here or just print a warning?