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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Mon, 23 May 2016 11:30:57 +0800
From:	"Wei Hu (Xavier)" <xavier.huwei@...wei.com>
To:	Doug Ledford <dledford@...hat.com>, Lijun Ou <oulijun@...wei.com>,
	<sean.hefty@...el.com>, <hal.rosenstock@...il.com>,
	<davem@...emloft.net>, <jeffrey.t.kirsher@...el.com>,
	<jiri@...lanox.com>, <ogerlitz@...lanox.com>
CC:	<linux-rdma@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<netdev@...r.kernel.org>, <gongyangming@...wei.com>,
	<xiaokun@...wei.com>, <tangchaofei@...wei.com>,
	<haifeng.wei@...wei.com>, <yisen.zhuang@...wei.com>,
	<yankejian@...wei.com>, <charles.chenxin@...wei.com>,
	<linuxarm@...wei.com>
Subject: Re: [RESEND PATCH v7 17/21] IB/hns: Add QP operations support

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);

	..// <snip>

	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);

	..// <snip>

	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.
>