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
