net: socionext: replace napi_alloc_frag with the netdev variant on init

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
Related show

Commit Message

Ilias Apalodimas April 18, 2019, 6:27 p.m.
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

Comments

Ard Biesheuvel April 18, 2019, 7:50 p.m. | #1
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

>
Ilias Apalodimas April 18, 2019, 8:24 p.m. | #2
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

> >
Ard Biesheuvel April 19, 2019, 6:46 a.m. | #3
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

> > >
Jassi Brar April 19, 2019, 11:48 p.m. | #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.
David Miller April 23, 2019, 4:40 a.m. | #5
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.

Patch

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;