Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756982AbcK2KRL (ORCPT ); Tue, 29 Nov 2016 05:17:11 -0500 Received: from mail.free-electrons.com ([62.4.15.54]:48619 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756479AbcK2KRJ (ORCPT ); Tue, 29 Nov 2016 05:17:09 -0500 From: Gregory CLEMENT To: Marcin Wojtas Cc: "David S. Miller" , linux-kernel@vger.kernel.org, netdev@vger.kernel.org, Jisheng Zhang , Arnd Bergmann , Jason Cooper , Andrew Lunn , Sebastian Hesselbarth , Thomas Petazzoni , "linux-arm-kernel\@lists.infradead.org" , Nadav Haklai , Dmitri Epshtein , Yelena Krivosheev Subject: Re: [PATCH v3 net-next 2/6] net: mvneta: Use cacheable memory to store the rx buffer virtual address References: Date: Tue, 29 Nov 2016 11:17:06 +0100 In-Reply-To: (Marcin Wojtas's message of "Tue, 29 Nov 2016 10:50:39 +0100") Message-ID: <87wpfmfv4t.fsf@free-electrons.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9168 Lines: 222 Hi Marcin, On mar., nov. 29 2016, Marcin Wojtas wrote: > Hi Gregory, > > Apparently HWBM had a mistake in implementation, please see below. > > 2016-11-29 10:37 GMT+01:00 Gregory CLEMENT : >> Until now the virtual address of the received buffer were stored in the >> cookie field of the rx descriptor. However, this field is 32-bits only >> which prevents to use the driver on a 64-bits architecture. >> >> With this patch the virtual address is stored in an array not shared with >> the hardware (no more need to use the DMA API). Thanks to this, it is >> possible to use cache contrary to the access of the rx descriptor member. >> >> The change is done in the swbm path only because the hwbm uses the cookie >> field, this also means that currently the hwbm is not usable in 64-bits. >> >> Signed-off-by: Gregory CLEMENT >> --- >> drivers/net/ethernet/marvell/mvneta.c | 93 ++++++++++++++++++++++++---- >> 1 file changed, 81 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c >> index 1b84f746d748..32b142d0e44e 100644 >> --- a/drivers/net/ethernet/marvell/mvneta.c >> +++ b/drivers/net/ethernet/marvell/mvneta.c >> @@ -561,6 +561,9 @@ struct mvneta_rx_queue { >> u32 pkts_coal; >> u32 time_coal; >> >> + /* Virtual address of the RX buffer */ >> + void **buf_virt_addr; >> + >> /* Virtual address of the RX DMA descriptors array */ >> struct mvneta_rx_desc *descs; >> >> @@ -1573,10 +1576,14 @@ static void mvneta_tx_done_pkts_coal_set(struct mvneta_port *pp, >> >> /* Handle rx descriptor fill by setting buf_cookie and buf_phys_addr */ >> static void mvneta_rx_desc_fill(struct mvneta_rx_desc *rx_desc, >> - u32 phys_addr, u32 cookie) >> + u32 phys_addr, void *virt_addr, >> + struct mvneta_rx_queue *rxq) >> { >> - rx_desc->buf_cookie = cookie; >> + int i; >> + >> rx_desc->buf_phys_addr = phys_addr; >> + i = rx_desc - rxq->descs; >> + rxq->buf_virt_addr[i] = virt_addr; >> } >> >> /* Decrement sent descriptors counter */ >> @@ -1781,7 +1788,8 @@ EXPORT_SYMBOL_GPL(mvneta_frag_free); >> >> /* Refill processing for SW buffer management */ >> static int mvneta_rx_refill(struct mvneta_port *pp, >> - struct mvneta_rx_desc *rx_desc) >> + struct mvneta_rx_desc *rx_desc, >> + struct mvneta_rx_queue *rxq) >> >> { >> dma_addr_t phys_addr; >> @@ -1799,7 +1807,7 @@ static int mvneta_rx_refill(struct mvneta_port *pp, >> return -ENOMEM; >> } >> >> - mvneta_rx_desc_fill(rx_desc, phys_addr, (u32)data); >> + mvneta_rx_desc_fill(rx_desc, phys_addr, data, rxq); >> return 0; >> } >> >> @@ -1861,7 +1869,12 @@ static void mvneta_rxq_drop_pkts(struct mvneta_port *pp, >> >> for (i = 0; i < rxq->size; i++) { >> struct mvneta_rx_desc *rx_desc = rxq->descs + i; >> - void *data = (void *)rx_desc->buf_cookie; >> + void *data; >> + >> + if (!pp->bm_priv) >> + data = rxq->buf_virt_addr[i]; >> + else >> + data = (void *)(uintptr_t)rx_desc->buf_cookie; >> >> dma_unmap_single(pp->dev->dev.parent, rx_desc->buf_phys_addr, >> MVNETA_RX_BUF_SIZE(pp->pkt_size), DMA_FROM_DEVICE); >> @@ -1894,12 +1907,13 @@ static int mvneta_rx_swbm(struct mvneta_port *pp, int rx_todo, >> unsigned char *data; >> dma_addr_t phys_addr; >> u32 rx_status, frag_size; >> - int rx_bytes, err; >> + int rx_bytes, err, index; >> >> rx_done++; >> rx_status = rx_desc->status; >> rx_bytes = rx_desc->data_size - (ETH_FCS_LEN + MVNETA_MH_SIZE); >> - data = (unsigned char *)rx_desc->buf_cookie; >> + index = rx_desc - rxq->descs; >> + data = (unsigned char *)rxq->buf_virt_addr[index]; >> phys_addr = rx_desc->buf_phys_addr; >> >> if (!mvneta_rxq_desc_is_first_last(rx_status) || >> @@ -1938,7 +1952,7 @@ static int mvneta_rx_swbm(struct mvneta_port *pp, int rx_todo, >> } >> >> /* Refill processing */ >> - err = mvneta_rx_refill(pp, rx_desc); >> + err = mvneta_rx_refill(pp, rx_desc, rxq); >> if (err) { >> netdev_err(dev, "Linux processing - Can't refill\n"); >> rxq->missed++; >> @@ -2020,7 +2034,7 @@ static int mvneta_rx_hwbm(struct mvneta_port *pp, int rx_todo, >> rx_done++; >> rx_status = rx_desc->status; >> rx_bytes = rx_desc->data_size - (ETH_FCS_LEN + MVNETA_MH_SIZE); >> - data = (unsigned char *)rx_desc->buf_cookie; >> + data = (u8 *)(uintptr_t)rx_desc->buf_cookie; >> phys_addr = rx_desc->buf_phys_addr; >> pool_id = MVNETA_RX_GET_BM_POOL_ID(rx_desc); >> bm_pool = &pp->bm_priv->bm_pools[pool_id]; >> @@ -2708,6 +2722,56 @@ static int mvneta_poll(struct napi_struct *napi, int budget) >> return rx_done; >> } >> >> +/* Refill processing for HW buffer management */ >> +static int mvneta_rx_hwbm_refill(struct mvneta_port *pp, >> + struct mvneta_rx_desc *rx_desc) >> + >> +{ >> + dma_addr_t phys_addr; >> + void *data; >> + >> + data = mvneta_frag_alloc(pp->frag_size); >> + if (!data) >> + return -ENOMEM; >> + >> + phys_addr = dma_map_single(pp->dev->dev.parent, data, >> + MVNETA_RX_BUF_SIZE(pp->pkt_size), >> + DMA_FROM_DEVICE); >> + if (unlikely(dma_mapping_error(pp->dev->dev.parent, phys_addr))) { >> + mvneta_frag_free(pp->frag_size, data); >> + return -ENOMEM; >> + } >> + >> + rx_desc->buf_phys_addr = phys_addr; >> + rx_desc->buf_cookie = (uintptr_t)data; >> + >> + return 0; >> +} >> + >> +/* Handle rxq fill: allocates rxq skbs; called when initializing a port */ >> +static int mvneta_rxq_bm_fill(struct mvneta_port *pp, >> + struct mvneta_rx_queue *rxq, >> + int num) >> +{ >> + int i; >> + >> + for (i = 0; i < num; i++) { >> + memset(rxq->descs + i, 0, sizeof(struct mvneta_rx_desc)); >> + if (mvneta_rx_hwbm_refill(pp, rxq->descs + i) != 0) { >> + netdev_err(pp->dev, "%s:rxq %d, %d of %d buffs filled\n", >> + __func__, rxq->id, i, num); >> + break; >> + } >> + } >> + >> + /* Add this number of RX descriptors as non occupied (ready to >> + * get packets) >> + */ >> + mvneta_rxq_non_occup_desc_add(pp, rxq, i); >> + >> + return i; >> +} >> + >> /* Handle rxq fill: allocates rxq skbs; called when initializing a port */ >> static int mvneta_rxq_fill(struct mvneta_port *pp, struct mvneta_rx_queue *rxq, >> int num) >> @@ -2716,7 +2780,7 @@ static int mvneta_rxq_fill(struct mvneta_port *pp, struct mvneta_rx_queue *rxq, >> >> for (i = 0; i < num; i++) { >> memset(rxq->descs + i, 0, sizeof(struct mvneta_rx_desc)); >> - if (mvneta_rx_refill(pp, rxq->descs + i) != 0) { >> + if (mvneta_rx_refill(pp, rxq->descs + i, rxq) != 0) { >> netdev_err(pp->dev, "%s:rxq %d, %d of %d buffs filled\n", >> __func__, rxq->id, i, num); >> break; >> @@ -2784,14 +2848,14 @@ static int mvneta_rxq_init(struct mvneta_port *pp, >> mvneta_rxq_buf_size_set(pp, rxq, >> MVNETA_RX_BUF_SIZE(pp->pkt_size)); >> mvneta_rxq_bm_disable(pp, rxq); >> + mvneta_rxq_fill(pp, rxq, rxq->size); >> } else { >> mvneta_rxq_bm_enable(pp, rxq); >> mvneta_rxq_long_pool_set(pp, rxq); >> mvneta_rxq_short_pool_set(pp, rxq); >> + mvneta_rxq_bm_fill(pp, rxq, rxq->size); > > Manual filling descriptors with new buffers is redundant. For HWBM, > all buffers are allocated in mvneta_bm_construct() and in runtime they > are put into descriptors by hardware. I think it's enough to add here: > mvneta_rxq_non_occup_desc_add(pp, rxq, rxq->size); > > And remove mvneta_rxq_bm_fill and mvneta_rx_hwbm_refill. You're right. I will do it it will simplify the code. Thanks, Gregory > > Best regards, > Marcin -- Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com