On 10/31/2018 10:24 AM, Jason Gunthorpe wrote:
> On Wed, Oct 31, 2018 at 02:42:38PM +0200, Shamir Rabinovitch wrote:
>> Bypassing ib_core leave the restrack and other fields uninitialized.
>>
>> Signed-off-by: Shamir Rabinovitch <shamir.rabinovitch@xxxxxxxxxx>
>> drivers/infiniband/hw/mlx5/main.c | 30 +++++++++++++++---------------
>> 1 file changed, 15 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
>> index be701d4..97b44e9 100644
>> +++ b/drivers/infiniband/hw/mlx5/main.c
>> @@ -4565,26 +4565,19 @@ static int create_dev_resources(struct mlx5_ib_resources *devr)
>>
>> mutex_init(&devr->mutex);
>>
>> - devr->p0 = mlx5_ib_alloc_pd(&dev->ib_dev, NULL, NULL);
>> + /* must call ib_core here to initialize restrack! */
>> + devr->p0 = ib_alloc_pd(&dev->ib_dev, 0);
>> if (IS_ERR(devr->p0)) {
>> ret = PTR_ERR(devr->p0);
>> goto error0;
>> }
>> - devr->p0->device = &dev->ib_dev;
>> - devr->p0->uobject = NULL;
>> - atomic_set(&devr->p0->usecnt, 0);
>>
>> - devr->c0 = mlx5_ib_create_cq(&dev->ib_dev, &cq_attr, NULL, NULL);
>> + /* must call ib_core here to initialize restrack! */
>> + devr->c0 = ib_create_cq(&dev->ib_dev, NULL, NULL, NULL, &cq_attr);
>> if (IS_ERR(devr->c0)) {
>> ret = PTR_ERR(devr->c0);
>> goto error1;
>> }
>
> I think the issue here is that the device is not yet registered at
> this point.
>
>> @@ -4662,14 +4656,16 @@ static int create_dev_resources(struct mlx5_ib_resources *devr)
>>
>> error5:
>> mlx5_ib_destroy_srq(devr->s0);
>> + atomic_dec(&devr->p0->usecnt);
>> + atomic_dec(&devr->s0->ext.cq->usecnt);
>> error4:
>> mlx5_ib_dealloc_xrcd(devr->x1);
>> error3:
>> mlx5_ib_dealloc_xrcd(devr->x0);
>> error2:
>> - mlx5_ib_destroy_cq(devr->c0);
>> + ib_destroy_cq(devr->c0);
>> error1:
>> - mlx5_ib_dealloc_pd(devr->p0);
>> + ib_dealloc_pd(devr->p0);
>> error0:
>> return ret;
>> }
>> @@ -4681,11 +4677,15 @@ static void destroy_dev_resources(struct mlx5_ib_resources *devr)
>> int port;
>>
>> mlx5_ib_destroy_srq(devr->s1);
>> + atomic_dec(&devr->p0->usecnt);
>> + atomic_dec(&devr->s1->ext.cq->usecnt);
>> mlx5_ib_destroy_srq(devr->s0);
>> + atomic_dec(&devr->p0->usecnt);
>> + atomic_dec(&devr->s0->ext.cq->usecnt);
>> mlx5_ib_dealloc_xrcd(devr->x0);
>> mlx5_ib_dealloc_xrcd(devr->x1);
>> - mlx5_ib_destroy_cq(devr->c0);
>> - mlx5_ib_dealloc_pd(devr->p0);
>> + ib_destroy_cq(devr->c0);
>> + ib_dealloc_pd(devr->p0);
>
> and has been de-registered by this point.
>
> Calling ib_* functions on an unregistered device is not a good idea.
>
> Mark?
Yep, I don't like it, any change to the create/destroy functions
will now need to take into account that:"Don't play with struct ib_device too much
as mlx5 does some funky stuff before the device is registered"
Seems like a bad idea.
>
> Jason
>
Mark
