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

[Doug Ledford <dledford@redhat.com>]

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

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@redhat.com>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]