>Subject: Re: [PATCH for-rc] RDMA/qder: Fix memory leak of iwcm pointer
>
>
>
>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).
>
Ah ok! Go ahead and post it then. I ll back off. Thanks!
