[PATCH for-next 0/3] RDMA/hns: Updates of CMDQ

[PATCH for-next 0/3] RDMA/hns: Updates of CMDQ

[Weihang Li]

This series contains few changes about CMDQ of HIP08/09. The ibdev may be
null while the driver is resetting, so we don't use ibdev_err for prints.

Lang Cheng (2):
  RDMA/hns: Enable all CMDQ context
  RDMA/hns: Support more return types of command queue

Wenpeng Liang (1):
  RDMA/hns: Modify prints for mailbox and command queue

 drivers/infiniband/hw/hns/hns_roce_cmd.c    | 79 ++++++++++++++---------------
 drivers/infiniband/hw/hns/hns_roce_device.h |  6 +--
 drivers/infiniband/hw/hns/hns_roce_hw_v2.c  |  9 +++-
 drivers/infiniband/hw/hns/hns_roce_hw_v2.h  | 18 +++++--
 4 files changed, 59 insertions(+), 53 deletions(-)

-- 
2.8.1

[PATCH for-next 1/3] RDMA/hns: Enable all CMDQ context

[Weihang Li]

From: Lang Cheng <chenglang@huawei.com>

Fix error of cmd's context number calculation algorithm to enable all of
32 cmd entries and support 32 concurrent accesses.

Signed-off-by: Lang Cheng <chenglang@huawei.com>
Signed-off-by: Weihang Li <liweihang@huawei.com>
---
 drivers/infiniband/hw/hns/hns_roce_cmd.c    | 62 ++++++++++++-----------------
 drivers/infiniband/hw/hns/hns_roce_device.h |  6 +--
 2 files changed, 27 insertions(+), 41 deletions(-)

diff --git a/drivers/infiniband/hw/hns/hns_roce_cmd.c b/drivers/infiniband/hw/hns/hns_roce_cmd.c
index 339e3fd..46900d0 100644
--- a/drivers/infiniband/hw/hns/hns_roce_cmd.c
+++ b/drivers/infiniband/hw/hns/hns_roce_cmd.c
@@ -38,22 +38,14 @@
 
 #define CMD_POLL_TOKEN 0xffff
 #define CMD_MAX_NUM 32
-#define CMD_TOKEN_MASK 0x1f
 
 static int hns_roce_cmd_mbox_post_hw(struct hns_roce_dev *hr_dev, u64 in_param,
 				     u64 out_param, u32 in_modifier,
 				     u8 op_modifier, u16 op, u16 token,
 				     int event)
 {
-	struct hns_roce_cmdq *cmd = &hr_dev->cmd;
-	int ret;
-
-	mutex_lock(&cmd->hcr_mutex);
-	ret = hr_dev->hw->post_mbox(hr_dev, in_param, out_param, in_modifier,
-				    op_modifier, op, token, event);
-	mutex_unlock(&cmd->hcr_mutex);
-
-	return ret;
+	return hr_dev->hw->post_mbox(hr_dev, in_param, out_param, in_modifier,
+				     op_modifier, op, token, event);
 }
 
 /* this should be called with "poll_sem" */
@@ -96,15 +88,18 @@ void hns_roce_cmd_event(struct hns_roce_dev *hr_dev, u16 token, u8 status,
 	struct hns_roce_cmd_context *context =
 		&hr_dev->cmd.context[token % hr_dev->cmd.max_cmds];
 
-	if (token != context->token)
+	if (unlikely(token != context->token)) {
+		dev_err_ratelimited(hr_dev->dev,
+				    "[cmd] invalid ae token %x,context token is %x!\n",
+				    token, context->token);
 		return;
+	}
 
 	context->result = (status == HNS_ROCE_CMD_SUCCESS) ? 0 : (-EIO);
 	context->out_param = out_param;
 	complete(&context->done);
 }
 
-/* this should be called with "use_events" */
 static int __hns_roce_cmd_mbox_wait(struct hns_roce_dev *hr_dev, u64 in_param,
 				    u64 out_param, unsigned long in_modifier,
 				    u8 op_modifier, u16 op,
@@ -116,13 +111,18 @@ static int __hns_roce_cmd_mbox_wait(struct hns_roce_dev *hr_dev, u64 in_param,
 	int ret;
 
 	spin_lock(&cmd->context_lock);
-	WARN_ON(cmd->free_head < 0);
-	context = &cmd->context[cmd->free_head];
-	context->token += cmd->token_mask + 1;
-	cmd->free_head = context->next;
+
+	do {
+		context = &cmd->context[cmd->free_head];
+		cmd->free_head = context->next;
+	} while (context->busy);
+
+	context->busy = 1;
+	context->token += cmd->max_cmds;
+
 	spin_unlock(&cmd->context_lock);
 
-	init_completion(&context->done);
+	reinit_completion(&context->done);
 
 	ret = hns_roce_cmd_mbox_post_hw(hr_dev, in_param, out_param,
 					in_modifier, op_modifier, op,
@@ -130,30 +130,19 @@ static int __hns_roce_cmd_mbox_wait(struct hns_roce_dev *hr_dev, u64 in_param,
 	if (ret)
 		goto out;
 
-	/*
-	 * It is timeout when wait_for_completion_timeout return 0
-	 * The return value is the time limit set in advance
-	 * how many seconds showing
-	 */
 	if (!wait_for_completion_timeout(&context->done,
 					 msecs_to_jiffies(timeout))) {
-		dev_err(dev, "[cmd]wait_for_completion_timeout timeout\n");
+		dev_err(dev, "[cmd] token %x timeout, drop.\n", context->token);
 		ret = -EBUSY;
 		goto out;
 	}
 
 	ret = context->result;
-	if (ret) {
-		dev_err(dev, "[cmd]event mod cmd process error!err=%d\n", ret);
-		goto out;
-	}
+	if (ret)
+		dev_err(dev, "[cmd] token %x error %d\n", context->token, ret);
 
 out:
-	spin_lock(&cmd->context_lock);
-	context->next = cmd->free_head;
-	cmd->free_head = context - cmd->context;
-	spin_unlock(&cmd->context_lock);
-
+	context->busy = 0;
 	return ret;
 }
 
@@ -208,7 +197,6 @@ int hns_roce_cmd_init(struct hns_roce_dev *hr_dev)
 {
 	struct device *dev = hr_dev->dev;
 
-	mutex_init(&hr_dev->cmd.hcr_mutex);
 	sema_init(&hr_dev->cmd.poll_sem, 1);
 	hr_dev->cmd.use_events = 0;
 	hr_dev->cmd.max_cmds = CMD_MAX_NUM;
@@ -239,16 +227,16 @@ int hns_roce_cmd_use_events(struct hns_roce_dev *hr_dev)
 	for (i = 0; i < hr_cmd->max_cmds; ++i) {
 		hr_cmd->context[i].token = i;
 		hr_cmd->context[i].next = i + 1;
+		init_completion(&hr_cmd->context[i].done);
 	}
-
-	hr_cmd->context[hr_cmd->max_cmds - 1].next = -1;
+	hr_cmd->context[hr_cmd->max_cmds - 1].next = 0;
 	hr_cmd->free_head = 0;
 
 	sema_init(&hr_cmd->event_sem, hr_cmd->max_cmds);
 	spin_lock_init(&hr_cmd->context_lock);
 
-	hr_cmd->token_mask = CMD_TOKEN_MASK;
 	hr_cmd->use_events = 1;
+	down(&hr_cmd->poll_sem);
 
 	return 0;
 }
@@ -259,6 +247,8 @@ void hns_roce_cmd_use_polling(struct hns_roce_dev *hr_dev)
 
 	kfree(hr_cmd->context);
 	hr_cmd->use_events = 0;
+
+	up(&hr_cmd->poll_sem);
 }
 
 struct hns_roce_cmd_mailbox *
diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h b/drivers/infiniband/hw/hns/hns_roce_device.h
index eb2ccb8..9c20c3a 100644
--- a/drivers/infiniband/hw/hns/hns_roce_device.h
+++ b/drivers/infiniband/hw/hns/hns_roce_device.h
@@ -556,6 +556,7 @@ struct hns_roce_cmd_context {
 	int			next;
 	u64			out_param;
 	u16			token;
+	u16			busy;
 };
 
 struct hns_roce_cmdq {
@@ -572,11 +573,6 @@ struct hns_roce_cmdq {
 	int			free_head;
 	struct hns_roce_cmd_context *context;
 	/*
-	 * Result of get integer part
-	 * which max_comds compute according a power of 2
-	 */
-	u16			token_mask;
-	/*
 	 * Process whether use event mode, init default non-zero
 	 * After the event queue of cmd event ready,
 	 * can switch into event mode
-- 
2.8.1

Re: [PATCH for-next 1/3] RDMA/hns: Enable all CMDQ context

[Leon Romanovsky]

On Thu, Apr 01, 2021 at 03:32:19PM +0800, Weihang Li wrote:
> From: Lang Cheng <chenglang@huawei.com>
> 
> Fix error of cmd's context number calculation algorithm to enable all of
> 32 cmd entries and support 32 concurrent accesses.
> 
> Signed-off-by: Lang Cheng <chenglang@huawei.com>
> Signed-off-by: Weihang Li <liweihang@huawei.com>
> ---
>  drivers/infiniband/hw/hns/hns_roce_cmd.c    | 62 ++++++++++++-----------------
>  drivers/infiniband/hw/hns/hns_roce_device.h |  6 +--
>  2 files changed, 27 insertions(+), 41 deletions(-)

<...>

> -	WARN_ON(cmd->free_head < 0);
> -	context = &cmd->context[cmd->free_head];
> -	context->token += cmd->token_mask + 1;
> -	cmd->free_head = context->next;
> +
> +	do {
> +		context = &cmd->context[cmd->free_head];
> +		cmd->free_head = context->next;
> +	} while (context->busy);
> +
> +	context->busy = 1;

This "busy" flag after do-while together with release in __hns_roce_cmd_mbox_wait()
is interesting thing. Are you sure that it won't loop forever here?

Thanks

Re: [PATCH for-next 1/3] RDMA/hns: Enable all CMDQ context

[Lang Cheng]

[-- Attachment #1: Type: text/plain, Size: 1510 bytes --]



On 2021/4/1 21:08, Leon Romanovsky wrote:
> On Thu, Apr 01, 2021 at 03:32:19PM +0800, Weihang Li wrote:
>> From: Lang Cheng <chenglang@huawei.com>
>>
>> Fix error of cmd's context number calculation algorithm to enable all of
>> 32 cmd entries and support 32 concurrent accesses.
>>
>> Signed-off-by: Lang Cheng <chenglang@huawei.com>
>> Signed-off-by: Weihang Li <liweihang@huawei.com>
>> ---
>>   drivers/infiniband/hw/hns/hns_roce_cmd.c    | 62 ++++++++++++-----------------
>>   drivers/infiniband/hw/hns/hns_roce_device.h |  6 +--
>>   2 files changed, 27 insertions(+), 41 deletions(-)
> 
> <...>
> 
>> -	WARN_ON(cmd->free_head < 0);
>> -	context = &cmd->context[cmd->free_head];
>> -	context->token += cmd->token_mask + 1;
>> -	cmd->free_head = context->next;
>> +
>> +	do {
>> +		context = &cmd->context[cmd->free_head];
>> +		cmd->free_head = context->next;
>> +	} while (context->busy);
>> +
>> +	context->busy = 1;
> 
> This "busy" flag after do-while together with release in __hns_roce_cmd_mbox_wait()
> is interesting thing. Are you sure that it won't loop forever here?
> 

When initializing resources in hns_roce_cmd_use_events(), ensure that 
the number of semaphores is consistent with the depth of context[].

int hns_roce_cmd_use_events( )
{
	hr_cmd->context = kcalloc(hr_cmd->max_cmds, ...);
	sema_init(&hr_cmd->event_sem, hr_cmd->max_cmds);
}

Then, when someone gets the event_sem in hns_roce_cmd_mbox_wait(), it 
means that there must be a not busy context.

Thanks.

> Thanks
> .
> 

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: chenglang.vcf --]
[-- Type: text/x-vcard; name="chenglang.vcf", Size: 4 bytes --]

null

Re: [PATCH for-next 1/3] RDMA/hns: Enable all CMDQ context

[Leon Romanovsky]

On Fri, Apr 02, 2021 at 09:26:38AM +0800, Lang Cheng wrote:
> 
> 
> On 2021/4/1 21:08, Leon Romanovsky wrote:
> > On Thu, Apr 01, 2021 at 03:32:19PM +0800, Weihang Li wrote:
> > > From: Lang Cheng <chenglang@huawei.com>
> > > 
> > > Fix error of cmd's context number calculation algorithm to enable all of
> > > 32 cmd entries and support 32 concurrent accesses.
> > > 
> > > Signed-off-by: Lang Cheng <chenglang@huawei.com>
> > > Signed-off-by: Weihang Li <liweihang@huawei.com>
> > > ---
> > >   drivers/infiniband/hw/hns/hns_roce_cmd.c    | 62 ++++++++++++-----------------
> > >   drivers/infiniband/hw/hns/hns_roce_device.h |  6 +--
> > >   2 files changed, 27 insertions(+), 41 deletions(-)
> > 
> > <...>
> > 
> > > -	WARN_ON(cmd->free_head < 0);
> > > -	context = &cmd->context[cmd->free_head];
> > > -	context->token += cmd->token_mask + 1;
> > > -	cmd->free_head = context->next;
> > > +
> > > +	do {
> > > +		context = &cmd->context[cmd->free_head];
> > > +		cmd->free_head = context->next;
> > > +	} while (context->busy);
> > > +
> > > +	context->busy = 1;
> > 
> > This "busy" flag after do-while together with release in __hns_roce_cmd_mbox_wait()
> > is interesting thing. Are you sure that it won't loop forever here?
> > 
> 
> When initializing resources in hns_roce_cmd_use_events(), ensure that the
> number of semaphores is consistent with the depth of context[].
> 
> int hns_roce_cmd_use_events( )
> {
> 	hr_cmd->context = kcalloc(hr_cmd->max_cmds, ...);
> 	sema_init(&hr_cmd->event_sem, hr_cmd->max_cmds);
> }
> 
> Then, when someone gets the event_sem in hns_roce_cmd_mbox_wait(), it means
> that there must be a not busy context.

OK, thanks

> 
> Thanks.
> 
> > Thanks
> > .
> > 

> null

[PATCH for-next 2/3] RDMA/hns: Support more return types of command queue

[Weihang Li]

From: Lang Cheng <chenglang@huawei.com>

Add error code definition according to the return code from firmware to
help find out more detailed reasons why a command fails to be sent.

Signed-off-by: Lang Cheng <chenglang@huawei.com>
Signed-off-by: Weihang Li <liweihang@huawei.com>
---
 drivers/infiniband/hw/hns/hns_roce_hw_v2.h | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.h b/drivers/infiniband/hw/hns/hns_roce_hw_v2.h
index ffdae15..95bee6b 100644
--- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.h
+++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.h
@@ -257,10 +257,20 @@ enum {
 };
 
 enum hns_roce_cmd_return_status {
-	CMD_EXEC_SUCCESS	= 0,
-	CMD_NO_AUTH		= 1,
-	CMD_NOT_EXEC		= 2,
-	CMD_QUEUE_FULL		= 3,
+	CMD_EXEC_SUCCESS,
+	CMD_NO_AUTH,
+	CMD_NOT_EXIST,
+	CMD_CRQ_FULL,
+	CMD_NEXT_ERR,
+	CMD_NOT_EXEC,
+	CMD_PARA_ERR,
+	CMD_RESULT_ERR,
+	CMD_TIMEOUT,
+	CMD_HILINK_ERR,
+	CMD_INFO_ILLEGAL,
+	CMD_INVALID,
+	CMD_ROH_CHECK_FAIL,
+	CMD_OTHER_ERR = 0xff
 };
 
 enum hns_roce_sgid_type {
-- 
2.8.1

[PATCH for-next 3/3] RDMA/hns: Modify prints for mailbox and command queue

[Weihang Li]

From: Wenpeng Liang <liangwenpeng@huawei.com>

Use ratelimited print in mbox and cmq. And print mailbox operation if
mailbox fails because it's useful information for the user.

Signed-off-by: Wenpeng Liang <liangwenpeng@huawei.com>
Signed-off-by: Lang Cheng <chenglang@huawei.com>
Signed-off-by: Weihang Li <liweihang@huawei.com>
---
 drivers/infiniband/hw/hns/hns_roce_cmd.c   | 21 +++++++++++++--------
 drivers/infiniband/hw/hns/hns_roce_hw_v2.c |  9 +++++++--
 2 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/infiniband/hw/hns/hns_roce_cmd.c b/drivers/infiniband/hw/hns/hns_roce_cmd.c
index 46900d0..5c7bd0e 100644
--- a/drivers/infiniband/hw/hns/hns_roce_cmd.c
+++ b/drivers/infiniband/hw/hns/hns_roce_cmd.c
@@ -54,14 +54,15 @@ static int __hns_roce_cmd_mbox_poll(struct hns_roce_dev *hr_dev, u64 in_param,
 				    u8 op_modifier, u16 op,
 				    unsigned int timeout)
 {
-	struct device *dev = hr_dev->dev;
 	int ret;
 
 	ret = hns_roce_cmd_mbox_post_hw(hr_dev, in_param, out_param,
 					in_modifier, op_modifier, op,
 					CMD_POLL_TOKEN, 0);
 	if (ret) {
-		dev_err(dev, "[cmd_poll]hns_roce_cmd_mbox_post_hw failed\n");
+		dev_err_ratelimited(hr_dev->dev,
+				    "failed to post mailbox %x in poll mode, ret = %d.\n",
+				    op, ret);
 		return ret;
 	}
 
@@ -127,19 +128,25 @@ static int __hns_roce_cmd_mbox_wait(struct hns_roce_dev *hr_dev, u64 in_param,
 	ret = hns_roce_cmd_mbox_post_hw(hr_dev, in_param, out_param,
 					in_modifier, op_modifier, op,
 					context->token, 1);
-	if (ret)
+	if (ret) {
+		dev_err_ratelimited(dev,
+				    "failed to post mailbox %x in event mode, ret = %d.\n",
+				    op, ret);
 		goto out;
+	}
 
 	if (!wait_for_completion_timeout(&context->done,
 					 msecs_to_jiffies(timeout))) {
-		dev_err(dev, "[cmd] token %x timeout, drop.\n", context->token);
+		dev_err_ratelimited(dev, "[cmd] token %x mailbox %x timeout.\n",
+				    context->token, op);
 		ret = -EBUSY;
 		goto out;
 	}
 
 	ret = context->result;
 	if (ret)
-		dev_err(dev, "[cmd] token %x error %d\n", context->token, ret);
+		dev_err_ratelimited(dev, "[cmd] token %x mailbox %x error %d\n",
+				    context->token, op, ret);
 
 out:
 	context->busy = 0;
@@ -195,12 +202,10 @@ int hns_roce_cmd_mbox(struct hns_roce_dev *hr_dev, u64 in_param, u64 out_param,
 
 int hns_roce_cmd_init(struct hns_roce_dev *hr_dev)
 {
-	struct device *dev = hr_dev->dev;
-
 	sema_init(&hr_dev->cmd.poll_sem, 1);
 	hr_dev->cmd.use_events = 0;
 	hr_dev->cmd.max_cmds = CMD_MAX_NUM;
-	hr_dev->cmd.pool = dma_pool_create("hns_roce_cmd", dev,
+	hr_dev->cmd.pool = dma_pool_create("hns_roce_cmd", hr_dev->dev,
 					   HNS_ROCE_MAILBOX_SIZE,
 					   HNS_ROCE_MAILBOX_SIZE, 0);
 	if (!hr_dev->cmd.pool)
diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
index 783388b..0b281fc 100644
--- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
+++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
@@ -1151,6 +1151,9 @@ static int hns_roce_alloc_cmq_desc(struct hns_roce_dev *hr_dev,
 		ring->desc_dma_addr = 0;
 		kfree(ring->desc);
 		ring->desc = NULL;
+
+		dev_err_ratelimited(hr_dev->dev,
+				    "failed to map cmq desc addr.\n");
 		return -ENOMEM;
 	}
 
@@ -1225,14 +1228,16 @@ static int hns_roce_v2_cmq_init(struct hns_roce_dev *hr_dev)
 	/* Init CSQ */
 	ret = hns_roce_init_cmq_ring(hr_dev, TYPE_CSQ);
 	if (ret) {
-		dev_err(hr_dev->dev, "Init CSQ error, ret = %d.\n", ret);
+		dev_err_ratelimited(hr_dev->dev,
+				    "failed to init CSQ, ret = %d.\n", ret);
 		return ret;
 	}
 
 	/* Init CRQ */
 	ret = hns_roce_init_cmq_ring(hr_dev, TYPE_CRQ);
 	if (ret) {
-		dev_err(hr_dev->dev, "Init CRQ error, ret = %d.\n", ret);
+		dev_err_ratelimited(hr_dev->dev,
+				    "failed to init CRQ, ret = %d.\n", ret);
 		goto err_crq;
 	}
 
-- 
2.8.1

Re: [PATCH for-next 0/3] RDMA/hns: Updates of CMDQ

[Jason Gunthorpe]

On Thu, Apr 01, 2021 at 03:32:18PM +0800, Weihang Li wrote:
> This series contains few changes about CMDQ of HIP08/09. The ibdev may be
> null while the driver is resetting, so we don't use ibdev_err for prints.
> 
> Lang Cheng (2):
>   RDMA/hns: Enable all CMDQ context
>   RDMA/hns: Support more return types of command queue
> 
> Wenpeng Liang (1):
>   RDMA/hns: Modify prints for mailbox and command queue

Applied to for-next, thanks

Jason