On 4/25/19 12:10 AM, Saleem, Shiraz wrote:
>> Subject: Re: [PATCH for-rc] RDMA/qder: Fix memory leak of iwcm pointer
>>
>> On Tue, Apr 23, 2019 at 07:44:51PM +0300, Kamal Heib wrote:
>>>>> dealloc_driver() is something that introduced recently and I'm
>>>>> interested in backport this fix to RHEL versions that don't include
>>>>> the dealloc_driver(), that's why I'm targeting this fix to for-rc.
>>>>
>>>> Distro constraints are not really a reason to do something sub
>>>> optimal upstream...
>>>
>>> OK, What about -stable upstream versions?
>>
>> Greg takes dependent patches generally if asked
>>
>>>>>>> I think that we need to define a new interface in the core
>>>>>>> (iwcm.c) for the IWARP devices to use when {allocate, setting ops
>>>>>>> to, release} iwcm, what do you think?
>>>>>>
>>>>>> Why?
>>>>>>
>>>>>
>>>>> 1- Avoid bugs like this.
>>>>> 2- Reduce code duplication in the providers.
>>>>> 3- Make the providers code more cleaner.
>>>>>
>>>>> I'm suggesting to introduce the following two functions and modify
>>>>> the IWARP providers to use them.
>>>>>
>>>>> int iw_cm_register_device(struct ib_device *ibdev, char *ifname,
>>>>> const struct iw_cm_ops *ops)
>>>>> {
>>>>> ibdev->iw_cm_dev = kzalloc(sizeof(*ibdev->iw_cm_dev), GFP_KERNEL);
>>>>> if (!ibdev->iw_cm_dev)
>>>>> return -ENOMEM;
>>>>>
>>>>> ibdev->iw_cm_dev->ops = ops;
>>>>> memcpy(ibdev->iw_cm_dev->ifname, ifname,
>>>>> sizeof(ibdev->iw_cm_dev->ifname));
>>>>
>>>> Why not just have this as part of the normal register process and
>>>> pass the iw_cm_ops through the existing ops structure?
>>>>
>>>
>>> Do you mean to pass the ifname as a parameter to
>>> ib_register_device()
>>
>> No, drivers can set it before calling register_device - not really sure what it is for.
>>
>>> and modify the ib_device_ops to include the following callbacks from
>>> iw_cm_verbs?
>>
>> That or a pointer to a struct with them, yes.
>>
>>> If that is the case then we can add the "ifname" & "driver_flags" to
>>> the ib_device struct and remove the iw_cm_verbs struct, which will
>>> mean that no need to implement the dealloc_driver() callback, because
>>> the iwcm pointer isn't allocated.
>>
>> Yes, this makes more sense since there is really nothing in this struct except ops
>> function pointers which belong in the ops struct anyhow.
>>
>
> Jason/Kamal - This was brought up during the irdma RFC review as well and we already have a patch
> for this. Can send it out tomorrow.
>
> Shiraz
>
Hi Shiraz,
I already prepared the patch and I'm doing some testing for it right now (before
posting it).
Thanks,
Kamal
