On Wed, Dec 04, 2013 at 03:24:18PM +0800, ZHAO Gang wrote: > The original code allocate rx dma memory in several dma_alloc_coherent calls, > which causes some problems: > 1. since dma_alloc_coherent allocate at least one page memory, it wastes some > memory when allocation size is smaller than one page. > 2. it causes et131x_rx_dma_memory_free as complex as et131x_rx_dma_memory_alloc > > Instead, allocate all rx dma memory in one dma_alloc_coherent call makes less code, > makes it easy to handle dma allocation error, and makes et131x_rx_dma_memory_free > as simple as it could be. > > Signed-off-by: ZHAO Gang <gamerh2o@xxxxxxxxx> > --- > drivers/staging/et131x/et131x.c | 202 ++++++++++++---------------------------- > 1 file changed, 59 insertions(+), 143 deletions(-) Hi Zhao, I haven't had time to look at these last few patches in detail yet, but I do have some comments - The code is an improvement in that it reduces the number of lines of code, but you're also making the driver more susceptible to memory allocation issues, as you're asking for a single large block of memory and not several smaller ones. I'm not sure if that will have an appreciable impact yet. Also, I think a better way to do this allocation would be to create a single struct with the items to be allocated, where as few as possible of the struct members are of dynamic size (drivers/net/ethernet/cirrus/ep93xx_eth.c is a nice example of this) - although this reason alone would not be enough to reject the alloc patches, but the previous reason might. Cheers, Mark _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
