I don't think this is really a merge-blocker, but this code seems to have faith in the talismanic power of atomic_t to avoid races in nearby code. The following anti-pattern appears in a few other variants in the driver as well (just search for atomic_inc_return).
> + if (atomic_inc_return(&irp->relock_timer_active) == 1) { > + init_timer(&irp->relock_timer); > + irp->relock_timer.function = qib_run_relock; > + irp->relock_timer.data = (unsigned long) dd; > + irp->relock_interval = timeout; > + irp->relock_timer.expires = jiffies + timeout; > + add_timer(&irp->relock_timer); > + } else { > + irp->relock_interval = timeout; > + mod_timer(&irp->relock_timer, jiffies + timeout); > + atomic_dec(&irp->relock_timer_active); > + } Think about if two threads hit this code at the same time: if (atomic_inc_return(&active) == 1) { /* yep, first time through */ if (atomic_inc_return(&active) == 1) { /* nope, go to else */ } else { irp->relock_interval = timeout; /* err, who set up this timer? */ mod_timer(&irp->relock_timer, jiffies + timeout); atomic_dec(&irp->relock_timer_active); } /* a bit too late, the timer might already have run */ init_timer(&irp->relock_timer); irp->relock_timer.function = qib_run_relock; irp->relock_timer.data = (unsigned long) dd; irp->relock_interval = timeout; irp->relock_timer.expires = jiffies + timeout; add_timer(&irp->relock_timer); Now, maybe this is safe because the code is actually never running concurrently, but using this atomic_t relock_timer_active makes it really hard to see that this code is safe. Why not just use a proper lock for this, since it doesn't look particularly performance critical (and an atomic_t is really the same cost as a spinlock anyway)? As I said, not a merge-blocker but it would be nice to get this fixed someday before everyone loses interest in this driver. - R. -- Roland Dreier <rola...@cisco.com> || For corporate legal information go to: http://www.cisco.com/web/about/doing_business/legal/cri/index.html -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html