On Wed, Nov 27, 2013 at 6:06 PM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> Ok, yeah. This patch is the right thing. I had a couple minor style
> complaints.
I am happy to hear this.
>
> On Wed, Nov 27, 2013 at 03:45:12PM +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 | 219 +++++++++++++---------------------------
>> 1 file changed, 72 insertions(+), 147 deletions(-)
>>
>> diff --git a/drivers/staging/et131x/et131x.c b/drivers/staging/et131x/et131x.c
>> index 27da1db..409949f 100644
>> --- a/drivers/staging/et131x/et131x.c
>> +++ b/drivers/staging/et131x/et131x.c
>> @@ -304,6 +304,7 @@ struct rx_ring {
>> u32 num_ready_recv;
>>
>> u32 num_rfd;
>> + u32 dma_size;
>>
>> bool unfinished_receives;
>> };
>> @@ -2186,21 +2187,16 @@ static inline u32 bump_free_buff_ring(u32 *free_buff_ring, u32 limit)
>> return tmp_free_buff_ring;
>> }
>>
>> -/* et131x_rx_dma_memory_alloc
>> - * @adapter: pointer to our private adapter structure
>> - *
>> - * Returns 0 on success and errno on failure (as defined in errno.h)
>> - *
>> - * Allocates Free buffer ring 1 for sure, free buffer ring 0 if required,
>> - * and the Packet Status Ring.
>> - */
>> static int et131x_rx_dma_memory_alloc(struct et131x_adapter *adapter)
>> {
>> u8 id;
>> u32 i, j;
>> - u32 bufsize;
>> - u32 pktstat_ringsize;
>> - u32 fbr_chunksize;
>> + u32 dma_size;
>> + u32 fbr_size;
>> + u32 pktstat_ring_size;
>> + u32 fbr_chunk_size;
>
> Get rid the fbr_chunk_size variable.
>
I will do this change.
>> + dma_addr_t dma_addr;
>> + void *virt_addr;
>> struct rx_ring *rx_ring;
>> struct fbr_lookup *fbr;
>>
>> @@ -2208,8 +2204,11 @@ static int et131x_rx_dma_memory_alloc(struct et131x_adapter *adapter)
>> rx_ring = &adapter->rx_ring;
>>
>> /* Alloc memory for the lookup table */
>> - rx_ring->fbr[0] = kmalloc(sizeof(struct fbr_lookup), GFP_KERNEL);
>> - rx_ring->fbr[1] = kmalloc(sizeof(struct fbr_lookup), GFP_KERNEL);
>> + rx_ring->fbr[0] = kmalloc(sizeof(struct fbr_lookup) * 2, GFP_KERNEL);
>> + if (!rx_ring->fbr[0])
>> + return -ENOMEM;
>> +
>> + rx_ring->fbr[1] = rx_ring->fbr[0] + 1;
>>
>> /* The first thing we will do is configure the sizes of the buffer
>> * rings. These will change based on jumbo packet support. Larger
>
> Don't do this...
The reason I have said in previous reply.
>
>> @@ -2246,48 +2245,53 @@ static int et131x_rx_dma_memory_alloc(struct et131x_adapter *adapter)
>> rx_ring->fbr[1]->num_entries = 128;
>> }
>>
>> - adapter->rx_ring.psr_num_entries =
>> - adapter->rx_ring.fbr[0]->num_entries +
>> - adapter->rx_ring.fbr[1]->num_entries;
>> + rx_ring->psr_num_entries =
>> + rx_ring->fbr[0]->num_entries + rx_ring->fbr[1]->num_entries;
>> +
>> + dma_size = sizeof(struct fbr_desc) * rx_ring->psr_num_entries;
>> +
>> + pktstat_ring_size =
>> + sizeof(struct pkt_stat_desc) * rx_ring->psr_num_entries;
>> +
>> + dma_size += pktstat_ring_size;
>> + dma_size += sizeof(struct rx_status_block);
>>
>
> Calculate pktstat_ring_size before dma_size so it's not jumbled
> together.
>
>
> rx_ring->psr_num_entries = rx_ring->fbr[0]->num_entries +
> rx_ring->fbr[1]->num_entries;
> pktstat_ring_size = sizeof(struct pkt_stat_desc) *
> rx_ring->psr_num_entries;
>
> dma_size = sizeof(struct fbr_desc) * rx_ring->psr_num_entries;
> dma_size += pktstat_ring_size;
> dma_size += sizeof(struct rx_status_block);
> for (id = 0; id < NUM_FBRS; id++)
> dma_size += FBR_CHUNKS * rx_ring->fbr[id]->buffsize;
>
> rx_ring->dma_size = dma_size;
>
>
I will do these changes.
>> for (id = 0; id < NUM_FBRS; id++) {
>> - fbr = rx_ring->fbr[id];
>> + fbr_chunk_size = FBR_CHUNKS * rx_ring->fbr[id]->buffsize;
>> + dma_size += fbr_chunk_size;
>> + }
>>
>> - /* Allocate an area of memory for Free Buffer Ring */
>> - bufsize = sizeof(struct fbr_desc) * fbr->num_entries;
>> - fbr->ring_virtaddr = dma_alloc_coherent(&adapter->pdev->dev,
>> - bufsize,
>> - &fbr->ring_physaddr,
>> - GFP_KERNEL);
>> + rx_ring->dma_size = dma_size;
>>
>> - if (!fbr->ring_virtaddr) {
>> - dev_err(&adapter->pdev->dev,
>> - "Cannot alloc memory for Free Buffer Ring %d\n", id);
>> - return -ENOMEM;
>> - }
>> + virt_addr = dma_alloc_coherent(&adapter->pdev->dev,
>> + dma_size,
>> + &dma_addr,
>> + GFP_KERNEL);
>> +
>> + if (!virt_addr) {
>
> Don't put a blank line between the allocation and the check.
>
Got it.
>> + kfree(rx_ring->fbr[0]);
>> + dev_err(&adapter->pdev->dev,
>> + "Cannot allocate memory for Rx ring\n");
>> + return -ENOMEM;
>> }
>>
>> + /* Now we distribute the pie */
>> for (id = 0; id < NUM_FBRS; id++) {
>> fbr = rx_ring->fbr[id];
>> - fbr_chunksize = (FBR_CHUNKS * fbr->buffsize);
>> + fbr->ring_virtaddr = virt_addr;
>> + fbr->ring_physaddr = dma_addr;
>> + /* Update the pointer */
>
> These "update the pointer" comments are not needed (obvious).
>
Got it.
>> + fbr_size = sizeof(struct fbr_desc) * fbr->num_entries;
>> + virt_addr += fbr_size;
>> + dma_addr += fbr_size;
>>
>> for (i = 0; i < (fbr->num_entries / FBR_CHUNKS); i++) {
>> - dma_addr_t fbr_tmp_physaddr;
>> -
>> - fbr->mem_virtaddrs[i] =
>> - dma_alloc_coherent(&adapter->pdev->dev,
>> - fbr_chunksize,
>> - &fbr->mem_physaddrs[i],
>> - GFP_KERNEL);
>> -
>> - if (!fbr->mem_virtaddrs[i]) {
>> - dev_err(&adapter->pdev->dev,
>> - "Could not alloc memory\n");
>> - return -ENOMEM;
>> - }
>> -
>> - /* See NOTE in "Save Physical Address" comment above */
>> - fbr_tmp_physaddr = fbr->mem_physaddrs[i];
>> + fbr->mem_virtaddrs[i] = virt_addr;
>> + fbr->mem_physaddrs[i] = dma_addr;
>> + /* Update the pointer */
>> + fbr_chunk_size = FBR_CHUNKS * fbr->buffsize;
>> + virt_addr += fbr_chunk_size;
>> + /* dma_addr is updated below */
>
> It might be clearer to move both updates inside the loop.
>
>>
>> for (j = 0; j < FBR_CHUNKS; j++) {
>> u32 index = (i * FBR_CHUNKS) + j;
>> @@ -2302,51 +2306,31 @@ static int et131x_rx_dma_memory_alloc(struct et131x_adapter *adapter)
>> * descriptor so the device can access it
>> */
>> fbr->bus_high[index] =
>> - upper_32_bits(fbr_tmp_physaddr);
>> + upper_32_bits(dma_addr);
>> fbr->bus_low[index] =
>> - lower_32_bits(fbr_tmp_physaddr);
>> -
>> - fbr_tmp_physaddr += fbr->buffsize;
>> + lower_32_bits(dma_addr);
>
> These two can don't need the extra line break. They fit in 80 characters now.
>
> fbr->bus_high[index] = upper_32_bits(dma_addr);
> fbr->bus_low[index] = lower_32_bits(dma_addr);
>
Got it.
>> + /* Update the pointer */
>> + dma_addr += fbr->buffsize;
>
> Do the virt_addr update here as well.
> virt_addr += fbr->buffsize;
>
The inner loop's aim is to record bus address, update virt_addr here
seems a noise. I still feel it's good to update virt_addr outside the
inner loop.
>> }
>> }
>> }
>>
>> - /* Allocate an area of memory for FIFO of Packet Status ring entries */
>> - pktstat_ringsize =
>> - sizeof(struct pkt_stat_desc) * adapter->rx_ring.psr_num_entries;
>> + rx_ring->ps_ring_virtaddr = virt_addr;
>> + rx_ring->ps_ring_physaddr = dma_addr;
>> + /* Update the pointer */
>> + virt_addr += pktstat_ring_size;
>> + dma_addr += pktstat_ring_size;
>>
>> - rx_ring->ps_ring_virtaddr = dma_alloc_coherent(&adapter->pdev->dev,
>> - pktstat_ringsize,
>> - &rx_ring->ps_ring_physaddr,
>> - GFP_KERNEL);
>> + rx_ring->rx_status_block = virt_addr;
>> + rx_ring->rx_status_bus = dma_addr;
>> + /* Finally, don't need to update pointer here :-) */
>>
>> - if (!rx_ring->ps_ring_virtaddr) {
>> - dev_err(&adapter->pdev->dev,
>> - "Cannot alloc memory for Packet Status Ring\n");
>> - return -ENOMEM;
>> - }
>> pr_info("Packet Status Ring %llx\n",
>> - (unsigned long long) rx_ring->ps_ring_physaddr);
>> + (unsigned long long)rx_ring->ps_ring_physaddr);
>> + pr_info("Receive Status Ring %llx\n",
>> + (unsigned long long)rx_ring->rx_status_bus);
>
> This is noise, but we should remove it in a separate patch.
>
I still don't get used to the custom that we must strictly separate
the code style changes from others, but I will try hard to change my
mind.
>>
>> - /* NOTE : dma_alloc_coherent(), used above to alloc DMA regions,
>> - * ALWAYS returns SAC (32-bit) addresses. If DAC (64-bit) addresses
>> - * are ever returned, make sure the high part is retrieved here before
>> - * storing the adjusted address.
>> - */
>> -
>> - /* Allocate an area of memory for writeback of status information */
>> - rx_ring->rx_status_block = dma_alloc_coherent(&adapter->pdev->dev,
>> - sizeof(struct rx_status_block),
>> - &rx_ring->rx_status_bus,
>> - GFP_KERNEL);
>> - if (!rx_ring->rx_status_block) {
>> - dev_err(&adapter->pdev->dev,
>> - "Cannot alloc memory for Status Block\n");
>> - return -ENOMEM;
>> - }
>> rx_ring->num_rfd = NIC_DEFAULT_NUM_RFD;
>> - pr_info("PRS %llx\n", (unsigned long long)rx_ring->rx_status_bus);
>> -
>> /* The RFDs are going to be put on lists later on, so initialize the
>> * lists now.
>> */
>> @@ -2354,18 +2338,10 @@ static int et131x_rx_dma_memory_alloc(struct et131x_adapter *adapter)
>> return 0;
>> }
>>
>> -/* et131x_rx_dma_memory_free - Free all memory allocated within this module.
>> - * @adapter: pointer to our private adapter structure
>> - */
>> static void et131x_rx_dma_memory_free(struct et131x_adapter *adapter)
>> {
>> - u8 id;
>> - u32 index;
>> - u32 bufsize;
>> - u32 pktstat_ringsize;
>> struct rfd *rfd;
>> struct rx_ring *rx_ring;
>> - struct fbr_lookup *fbr;
>>
>> /* Setup some convenience pointers */
>> rx_ring = &adapter->rx_ring;
>> @@ -2374,75 +2350,24 @@ static void et131x_rx_dma_memory_free(struct et131x_adapter *adapter)
>> WARN_ON(rx_ring->num_ready_recv != rx_ring->num_rfd);
>>
>> while (!list_empty(&rx_ring->recv_list)) {
>> - rfd = (struct rfd *) list_entry(rx_ring->recv_list.next,
>> - struct rfd, list_node);
>> + rfd = list_entry(rx_ring->recv_list.next,
>> + struct rfd,
>> + list_node);
>
> Don't mix unrelated white space changes into the function.
>
This change remove the unnecessary pointer type convertion, but I
think I must add a code style change patch and do it there :-)
>>
>> list_del(&rfd->list_node);
>> rfd->skb = NULL;
>> kfree(rfd);
>> }
>>
>
> regards,
> dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
