>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
