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] [thread-next>] [day] [month] [year] [list]

Message-ID: <bf001e31-982a-c51d-782e-81ee19f9de09@redhat.com>
Date:	Fri, 13 May 2016 17:52:04 -0400
From:	Doug Ledford <dledford@...hat.com>
To:	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

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.

-- 
Doug Ledford <dledford@...hat.com>
              GPG KeyID: 0E572FDD

Download attachment "signature.asc" of type "application/pgp-signature" (885 bytes)