From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752051AbcF1GcX (ORCPT ); Tue, 28 Jun 2016 02:32:23 -0400 Received: from szxga02-in.huawei.com ([119.145.14.65]:59875 "EHLO szxga02-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750753AbcF1GcV (ORCPT ); Tue, 28 Jun 2016 02:32:21 -0400 Subject: Re: [PATCH v10 04/22] IB/hns: Add RoCE engine reset function To: oulijun , Leon Romanovsky , References: <1466087730-54856-1-git-send-email-oulijun@huawei.com> <1466087730-54856-5-git-send-email-oulijun@huawei.com> <20160624145938.GE23995@leon.nu> <576E5C21.5030904@huawei.com> <20160627080122.GA3584@leon.nu> <5770E465.1070702@huawei.com> CC: , , , , , , , , , , , , , , , , From: "Wei Hu (Xavier)" Message-ID: <577219CD.2000604@huawei.com> Date: Tue, 28 Jun 2016 14:31:41 +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: <5770E465.1070702@huawei.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.57.115.113] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A020206.577219DA.00B7,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: dde74408714ae80158f8f86ec29f3699 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2016/6/27 16:31, oulijun wrote: > Hi, Leon > 在 2016/6/27 16:01, Leon Romanovsky 写道: >> On Sat, Jun 25, 2016 at 06:25:37PM +0800, Wei Hu (Xavier) wrote: >>> >>> On 2016/6/24 22:59, Leon Romanovsky wrote: >>>> On Thu, Jun 16, 2016 at 10:35:12PM +0800, Lijun Ou wrote: >>>>> This patch mainly added reset flow of RoCE engine in RoCE >>>>> driver. It is necessary when RoCE is loaded and removed. >>>>> >>>>> Signed-off-by: Wei Hu >>>>> Signed-off-by: Nenglong Zhao >>>>> Signed-off-by: Lijun Ou >>>>> --- >> ... >> >>>>> + >>>>> +#define SLEEP_TIME_INTERVAL 20 >>>>> + >>>>> +extern int hns_dsaf_roce_reset(struct fwnode_handle *dsaf_fwnode, bool enable); >>>> Why did you add this extern? >>>> You already exported this function. >>>> drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c:EXPORT_SYMBOL(hns_dsaf_roce_reset); >>> Hi, Leon >>> >>> The function named hns_dsaf_roce_reset is defined in hns_dsaf_main.c >>> It exists in hns_dsaf.ko(ethernet driver) >>> >>> RoCE driver will call this function. >>> >>> Your suggestion is that delete "extern" as below: >>> In /drivers/infiniband/hw/hns/hns_roce_hw_v1.h: >>> >>> int hns_dsaf_roce_reset(struct fwnode_handle *dsaf_fwnode, bool >>> enable); >>> >>> Right? or other soultion? >> You placed it in header file. >> Please move it to your hns_roce_hw_v1.c file. >> > You suggest to do as follows, right? > in hns_roce_hw_v1.c > int hns_dsaf_roce_reset(struct fwnode_handle *dsaf_fwnode, bool enable); > > and delete the keyword extern > > Bcause reserve the extern in hns_roce_hw_v1.c, the checkpatch is not pass. Hi, Leon & Doug Ledford If we move it to hns_roce_hw_v1.c file as below: int hns_dsaf_roce_reset(struct fwnode_handle *dsaf_fwnode, bool enable); The result of checkpatch is warning. We prepare to add a head file for this function as below: In the directory of include\linux, mkdir hns. add hns_driver.h in include\linux\hns. In the file of hns_driver.h, the declaration: int hns_dsaf_roce_reset(struct fwnode_handle *dsaf_fwnode, bool enable); What do you think about? Regards Wei Hu >>> >>> Regards >>> Wei Hu >>>>> + >>>>> +#endif >>>>> diff --git a/drivers/infiniband/hw/hns/hns_roce_main.c b/drivers/infiniband/hw/hns/hns_roce_main.c >>>>> index 8924ce3..d5ccce2 100644 >>>>> --- a/drivers/infiniband/hw/hns/hns_roce_main.c >>>>> +++ b/drivers/infiniband/hw/hns/hns_roce_main.c >>>>> @@ -71,7 +71,9 @@ static int hns_roce_get_cfg(struct hns_roce_dev *hr_dev) >>>>> struct platform_device *pdev = NULL; >>>>> struct resource *res; >>>>> - if (!of_device_is_compatible(np, "hisilicon,hns-roce-v1")) { >>>>> + if (of_device_is_compatible(np, "hisilicon,hns-roce-v1")) { >>>>> + hr_dev->hw = &hns_roce_hw_v1; >>>>> + } else { >>>>> dev_err(dev, "device no compatible!\n"); >>>>> return -EINVAL; >>>>> } >>>>> @@ -118,6 +120,11 @@ static int hns_roce_get_cfg(struct hns_roce_dev *hr_dev) >>>>> return 0; >>>>> } >>>>> +static int hns_roce_engine_reset(struct hns_roce_dev *hr_dev, bool enable) >>>>> +{ >>>>> + return hr_dev->hw->reset(hr_dev, enable); >>>>> +} >>>>> + >>>>> /** >>>>> * hns_roce_probe - RoCE driver entrance >>>>> * @pdev: pointer to platform device >>>>> @@ -156,6 +163,12 @@ static int hns_roce_probe(struct platform_device *pdev) >>>>> goto error_failed_get_cfg; >>>>> } >>>>> + ret = hns_roce_engine_reset(hr_dev, true); >>>> Do you have better solution to sense device without doing full reset of >>>> your hardware? >>> Hi, Leon >>> >>> In this place, we need reset RoCEE engine to ensure that RoCE engine can >>> work correctly. >>> Hip06 Soc only support full reset RoCEE engine. >>> >>> Regards >>> Wei Hu >>> >>>>> + if (ret) { >>>>> + dev_err(dev, "Reset roce engine failed!\n"); >>>>> + goto error_failed_get_cfg; >>>>> + } >>>>> + >>>>> error_failed_get_cfg: >>>>> ib_dealloc_device(&hr_dev->ib_dev); >>>>> @@ -170,6 +183,8 @@ static int hns_roce_remove(struct platform_device *pdev) >>>>> { >>>>> struct hns_roce_dev *hr_dev = platform_get_drvdata(pdev); >>>>> + (void)hns_roce_engine_reset(hr_dev, false); >>>> Any reason to do explicit casting? >>> Hi, Leon >>> >>> /** >>> * hns_roce_engine_reset - reset roce >>> * @hr_dev: roce device struct pointer >>> * @enable: true -- drop reset, false -- reset >>> * return 0 - success , negative --fail >>> */ >>> static int hns_roce_engine_reset(struct hns_roce_dev *hr_dev, bool enable); >>> >>> hns_roce_engine_reset->hns_roce_v1_reset->hns_dsaf_roce_reset >>> >>> The err branch of hns_roce_engine_reset as below： >>> int hns_dsaf_roce_reset(struct fwnode_handle *dsaf_fwnode, bool enable) >>> { >>> <...> >>> if (!is_of_node(dsaf_fwnode)) { >>> pr_err("hisi_dsaf: Only support DT node!\n"); >>> return -EINVAL; >>> } >>> >>> pdev = of_find_device_by_node(to_of_node(dsaf_fwnode)); >>> dsaf_dev = dev_get_drvdata(&pdev->dev); >>> if (AE_IS_VER1(dsaf_dev->dsaf_ver)) { >>> dev_err(dsaf_dev->dev, "%s v1 chip do not support roce!\n", >>> dsaf_dev->ae_dev.name); >>> return -ENODEV; >>> } >>> <...> >>> } >>> >>> When the cpu is processing hns_roce_engine_reset(hr_dev, false), >>> hns_roce_engine_reset(hr_dev, true) have been alomost processed >>> sucessfully. >>> From the err branch of hns_roce_engine_reset, we found at this time >>> hns_roce_engine_reset(hr_dev, true) could not return err. >>> >>> In hns_roce_remove function, we call hns_roce_engine_reset(hr_dev, false), >>> and doesn't need to judge the return value. >> Do you see any compilation warning for this part of code? >> >> struct hns_roce_dev *hr_dev = platform_get_drvdata(pdev); >> + hns_roce_engine_reset(hr_dev, false); >> >> instead of >> >> struct hns_roce_dev *hr_dev = platform_get_drvdata(pdev); >> + (void)hns_roce_engine_reset(hr_dev, false); >> > No warning. > However, the result of PClint checking is error, because the hns_roce_engine_reset have return value. > > thanks > Lijun Ou > > > > . >