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 <[email protected]>
GPG KeyID: 0E572FDD
signature.asc
Description: OpenPGP digital signature
