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
<...>
> > +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).
Re: [PATCH v5 08/16] IB/pvrdma: Add device command support
Minor question/suggestion inline.
(sorry for missing it till now).
Yuval
On Sat, Sep 24, 2016 at 04:21:32PM -0700, Adit Ranadive wrote:
> This patch enables posting Verb requests and receiving responses to/from
> the backend PVRDMA emulation layer.
>
> Reviewed-by: Yuval Shaia
> Reviewed-by: Jorgen Hansen
> Reviewed-by: George Zhang
> Reviewed-by: Aditya Sarwade
> Reviewed-by: Bryan Tan
> Signed-off-by: Adit Ranadive
> ---
> Changes v4->v5:
> - Moved the timeout to pvrdma_cmd_recv.
> - Added additional response code parameter to pvrdma_cmd_post.
>
> Changes v3->v4:
> - Removed the min check and added a BUILD_BUG_ON check for size.
>
> Changes v2->v3:
> - Converted pvrdma_cmd_recv to inline.
> - Added a min check in the memcpy to cmd_slot.
> - Removed the boolean from pvrdma_cmd_post.
> ---
> drivers/infiniband/hw/pvrdma/pvrdma_cmd.c | 117
> ++
> 1 file changed, 117 insertions(+)
> create mode 100644 drivers/infiniband/hw/pvrdma/pvrdma_cmd.c
>
> diff --git a/drivers/infiniband/hw/pvrdma/pvrdma_cmd.c
> b/drivers/infiniband/hw/pvrdma/pvrdma_cmd.c
> new file mode 100644
> index 000..21f1af8
> --- /dev/null
> +++ b/drivers/infiniband/hw/pvrdma/pvrdma_cmd.c
> @@ -0,0 +1,117 @@
> +/*
> + * Copyright (c) 2012-2016 VMware, Inc. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of EITHER the GNU General Public License
> + * version 2 as published by the Free Software Foundation or the BSD
> + * 2-Clause License. This program is distributed in the hope that it
> + * will be useful, but WITHOUT ANY WARRANTY; WITHOUT EVEN THE IMPLIED
> + * WARRANTY OF MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE.
> + * See the GNU General Public License version 2 for more details at
> + * http://www.gnu.org/licenses/old-licenses/gpl-2.0.en.html.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program available in the file COPYING in the main
> + * directory of this source tree.
> + *
> + * The BSD 2-Clause License
> + *
> + * Redistribution and use in source and binary forms, with or
> + * without modification, are permitted provided that the following
> + * conditions are met:
> + *
> + * - Redistributions of source code must retain the above
> + *copyright notice, this list of conditions and the following
> + *disclaimer.
> + *
> + * - Redistributions in binary form must reproduce the above
> + *copyright notice, this list of conditions and the following
> + *disclaimer in the documentation and/or other materials
> + *provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
> + * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
> + * COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
> + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
> + * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
> + * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED
> + * OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include
> +
> +#include "pvrdma.h"
> +
> +#define PVRDMA_CMD_TIMEOUT 1 /* ms */
> +
> +static inline int pvrdma_cmd_recv(struct pvrdma_dev *dev,
> + union pvrdma_cmd_resp *resp,
> + unsigned resp_code)
> +{
> + int err;
> +
> + dev_dbg(&dev->pdev->dev, "receive response from device\n");
> +
> + err = wait_for_completion_interruptible_timeout(&dev->cmd_done,
> + msecs_to_jiffies(PVRDMA_CMD_TIMEOUT));
> + if (err == 0 || err == -ERESTARTSYS) {
> + dev_warn(&dev->pdev->dev,
> + "completion timeout or interrupted\n");
> + return -ETIMEDOUT;
> + }
> +
> + spin_lock(&dev->cmd_lock);
> + memcpy(resp, dev->resp_slot, sizeof(*resp));
> + spin_unlock(&dev->cmd_lock);
> +
> + if (resp->hdr.ack != resp_code) {
> + dev_warn(&dev->pdev->dev,
> + "unknown response %#x expected %#x\n",
> + resp->hdr.ack, resp_code);
> + return -EFAULT;
> + }
> +
> + return 0;
> +}
> +
> +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(sizeo
[PATCH v5 08/16] IB/pvrdma: Add device command support
This patch enables posting Verb requests and receiving responses to/from
the backend PVRDMA emulation layer.
Reviewed-by: Yuval Shaia
Reviewed-by: Jorgen Hansen
Reviewed-by: George Zhang
Reviewed-by: Aditya Sarwade
Reviewed-by: Bryan Tan
Signed-off-by: Adit Ranadive
---
Changes v4->v5:
- Moved the timeout to pvrdma_cmd_recv.
- Added additional response code parameter to pvrdma_cmd_post.
Changes v3->v4:
- Removed the min check and added a BUILD_BUG_ON check for size.
Changes v2->v3:
- Converted pvrdma_cmd_recv to inline.
- Added a min check in the memcpy to cmd_slot.
- Removed the boolean from pvrdma_cmd_post.
---
drivers/infiniband/hw/pvrdma/pvrdma_cmd.c | 117 ++
1 file changed, 117 insertions(+)
create mode 100644 drivers/infiniband/hw/pvrdma/pvrdma_cmd.c
diff --git a/drivers/infiniband/hw/pvrdma/pvrdma_cmd.c
b/drivers/infiniband/hw/pvrdma/pvrdma_cmd.c
new file mode 100644
index 000..21f1af8
--- /dev/null
+++ b/drivers/infiniband/hw/pvrdma/pvrdma_cmd.c
@@ -0,0 +1,117 @@
+/*
+ * Copyright (c) 2012-2016 VMware, Inc. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of EITHER the GNU General Public License
+ * version 2 as published by the Free Software Foundation or the BSD
+ * 2-Clause License. This program is distributed in the hope that it
+ * will be useful, but WITHOUT ANY WARRANTY; WITHOUT EVEN THE IMPLIED
+ * WARRANTY OF MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE.
+ * See the GNU General Public License version 2 for more details at
+ * http://www.gnu.org/licenses/old-licenses/gpl-2.0.en.html.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program available in the file COPYING in the main
+ * directory of this source tree.
+ *
+ * The BSD 2-Clause License
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * - Redistributions of source code must retain the above
+ *copyright notice, this list of conditions and the following
+ *disclaimer.
+ *
+ * - Redistributions in binary form must reproduce the above
+ *copyright notice, this list of conditions and the following
+ *disclaimer in the documentation and/or other materials
+ *provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
+ * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
+ * COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+ * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
+ * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+ * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED
+ * OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include
+
+#include "pvrdma.h"
+
+#define PVRDMA_CMD_TIMEOUT 1 /* ms */
+
+static inline int pvrdma_cmd_recv(struct pvrdma_dev *dev,
+ union pvrdma_cmd_resp *resp,
+ unsigned resp_code)
+{
+ int err;
+
+ dev_dbg(&dev->pdev->dev, "receive response from device\n");
+
+ err = wait_for_completion_interruptible_timeout(&dev->cmd_done,
+ msecs_to_jiffies(PVRDMA_CMD_TIMEOUT));
+ if (err == 0 || err == -ERESTARTSYS) {
+ dev_warn(&dev->pdev->dev,
+"completion timeout or interrupted\n");
+ return -ETIMEDOUT;
+ }
+
+ spin_lock(&dev->cmd_lock);
+ memcpy(resp, dev->resp_slot, sizeof(*resp));
+ spin_unlock(&dev->cmd_lock);
+
+ if (resp->hdr.ack != resp_code) {
+ dev_warn(&dev->pdev->dev,
+"unknown response %#x expected %#x\n",
+resp->hdr.ack, resp_code);
+ return -EFAULT;
+ }
+
+ return 0;
+}
+
+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 req
