Re: [PATCH v3 4/5] dma: cppi41: only allocate descriptor memory once

[Daniel Mack <zonque@gmail.com>]

Hi Sebastian,

sorry for the long delay, I got distracted by other things.


On 26.09.2013 10:26, Sebastian Andrzej Siewior wrote:
> * Daniel Mack | 2013-09-22 16:50:03 [+0200]:
> 
>> cdd->cd and cdd->descs_phys are allocated DESCS_AREAS times from
>> init_descs() and freed as often from purge_descs(). This leads to both
>> memory leaks and double-frees.
>>
>> Fix this by pulling the calls to dma_{alloc,free}_coherent() out of the
>> loops.
>>
>> While at it, remove the intermediate variable mem_decs (I guess it was
>> only there to make the code comply to the 80-chars CodingSytle rule).
>>
>> Signed-off-by: Daniel Mack <zonque@gmail.com>
> 
> Please don't merge the memory descriptors. The idea was initially to
> allocate multiple small descriptors instead one big. The descrriptor
> turned out to be enough so it looks like the way it looks.
> If you want to clean this up, please either remove the for loop and
> allocate only one memory area or please prepare for multiple descripors
> (I think two is the upper limit).

Well, I didn't merge the descriptors. Look again at my changes please.

A simplified version of the code as it stands is:

  for (i = 0; i < DESCS_AREAS; i++)
    cdd->cd = dma_alloc_coherent(dev, ..., &cdd->descs_phys, GFP_KERNEL);

  for (i = 0; i < DESCS_AREAS; i++)
    dma_free_coherent(dev, mem_decs, cdd->cd, cdd->descs_phys);

So you're effectively allocating and freeing the same pointer
DESCS_AREAS times, which is certainly not what you wanted.

And this just doesn't hit you because DESCS_AREAS is always 1:

  BUILD_BUG_ON(DESCS_AREAS != 1);


So, after all, my patch doesn't really change any of the runtime
behaviour. Consider it a cosmetic cleanup if you wish :)


Thanks,
Daniel