On Fri, Dec 07, 2018 at 04:46:32PM +0800, oulijun wrote:
> 在 2018/11/24 2:17, Jason Gunthorpe 写道:
> > On Fri, Nov 23, 2018 at 08:10:04PM +0200, Leon Romanovsky wrote:
> >> On Fri, Nov 23, 2018 at 04:26:36PM +0800, Lijun Ou wrote:
> >>> This patch refactors the code of implementing qp state transition.
> >>>
> >>> Signed-off-by: Lijun Ou <oulijun@xxxxxxxxxx>
> >>> drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 16 +---------------
> >>> drivers/infiniband/hw/hns/hns_roce_hw_v2.h | 19 +++++++++++++++++++
> >>> 2 files changed, 20 insertions(+), 15 deletions(-)
> >> "Refactor the code" means that it improves something, like line count or readability.
> >> Sometimes, it is needed to remove code duplication, but in your case, you simply moved a
> >> couple of lines from one place to another.
> >>
> >> Thanks
> >>
> >>> diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
> >>> index 3528f2f..66d66e9 100644
> >>> +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
> >>> @@ -3846,21 +3846,7 @@ static int hns_roce_v2_modify_qp(struct ib_qp *ibqp,
> >>> qpc_mask);
> >>> if (ret)
> >>> goto out;
> >>> - } else if ((cur_state == IB_QPS_RTS && new_state == IB_QPS_RTS) ||
> >>> - (cur_state == IB_QPS_SQE && new_state == IB_QPS_RTS) ||
> >>> - (cur_state == IB_QPS_RTS && new_state == IB_QPS_SQD) ||
> >>> - (cur_state == IB_QPS_SQD && new_state == IB_QPS_SQD) ||
> >>> - (cur_state == IB_QPS_SQD && new_state == IB_QPS_RTS) ||
> >>> - (cur_state == IB_QPS_INIT && new_state == IB_QPS_RESET) ||
> >>> - (cur_state == IB_QPS_RTR && new_state == IB_QPS_RESET) ||
> >>> - (cur_state == IB_QPS_RTS && new_state == IB_QPS_RESET) ||
> >>> - (cur_state == IB_QPS_ERR && new_state == IB_QPS_RESET) ||
> >>> - (cur_state == IB_QPS_INIT && new_state == IB_QPS_ERR) ||
> >>> - (cur_state == IB_QPS_RTR && new_state == IB_QPS_ERR) ||
> >>> - (cur_state == IB_QPS_RTS && new_state == IB_QPS_ERR) ||
> >>> - (cur_state == IB_QPS_SQD && new_state == IB_QPS_ERR) ||
> >>> - (cur_state == IB_QPS_SQE && new_state == IB_QPS_ERR) ||
> >>> - (cur_state == IB_QPS_ERR && new_state == IB_QPS_ERR)) {
> >>> + } else if (V2_QP_SUPPORT_STATE(cur_state, new_state)) {
> >>> /* Nothing */
> >>> ;
> >>> } else {
> >>> diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.h b/drivers/infiniband/hw/hns/hns_roce_hw_v2.h
> >>> index e6e3d8fb..7b308ac 100644
> >>> +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.h
> >>> @@ -133,6 +133,25 @@
> >>> (step_idx == 1 && hop_num == 1) || \
> >>> (step_idx == 2 && hop_num == 2))
> >>>
> >>> +#define V2_QP_SUPPORT_STATE(cur_state, new_state) \
> >>> + ((cur_state == IB_QPS_RTS && new_state == IB_QPS_RTS) || \
> >>> + (cur_state == IB_QPS_SQE && new_state == IB_QPS_RTS) || \
> >>> + (cur_state == IB_QPS_RTS && new_state == IB_QPS_SQD) || \
> >>> + (cur_state == IB_QPS_SQD && new_state == IB_QPS_SQD) || \
> >>> + (cur_state == IB_QPS_SQD && new_state == IB_QPS_RTS) || \
> >>> + (cur_state == IB_QPS_INIT && new_state == IB_QPS_RESET) || \
> >>> + (cur_state == IB_QPS_RTR && new_state == IB_QPS_RESET) || \
> >>> + (cur_state == IB_QPS_RTS && new_state == IB_QPS_RESET) || \
> >>> + (cur_state == IB_QPS_ERR && new_state == IB_QPS_RESET) || \
> >>> + (cur_state == IB_QPS_SQD && new_state == IB_QPS_RESET) || \
> >>> + (cur_state == IB_QPS_SQE && new_state == IB_QPS_RESET) || \
> >>> + (cur_state == IB_QPS_INIT && new_state == IB_QPS_ERR) || \
> >>> + (cur_state == IB_QPS_RTR && new_state == IB_QPS_ERR) || \
> >>> + (cur_state == IB_QPS_RTS && new_state == IB_QPS_ERR) || \
> >>> + (cur_state == IB_QPS_SQD && new_state == IB_QPS_ERR) || \
> >>> + (cur_state == IB_QPS_SQE && new_state == IB_QPS_ERR) || \
> >>> + (cur_state == IB_QPS_ERR && new_state == IB_QPS_ERR))
> > And please don't use macros if a static inline function will do, and
> > don't use marcos wrongly by forgetting to enclose parameters with ()
> >
> > Jason
> >
> > .
> Hi, jason
> if use static function, it will add complexity by sourcemonitor tool for new function.
No idea what that is.
'do not write functions as macros' trumps any tool you may be using.
Jason
