diff mbox series

[net-next,2/3] net: hns3: Fix for VF mailbox receiving unknown message

Message ID 20180605114201.29900-3-salil.mehta@huawei.com
State Superseded
Headers show
Series Bug fixes & optimization for HNS3 Driver | expand

Commit Message

Salil Mehta June 5, 2018, 11:42 a.m. UTC 
From: Xi Wang <wangxi11@huawei.com>


Before the firmware updates the crq's tail pointer, if the VF driver
reads the data in the crq, the data may be incomplete at this time,
which will lead to the driver read an unknown message.

This patch fixes it by checking if crq is empty before reading the
message.

Fixes: b11a0bb231f3 ("net: hns3: Add mailbox support to VF driver")
Signed-off-by: Xi Wang <wangxi11@huawei.com>

Signed-off-by: Peng Li <lipeng321@huawei.com>

Signed-off-by: Salil Mehta <salil.mehta@huawei.com>

---
 .../ethernet/hisilicon/hns3/hns3vf/hclgevf_mbx.c   | 23 +++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

-- 
2.7.4

Comments

David Miller June 5, 2018, 6:40 p.m. UTC | #1 
From: Salil Mehta <salil.mehta@huawei.com>

Date: Tue, 5 Jun 2018 12:42:00 +0100

> +		if (unlikely(!hnae3_get_bit(flag, HCLGEVF_CMDQ_RX_OUTVLD_B))) {


This breaks the build, there is no such symbol named hnae3_get_bit().
kernel test robot June 6, 2018, 8:48 a.m. UTC | #2 
Hi Xi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Salil-Mehta/Bug-fixes-optimization-for-HNS3-Driver/20180606-155040
config: xtensa-allmodconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 8.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=xtensa 

All errors (new ones prefixed by >>):

   In file included from include/linux/init.h:5,
                    from drivers/net/ethernet/hisilicon/hns3/hclge_mbx.h:6,
                    from drivers/net/ethernet//hisilicon/hns3/hns3vf/hclgevf_mbx.c:4:
   drivers/net/ethernet//hisilicon/hns3/hns3vf/hclgevf_mbx.c: In function 'hclgevf_mbx_handler':
>> drivers/net/ethernet//hisilicon/hns3/hns3vf/hclgevf_mbx.c:155:17: error: implicit declaration of function 'hnae3_get_bit'; did you mean 'hnae_get_bit'? [-Werror=implicit-function-declaration]

      if (unlikely(!hnae3_get_bit(flag, HCLGEVF_CMDQ_RX_OUTVLD_B))) {
                    ^~~~~~~~~~~~~
   include/linux/compiler.h:77:42: note: in definition of macro 'unlikely'
    # define unlikely(x) __builtin_expect(!!(x), 0)
                                             ^
   cc1: some warnings being treated as errors

vim +155 drivers/net/ethernet//hisilicon/hns3/hns3vf/hclgevf_mbx.c

     3	
   > 4	#include "hclge_mbx.h"

     5	#include "hclgevf_main.h"
     6	#include "hnae3.h"
     7	
     8	static void hclgevf_reset_mbx_resp_status(struct hclgevf_dev *hdev)
     9	{
    10		/* this function should be called with mbx_resp.mbx_mutex held
    11		 * to prtect the received_response from race condition
    12		 */
    13		hdev->mbx_resp.received_resp  = false;
    14		hdev->mbx_resp.origin_mbx_msg = 0;
    15		hdev->mbx_resp.resp_status    = 0;
    16		memset(hdev->mbx_resp.additional_info, 0, HCLGE_MBX_MAX_RESP_DATA_SIZE);
    17	}
    18	
    19	/* hclgevf_get_mbx_resp: used to get a response from PF after VF sends a mailbox
    20	 * message to PF.
    21	 * @hdev: pointer to struct hclgevf_dev
    22	 * @resp_msg: pointer to store the original message type and response status
    23	 * @len: the resp_msg data array length.
    24	 */
    25	static int hclgevf_get_mbx_resp(struct hclgevf_dev *hdev, u16 code0, u16 code1,
    26					u8 *resp_data, u16 resp_len)
    27	{
    28	#define HCLGEVF_MAX_TRY_TIMES	500
    29	#define HCLGEVF_SLEEP_USCOEND	1000
    30		struct hclgevf_mbx_resp_status *mbx_resp;
    31		u16 r_code0, r_code1;
    32		int i = 0;
    33	
    34		if (resp_len > HCLGE_MBX_MAX_RESP_DATA_SIZE) {
    35			dev_err(&hdev->pdev->dev,
    36				"VF mbx response len(=%d) exceeds maximum(=%d)\n",
    37				resp_len,
    38				HCLGE_MBX_MAX_RESP_DATA_SIZE);
    39			return -EINVAL;
    40		}
    41	
    42		while ((!hdev->mbx_resp.received_resp) && (i < HCLGEVF_MAX_TRY_TIMES)) {
    43			udelay(HCLGEVF_SLEEP_USCOEND);
    44			i++;
    45		}
    46	
    47		if (i >= HCLGEVF_MAX_TRY_TIMES) {
    48			dev_err(&hdev->pdev->dev,
    49				"VF could not get mbx resp(=%d) from PF in %d tries\n",
    50				hdev->mbx_resp.received_resp, i);
    51			return -EIO;
    52		}
    53	
    54		mbx_resp = &hdev->mbx_resp;
    55		r_code0 = (u16)(mbx_resp->origin_mbx_msg >> 16);
    56		r_code1 = (u16)(mbx_resp->origin_mbx_msg & 0xff);
    57	
    58		if (mbx_resp->resp_status)
    59			return mbx_resp->resp_status;
    60	
    61		if (resp_data)
    62			memcpy(resp_data, &mbx_resp->additional_info[0], resp_len);
    63	
    64		hclgevf_reset_mbx_resp_status(hdev);
    65	
    66		if (!(r_code0 == code0 && r_code1 == code1 && !mbx_resp->resp_status)) {
    67			dev_err(&hdev->pdev->dev,
    68				"VF could not match resp code(code0=%d,code1=%d), %d",
    69				code0, code1, mbx_resp->resp_status);
    70			return -EIO;
    71		}
    72	
    73		return 0;
    74	}
    75	
    76	int hclgevf_send_mbx_msg(struct hclgevf_dev *hdev, u16 code, u16 subcode,
    77				 const u8 *msg_data, u8 msg_len, bool need_resp,
    78				 u8 *resp_data, u16 resp_len)
    79	{
    80		struct hclge_mbx_vf_to_pf_cmd *req;
    81		struct hclgevf_desc desc;
    82		int status;
    83	
    84		req = (struct hclge_mbx_vf_to_pf_cmd *)desc.data;
    85	
    86		/* first two bytes are reserved for code & subcode */
    87		if (msg_len > (HCLGE_MBX_MAX_MSG_SIZE - 2)) {
    88			dev_err(&hdev->pdev->dev,
    89				"VF send mbx msg fail, msg len %d exceeds max len %d\n",
    90				msg_len, HCLGE_MBX_MAX_MSG_SIZE);
    91			return -EINVAL;
    92		}
    93	
    94		hclgevf_cmd_setup_basic_desc(&desc, HCLGEVF_OPC_MBX_VF_TO_PF, false);
    95		req->msg[0] = code;
    96		req->msg[1] = subcode;
    97		memcpy(&req->msg[2], msg_data, msg_len);
    98	
    99		/* synchronous send */
   100		if (need_resp) {
   101			mutex_lock(&hdev->mbx_resp.mbx_mutex);
   102			hclgevf_reset_mbx_resp_status(hdev);
   103			status = hclgevf_cmd_send(&hdev->hw, &desc, 1);
   104			if (status) {
   105				dev_err(&hdev->pdev->dev,
   106					"VF failed(=%d) to send mbx message to PF\n",
   107					status);
   108				mutex_unlock(&hdev->mbx_resp.mbx_mutex);
   109				return status;
   110			}
   111	
   112			status = hclgevf_get_mbx_resp(hdev, code, subcode, resp_data,
   113						      resp_len);
   114			mutex_unlock(&hdev->mbx_resp.mbx_mutex);
   115		} else {
   116			/* asynchronous send */
   117			status = hclgevf_cmd_send(&hdev->hw, &desc, 1);
   118			if (status) {
   119				dev_err(&hdev->pdev->dev,
   120					"VF failed(=%d) to send mbx message to PF\n",
   121					status);
   122				return status;
   123			}
   124		}
   125	
   126		return status;
   127	}
   128	
   129	static bool hclgevf_cmd_crq_empty(struct hclgevf_hw *hw)
   130	{
   131		u32 tail = hclgevf_read_dev(hw, HCLGEVF_NIC_CRQ_TAIL_REG);
   132	
   133		return tail == hw->cmq.crq.next_to_use;
   134	}
   135	
   136	void hclgevf_mbx_handler(struct hclgevf_dev *hdev)
   137	{
   138		struct hclgevf_mbx_resp_status *resp;
   139		struct hclge_mbx_pf_to_vf_cmd *req;
   140		struct hclgevf_cmq_ring *crq;
   141		struct hclgevf_desc *desc;
   142		u16 *msg_q;
   143		u16 flag;
   144		u8 *temp;
   145		int i;
   146	
   147		resp = &hdev->mbx_resp;
   148		crq = &hdev->hw.cmq.crq;
   149	
   150		while (!hclgevf_cmd_crq_empty(&hdev->hw)) {
   151			desc = &crq->desc[crq->next_to_use];
   152			req = (struct hclge_mbx_pf_to_vf_cmd *)desc->data;
   153	
   154			flag = le16_to_cpu(crq->desc[crq->next_to_use].flag);
 > 155			if (unlikely(!hnae3_get_bit(flag, HCLGEVF_CMDQ_RX_OUTVLD_B))) {

   156				dev_warn(&hdev->pdev->dev,
   157					 "dropped invalid mailbox message, code = %d\n",
   158					 req->msg[0]);
   159	
   160				/* dropping/not processing this invalid message */
   161				crq->desc[crq->next_to_use].flag = 0;
   162				hclge_mbx_ring_ptr_move_crq(crq);
   163				continue;
   164			}
   165	
   166			/* synchronous messages are time critical and need preferential
   167			 * treatment. Therefore, we need to acknowledge all the sync
   168			 * responses as quickly as possible so that waiting tasks do not
   169			 * timeout and simultaneously queue the async messages for later
   170			 * prcessing in context of mailbox task i.e. the slow path.
   171			 */
   172			switch (req->msg[0]) {
   173			case HCLGE_MBX_PF_VF_RESP:
   174				if (resp->received_resp)
   175					dev_warn(&hdev->pdev->dev,
   176						 "VF mbx resp flag not clear(%d)\n",
   177						 req->msg[1]);
   178				resp->received_resp = true;
   179	
   180				resp->origin_mbx_msg = (req->msg[1] << 16);
   181				resp->origin_mbx_msg |= req->msg[2];
   182				resp->resp_status = req->msg[3];
   183	
   184				temp = (u8 *)&req->msg[4];
   185				for (i = 0; i < HCLGE_MBX_MAX_RESP_DATA_SIZE; i++) {
   186					resp->additional_info[i] = *temp;
   187					temp++;
   188				}
   189				break;
   190			case HCLGE_MBX_LINK_STAT_CHANGE:
   191			case HCLGE_MBX_ASSERTING_RESET:
   192				/* set this mbx event as pending. This is required as we
   193				 * might loose interrupt event when mbx task is busy
   194				 * handling. This shall be cleared when mbx task just
   195				 * enters handling state.
   196				 */
   197				hdev->mbx_event_pending = true;
   198	
   199				/* we will drop the async msg if we find ARQ as full
   200				 * and continue with next message
   201				 */
   202				if (hdev->arq.count >= HCLGE_MBX_MAX_ARQ_MSG_NUM) {
   203					dev_warn(&hdev->pdev->dev,
   204						 "Async Q full, dropping msg(%d)\n",
   205						 req->msg[1]);
   206					break;
   207				}
   208	
   209				/* tail the async message in arq */
   210				msg_q = hdev->arq.msg_q[hdev->arq.tail];
   211				memcpy(&msg_q[0], req->msg, HCLGE_MBX_MAX_ARQ_MSG_SIZE);
   212				hclge_mbx_tail_ptr_move_arq(hdev->arq);
   213				hdev->arq.count++;
   214	
   215				hclgevf_mbx_task_schedule(hdev);
   216	
   217				break;
   218			default:
   219				dev_err(&hdev->pdev->dev,
   220					"VF received unsupported(%d) mbx msg from PF\n",
   221					req->msg[0]);
   222				break;
   223			}
   224			crq->desc[crq->next_to_use].flag = 0;
   225			hclge_mbx_ring_ptr_move_crq(crq);
   226		}
   227	
   228		/* Write back CMDQ_RQ header pointer, M7 need this pointer */
   229		hclgevf_write_dev(&hdev->hw, HCLGEVF_NIC_CRQ_HEAD_REG,
   230				  crq->next_to_use);
   231	}
   232	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Salil Mehta June 6, 2018, 1:10 p.m. UTC | #3 
Hi Dave,
Sorry about this. I have fixed this in the V2 patch floated just now.

Best regards
Salil.
> -----Original Message-----

> From: David Miller [mailto:davem@davemloft.net]

> Sent: Tuesday, June 05, 2018 7:41 PM

> To: Salil Mehta <salil.mehta@huawei.com>

> Cc: Zhuangyuzeng (Yisen) <yisen.zhuang@huawei.com>; lipeng (Y)

> <lipeng321@huawei.com>; mehta.salil@opnsrc.net; netdev@vger.kernel.org;

> linux-kernel@vger.kernel.org; Linuxarm <linuxarm@huawei.com>; wangxi

> (M) <wangxi11@huawei.com>

> Subject: Re: [PATCH net-next 2/3] net: hns3: Fix for VF mailbox

> receiving unknown message

> 

> From: Salil Mehta <salil.mehta@huawei.com>

> Date: Tue, 5 Jun 2018 12:42:00 +0100

> 

> > +		if (unlikely(!hnae3_get_bit(flag,

> HCLGEVF_CMDQ_RX_OUTVLD_B))) {

> 

> This breaks the build, there is no such symbol named hnae3_get_bit().
diff mbox series

Patch

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_mbx.c b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_mbx.c
index a286184..173ca27 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_mbx.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3vf/hclgevf_mbx.c
@@ -126,6 +126,13 @@  int hclgevf_send_mbx_msg(struct hclgevf_dev *hdev, u16 code, u16 subcode,
 	return status;
 }
 
+static bool hclgevf_cmd_crq_empty(struct hclgevf_hw *hw)
+{
+	u32 tail = hclgevf_read_dev(hw, HCLGEVF_NIC_CRQ_TAIL_REG);
+
+	return tail == hw->cmq.crq.next_to_use;
+}
+
 void hclgevf_mbx_handler(struct hclgevf_dev *hdev)
 {
 	struct hclgevf_mbx_resp_status *resp;
@@ -140,11 +147,22 @@  void hclgevf_mbx_handler(struct hclgevf_dev *hdev)
 	resp = &hdev->mbx_resp;
 	crq = &hdev->hw.cmq.crq;
 
-	flag = le16_to_cpu(crq->desc[crq->next_to_use].flag);
-	while (hnae_get_bit(flag, HCLGEVF_CMDQ_RX_OUTVLD_B)) {
+	while (!hclgevf_cmd_crq_empty(&hdev->hw)) {
 		desc = &crq->desc[crq->next_to_use];
 		req = (struct hclge_mbx_pf_to_vf_cmd *)desc->data;
 
+		flag = le16_to_cpu(crq->desc[crq->next_to_use].flag);
+		if (unlikely(!hnae3_get_bit(flag, HCLGEVF_CMDQ_RX_OUTVLD_B))) {
+			dev_warn(&hdev->pdev->dev,
+				 "dropped invalid mailbox message, code = %d\n",
+				 req->msg[0]);
+
+			/* dropping/not processing this invalid message */
+			crq->desc[crq->next_to_use].flag = 0;
+			hclge_mbx_ring_ptr_move_crq(crq);
+			continue;
+		}
+
 		/* synchronous messages are time critical and need preferential
 		 * treatment. Therefore, we need to acknowledge all the sync
 		 * responses as quickly as possible so that waiting tasks do not
@@ -205,7 +223,6 @@  void hclgevf_mbx_handler(struct hclgevf_dev *hdev)
 		}
 		crq->desc[crq->next_to_use].flag = 0;
 		hclge_mbx_ring_ptr_move_crq(crq);
-		flag = le16_to_cpu(crq->desc[crq->next_to_use].flag);
 	}
 
 	/* Write back CMDQ_RQ header pointer, M7 need this pointer */