Re: [PATCH rdma-rc 1/3] RDMA/hns: Fix the Oops during rmmod or insmod ko when reset occurs
On 2019/1/16 6:02, Jason Gunthorpe wrote:
> On Tue, Jan 15, 2019 at 09:48:01AM +0800, Wei Hu (Xavier) wrote:
>>
>> On 2019/1/15 6:06, Jason Gunthorpe wrote:
>>> On Sat, Jan 12, 2019 at 03:55:31PM +0800, Wei Hu (Xavier) wrote:
On 2019/1/12 5:34, Jason Gunthorpe wrote:
> On Thu, Jan 10, 2019 at 09:57:41PM +0800, Wei Hu (Xavier) wrote:
>> +/* Check the status of the current software reset process, if in
>> + * software reset process, wait until software reset process
>> finished,
>> + * in order to ensure that reset process and this function will
>> not call
>> + * __hns_roce_hw_v2_uninit_instance at the same time.
>> + * If a timeout occurs, it indicates that the network subsystem
>> has
>> + * encountered a serious error and cannot be recovered from the
>> reset
>> + * processing.
>> + */
>> +if (ops->ae_dev_resetting(handle)) {
>> +dev_warn(dev, "Device is busy in resetting state.
>> waiting.\n");
>> +end = msecs_to_jiffies(HNS_ROCE_V2_RST_PRC_MAX_TIME) +
>> jiffies;
>> +while (ops->ae_dev_resetting(handle) &&
>> + time_before(jiffies, end))
>> +msleep(20);
> Really? Does this have to be so ugly? Why isn't there just a simple
> lock someplace that is held during reset?
>
> I'm skeptical that all this strange looking stuff is properly locked
> and concurrency safe.
Hi, Jason
The hns3 NIC driver notifies the hns RoCE driver to perform
reset related processing by calling the .reset_notify() interface
registered by the RoCE driver.
There is a constraint on the hip08 chip, the NIC driver needs to
stop the flow before hardware startup reset, otherwise the chip
may hang up.
We've also thought about using locks, but found using locks can
lead to more serious problems because of that restriction of the
chip.
If using locks here, reset processing may wait for uninstallation
to complete, this may lead that NIC driver fails to stop the flow
in time in the reset process, thus causing the chip to hang up.
>>> If you are sleeping then I'm sure a lock can be used instead, how
>>> would it be any different?
>> Hi, Jason
>> If using locks here, reset process may wait until uninstallation to
>> complete,
>> it may trigger the chip constraint, causing chip to hang up.
>> But if using sleeping here, there will notthe case that reset
>> process wait until
>>uninstallation to complete, then will not trigger the chip
>> constraint.
> But how is this even right? If ops->ae_dev_resetting can change at any
> time, and you need to wait for it here, without locks can't it just
> change instantly after the if statement?
>
> I think it shows the concurrancy & locking is not done right when I
> see loops reading shared data and spinning on them with msleep.
Hi, Jason
Thanks for your comments,
We will modify the related process in hns NIC driver and delete checking
whether in the reset state and waiting for the reset to complete in
hns_roce_hw_v2_uninit_instance function, and will send patch V2 for
rdma-next branch. Thanks
Regards
Xavier
> Jason
>
> .
>
Re: [PATCH rdma-rc 1/3] RDMA/hns: Fix the Oops during rmmod or insmod ko when reset occurs
On Tue, Jan 15, 2019 at 09:48:01AM +0800, Wei Hu (Xavier) wrote:
>
>
> On 2019/1/15 6:06, Jason Gunthorpe wrote:
> > On Sat, Jan 12, 2019 at 03:55:31PM +0800, Wei Hu (Xavier) wrote:
> >>
> >> On 2019/1/12 5:34, Jason Gunthorpe wrote:
> >>> On Thu, Jan 10, 2019 at 09:57:41PM +0800, Wei Hu (Xavier) wrote:
> +/* Check the status of the current software reset process, if in
> + * software reset process, wait until software reset process
> finished,
> + * in order to ensure that reset process and this function will
> not call
> + * __hns_roce_hw_v2_uninit_instance at the same time.
> + * If a timeout occurs, it indicates that the network subsystem
> has
> + * encountered a serious error and cannot be recovered from the
> reset
> + * processing.
> + */
> +if (ops->ae_dev_resetting(handle)) {
> +dev_warn(dev, "Device is busy in resetting state.
> waiting.\n");
> +end = msecs_to_jiffies(HNS_ROCE_V2_RST_PRC_MAX_TIME) +
> jiffies;
> +while (ops->ae_dev_resetting(handle) &&
> + time_before(jiffies, end))
> +msleep(20);
> >>> Really? Does this have to be so ugly? Why isn't there just a simple
> >>> lock someplace that is held during reset?
> >>>
> >>> I'm skeptical that all this strange looking stuff is properly locked
> >>> and concurrency safe.
> >> Hi, Jason
> >>
> >> The hns3 NIC driver notifies the hns RoCE driver to perform
> >> reset related processing by calling the .reset_notify() interface
> >> registered by the RoCE driver.
> >>
> >> There is a constraint on the hip08 chip, the NIC driver needs to
> >> stop the flow before hardware startup reset, otherwise the chip
> >> may hang up.
> >>
> >> We've also thought about using locks, but found using locks can
> >> lead to more serious problems because of that restriction of the
> >> chip.
> >> If using locks here, reset processing may wait for uninstallation
> >> to complete, this may lead that NIC driver fails to stop the flow
> >> in time in the reset process, thus causing the chip to hang up.
> > If you are sleeping then I'm sure a lock can be used instead, how
> > would it be any different?
> Hi, Jason
> If using locks here, reset process may wait until uninstallation to
> complete,
> it may trigger the chip constraint, causing chip to hang up.
> But if using sleeping here, there will notthe case that reset
> process wait until
>uninstallation to complete, then will not trigger the chip
> constraint.
But how is this even right? If ops->ae_dev_resetting can change at any
time, and you need to wait for it here, without locks can't it just
change instantly after the if statement?
I think it shows the concurrancy & locking is not done right when I
see loops reading shared data and spinning on them with msleep.
Jason
Re: [PATCH rdma-rc 1/3] RDMA/hns: Fix the Oops during rmmod or insmod ko when reset occurs
On 2019/1/15 6:06, Jason Gunthorpe wrote:
> On Sat, Jan 12, 2019 at 03:55:31PM +0800, Wei Hu (Xavier) wrote:
>>
>> On 2019/1/12 5:34, Jason Gunthorpe wrote:
>>> On Thu, Jan 10, 2019 at 09:57:41PM +0800, Wei Hu (Xavier) wrote:
+ /* Check the status of the current software reset process, if in
+ * software reset process, wait until software reset process finished,
+ * in order to ensure that reset process and this function will not call
+ * __hns_roce_hw_v2_uninit_instance at the same time.
+ * If a timeout occurs, it indicates that the network subsystem has
+ * encountered a serious error and cannot be recovered from the reset
+ * processing.
+ */
+ if (ops->ae_dev_resetting(handle)) {
+ dev_warn(dev, "Device is busy in resetting state. waiting.\n");
+ end = msecs_to_jiffies(HNS_ROCE_V2_RST_PRC_MAX_TIME) + jiffies;
+ while (ops->ae_dev_resetting(handle) &&
+ time_before(jiffies, end))
+ msleep(20);
>>> Really? Does this have to be so ugly? Why isn't there just a simple
>>> lock someplace that is held during reset?
>>>
>>> I'm skeptical that all this strange looking stuff is properly locked
>>> and concurrency safe.
>> Hi, Jason
>>
>> The hns3 NIC driver notifies the hns RoCE driver to perform
>> reset related processing by calling the .reset_notify() interface
>> registered by the RoCE driver.
>>
>> There is a constraint on the hip08 chip, the NIC driver needs to
>> stop the flow before hardware startup reset, otherwise the chip
>> may hang up.
>>
>> We've also thought about using locks, but found using locks can
>> lead to more serious problems because of that restriction of the
>> chip.
>> If using locks here, reset processing may wait for uninstallation
>> to complete, this may lead that NIC driver fails to stop the flow
>> in time in the reset process, thus causing the chip to hang up.
> If you are sleeping then I'm sure a lock can be used instead, how
> would it be any different?
Hi, Jason
If using locks here, reset process may wait until uninstallation to
complete,
it may trigger the chip constraint, causing chip to hang up.
But if using sleeping here, there will notthe case that reset
process wait until
uninstallation to complete, then will not trigger the chip
constraint.
Thanks
reset process
lock
unlock
uninitallation
lock
...
unlock
Regards
Xavier
> Jason
>
> .
>
Re: [PATCH rdma-rc 1/3] RDMA/hns: Fix the Oops during rmmod or insmod ko when reset occurs
On Sat, Jan 12, 2019 at 03:55:31PM +0800, Wei Hu (Xavier) wrote:
>
>
> On 2019/1/12 5:34, Jason Gunthorpe wrote:
> > On Thu, Jan 10, 2019 at 09:57:41PM +0800, Wei Hu (Xavier) wrote:
> >> + /* Check the status of the current software reset process, if in
> >> + * software reset process, wait until software reset process finished,
> >> + * in order to ensure that reset process and this function will not call
> >> + * __hns_roce_hw_v2_uninit_instance at the same time.
> >> + * If a timeout occurs, it indicates that the network subsystem has
> >> + * encountered a serious error and cannot be recovered from the reset
> >> + * processing.
> >> + */
> >> + if (ops->ae_dev_resetting(handle)) {
> >> + dev_warn(dev, "Device is busy in resetting state. waiting.\n");
> >> + end = msecs_to_jiffies(HNS_ROCE_V2_RST_PRC_MAX_TIME) + jiffies;
> >> + while (ops->ae_dev_resetting(handle) &&
> >> + time_before(jiffies, end))
> >> + msleep(20);
> > Really? Does this have to be so ugly? Why isn't there just a simple
> > lock someplace that is held during reset?
> >
> > I'm skeptical that all this strange looking stuff is properly locked
> > and concurrency safe.
> Hi, Jason
>
> The hns3 NIC driver notifies the hns RoCE driver to perform
> reset related processing by calling the .reset_notify() interface
> registered by the RoCE driver.
>
> There is a constraint on the hip08 chip, the NIC driver needs to
> stop the flow before hardware startup reset, otherwise the chip
> may hang up.
>
> We've also thought about using locks, but found using locks can
> lead to more serious problems because of that restriction of the
> chip.
> If using locks here, reset processing may wait for uninstallation
> to complete, this may lead that NIC driver fails to stop the flow
> in time in the reset process, thus causing the chip to hang up.
If you are sleeping then I'm sure a lock can be used instead, how
would it be any different?
Jason
Re: [PATCH rdma-rc 1/3] RDMA/hns: Fix the Oops during rmmod or insmod ko when reset occurs
On 2019/1/12 5:34, Jason Gunthorpe wrote:
> On Thu, Jan 10, 2019 at 09:57:41PM +0800, Wei Hu (Xavier) wrote:
>> +/* Check the status of the current software reset process, if in
>> + * software reset process, wait until software reset process finished,
>> + * in order to ensure that reset process and this function will not call
>> + * __hns_roce_hw_v2_uninit_instance at the same time.
>> + * If a timeout occurs, it indicates that the network subsystem has
>> + * encountered a serious error and cannot be recovered from the reset
>> + * processing.
>> + */
>> +if (ops->ae_dev_resetting(handle)) {
>> +dev_warn(dev, "Device is busy in resetting state. waiting.\n");
>> +end = msecs_to_jiffies(HNS_ROCE_V2_RST_PRC_MAX_TIME) + jiffies;
>> +while (ops->ae_dev_resetting(handle) &&
>> + time_before(jiffies, end))
>> +msleep(20);
> Really? Does this have to be so ugly? Why isn't there just a simple
> lock someplace that is held during reset?
>
> I'm skeptical that all this strange looking stuff is properly locked
> and concurrency safe.
Hi, Jason
The hns3 NIC driver notifies the hns RoCE driver to perform
reset related processing by calling the .reset_notify() interface
registered by the RoCE driver.
There is a constraint on the hip08 chip, the NIC driver needs to
stop the flow before hardware startup reset, otherwise the chip
may hang up.
We've also thought about using locks, but found using locks can
lead to more serious problems because of that restriction of the
chip.
If using locks here, reset processing may wait for uninstallation
to complete, this may lead that NIC driver fails to stop the flow
in time in the reset process, thus causing the chip to hang up.
Regards
Xavier
> Also, this series seems a bit big for -rc
>
> Jason
>
> .
>
Re: [PATCH rdma-rc 1/3] RDMA/hns: Fix the Oops during rmmod or insmod ko when reset occurs
On Thu, Jan 10, 2019 at 09:57:41PM +0800, Wei Hu (Xavier) wrote:
> + /* Check the status of the current software reset process, if in
> + * software reset process, wait until software reset process finished,
> + * in order to ensure that reset process and this function will not call
> + * __hns_roce_hw_v2_uninit_instance at the same time.
> + * If a timeout occurs, it indicates that the network subsystem has
> + * encountered a serious error and cannot be recovered from the reset
> + * processing.
> + */
> + if (ops->ae_dev_resetting(handle)) {
> + dev_warn(dev, "Device is busy in resetting state. waiting.\n");
> + end = msecs_to_jiffies(HNS_ROCE_V2_RST_PRC_MAX_TIME) + jiffies;
> + while (ops->ae_dev_resetting(handle) &&
> +time_before(jiffies, end))
> + msleep(20);
Really? Does this have to be so ugly? Why isn't there just a simple
lock someplace that is held during reset?
I'm skeptical that all this strange looking stuff is properly locked
and concurrency safe.
Also, this series seems a bit big for -rc
Jason
[PATCH rdma-rc 1/3] RDMA/hns: Fix the Oops during rmmod or insmod ko when reset occurs
In the reset process, the hns3 NIC driver notifies the RoCE driver
to perform reset related processing by calling the .reset_notify()
interface registered by the RoCE driver in hip08 SoC.
In the current version, if a reset occurs simultaneously during
the execution of rmmod or insmod ko, there may be Oops error as below:
[ 650.375307] Internal error: Oops: 8607 [#1] PREEMPT SMP
[ 650.381223] Modules linked in: hns_roce(O) hns3(O) hclge(O) hnae3(O) [last
unloaded: hns_roce_hw_v2]
[ 650.391279] CPU: 0 PID: 14 Comm: kworker/0:1 Tainted: G O
4.19.0-ge00d540 #1
[ 650.399875] Hardware name: Huawei Technologies Co., Ltd.
[ 650.411393] Workqueue: events hclge_reset_service_task [hclge]
[ 650.417576] pstate: 60c9 (nZCv daif +PAN +UAO)
[ 650.422618] pc : 0x0100b0b8
[ 650.426282] lr : 0x0100aea0
[ 650.429914] sp : 09afbab0
[ 650.433388] x29: 09afbab0 x28: 0800
[ 650.439012] x27: 7ff0 x26: 80002f90c004
[ 650.444599] x25: 07ff x24: 08f97000
[ 650.450165] x23: 80003efee0a8 x22: 1000
[ 650.455730] x21: 80002f917ff0 x20: 8000286ea070
[ 650.461289] x19: 0800 x18: 0400
[ 650.466868] x17: c4d3225d x16: 21b8
[ 650.472423] x15: 0400 x14: 0400
[ 650.477990] x13: x12: 80003fac6e30
[ 650.483552] x11: 800036303000 x10: 0001
[ 650.489103] x9 : x8 : 80003016d000
[ 650.494662] x7 : x6 : 003f
[ 650.500218] x5 : 0040 x4 :
[ 650.505783] x3 : 0004 x2 : 07ff
[ 650.511349] x1 : x0 :
[ 650.517002] Process kworker/0:1 (pid: 14, stack limit = 0xaf8f0ad9)
[ 650.524164] Call trace:
[ 650.526814] 0x0100b0b8
[ 650.530108] 0x0100b3a0
[ 650.534336] hns_roce_init+0x624/0xc88 [hns_roce]
[ 650.539196] 0x01002df8
[ 650.542488] 0x01006960
[ 650.546032] hclge_notify_roce_client+0x74/0xe0 [hclge]
[ 650.551637] hclge_reset_service_task+0xa58/0xbc0 [hclge]
[ 650.557479] process_one_work+0x1e4/0x458
[ 650.561738] worker_thread+0x40/0x450
[ 650.565662] kthread+0x12c/0x130
[ 650.569247] ret_from_fork+0x10/0x18
[ 650.573235] Code: bad PC value
[ 650.576663] ---[ end trace 97e6a2fe783337a8 ]---
In the reset process, we will release the resources firstly,
and after the hardware reset is completed, we will reapply
resources and reconfigure the hardware. We can solve this
bug by avoiding simultaneous installation or uninstallation
and reset process.
Fixes: cb7a94c9c808 ("RDMA/hns: Add reset process for RoCE in hip08")
Signed-off-by: Wei Hu (Xavier)
---
drivers/infiniband/hw/hns/hns_roce_device.h | 21 +
drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 136 +---
drivers/infiniband/hw/hns/hns_roce_hw_v2.h | 7 ++
3 files changed, 151 insertions(+), 13 deletions(-)
diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h
b/drivers/infiniband/hw/hns/hns_roce_device.h
index 509e467..f4cac63 100644
--- a/drivers/infiniband/hw/hns/hns_roce_device.h
+++ b/drivers/infiniband/hw/hns/hns_roce_device.h
@@ -216,6 +216,26 @@ enum {
HNS_ROCE_DB_PER_PAGE = PAGE_SIZE / 4
};
+enum hns_roce_reset_stage {
+ HNS_ROCE_STATE_NON_RST,
+ HNS_ROCE_STATE_RST_BEF_DOWN,
+ HNS_ROCE_STATE_RST_DOWN,
+ HNS_ROCE_STATE_RST_UNINIT,
+ HNS_ROCE_STATE_RST_INIT,
+ HNS_ROCE_STATE_RST_INITED,
+};
+
+enum hns_roce_instance_state {
+ HNS_ROCE_STATE_NON_INIT,
+ HNS_ROCE_STATE_INIT,
+ HNS_ROCE_STATE_INITED,
+ HNS_ROCE_STATE_UNINIT,
+};
+
+enum {
+ HNS_ROCE_RST_DIRECT_RETURN = 0,
+};
+
#define HNS_ROCE_CMD_SUCCESS 1
#define HNS_ROCE_PORT_DOWN 0
@@ -898,6 +918,7 @@ struct hns_roce_dev {
spinlock_t bt_cmd_lock;
boolactive;
boolis_reset;
+ unsigned long reset_cnt;
struct hns_roce_ib_iboe iboe;
struct list_headpgdir_list;
diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
index 3a66945..dadb685 100644
--- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
+++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
@@ -5800,6 +5800,7 @@ static int hns_roce_v2_post_srq_recv(struct ib_srq *ibsrq,
static int hns_roce_hw_v2_get_cfg(struct hns_roce_dev *hr_dev,
struct hnae3_handle *handle)
{
+ struct hns_roce_v2_priv *priv = hr_dev->priv;
const struct pci_device_id *id;
int i;
@@ -5830,10 +5831,13 @@ static int hns_roce_hw_v2_get_cfg(struct hns_roce_dev
*hr_dev,
hr_dev->cmd_mod = 1;
hr_dev->loop_idc = 0;
+ hr_dev->reset_cnt =
