Re: [PATCH v10 04/22] IB/hns: Add RoCE engine reset function

[Leon Romanovsky <leon@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2592 bytes --]

On Tue, Jun 28, 2016 at 02:31:41PM +0800, Wei Hu (Xavier) wrote:
> 
> 
> 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 <xavier.huwei@huawei.com>
> >>>>>Signed-off-by: Nenglong Zhao <zhaonenglong@hisilicon.com>
> >>>>>Signed-off-by: Lijun Ou <oulijun@huawei.com>
> >>>>>---
> >>...
> >>
> >>>>>+
> >>>>>+#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?
> 
>

Please avoid creating new directories/files under include/linux,
especially for one function only.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]