| Message ID | 1466087730-54856-5-git-send-email-oulijun@huawei.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
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> > --- > PATCH v9/v8: > - No change over the PATCH v7 > > PATCH v7: > This fixes the comments given by Leon Romanovsky over the PATCH v6: > Link: https://lkml.org/lkml/2016/5/3/733 > > PATCH v6: > - No change over the PATCH v5 > > PATCH v5: > - The initial patch which was redesigned based on the second patch > in PATCH v4 > --- > --- > drivers/infiniband/hw/hns/hns_roce_device.h | 7 +++ > drivers/infiniband/hw/hns/hns_roce_hw_v1.c | 72 +++++++++++++++++++++++++++++ > drivers/infiniband/hw/hns/hns_roce_hw_v1.h | 40 ++++++++++++++++ > drivers/infiniband/hw/hns/hns_roce_main.c | 17 ++++++- > 4 files changed, 135 insertions(+), 1 deletion(-) > create mode 100644 drivers/infiniband/hw/hns/hns_roce_hw_v1.c > create mode 100644 drivers/infiniband/hw/hns/hns_roce_hw_v1.h > > diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h b/drivers/infiniband/hw/hns/hns_roce_device.h > index 946b470..b857c76 100644 > --- a/drivers/infiniband/hw/hns/hns_roce_device.h > +++ b/drivers/infiniband/hw/hns/hns_roce_device.h > @@ -56,6 +56,10 @@ struct hns_roce_caps { > u8 num_ports; > }; > > +struct hns_roce_hw { > + int (*reset)(struct hns_roce_dev *hr_dev, bool enable); > +}; > + > struct hns_roce_dev { > struct ib_device ib_dev; > struct platform_device *pdev; > @@ -68,6 +72,9 @@ struct hns_roce_dev { > > int cmd_mod; > int loop_idc; > + struct hns_roce_hw *hw; > }; > > +extern struct hns_roce_hw hns_roce_hw_v1; > + > #endif /* _HNS_ROCE_DEVICE_H */ > diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v1.c b/drivers/infiniband/hw/hns/hns_roce_hw_v1.c > new file mode 100644 > index 0000000..198be3b > --- /dev/null > +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v1.c > @@ -0,0 +1,72 @@ > +/* > + * Copyright (c) 2016 Hisilicon Limited. > + * > + * This software is available to you under a choice of one of two > + * licenses. You may choose to be licensed under the terms of the GNU > + * General Public License (GPL) Version 2, available from the file > + * COPYING in the main directory of this source tree, or the > + * OpenIB.org BSD license below: > + * > + * Redistribution and use in source and binary forms, with or > + * without modification, are permitted provided that the following > + * conditions are met: > + * > + * - Redistributions of source code must retain the above > + * copyright notice, this list of conditions and the following > + * disclaimer. > + * > + * - Redistributions in binary form must reproduce the above > + * copyright notice, this list of conditions and the following > + * disclaimer in the documentation and/or other materials > + * provided with the distribution. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND > + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS > + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN > + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN > + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE > + * SOFTWARE. > + */ > + > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_address.h> > +#include <linux/of_platform.h> > +#include <linux/platform_device.h> > +#include "hns_roce_device.h" > +#include "hns_roce_hw_v1.h" > + > +/** > + * hns_roce_v1_reset - reset roce > + * @hr_dev: roce device struct pointer > + * @enable: true -- drop reset, false -- reset > + * return 0 - success , negative --fail > + */ > +int hns_roce_v1_reset(struct hns_roce_dev *hr_dev, bool enable) > +{ > + struct device_node *dsaf_node; > + struct device *dev = &hr_dev->pdev->dev; > + struct device_node *np = dev->of_node; > + int ret; > + > + dsaf_node = of_parse_phandle(np, "dsaf-handle", 0); > + > + if (!enable) { > + ret = hns_dsaf_roce_reset(&dsaf_node->fwnode, false); > + } else { > + ret = hns_dsaf_roce_reset(&dsaf_node->fwnode, false); Move this line out of if-else and leave "if (enable)" part only. > + if (ret) > + return ret; > + > + msleep(SLEEP_TIME_INTERVAL); Nice, here you used define and in other places just hardcoded 20 (msleep(20)). Please give meaningful definition to 20. > + ret = hns_dsaf_roce_reset(&dsaf_node->fwnode, true); > + } > + > + return ret; Indentation > +} > + > +struct hns_roce_hw hns_roce_hw_v1 = { > + .reset = hns_roce_v1_reset, > +}; > diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v1.h b/drivers/infiniband/hw/hns/hns_roce_hw_v1.h > new file mode 100644 > index 0000000..a8c0c1d > --- /dev/null > +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v1.h > @@ -0,0 +1,40 @@ > +/* > + * Copyright (c) 2016 Hisilicon Limited. > + * > + * This software is available to you under a choice of one of two > + * licenses. You may choose to be licensed under the terms of the GNU > + * General Public License (GPL) Version 2, available from the file > + * COPYING in the main directory of this source tree, or the > + * OpenIB.org BSD license below: > + * > + * Redistribution and use in source and binary forms, with or > + * without modification, are permitted provided that the following > + * conditions are met: > + * > + * - Redistributions of source code must retain the above > + * copyright notice, this list of conditions and the following > + * disclaimer. > + * > + * - Redistributions in binary form must reproduce the above > + * copyright notice, this list of conditions and the following > + * disclaimer in the documentation and/or other materials > + * provided with the distribution. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND > + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS > + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN > + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN > + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE > + * SOFTWARE. > + */ > + > +#ifndef _HNS_ROCE_HW_V1_H > +#define _HNS_ROCE_HW_V1_H > + > +#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); > + > +#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? > + 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? > + > ib_dealloc_device(&hr_dev->ib_dev); > > return 0; > -- > 1.9.1 >
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. > > > 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);
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. >> >> >> 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
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? 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 > > > > . >
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.
diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h b/drivers/infiniband/hw/hns/hns_roce_device.h index 946b470..b857c76 100644 --- a/drivers/infiniband/hw/hns/hns_roce_device.h +++ b/drivers/infiniband/hw/hns/hns_roce_device.h @@ -56,6 +56,10 @@ struct hns_roce_caps { u8 num_ports; }; +struct hns_roce_hw { + int (*reset)(struct hns_roce_dev *hr_dev, bool enable); +}; + struct hns_roce_dev { struct ib_device ib_dev; struct platform_device *pdev; @@ -68,6 +72,9 @@ struct hns_roce_dev { int cmd_mod; int loop_idc; + struct hns_roce_hw *hw; }; +extern struct hns_roce_hw hns_roce_hw_v1; + #endif /* _HNS_ROCE_DEVICE_H */ diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v1.c b/drivers/infiniband/hw/hns/hns_roce_hw_v1.c new file mode 100644 index 0000000..198be3b --- /dev/null +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v1.c @@ -0,0 +1,72 @@ +/* + * Copyright (c) 2016 Hisilicon Limited. + * + * This software is available to you under a choice of one of two + * licenses. You may choose to be licensed under the terms of the GNU + * General Public License (GPL) Version 2, available from the file + * COPYING in the main directory of this source tree, or the + * OpenIB.org BSD license below: + * + * Redistribution and use in source and binary forms, with or + * without modification, are permitted provided that the following + * conditions are met: + * + * - Redistributions of source code must retain the above + * copyright notice, this list of conditions and the following + * disclaimer. + * + * - Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following + * disclaimer in the documentation and/or other materials + * provided with the distribution. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/of_platform.h> +#include <linux/platform_device.h> +#include "hns_roce_device.h" +#include "hns_roce_hw_v1.h" + +/** + * hns_roce_v1_reset - reset roce + * @hr_dev: roce device struct pointer + * @enable: true -- drop reset, false -- reset + * return 0 - success , negative --fail + */ +int hns_roce_v1_reset(struct hns_roce_dev *hr_dev, bool enable) +{ + struct device_node *dsaf_node; + struct device *dev = &hr_dev->pdev->dev; + struct device_node *np = dev->of_node; + int ret; + + dsaf_node = of_parse_phandle(np, "dsaf-handle", 0); + + if (!enable) { + ret = hns_dsaf_roce_reset(&dsaf_node->fwnode, false); + } else { + ret = hns_dsaf_roce_reset(&dsaf_node->fwnode, false); + if (ret) + return ret; + + msleep(SLEEP_TIME_INTERVAL); + ret = hns_dsaf_roce_reset(&dsaf_node->fwnode, true); + } + + return ret; +} + +struct hns_roce_hw hns_roce_hw_v1 = { + .reset = hns_roce_v1_reset, +}; diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v1.h b/drivers/infiniband/hw/hns/hns_roce_hw_v1.h new file mode 100644 index 0000000..a8c0c1d --- /dev/null +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v1.h @@ -0,0 +1,40 @@ +/* + * Copyright (c) 2016 Hisilicon Limited. + * + * This software is available to you under a choice of one of two + * licenses. You may choose to be licensed under the terms of the GNU + * General Public License (GPL) Version 2, available from the file + * COPYING in the main directory of this source tree, or the + * OpenIB.org BSD license below: + * + * Redistribution and use in source and binary forms, with or + * without modification, are permitted provided that the following + * conditions are met: + * + * - Redistributions of source code must retain the above + * copyright notice, this list of conditions and the following + * disclaimer. + * + * - Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following + * disclaimer in the documentation and/or other materials + * provided with the distribution. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +#ifndef _HNS_ROCE_HW_V1_H +#define _HNS_ROCE_HW_V1_H + +#define SLEEP_TIME_INTERVAL 20 + +extern int hns_dsaf_roce_reset(struct fwnode_handle *dsaf_fwnode, bool enable); + +#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); + 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); + ib_dealloc_device(&hr_dev->ib_dev); return 0;