[net-next,v3,5/7] net: lan966x: Update dma_dir of page_pool_params
Commit Message
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(-)
Comments
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
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
@@ -81,6 +81,9 @@ static int lan966x_fdma_rx_alloc_page_pool(struct lan966x_rx *rx)
struct lan966x_port *port;
int i;
+ if (lan966x_xdp_present(lan966x))
+ pp_params.dma_dir = DMA_BIDIRECTIONAL;
+
rx->page_pool = page_pool_create(&pp_params);
for (i = 0; i < lan966x->num_phys_ports; ++i) {
@@ -827,16 +830,11 @@ static int lan966x_fdma_get_max_frame(struct lan966x *lan966x)
XDP_PACKET_HEADROOM;
}
-int lan966x_fdma_change_mtu(struct lan966x *lan966x)
+static int __lan966x_fdma_reload(struct lan966x *lan966x, int max_mtu)
{
- int max_mtu;
int err;
u32 val;
- max_mtu = lan966x_fdma_get_max_frame(lan966x);
- if (max_mtu == lan966x->rx.max_mtu)
- return 0;
-
/* Disable the CPU port */
lan_rmw(QSYS_SW_PORT_MODE_PORT_ENA_SET(0),
QSYS_SW_PORT_MODE_PORT_ENA,
@@ -862,6 +860,25 @@ int lan966x_fdma_change_mtu(struct lan966x *lan966x)
return err;
}
+int lan966x_fdma_change_mtu(struct lan966x *lan966x)
+{
+ int max_mtu;
+
+ max_mtu = lan966x_fdma_get_max_frame(lan966x);
+ if (max_mtu == lan966x->rx.max_mtu)
+ return 0;
+
+ return __lan966x_fdma_reload(lan966x, max_mtu);
+}
+
+int lan966x_fdma_reload_page_pool(struct lan966x *lan966x)
+{
+ int max_mtu;
+
+ max_mtu = lan966x_fdma_get_max_frame(lan966x);
+ return __lan966x_fdma_reload(lan966x, max_mtu);
+}
+
void lan966x_fdma_netdev_init(struct lan966x *lan966x, struct net_device *dev)
{
if (lan966x->fdma_ndev)
@@ -466,6 +466,7 @@ void lan966x_fdma_netdev_deinit(struct lan966x *lan966x, struct net_device *dev)
int lan966x_fdma_init(struct lan966x *lan966x);
void lan966x_fdma_deinit(struct lan966x *lan966x);
irqreturn_t lan966x_fdma_irq_handler(int irq, void *args);
+int lan966x_fdma_reload_page_pool(struct lan966x *lan966x);
int lan966x_lag_port_join(struct lan966x_port *port,
struct net_device *brport_dev,
@@ -556,6 +557,7 @@ int lan966x_xdp(struct net_device *dev, struct netdev_bpf *xdp);
int lan966x_xdp_run(struct lan966x_port *port,
struct page *page,
u32 data_len);
+bool lan966x_xdp_present(struct lan966x *lan966x);
static inline bool lan966x_xdp_port_present(struct lan966x_port *port)
{
return !!port->xdp_prog;
@@ -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;
+
+ 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);
@@ -62,6 +77,21 @@ int lan966x_xdp_run(struct lan966x_port *port, struct page *page, u32 data_len)
}
}
+bool lan966x_xdp_present(struct lan966x *lan966x)
+{
+ int p;
+
+ for (p = 0; p < lan966x->num_phys_ports; ++p) {
+ if (!lan966x->ports[p])
+ continue;
+
+ if (lan966x_xdp_port_present(lan966x->ports[p]))
+ return true;
+ }
+
+ return false;
+}
+
int lan966x_xdp_port_init(struct lan966x_port *port)
{
struct lan966x *lan966x = port->lan966x;