| Message ID | 20230704082916.2135501-2-wei.fang@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: fec: fix some issues of ndo_xdp_xmit() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/fixes_present | success | Fixes tag present in non-next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 8 this patch: 8 |
| netdev/cc_maintainers | success | CCed 14 of 14 maintainers |
| netdev/build_clang | fail | Errors and warnings before: 18 this patch: 18 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 8 this patch: 8 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 37 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Tue, Jul 04, 2023 at 04:29:14PM +0800, wei.fang@nxp.com wrote: > From: Wei Fang <wei.fang@nxp.com> > > When a XDP program is installed or uninstalled, fec_restart() will > be invoked to reset MAC and buffer descriptor rings. It's reasonable > not to transmit any packet during the process of reset. However, the > NETDEV_XDP_ACT_NDO_XMIT bit of xdp_features is enabled by default, > that is to say, it's possible that the fec_enet_xdp_xmit() will be > invoked even if the process of reset is not finished. In this case, > the redirected XDP frames might be dropped and available transmit BDs > may be incorrectly deemed insufficient. So this patch disable the > NETDEV_XDP_ACT_NDO_XMIT feature by default and dynamically configure > this feature when the bpf program is installed or uninstalled. I don't know much about XDP, so please excuse what might be a stupid question. Is this a generic issue? Should this xdp_features_clear_redirect_target(dev) / xdp_features_set_redirect_target(dev, false) be done in the core? Andrew
Andrew Lunn <andrew@lunn.ch> writes: > On Tue, Jul 04, 2023 at 04:29:14PM +0800, wei.fang@nxp.com wrote: >> From: Wei Fang <wei.fang@nxp.com> >> >> When a XDP program is installed or uninstalled, fec_restart() will >> be invoked to reset MAC and buffer descriptor rings. It's reasonable >> not to transmit any packet during the process of reset. However, the >> NETDEV_XDP_ACT_NDO_XMIT bit of xdp_features is enabled by default, >> that is to say, it's possible that the fec_enet_xdp_xmit() will be >> invoked even if the process of reset is not finished. In this case, >> the redirected XDP frames might be dropped and available transmit BDs >> may be incorrectly deemed insufficient. So this patch disable the >> NETDEV_XDP_ACT_NDO_XMIT feature by default and dynamically configure >> this feature when the bpf program is installed or uninstalled. > > I don't know much about XDP, so please excuse what might be a stupid > question. > > Is this a generic issue? Should this > xdp_features_clear_redirect_target(dev) / > xdp_features_set_redirect_target(dev, false) be done in the core? No, because not all drivers require an XDP program to be attached to support being a redirect target (which is one of the reasons we introduced these feature bits in the first place :)). -Toke
> -----Original Message----- > From: Toke Høiland-Jørgensen <toke@redhat.com> > Sent: 2023年7月5日 7:54 > To: Andrew Lunn <andrew@lunn.ch>; Wei Fang <wei.fang@nxp.com> > Cc: 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; Shenwei Wang > <shenwei.wang@nxp.com>; Clark Wang <xiaoning.wang@nxp.com>; > netdev@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>; > linux-kernel@vger.kernel.org; bpf@vger.kernel.org > Subject: Re: [PATCH net 1/3] net: fec: dynamically set the > NETDEV_XDP_ACT_NDO_XMIT feature of XDP > > Andrew Lunn <andrew@lunn.ch> writes: > > > On Tue, Jul 04, 2023 at 04:29:14PM +0800, wei.fang@nxp.com wrote: > >> From: Wei Fang <wei.fang@nxp.com> > >> > >> When a XDP program is installed or uninstalled, fec_restart() will be > >> invoked to reset MAC and buffer descriptor rings. It's reasonable not > >> to transmit any packet during the process of reset. However, the > >> NETDEV_XDP_ACT_NDO_XMIT bit of xdp_features is enabled by default, > >> that is to say, it's possible that the fec_enet_xdp_xmit() will be > >> invoked even if the process of reset is not finished. In this case, > >> the redirected XDP frames might be dropped and available transmit BDs > >> may be incorrectly deemed insufficient. So this patch disable the > >> NETDEV_XDP_ACT_NDO_XMIT feature by default and dynamically > configure > >> this feature when the bpf program is installed or uninstalled. > > > > I don't know much about XDP, so please excuse what might be a stupid > > question. > > > > Is this a generic issue? Should this > > xdp_features_clear_redirect_target(dev) / > > xdp_features_set_redirect_target(dev, false) be done in the core? > > No, because not all drivers require an XDP program to be attached to support > being a redirect target (which is one of the reasons we introduced these > feature bits in the first place :)). > Hi Toke, Thanks for your explanation so much. :)
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index 8fbe47703d47..9ce0319b33c3 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -3732,12 +3732,18 @@ static int fec_enet_bpf(struct net_device *dev, struct netdev_bpf *bpf) if (fep->quirks & FEC_QUIRK_SWAP_FRAME) return -EOPNOTSUPP; + if (!bpf->prog) + xdp_features_clear_redirect_target(dev); + if (is_run) { napi_disable(&fep->napi); netif_tx_disable(dev); } old_prog = xchg(&fep->xdp_prog, bpf->prog); + if (old_prog) + bpf_prog_put(old_prog); + fec_restart(dev); if (is_run) { @@ -3745,8 +3751,8 @@ static int fec_enet_bpf(struct net_device *dev, struct netdev_bpf *bpf) netif_tx_start_all_queues(dev); } - if (old_prog) - bpf_prog_put(old_prog); + if (bpf->prog) + xdp_features_set_redirect_target(dev, false); return 0; @@ -4016,8 +4022,7 @@ static int fec_enet_init(struct net_device *ndev) if (!(fep->quirks & FEC_QUIRK_SWAP_FRAME)) ndev->xdp_features = NETDEV_XDP_ACT_BASIC | - NETDEV_XDP_ACT_REDIRECT | - NETDEV_XDP_ACT_NDO_XMIT; + NETDEV_XDP_ACT_REDIRECT; fec_restart(ndev);