Hello, On Mon, 18 Apr 2022 14:57:06 +0000 Saleem, Shiraz wrote: > > There is a deadlock in irdma_cleanup_cm_core(), which is shown > > below: > > > > (Thread 1) | (Thread 2) > > | irdma_schedule_cm_timer() > > irdma_cleanup_cm_core() | add_timer() > > spin_lock_irqsave() //(1) | (wait a time) > > ... | irdma_cm_timer_tick() > > del_timer_sync() | spin_lock_irqsave() //(2) > > (wait timer to stop) | ... > > > > We hold cm_core->ht_lock in position (1) of thread 1 and use del_timer_sync() > > to wait timer to stop, but timer handler also need cm_core->ht_lock in position (2) > > of thread 2. > > As a result, irdma_cleanup_cm_core() will block forever. > > > > This patch removes the check of timer_pending() in irdma_cleanup_cm_core(), > > because the del_timer_sync() function will just return directly if there isn't a > > pending timer. As a result, the lock is redundant, because there is no resource it > > could protect. > > > > Signed-off-by: Duoming Zhou <duoming@xxxxxxxxxx> > > --- > > Changes in V5: > > - Remove mod_timer() in irdma_schedule_cm_timer and irdma_cm_timer_tick. > > > > drivers/infiniband/hw/irdma/cm.c | 5 +---- > > 1 file changed, 1 insertion(+), 4 deletions(-) > > > > diff --git a/drivers/infiniband/hw/irdma/cm.c b/drivers/infiniband/hw/irdma/cm.c > > index dedb3b7edd8..4b6b1065f85 100644 > > --- a/drivers/infiniband/hw/irdma/cm.c > > +++ b/drivers/infiniband/hw/irdma/cm.c > > @@ -3251,10 +3251,7 @@ void irdma_cleanup_cm_core(struct > > irdma_cm_core *cm_core) > > if (!cm_core) > > return; > > > > - spin_lock_irqsave(&cm_core->ht_lock, flags); > > - if (timer_pending(&cm_core->tcp_timer)) > > - del_timer_sync(&cm_core->tcp_timer); > > - spin_unlock_irqrestore(&cm_core->ht_lock, flags); > > + del_timer_sync(&cm_core->tcp_timer); > > > > destroy_workqueue(cm_core->event_wq); > > cm_core->dev->ws_reset(&cm_core->iwdev->vsi); > > -- > > I am not sure the deadlock is possible practically since all CM nodes should be culled by the time we get to irdma_cleanup_cm_core. I think the deadlock is possible, because the timer is a delay mechanism that could execute at any time, although all CM nodes are culled. > However, timer_pending check and locks are redundant and should be removed. > > The subject line for patches to our driver are typically prefixed with "RDMA/irdma: " I sent "[PATCH V6] RDMA/irdma: Fix deadlock in irdma_cleanup_cm_core()" just now. Best regards, Duoming Zhou
