On Mon, Feb 19, 2018 at 03:11:49PM -0500, Doug Ledford wrote:
> On Fri, 2018-02-16 at 17:51 +0200, Leon Romanovsky wrote:
> > > >> +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.
>
> Leon, are you actually objecting here, or just noting that you would
> prefer a different style?
Sometimes, it makes sense to provide wrappers e.g. different get/put,
to_..(), from_..()) calls for annotate ambiguous ++/--. But locks
are different, they need to be clear and can't be hided.
So yes, I'm objecting here for this lock/unlock wrapper and would like
to see more pushback from maintainers on extensive usage of ridiculous
wrappers (including mlx code).
Thanks
>
> --
> Doug Ledford <dledford@xxxxxxxxxx>
> GPG KeyID: B826A3330E572FDD
> Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
Attachment:
signature.asc
Description: PGP signature
