Re: [RESEND PATCH v7 17/21] IB/hns: Add QP operations support

[Wei Hu (Xavier)]

Hi, Doug Ledford
   In hns_roce_cmd_wait and hns_roce_cmd_poll, there are down 
seminal operations,

   So, there are not deadlock and conflict, right?

static int hns_roce_cmd_poll(struct hns_roce_dev *hr_dev, u64 in_param,
 u64 *out_param, unsigned long in_modifier,
 u8 op_modifier, u16 op, unsigned long timeout)
{
struct device *dev = _dev->pdev->dev;
u8 __iomem *hcr = hr_dev->cmd.hcr;
unsigned long end = 0;
u32 status = 0;
int ret;

down(_dev->cmd.poll_sem);

..// 

up (_dev->cmd.poll_sem);
return ret;
}

static int hns_roce_cmd_wait(struct hns_roce_dev *hr_dev, u64 in_param,
 u64 *out_param, unsigned long in_modifier,
 u8 op_modifier, u16 op, unsigned long timeout)
{
struct hns_roce_cmdq *cmd = _dev->cmd;
struct device *dev = _dev->pdev->dev;
struct hns_roce_cmd_context *context;
int ret = 0;

down(>event_sem);

..// 

up (>event_sem);
return ret;
}

int __hns_roce_cmd(struct hns_roce_dev *hr_dev, u64 in_param, u64 *out_param,
   unsigned long in_modifier, u8 op_modifier, u16 op,
   unsigned long timeout)
{
if (hr_dev->cmd.use_events)
return hns_roce_cmd_wait(hr_dev, in_param, out_param,
 in_modifier, op_modifier, op, timeout);
else
return hns_roce_cmd_poll(hr_dev, in_param, out_param,
 in_modifier, op_modifier, op, timeout);
}



Thanks.

Regards
Wei Hu

On 2016/5/14 5:52, Doug Ledford wrote:

On 05/09/2016 11:04 PM, Lijun Ou wrote:

+int __hns_roce_cmd(struct hns_roce_dev *hr_dev, u64 in_param, u64 *out_param,
+  unsigned long in_modifier, u8 op_modifier, u16 op,
+  unsigned long timeout);
+
+/* Invoke a command with no output parameter */
+static inline int hns_roce_cmd(struct hns_roce_dev *hr_dev, u64 in_param,
+  unsigned long in_modifier, u8 op_modifier,
+  u16 op, unsigned long timeout)
+{
+   return __hns_roce_cmd(hr_dev, in_param, NULL, in_modifier,
+ op_modifier, op, timeout);
+}
+
+/* Invoke a command with an output mailbox */
+static inline int hns_roce_cmd_box(struct hns_roce_dev *hr_dev, u64 in_param,
+  u64 out_param, unsigned long in_modifier,
+  u8 op_modifier, u16 op,
+  unsigned long timeout)
+{
+   return __hns_roce_cmd(hr_dev, in_param, _param, in_modifier,
+ op_modifier, op, timeout);
+}

This will make people scratch their head in the future.  You are using
two commands to map to one command without there being any locking
involved.  The typical convention for routine_1() -> __routine_1() is
that the __ version requires that it be called while locked, and the
version without a __ does the locking before calling it.  That way a
used can always know if they aren't currently holding the appropriate
lock, then they6 call routine_1() and if they are, they call
__routine_1() to avoid a deadlock.  I would suggest changing the name of
__hns_roce_cmd to hns_roce_cmd_box and completely remove the existing
hns_roce_cmd_box inline, and then change the hns_roce_cmd() inline to
directly call hns_roce_cmd_box() which will then select between
event/poll command sends.

Re: [RESEND PATCH v7 17/21] IB/hns: Add QP operations support

[Doug Ledford]

On 05/09/2016 11:04 PM, Lijun Ou wrote:
> +static void hns_roce_v1_cq_clean(struct hns_roce_cq *hr_cq, u32 qpn,
> +  struct hns_roce_srq *srq)
> +{
> + spin_lock_irq(_cq->lock);
> + hns_roce_v1_clean_cq(hr_cq, qpn, srq);
> + spin_unlock_irq(_cq->lock);
> +}

This is a perfect example of what I was talking about in my last email.
The convention here would be to name the main function
__hns_roce_v1_cq_clean and the wrapper hns_roce_v1_cq_clean.  Instead,
you have one named cq_clean and one named clean_cq.  Keeping straight
which of those locks itself and which needs to be called with the lock
held is nigh impossible.

-- 
Doug Ledford 
  GPG KeyID: 0E572FDD




signature.asc
Description: OpenPGP digital signature

Re: [RESEND PATCH v7 17/21] IB/hns: Add QP operations support

[Doug Ledford]

On 05/09/2016 11:04 PM, Lijun Ou wrote:
> +int __hns_roce_cmd(struct hns_roce_dev *hr_dev, u64 in_param, u64 *out_param,
> +unsigned long in_modifier, u8 op_modifier, u16 op,
> +unsigned long timeout);
> +
> +/* Invoke a command with no output parameter */
> +static inline int hns_roce_cmd(struct hns_roce_dev *hr_dev, u64 in_param,
> +unsigned long in_modifier, u8 op_modifier,
> +u16 op, unsigned long timeout)
> +{
> + return __hns_roce_cmd(hr_dev, in_param, NULL, in_modifier,
> +   op_modifier, op, timeout);
> +}
> +
> +/* Invoke a command with an output mailbox */
> +static inline int hns_roce_cmd_box(struct hns_roce_dev *hr_dev, u64 in_param,
> +u64 out_param, unsigned long in_modifier,
> +u8 op_modifier, u16 op,
> +unsigned long timeout)
> +{
> + return __hns_roce_cmd(hr_dev, in_param, _param, in_modifier,
> +   op_modifier, op, timeout);
> +}

This will make people scratch their head in the future.  You are using
two commands to map to one command without there being any locking
involved.  The typical convention for routine_1() -> __routine_1() is
that the __ version requires that it be called while locked, and the
version without a __ does the locking before calling it.  That way a
used can always know if they aren't currently holding the appropriate
lock, then they6 call routine_1() and if they are, they call
__routine_1() to avoid a deadlock.  I would suggest changing the name of
__hns_roce_cmd to hns_roce_cmd_box and completely remove the existing
hns_roce_cmd_box inline, and then change the hns_roce_cmd() inline to
directly call hns_roce_cmd_box() which will then select between
event/poll command sends.

-- 
Doug Ledford 
  GPG KeyID: 0E572FDD




signature.asc
Description: OpenPGP digital signature

[RESEND PATCH v7 17/21] IB/hns: Add QP operations support

[Lijun Ou]

This patch was implementing for queue pair operations. QP Consists
of a Send Work Queue and a Receive Work Queue. Send and receive
queues are always created as a pair and remain that way throughout
their lifetime. A Queue Pair is identified by its Queue Pair Number.
QP operations as follows:
1. create QP. When a QP is created, a complete set of initial
   attributes must be specified by the Consumer.
2. query QP. Returns the attribute list and current values for
   the specified QP.
3. modify QP. modify QP relative attributes by it.
4. destroy QP. When a QP is destroyed, any outstanding Work
   Requests are no longer considered to be in the scope of
   the Channel Interface. It is the responsibility of the
   Consumer to be able to clean up any resources
5. post send request. Builds one or more WQEs for the Send Queue
   in the specified QP.
6. post receive request. Builds one or more WQEs for the receive
   Queue in the specified QP.

Signed-off-by: Wei Hu 
Signed-off-by: Nenglong Zhao 
Signed-off-by: Lijun Ou 
---
 drivers/infiniband/hw/hns/hns_roce_alloc.c  |  134 +++
 drivers/infiniband/hw/hns/hns_roce_cmd.c|  222 +++-
 drivers/infiniband/hw/hns/hns_roce_cmd.h|   54 +-
 drivers/infiniband/hw/hns/hns_roce_common.h |   58 +
 drivers/infiniband/hw/hns/hns_roce_device.h |  167 +++
 drivers/infiniband/hw/hns/hns_roce_hw_v1.c  | 1642 +++
 drivers/infiniband/hw/hns/hns_roce_hw_v1.h  |  626 ++
 drivers/infiniband/hw/hns/hns_roce_icm.c|   56 +
 drivers/infiniband/hw/hns/hns_roce_icm.h|9 +
 drivers/infiniband/hw/hns/hns_roce_main.c   |   14 +-
 drivers/infiniband/hw/hns/hns_roce_mr.c |  160 +++
 drivers/infiniband/hw/hns/hns_roce_qp.c |  765 +
 drivers/infiniband/hw/hns/hns_roce_user.h   |6 +
 13 files changed, 3903 insertions(+), 10 deletions(-)

diff --git a/drivers/infiniband/hw/hns/hns_roce_alloc.c 
b/drivers/infiniband/hw/hns/hns_roce_alloc.c
index 972d2ac..7357a4d 100644
--- a/drivers/infiniband/hw/hns/hns_roce_alloc.c
+++ b/drivers/infiniband/hw/hns/hns_roce_alloc.c
@@ -47,6 +47,45 @@ void hns_roce_bitmap_free(struct hns_roce_bitmap *bitmap, 
unsigned long obj)
hns_roce_bitmap_free_range(bitmap, obj, 1);
 }
 
+int hns_roce_bitmap_alloc_range(struct hns_roce_bitmap *bitmap, int cnt,
+   int align, unsigned long *obj)
+{
+   int ret = 0;
+   int i;
+
+   if (likely(cnt == 1 && align == 1))
+   return hns_roce_bitmap_alloc(bitmap, obj);
+
+   spin_lock(>lock);
+
+   *obj = bitmap_find_next_zero_area(bitmap->table, bitmap->max,
+ bitmap->last, cnt, align - 1);
+   if (*obj >= bitmap->max) {
+   bitmap->top = (bitmap->top + bitmap->max + bitmap->reserved_top)
+  & bitmap->mask;
+   *obj = bitmap_find_next_zero_area(bitmap->table, bitmap->max, 0,
+ cnt, align - 1);
+   }
+
+   if (*obj < bitmap->max) {
+   for (i = 0; i < cnt; i++)
+   set_bit(*obj + i, bitmap->table);
+
+   if (*obj == bitmap->last) {
+   bitmap->last = (*obj + cnt);
+   if (bitmap->last >= bitmap->max)
+   bitmap->last = 0;
+   }
+   *obj |= bitmap->top;
+   } else {
+   ret = -1;
+   }
+
+   spin_unlock(>lock);
+
+   return ret;
+}
+
 void hns_roce_bitmap_free_range(struct hns_roce_bitmap *bitmap,
unsigned long obj, int cnt)
 {
@@ -94,6 +133,101 @@ void hns_roce_bitmap_cleanup(struct hns_roce_bitmap 
*bitmap)
kfree(bitmap->table);
 }
 
+void hns_roce_buf_free(struct hns_roce_dev *hr_dev, u32 size,
+  struct hns_roce_buf *buf)
+{
+   int i;
+   struct device *dev = _dev->pdev->dev;
+   u32 bits_per_long = BITS_PER_LONG;
+
+   if (buf->nbufs == 1) {
+   dma_free_coherent(dev, size, buf->direct.buf, buf->direct.map);
+   } else {
+   if (bits_per_long == 64)
+   vunmap(buf->direct.buf);
+
+   for (i = 0; i < buf->nbufs; ++i)
+   if (buf->page_list[i].buf)
+   dma_free_coherent(_dev->pdev->dev, PAGE_SIZE,
+ buf->page_list[i].buf,
+ buf->page_list[i].map);
+   kfree(buf->page_list);
+   }
+}
+
+int hns_roce_buf_alloc(struct hns_roce_dev *hr_dev, u32 size, u32 max_direct,
+  struct hns_roce_buf *buf)
+{
+   int i = 0;
+   dma_addr_t t;
+   struct page **pages;
+   struct device *dev = _dev->pdev->dev;
+   u32 bits_per_long = BITS_PER_LONG;
+
+