On Fri, Feb 16, 2018 at 05:09:15PM +0530, Selvin Xavier wrote:
> On Fri, Feb 16, 2018 at 1:08 PM, Leon Romanovsky <leon@xxxxxxxxxx> wrote:
> > On Thu, Feb 15, 2018 at 09:20:11PM -0800, Selvin Xavier wrote:
> >> Avoid system crash when destroy_qp is invoked while
> >> the driver is processing the poll_cq. Synchronize these
> >> functions using the cq_lock.
> >>
> >> Signed-off-by: Selvin Xavier <selvin.xavier@xxxxxxxxxxxx>
> >> ---
> >> drivers/infiniband/hw/bnxt_re/ib_verbs.c | 39 +++++++++++++++++++++++++++++---
> >> drivers/infiniband/hw/bnxt_re/ib_verbs.h | 2 ++
> >> drivers/infiniband/hw/bnxt_re/qplib_fp.c | 21 +++++------------
> >> drivers/infiniband/hw/bnxt_re/qplib_fp.h | 4 +++-
> >> 4 files changed, 47 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> >> index b7eb104..9cb9928 100644
> >> --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> >> +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> >> @@ -786,20 +786,51 @@ int bnxt_re_query_ah(struct ib_ah *ib_ah, struct rdma_ah_attr *ah_attr)
> >> return 0;
> >> }
> >>
> >> +static unsigned long bnxt_re_lock_cqs(struct bnxt_re_qp *qp)
> >> + __acquires(&qp->scq->cq_lock) __acquires(&qp->rcq->cq_lock)
> >> +{
> >> + unsigned long flags;
> >> +
> >> + spin_lock_irqsave(&qp->scq->cq_lock, flags);
> >> + if (qp->rcq != qp->scq)
> >> + spin_lock(&qp->rcq->cq_lock);
> >> + else
> >> + __acquire(&qp->rcq->cq_lock);
> >
> > Sorry for naive question, why do you need __acquire/__release? And why
> > can't spin_is_locked(&qp->rcq->cq_lock) be used for unlock code?
> >
> I used __acquire and __release for satisfying the sparse. Sparse used to
> give warnings "context imbalance - wrong count at exit" since the locking
> and unlocking happens from two different functions. seen similar implementation
> in other drivers too.
I see, actually I think that sparse complain was right and you went to
far by wrapping standard calls. I see that it is very popular in bnxt_re
code and the problem with that, that it makes very hard to refactor such
code in case of some global change/improvement.
>
> I feel spin_is_locked also can be used.. but it doesn't avoid the RCQ
> != SCQ check.
> Also, if feel the code symmetry is better in this way.
> >
> >> +
> >> + return flags;
> >> +}
> >> +
> >> +static void bnxt_re_unlock_cqs(struct bnxt_re_qp *qp,
> >> + unsigned long flags)
> >> + __releases(&qp->scq->cq_lock) __releases(&qp->rcq->cq_lock)
> >> +{
> >> + if (qp->rcq != qp->scq)
> >> + spin_unlock(&qp->rcq->cq_lock);
> >> + else
> >> + __release(&qp->rcq->cq_lock);
> >> + spin_unlock_irqrestore(&qp->scq->cq_lock, flags);
> >> +}
> >> +
> >
> > Thanks
Attachment:
signature.asc
Description: PGP signature
