On 07/08/2015 12:23 AM, Maninder Singh wrote:
> Hello,
>
>>> + for (i = 0; i < dev->caps.num_ports; i++)
>>> + kfree(dm[i]);
>>> goto out;
>>> }
>>> }
>>> --
>>> 1.7.9.5
>>>
>>
>> If you are going to change this, you might as well make it 100% correct:
>>
>> i—-;
>> while (i >= 0)
>> kfree(dm[i]);
>>
>> Then you don’t have to worry about whether kfree works on NULL, every item you free will be guaranteed to be non-NULL.
> Thanks for suggestion :)
> Sent new patch with described changes, I was thinking one more thing.
>
> In below code :-
> if (!ibdev->sriov.is_going_down)
> queue_work(ibdev->sriov.demux[i].ud_wq, &dm[i]->work);
> spin_unlock_irqrestore(&ibdev->sriov.going_down_lock, flags);
> }
> out:
> kfree(dm);
> return;
>
> dm is freed after queue_work, is it correct to free dm when other dm[i] are allocated ? i did not get it.
The dm is just there to give an easy way to refer to a variable number
of work structs. The flow is supposed to be something like this:
alloc(dm)
for(i=0;i<num_qps;i++)
dm[i] == alloc(work item);
for(i=0;i<num_qps;i++)
init dm[i] work item
queue dm[i] work item
free(dm)
In this scenario, all of the dm[i] items should be queued to delayed
work. When that work completes, it should then free these structs. So,
yes, the dm variable itself is just a temporary means of keeping all
those work struct pointers together. However, your question caused me
to look closely here, and I see that there is a bug. In particular, if
we check the sriov.is_going_down and as a result *don't* queue a work
item, then we end up leaking that work struct. In addition, I think
there is room to optimize this routine considerably. I'll post a patch
for that in a minute.
--
Doug Ledford <dledford@xxxxxxxxxx>
GPG KeyID: 0E572FDD
Attachment:
signature.asc
Description: OpenPGP digital signature
