On Sat, Jun 25, 2016 at 06:25:37PM +0800, Wei Hu (Xavier) wrote:
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 <redacted>
Signed-off-by: Lijun Ou <redacted>
---
...
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.
Regards
Wei Hu
quoted
quoted
+
+#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
quoted
quoted
+ 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);
Hi, Leon
在 2016/6/27 16:01, Leon Romanovsky 写道:
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.
quoted
Regards
Wei Hu
quoted
quoted
+
+#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
quoted
quoted
+ 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
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2016/6/27 16:31, oulijun wrote:
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?
Regards
Wei Huquoted
quoted
Regards
Wei Hu
quoted
quoted
+
+#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
quoted
quoted
+ 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
.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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.