From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752886AbcEWDbg (ORCPT ); Sun, 22 May 2016 23:31:36 -0400 Received: from szxga01-in.huawei.com ([58.251.152.64]:24603 "EHLO szxga01-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752746AbcEWDbf (ORCPT ); Sun, 22 May 2016 23:31:35 -0400 Subject: Re: [RESEND PATCH v7 17/21] IB/hns: Add QP operations support To: Doug Ledford , Lijun Ou , , , , , , References: <1462849483-67927-1-git-send-email-oulijun@huawei.com> <1462849483-67927-18-git-send-email-oulijun@huawei.com> CC: , , , , , , , , , , From: "Wei Hu (Xavier)" Message-ID: <57427971.6090300@huawei.com> Date: Mon, 23 May 2016 11:30:57 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.57.115.113] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A090204.5742797E.0041,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2013-06-18 04:22:30, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: 8a7d7e26dd5bd8d31b9ec33c5a93465f Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, Doug Ledford In hns_roce_cmd_wait and hns_roce_cmd_poll, there are down&up 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 = &hr_dev->pdev->dev; u8 __iomem *hcr = hr_dev->cmd.hcr; unsigned long end = 0; u32 status = 0; int ret; down(&hr_dev->cmd.poll_sem); ..// up (&hr_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 = &hr_dev->cmd; struct device *dev = &hr_dev->pdev->dev; struct hns_roce_cmd_context *context; int ret = 0; down(&cmd->event_sem); ..// up (&cmd->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, &out_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. >