Re: [PATCH v5 08/16] IB/pvrdma: Add device command support

On Mon, Sep 26, 2016 at 00:16:01AM -0700, Yuval Shaia wrote:

Minor question/suggestion inline.
(sorry for missing it till now).

Yuval

<...>

quoted

+int
+pvrdma_cmd_post(struct pvrdma_dev *dev, union pvrdma_cmd_req *req,
+		union pvrdma_cmd_resp *resp, unsigned resp_code)
+{
+	int err;
+
+	dev_dbg(&dev->pdev->dev, "post request to device\n");
+
+	/* Serializiation */
+	down(&dev->cmd_sema);
+
+	BUILD_BUG_ON(sizeof(union pvrdma_cmd_req) !=
+		     sizeof(struct pvrdma_cmd_modify_qp));
+
+	spin_lock(&dev->cmd_lock);
+	memcpy(dev->cmd_slot, req, sizeof(*req));
+	spin_unlock(&dev->cmd_lock);
+
+	init_completion(&dev->cmd_done);
+	pvrdma_write_reg(dev, PVRDMA_REG_REQUEST, 0);
+
+	/* Make sure the request is written before reading status. */
+	mb();
+
+	err = pvrdma_read_reg(dev, PVRDMA_REG_ERR);
+	if (err == 0) {
+		if (resp != NULL)
+			err = pvrdma_cmd_recv(dev, resp, resp_code);
+	} else {
+		dev_warn(&dev->pdev->dev, "failed to write request %d\n", err);
+	}

Please note that callers checks if err<0  while here err!=0 considered as
error.
Looking at similar code in pvrdma_pci_probe i can see that function
"translates" this error to -EINVAL. Suggesting to do the same here
(although -EINVAL is not a good choice, if device can be more friendly with
the return code then we can propagate it up or just print a suitable error
i.e. EAGAIN or ENOMEM otherwise EFAULT should be sufficient).

Thanks for pointing this out.
I dont think we return a positive error from the error register. So the
(err == 0) should be (err >= 0). Also, I'll set the err to -EFAULT if
read_reg fails (based on your feedback on the other patch).
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html