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

[Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>]

On 10/01/2013 03:09 PM, Daniel Mack wrote:
> Hi Sebastian,

Hi Daniel,

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

No problem.

> 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.

Well, no but since DESCS_AREAS is 1 it does not matter.

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

exactly.

> 
>   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 :)

But it looks strange (in my opinion at least). But now, that I look
again at it yes you moved the alloc out of the loop and replaced
mem_decs with the computation. So nothing changed but you moved it
outside.

Right I think one desc area should be enough so I would just remove the
for loop and DESCS_AREAS as well. How does this sound to you?

> 
> 
> Thanks,
> Daniel
> 

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html