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

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]

Date:	Tue, 21 Jun 2016 21:01:57 +0800
From:	"Wei Hu (Xavier)" <xavier.huwei@...wei.com>
To:	<leon@...nel.org>
CC:	Lijun Ou <oulijun@...wei.com>, <dledford@...hat.com>,
	<sean.hefty@...el.com>, <hal.rosenstock@...il.com>,
	<davem@...emloft.net>, <jeffrey.t.kirsher@...el.com>,
	<jiri@...lanox.com>, <ogerlitz@...lanox.com>,
	<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: [PATCH v10 06/22] IB/hns: Add initial cmd operation



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@...wei.com>
>>>> Signed-off-by: Nenglong Zhao <zhaonenglong@...ilicon.com>
>>>> Signed-off-by: Lijun Ou <oulijun@...wei.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
>>
>>
>>