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;
From:   Alexander Lobakin <alexandr.lobakin@intel.com>
To:     Horatiu Vultur <horatiu.vultur@microchip.com>
Cc:     Alexander Lobakin <alexandr.lobakin@intel.com>,
        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
Date:   Tue, 22 Nov 2022 12:43:39 +0100
Message-Id: <20221122114339.419188-1-alexandr.lobakin@intel.com>
In-Reply-To: <20221121212850.3212649-6-horatiu.vultur@microchip.com>
References: <20221121212850.3212649-1-horatiu.vultur@microchip.com> <20221121212850.3212649-6-horatiu.vultur@microchip.com>
MIME-Version: 1.0
Content-Type: text/plain
Content-Transfer-Encoding: 8bit
Precedence: bulk

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?

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