Re: [PATCH] net: socionext: replace napi_alloc_frag with the netdev variant on init

From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Cc: "<netdev@vger.kernel.org>" <netdev@vger.kernel.org>,
	Jaswinder Singh <jaswinder.singh@linaro.org>,
	"David S. Miller" <davem@davemloft.net>,
	Masahisa Kojima <masahisa.kojima@linaro.org>
Subject: Re: [PATCH] net: socionext: replace napi_alloc_frag with the netdev variant on init
Date: Fri, 19 Apr 2019 08:46:40 +0200	[thread overview]
Message-ID: <CAKv+Gu9fNBFrKBtbSYiEZeZ5MwzjjSmu1Fz+nAt6anr8Zb6r=g@mail.gmail.com> (raw)
In-Reply-To: <20190418202451.GA31868@apalos>

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