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

[Leon Romanovsky <leonro@mellanox.com>]

On Mon, May 25, 2020 at 01:42:15PM -0300, Jason Gunthorpe wrote:
> On Tue, May 19, 2020 at 03:43:34PM +0300, Yamin Friedman wrote:
>
> > +void ib_cq_pool_init(struct ib_device *dev)
> > +{
> > +	int i;
>
> I generally rather see unsigned types used for unsigned values
>
> > +
> > +	spin_lock_init(&dev->cq_pools_lock);
> > +	for (i = 0; i < ARRAY_SIZE(dev->cq_pools); i++)
> > +		INIT_LIST_HEAD(&dev->cq_pools[i]);
> > +}
> > +
> > +void ib_cq_pool_destroy(struct ib_device *dev)
> > +{
> > +	struct ib_cq *cq, *n;
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(dev->cq_pools); i++) {
> > +		list_for_each_entry_safe(cq, n, &dev->cq_pools[i],
> > +					 pool_entry) {
> > +			cq->shared = false;
> > +			ib_free_cq_user(cq, NULL);
>
> WARN_ON cqe_used == 0?

An opposite is better - WARN_ON(cqe_used).

<...>

> > @@ -1418,6 +1418,7 @@ int ib_register_device(struct ib_device *device, const char *name)
> >  		device->ops.dealloc_driver = dealloc_fn;
> >  		return ret;
> >  	}
> > +	ib_cq_pool_init(device);
> >  	ib_device_put(device);
>
> This look like wrong placement, it should be done before enable_device
> as enable_device triggers ULPs t start using the device and they might
> start allocating using this API.
>
> >  	return 0;
> > @@ -1446,6 +1447,7 @@ static void __ib_unregister_device(struct ib_device *ib_dev)
> >  	if (!refcount_read(&ib_dev->refcount))
> >  		goto out;
> >
> > +	ib_cq_pool_destroy(ib_dev);
> >  	disable_device(ib_dev);
>
> similar issue, should be after disable_device as ULPs are still
> running here

Sorry, this were my mistakes. I suggested to Yamin to put it here.

Thanks