Ok, yeah. This patch is the right thing. I had a couple minor style
complaints.
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 <gamer...@gmail.com>
> ---
> 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.
> + 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...
> @@ -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;
> 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.
> + 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).
> + 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);
> + /* Update the pointer */
> + dma_addr += fbr->buffsize;
Do the virt_addr update here as well.
virt_addr += fbr->buffsize;
> }
> }
> }
>
> - /* 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.
>
> - /* 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.
>
> list_del(&rfd->list_node);
> rfd->skb = NULL;
> kfree(rfd);
> }
>
regards,
dan carpenter
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
