Re: [PATCH net-next v3 5/7] net: lan966x: Update dma_dir of page_pool_params

[Alexander Lobakin]

From: Horatiu Vultur <horatiu.vultur@xxxxxxxxxxxxx>
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@xxxxxxxxxxxxx>
> ---
>  .../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