Re: [PATCH v10 06/22] IB/hns: Add initial cmd operation

["Wei Hu (Xavier)" <xavier.huwei@huawei.com>]

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 <xavier.huwei@huawei.com>
>>>> Signed-off-by: Nenglong Zhao <zhaonenglong@hisilicon.com>
>>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>>> ---
>>>> 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)
             {
                 <snip>
                 caps->max_cmds = 32;
                 <snip>
             }

         In hns_roce_cmd.c file:

   	  int hns_roce_cmd_init(struct hns_roce_dev *hr_dev)
	  {
	     <snip>
	     hr_dev->cmd.max_cmds = hr_dev->caps->max_cmds;
      		<snip>
           }

        Can you give more suggestions?


Regards
Wei Hu
>> Regards
>> Wei Hu
>>
>>
>>