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

On Tue, Jun 28, 2016 at 02:31:41PM +0800, Wei Hu (Xavier) wrote:

On 2016/6/27 16:31, oulijun wrote:

quoted

Hi, Leon
在 2016/6/27 16:01, Leon Romanovsky 写道:

quoted

On Sat, Jun 25, 2016 at 06:25:37PM +0800, Wei Hu (Xavier) wrote:

quoted

On 2016/6/24 22:59, Leon Romanovsky wrote:

quoted

On Thu, Jun 16, 2016 at 10:35:12PM +0800, Lijun Ou wrote:

quoted

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 <redacted>
Signed-off-by: Nenglong Zhao <zhaonenglong-C8/M+/jPZTeaMJb+Lgu22Q@public.gmane.org>
Signed-off-by: Lijun Ou <redacted>
---

...

quoted

quoted

quoted

+
+#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.