From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751811AbcFUNFJ (ORCPT ); Tue, 21 Jun 2016 09:05:09 -0400 Received: from szxga01-in.huawei.com ([58.251.152.64]:62502 "EHLO szxga01-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750978AbcFUNFC (ORCPT ); Tue, 21 Jun 2016 09:05:02 -0400 Subject: Re: [PATCH v10 06/22] IB/hns: Add initial cmd operation To: References: <1466087730-54856-1-git-send-email-oulijun@huawei.com> <1466087730-54856-7-git-send-email-oulijun@huawei.com> <20160620133310.GB4526@leon.nu> <57691C0B.8090509@huawei.com> <20160621112808.GA9762@leon.nu> CC: Lijun Ou , , , , , , , , , , , , , , , , , , From: "Wei Hu (Xavier)" Message-ID: <57693AC4.1070306@huawei.com> Date: Tue, 21 Jun 2016 21:01: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: <20160621112808.GA9762@leon.nu> 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.0A090202.57693AD2.0092,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: 58f71c7169d434af9c552e143372fa81 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2016/6/21 19:28, Leon Romanovsky wrote: > On Tue, Jun 21, 2016 at 06:50:51PM +0800, Wei Hu (Xavier) wrote: >> >> On 2016/6/20 21:33, Leon Romanovsky wrote: >>> On Thu, Jun 16, 2016 at 10:35:14PM +0800, Lijun Ou wrote: >>>> This patch added the operation for cmd, and added some functions >>>> for initializing eq table and selecting cmd mode. >>>> >>>> Signed-off-by: Wei Hu >>>> Signed-off-by: Nenglong Zhao >>>> Signed-off-by: Lijun Ou >>>> --- >>>> PATCH v9/v8/v7/v6: >>>> - No change over the PATCH v5 >>>> >>>> PATCH v5: >>>> - The initial patch which was redesigned based on the second patch >>>> in PATCH v4 >>>> --- >>> <...> >>> >>>> +#define CMD_MAX_NUM 32 >>>> + >>>> +int hns_roce_cmd_init(struct hns_roce_dev *hr_dev) >>>> +{ >>>> + struct device *dev = &hr_dev->pdev->dev; >>>> + >>>> + mutex_init(&hr_dev->cmd.hcr_mutex); >>>> + sema_init(&hr_dev->cmd.poll_sem, 1); >>>> + hr_dev->cmd.use_events = 0; >>>> + hr_dev->cmd.toggle = 1; >>>> + hr_dev->cmd.max_cmds = CMD_MAX_NUM; >>> <...> >>> >>>> + for (hr_cmd->token_mask = 1; hr_cmd->token_mask < hr_cmd->max_cmds; >>>> + hr_cmd->token_mask <<= 1) >>>> + ; >>>> + --hr_cmd->token_mask; >>> It doesn't look that you dynamically change max_cmds supported. >>> Why do you need to calculate token_mask dynamically? >> Hi, Leon >> >> 1. The four lines above are in the function named >> hns_roce_cmd_use_events. >> and now this function is only called once in hns_roce_probe. >> 2. In hns_roce_cmd_use_events, >> we use these 4 lines to achieve the value of hr_cmd->token_mask >> according to hr_cmd->max_cmds dynamically, >> then we only define one marco for hr_cmd->max_cmds as below: >> >> #define CMD_MAX_NUM 32 >> >> And it looks more flexible. > It is called over engineering. > I would recommend to you to remove it. > > We don't need over complicated code which is executed > once with need to maintain with zero benefit. > > The other places need such simplification too. Hi, Leon We will modify this place as below: In hns_roce_hw_v1.c(for hip06 soc) file: void hns_roce_v1_profile(struct hns_roce_dev *hr_dev) { caps->max_cmds = 32; } In hns_roce_cmd.c file: int hns_roce_cmd_init(struct hns_roce_dev *hr_dev) { hr_dev->cmd.max_cmds = hr_dev->caps->max_cmds; } Can you give more suggestions? Regards Wei Hu >> Regards >> Wei Hu >> >> >>