Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 22 Nov 2022 22:25:04 +0100
From:   Horatiu Vultur <horatiu.vultur@microchip.com>
To:     Alexander Lobakin <alexandr.lobakin@intel.com>
CC:     <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <bpf@vger.kernel.org>, <davem@davemloft.net>,
        <edumazet@google.com>, <kuba@kernel.org>, <pabeni@redhat.com>,
        <ast@kernel.org>, <daniel@iogearbox.net>, <hawk@kernel.org>,
        <john.fastabend@gmail.com>, <UNGLinuxDriver@microchip.com>
Subject: Re: [PATCH net-next v3 5/7] net: lan966x: Update dma_dir of
 page_pool_params
Message-ID: <20221122212504.nrwaucgab3lqqxpo@soft-dev3-1>
References: <20221121212850.3212649-1-horatiu.vultur@microchip.com>
 <20221121212850.3212649-6-horatiu.vultur@microchip.com>
 <20221122114339.419188-1-alexandr.lobakin@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
In-Reply-To: <20221122114339.419188-1-alexandr.lobakin@intel.com>
Precedence: bulk

The 11/22/2022 12:43, Alexander Lobakin wrote:
> 
> From: Horatiu Vultur <horatiu.vultur@microchip.com>
> Date: Mon, 21 Nov 2022 22:28:48 +0100
> 
> > To add support for XDP_TX it is required to be able to write to the DMA
> > area therefore it is required that the pages will be mapped using
> > DMA_BIDIRECTIONAL flag.
> > Therefore check if there are any xdp programs on the interfaces and in
> > that case set DMA_BIDRECTIONAL otherwise use DMA_FROM_DEVICE.
> > Therefore when a new XDP program is added it is required to redo the
> > page_pool.
> >
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > ---
> >  .../ethernet/microchip/lan966x/lan966x_fdma.c | 29 ++++++++++++++----
> >  .../ethernet/microchip/lan966x/lan966x_main.h |  2 ++
> >  .../ethernet/microchip/lan966x/lan966x_xdp.c  | 30 +++++++++++++++++++
> >  3 files changed, 55 insertions(+), 6 deletions(-)
> 
> [...]
> 
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c b/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c
> > index 8ebde1eb6a09c..05c5a28206558 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_xdp.c
> > @@ -11,6 +11,8 @@ static int lan966x_xdp_setup(struct net_device *dev, struct netdev_bpf *xdp)
> >       struct lan966x_port *port = netdev_priv(dev);
> >       struct lan966x *lan966x = port->lan966x;
> >       struct bpf_prog *old_prog;
> > +     bool old_xdp, new_xdp;
> > +     int err;
> >
> >       if (!lan966x->fdma) {
> >               NL_SET_ERR_MSG_MOD(xdp->extack,
> > @@ -18,7 +20,20 @@ static int lan966x_xdp_setup(struct net_device *dev, struct netdev_bpf *xdp)
> >               return -EOPNOTSUPP;
> >       }
> >
> > +     old_xdp = lan966x_xdp_present(lan966x);
> >       old_prog = xchg(&port->xdp_prog, xdp->prog);
> > +     new_xdp = lan966x_xdp_present(lan966x);
> > +
> > +     if (old_xdp != new_xdp)
> > +             goto out;
> 
> Shouldn't it be the other way around? E.g. when there's no prog and
> you're installing it or there is a prog and we're removing it from
> the interface, DMA dir must be changed, so we reload the Pools, but
> if `old_xdp == new_xdp` we should just hotswap them and goto out?

Argh! Yes, it needs to be the other way around.
> 
> > +
> > +     err = lan966x_fdma_reload_page_pool(lan966x);
> > +     if (err) {
> > +             xchg(&port->xdp_prog, old_prog);
> > +             return err;
> > +     }
> > +
> > +out:
> >       if (old_prog)
> >               bpf_prog_put(old_prog);
> >
> 
> [...]
> 
> > --
> > 2.38.0
> 
> Thanks,
> Olek

-- 
/Horatiu