Re: [PATCH V3 2/4] RDMA/core: Introduce shared CQ pool API

[Jason Gunthorpe <jgg@mellanox.com>]

On Mon, May 25, 2020 at 08:14:23AM -0700, Bart Van Assche wrote:
> On 2020-05-19 05:43, Yamin Friedman wrote:
> > +	/*
> > +	 * Allocated at least as many CQEs as requested, and otherwise
>            ^^^^^^^^^
>            allocate?
> 
> > +	spin_lock_irqsave(&dev->cq_pools_lock, flags);
> > +	list_splice(&tmp_list, &dev->cq_pools[poll_ctx - 1]);
> > +	spin_unlock_irqrestore(&dev->cq_pools_lock, flags);
> 
> Please add a WARN_ONCE() or WARN_ON_ONCE() statement that checks that
> poll_ctx >= 1.
> 
> > +struct ib_cq *ib_cq_pool_get(struct ib_device *dev, unsigned int nr_cqe,
> > +			     int comp_vector_hint,
> > +			     enum ib_poll_context poll_ctx)
> > +{
> > +	static unsigned int default_comp_vector;
> > +	int vector, ret, num_comp_vectors;
> > +	struct ib_cq *cq, *found = NULL;
> > +	unsigned long flags;
> > +
> > +	if (poll_ctx > ARRAY_SIZE(dev->cq_pools) || poll_ctx == IB_POLL_DIRECT)
> > +		return ERR_PTR(-EINVAL);
> 
> How about changing this into the following?
> 
> 	if ((unsigned)(poll_ctx - 1) >= ARRAY_SIZE(dev->cq_pools))
> 		return ...;
> 
> I think that change will make this code easier to verify.

Yuk also.. It would be alot better to re-order IB_POLL_DIRECT to the
end of the enum and use a IB_POLL_LAST_POOL_TYPE to exclude it
directly.

Jason