| Message ID | 1555612037-29217-1-git-send-email-ilias.apalodimas@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | net: socionext: replace napi_alloc_frag with the netdev variant on init | expand |
On Thu, 18 Apr 2019 at 11:27, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Use netdev_alloc_frag during the Rx ring setup instead napi_alloc_frag > Why? > Fixes: 4acb20b46214 ("net: socionext: different approach on DMA") > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > drivers/net/ethernet/socionext/netsec.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/socionext/netsec.c b/drivers/net/ethernet/socionext/netsec.c > index a18149720aa2..cba5881b2746 100644 > --- a/drivers/net/ethernet/socionext/netsec.c > +++ b/drivers/net/ethernet/socionext/netsec.c > @@ -673,7 +673,8 @@ static void netsec_process_tx(struct netsec_priv *priv) > } > > static void *netsec_alloc_rx_data(struct netsec_priv *priv, > - dma_addr_t *dma_handle, u16 *desc_len) > + dma_addr_t *dma_handle, u16 *desc_len, > + bool napi) > { > size_t total_len = SKB_DATA_ALIGN(sizeof(struct skb_shared_info)); > size_t payload_len = NETSEC_RX_BUF_SZ; > @@ -682,7 +683,7 @@ static void *netsec_alloc_rx_data(struct netsec_priv *priv, > > total_len += SKB_DATA_ALIGN(payload_len + NETSEC_SKB_PAD); > > - buf = napi_alloc_frag(total_len); > + buf = napi ? napi_alloc_frag(total_len) : netdev_alloc_frag(total_len); > if (!buf) > return NULL; > > @@ -765,7 +766,8 @@ static int netsec_process_rx(struct netsec_priv *priv, int budget) > /* allocate a fresh buffer and map it to the hardware. > * This will eventually replace the old buffer in the hardware > */ > - buf_addr = netsec_alloc_rx_data(priv, &dma_handle, &desc_len); > + buf_addr = netsec_alloc_rx_data(priv, &dma_handle, &desc_len, > + true); > if (unlikely(!buf_addr)) > break; > > @@ -1069,7 +1071,8 @@ static int netsec_setup_rx_dring(struct netsec_priv *priv) > void *buf; > u16 len; > > - buf = netsec_alloc_rx_data(priv, &dma_handle, &len); > + buf = netsec_alloc_rx_data(priv, &dma_handle, &len, > + false); > if (!buf) { > netsec_uninit_pkt_dring(priv, NETSEC_RING_RX); > goto err_out; > -- > 2.7.4 >
Hi Ard, > > Use netdev_alloc_frag during the Rx ring setup instead napi_alloc_frag > > > > Why? > The netdev variant is usable on any context since it disables interrupts. The napi variant of the call is supposed to be used under softirq context only > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > --- > > drivers/net/ethernet/socionext/netsec.c | 11 +++++++---- > > 1 file changed, 7 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/net/ethernet/socionext/netsec.c b/drivers/net/ethernet/socionext/netsec.c > > index a18149720aa2..cba5881b2746 100644 > > --- a/drivers/net/ethernet/socionext/netsec.c > > +++ b/drivers/net/ethernet/socionext/netsec.c > > @@ -673,7 +673,8 @@ static void netsec_process_tx(struct netsec_priv *priv) > > } > > > > static void *netsec_alloc_rx_data(struct netsec_priv *priv, > > - dma_addr_t *dma_handle, u16 *desc_len) > > + dma_addr_t *dma_handle, u16 *desc_len, > > + bool napi) > > { > > size_t total_len = SKB_DATA_ALIGN(sizeof(struct skb_shared_info)); > > size_t payload_len = NETSEC_RX_BUF_SZ; > > @@ -682,7 +683,7 @@ static void *netsec_alloc_rx_data(struct netsec_priv *priv, > > > > total_len += SKB_DATA_ALIGN(payload_len + NETSEC_SKB_PAD); > > > > - buf = napi_alloc_frag(total_len); > > + buf = napi ? napi_alloc_frag(total_len) : netdev_alloc_frag(total_len); > > if (!buf) > > return NULL; > > > > @@ -765,7 +766,8 @@ static int netsec_process_rx(struct netsec_priv *priv, int budget) > > /* allocate a fresh buffer and map it to the hardware. > > * This will eventually replace the old buffer in the hardware > > */ > > - buf_addr = netsec_alloc_rx_data(priv, &dma_handle, &desc_len); > > + buf_addr = netsec_alloc_rx_data(priv, &dma_handle, &desc_len, > > + true); > > if (unlikely(!buf_addr)) > > break; > > > > @@ -1069,7 +1071,8 @@ static int netsec_setup_rx_dring(struct netsec_priv *priv) > > void *buf; > > u16 len; > > > > - buf = netsec_alloc_rx_data(priv, &dma_handle, &len); > > + buf = netsec_alloc_rx_data(priv, &dma_handle, &len, > > + false); > > if (!buf) { > > netsec_uninit_pkt_dring(priv, NETSEC_RING_RX); > > goto err_out; > > -- > > 2.7.4 > >
On Thu, 18 Apr 2019 at 22:24, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Ard, > > > Use netdev_alloc_frag during the Rx ring setup instead napi_alloc_frag > > > > > > > Why? > > > The netdev variant is usable on any context since it disables interrupts. > The napi variant of the call is supposed to be used under softirq context > only > Helpful, thanks. In general, commit logs need to describe *why* you do something, since *what* you do is usually obvious from the patch itself, but why you do it isn't. With the above clarification added: Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > --- > > > drivers/net/ethernet/socionext/netsec.c | 11 +++++++---- > > > 1 file changed, 7 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/socionext/netsec.c b/drivers/net/ethernet/socionext/netsec.c > > > index a18149720aa2..cba5881b2746 100644 > > > --- a/drivers/net/ethernet/socionext/netsec.c > > > +++ b/drivers/net/ethernet/socionext/netsec.c > > > @@ -673,7 +673,8 @@ static void netsec_process_tx(struct netsec_priv *priv) > > > } > > > > > > static void *netsec_alloc_rx_data(struct netsec_priv *priv, > > > - dma_addr_t *dma_handle, u16 *desc_len) > > > + dma_addr_t *dma_handle, u16 *desc_len, > > > + bool napi) > > > { > > > size_t total_len = SKB_DATA_ALIGN(sizeof(struct skb_shared_info)); > > > size_t payload_len = NETSEC_RX_BUF_SZ; > > > @@ -682,7 +683,7 @@ static void *netsec_alloc_rx_data(struct netsec_priv *priv, > > > > > > total_len += SKB_DATA_ALIGN(payload_len + NETSEC_SKB_PAD); > > > > > > - buf = napi_alloc_frag(total_len); > > > + buf = napi ? napi_alloc_frag(total_len) : netdev_alloc_frag(total_len); > > > if (!buf) > > > return NULL; > > > > > > @@ -765,7 +766,8 @@ static int netsec_process_rx(struct netsec_priv *priv, int budget) > > > /* allocate a fresh buffer and map it to the hardware. > > > * This will eventually replace the old buffer in the hardware > > > */ > > > - buf_addr = netsec_alloc_rx_data(priv, &dma_handle, &desc_len); > > > + buf_addr = netsec_alloc_rx_data(priv, &dma_handle, &desc_len, > > > + true); > > > if (unlikely(!buf_addr)) > > > break; > > > > > > @@ -1069,7 +1071,8 @@ static int netsec_setup_rx_dring(struct netsec_priv *priv) > > > void *buf; > > > u16 len; > > > > > > - buf = netsec_alloc_rx_data(priv, &dma_handle, &len); > > > + buf = netsec_alloc_rx_data(priv, &dma_handle, &len, > > > + false); > > > if (!buf) { > > > netsec_uninit_pkt_dring(priv, NETSEC_RING_RX); > > > goto err_out; > > > -- > > > 2.7.4 > > >
On Fri, 19 Apr 2019 at 01:46, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > > On Thu, 18 Apr 2019 at 22:24, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > Hi Ard, > > > > Use netdev_alloc_frag during the Rx ring setup instead napi_alloc_frag > > > > > > > > > > Why? > > > > > The netdev variant is usable on any context since it disables interrupts. > > The napi variant of the call is supposed to be used under softirq context > > only > > > > Helpful, thanks. In general, commit logs need to describe *why* you do > something, since *what* you do is usually obvious from the patch > itself, but why you do it isn't. > > With the above clarification added: > > Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > Fine by me too. Acked-by: Jassi Brar <jaswinder.singh@linaro.org> Thanks.
From: Ard Biesheuvel <ard.biesheuvel@linaro.org> Date: Fri, 19 Apr 2019 08:46:40 +0200 > On Thu, 18 Apr 2019 at 22:24, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: >> >> Hi Ard, >> > > Use netdev_alloc_frag during the Rx ring setup instead napi_alloc_frag >> > > >> > >> > Why? >> > >> The netdev variant is usable on any context since it disables interrupts. >> The napi variant of the call is supposed to be used under softirq context >> only >> > > Helpful, thanks. In general, commit logs need to describe *why* you do > something, since *what* you do is usually obvious from the patch > itself, but why you do it isn't. > > With the above clarification added: > > Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> Just being clear, this patch needs to be reposted with the commit log extended as requested. Thank you.
diff --git a/drivers/net/ethernet/socionext/netsec.c b/drivers/net/ethernet/socionext/netsec.c index a18149720aa2..cba5881b2746 100644 --- a/drivers/net/ethernet/socionext/netsec.c +++ b/drivers/net/ethernet/socionext/netsec.c @@ -673,7 +673,8 @@ static void netsec_process_tx(struct netsec_priv *priv) } static void *netsec_alloc_rx_data(struct netsec_priv *priv, - dma_addr_t *dma_handle, u16 *desc_len) + dma_addr_t *dma_handle, u16 *desc_len, + bool napi) { size_t total_len = SKB_DATA_ALIGN(sizeof(struct skb_shared_info)); size_t payload_len = NETSEC_RX_BUF_SZ; @@ -682,7 +683,7 @@ static void *netsec_alloc_rx_data(struct netsec_priv *priv, total_len += SKB_DATA_ALIGN(payload_len + NETSEC_SKB_PAD); - buf = napi_alloc_frag(total_len); + buf = napi ? napi_alloc_frag(total_len) : netdev_alloc_frag(total_len); if (!buf) return NULL; @@ -765,7 +766,8 @@ static int netsec_process_rx(struct netsec_priv *priv, int budget) /* allocate a fresh buffer and map it to the hardware. * This will eventually replace the old buffer in the hardware */ - buf_addr = netsec_alloc_rx_data(priv, &dma_handle, &desc_len); + buf_addr = netsec_alloc_rx_data(priv, &dma_handle, &desc_len, + true); if (unlikely(!buf_addr)) break; @@ -1069,7 +1071,8 @@ static int netsec_setup_rx_dring(struct netsec_priv *priv) void *buf; u16 len; - buf = netsec_alloc_rx_data(priv, &dma_handle, &len); + buf = netsec_alloc_rx_data(priv, &dma_handle, &len, + false); if (!buf) { netsec_uninit_pkt_dring(priv, NETSEC_RING_RX); goto err_out;
Use netdev_alloc_frag during the Rx ring setup instead napi_alloc_frag Fixes: 4acb20b46214 ("net: socionext: different approach on DMA") Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> --- drivers/net/ethernet/socionext/netsec.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) -- 2.7.4