* [PATCH 0/2 net-next v4] igb/igc: fix XDP registration
@ 2022-01-14 19:45 Corinna Vinschen
2022-01-14 19:45 ` [PATCH 1/2 net-next v4] igc: avoid kernel warning when changing RX ring parameters Corinna Vinschen
2022-01-14 19:45 ` [PATCH 2/2 net-next v4] igb: refactor XDP registration Corinna Vinschen
0 siblings, 2 replies; 5+ messages in thread
From: Corinna Vinschen @ 2022-01-14 19:45 UTC (permalink / raw)
To: intel-wired-lan, netdev, Vinicius Costa Gomes
Cc: Lennert Buytenhek, Alexander Lobakin
Fix the kernel warning "Missing unregister, handled but fix driver"
when running, e.g.,
$ ethtool -G eth0 rx 1024
on igc. Remove memset hack from igb and align igb code to igc.
v3: use dev_err rather than netdev_err, just as in error case.
v4: fix a return copy/pasted from original igc code into the correct
`goto err', improve commit message.
Corinna Vinschen (2):
igc: avoid kernel warning when changing RX ring parameters
igb: refactor XDP registration
drivers/net/ethernet/intel/igb/igb_ethtool.c | 4 ----
drivers/net/ethernet/intel/igb/igb_main.c | 12 +++++++++---
drivers/net/ethernet/intel/igc/igc_main.c | 20 +++++++++++---------
3 files changed, 20 insertions(+), 16 deletions(-)
--
2.27.0
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH 1/2 net-next v4] igc: avoid kernel warning when changing RX ring parameters 2022-01-14 19:45 [PATCH 0/2 net-next v4] igb/igc: fix XDP registration Corinna Vinschen @ 2022-01-14 19:45 ` Corinna Vinschen 2022-01-17 14:57 ` Lennert Buytenhek 2022-01-14 19:45 ` [PATCH 2/2 net-next v4] igb: refactor XDP registration Corinna Vinschen 1 sibling, 1 reply; 5+ messages in thread From: Corinna Vinschen @ 2022-01-14 19:45 UTC (permalink / raw) To: intel-wired-lan, netdev, Vinicius Costa Gomes Cc: Lennert Buytenhek, Alexander Lobakin Calling ethtool changing the RX ring parameters like this: $ ethtool -G eth0 rx 1024 on igc triggers kernel warnings like this: [ 225.198467] ------------[ cut here ]------------ [ 225.198473] Missing unregister, handled but fix driver [ 225.198485] WARNING: CPU: 7 PID: 959 at net/core/xdp.c:168 xdp_rxq_info_reg+0x79/0xd0 [...] [ 225.198601] Call Trace: [ 225.198604] <TASK> [ 225.198609] igc_setup_rx_resources+0x3f/0xe0 [igc] [ 225.198617] igc_ethtool_set_ringparam+0x30e/0x450 [igc] [ 225.198626] ethnl_set_rings+0x18a/0x250 [ 225.198631] genl_family_rcv_msg_doit+0xca/0x110 [ 225.198637] genl_rcv_msg+0xce/0x1c0 [ 225.198640] ? rings_prepare_data+0x60/0x60 [ 225.198644] ? genl_get_cmd+0xd0/0xd0 [ 225.198647] netlink_rcv_skb+0x4e/0xf0 [ 225.198652] genl_rcv+0x24/0x40 [ 225.198655] netlink_unicast+0x20e/0x330 [ 225.198659] netlink_sendmsg+0x23f/0x480 [ 225.198663] sock_sendmsg+0x5b/0x60 [ 225.198667] __sys_sendto+0xf0/0x160 [ 225.198671] ? handle_mm_fault+0xb2/0x280 [ 225.198676] ? do_user_addr_fault+0x1eb/0x690 [ 225.198680] __x64_sys_sendto+0x20/0x30 [ 225.198683] do_syscall_64+0x38/0x90 [ 225.198687] entry_SYSCALL_64_after_hwframe+0x44/0xae [ 225.198693] RIP: 0033:0x7f7ae38ac3aa igc_ethtool_set_ringparam() copies the igc_ring structure but neglects to reset the xdp_rxq_info member before calling igc_setup_rx_resources(). This in turn calls xdp_rxq_info_reg() with an already registered xdp_rxq_info. Make sure to unregister the xdp_rxq_info structure first in igc_setup_rx_resources. Move xdp_rxq_info handling down to be the last action, thus allowing to remove the xdp_rxq_info_unreg call in the error path. Fixes: 73f1071c1d29 ("igc: Add support for XDP_TX action") Reported-by: Lennert Buytenhek <buytenh@arista.com> Signed-off-by: Corinna Vinschen <vinschen@redhat.com> --- drivers/net/ethernet/intel/igc/igc_main.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c index 2f17f36e94fd..56ed0f1463e5 100644 --- a/drivers/net/ethernet/intel/igc/igc_main.c +++ b/drivers/net/ethernet/intel/igc/igc_main.c @@ -505,14 +505,6 @@ int igc_setup_rx_resources(struct igc_ring *rx_ring) u8 index = rx_ring->queue_index; int size, desc_len, res; - res = xdp_rxq_info_reg(&rx_ring->xdp_rxq, ndev, index, - rx_ring->q_vector->napi.napi_id); - if (res < 0) { - netdev_err(ndev, "Failed to register xdp_rxq index %u\n", - index); - return res; - } - size = sizeof(struct igc_rx_buffer) * rx_ring->count; rx_ring->rx_buffer_info = vzalloc(size); if (!rx_ring->rx_buffer_info) @@ -534,10 +526,20 @@ int igc_setup_rx_resources(struct igc_ring *rx_ring) rx_ring->next_to_clean = 0; rx_ring->next_to_use = 0; + /* XDP RX-queue info */ + if (xdp_rxq_info_is_reg(&rx_ring->xdp_rxq)) + xdp_rxq_info_unreg(&rx_ring->xdp_rxq); + res = xdp_rxq_info_reg(&rx_ring->xdp_rxq, ndev, index, + rx_ring->q_vector->napi.napi_id); + if (res < 0) { + netdev_err(ndev, "Failed to register xdp_rxq index %u\n", + index); + goto err; + } + return 0; err: - xdp_rxq_info_unreg(&rx_ring->xdp_rxq); vfree(rx_ring->rx_buffer_info); rx_ring->rx_buffer_info = NULL; netdev_err(ndev, "Unable to allocate memory for Rx descriptor ring\n"); -- 2.27.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2 net-next v4] igc: avoid kernel warning when changing RX ring parameters 2022-01-14 19:45 ` [PATCH 1/2 net-next v4] igc: avoid kernel warning when changing RX ring parameters Corinna Vinschen @ 2022-01-17 14:57 ` Lennert Buytenhek 2022-01-17 15:57 ` Corinna Vinschen 0 siblings, 1 reply; 5+ messages in thread From: Lennert Buytenhek @ 2022-01-17 14:57 UTC (permalink / raw) To: Corinna Vinschen Cc: intel-wired-lan, netdev, Vinicius Costa Gomes, Alexander Lobakin On Fri, Jan 14, 2022 at 08:45:19PM +0100, Corinna Vinschen wrote: > Calling ethtool changing the RX ring parameters like this: > > $ ethtool -G eth0 rx 1024 > > on igc triggers kernel warnings like this: > > [ 225.198467] ------------[ cut here ]------------ > [ 225.198473] Missing unregister, handled but fix driver > [ 225.198485] WARNING: CPU: 7 PID: 959 at net/core/xdp.c:168 > xdp_rxq_info_reg+0x79/0xd0 > [...] > [ 225.198601] Call Trace: > [ 225.198604] <TASK> > [ 225.198609] igc_setup_rx_resources+0x3f/0xe0 [igc] > [ 225.198617] igc_ethtool_set_ringparam+0x30e/0x450 [igc] > [ 225.198626] ethnl_set_rings+0x18a/0x250 > [ 225.198631] genl_family_rcv_msg_doit+0xca/0x110 > [ 225.198637] genl_rcv_msg+0xce/0x1c0 > [ 225.198640] ? rings_prepare_data+0x60/0x60 > [ 225.198644] ? genl_get_cmd+0xd0/0xd0 > [ 225.198647] netlink_rcv_skb+0x4e/0xf0 > [ 225.198652] genl_rcv+0x24/0x40 > [ 225.198655] netlink_unicast+0x20e/0x330 > [ 225.198659] netlink_sendmsg+0x23f/0x480 > [ 225.198663] sock_sendmsg+0x5b/0x60 > [ 225.198667] __sys_sendto+0xf0/0x160 > [ 225.198671] ? handle_mm_fault+0xb2/0x280 > [ 225.198676] ? do_user_addr_fault+0x1eb/0x690 > [ 225.198680] __x64_sys_sendto+0x20/0x30 > [ 225.198683] do_syscall_64+0x38/0x90 > [ 225.198687] entry_SYSCALL_64_after_hwframe+0x44/0xae > [ 225.198693] RIP: 0033:0x7f7ae38ac3aa > > igc_ethtool_set_ringparam() copies the igc_ring structure but neglects to > reset the xdp_rxq_info member before calling igc_setup_rx_resources(). > This in turn calls xdp_rxq_info_reg() with an already registered xdp_rxq_info. > > Make sure to unregister the xdp_rxq_info structure first in > igc_setup_rx_resources. Move xdp_rxq_info handling down to be the last > action, thus allowing to remove the xdp_rxq_info_unreg call in the error path. > > Fixes: 73f1071c1d29 ("igc: Add support for XDP_TX action") > Reported-by: Lennert Buytenhek <buytenh@arista.com> > Signed-off-by: Corinna Vinschen <vinschen@redhat.com> > --- > drivers/net/ethernet/intel/igc/igc_main.c | 20 +++++++++++--------- > 1 file changed, 11 insertions(+), 9 deletions(-) > > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c > index 2f17f36e94fd..56ed0f1463e5 100644 > --- a/drivers/net/ethernet/intel/igc/igc_main.c > +++ b/drivers/net/ethernet/intel/igc/igc_main.c > @@ -505,14 +505,6 @@ int igc_setup_rx_resources(struct igc_ring *rx_ring) > u8 index = rx_ring->queue_index; > int size, desc_len, res; > > - res = xdp_rxq_info_reg(&rx_ring->xdp_rxq, ndev, index, > - rx_ring->q_vector->napi.napi_id); > - if (res < 0) { > - netdev_err(ndev, "Failed to register xdp_rxq index %u\n", > - index); > - return res; > - } > - > size = sizeof(struct igc_rx_buffer) * rx_ring->count; > rx_ring->rx_buffer_info = vzalloc(size); > if (!rx_ring->rx_buffer_info) > @@ -534,10 +526,20 @@ int igc_setup_rx_resources(struct igc_ring *rx_ring) > rx_ring->next_to_clean = 0; > rx_ring->next_to_use = 0; > > + /* XDP RX-queue info */ > + if (xdp_rxq_info_is_reg(&rx_ring->xdp_rxq)) > + xdp_rxq_info_unreg(&rx_ring->xdp_rxq); > + res = xdp_rxq_info_reg(&rx_ring->xdp_rxq, ndev, index, > + rx_ring->q_vector->napi.napi_id); > + if (res < 0) { > + netdev_err(ndev, "Failed to register xdp_rxq index %u\n", > + index); > + goto err; > + } > + > return 0; > > err: > - xdp_rxq_info_unreg(&rx_ring->xdp_rxq); > vfree(rx_ring->rx_buffer_info); > rx_ring->rx_buffer_info = NULL; > netdev_err(ndev, "Unable to allocate memory for Rx descriptor ring\n"); This patch fixes the warning... Tested-by: Lennert Buytenhek <buytenh@arista.com> ...but doesn't it now forget to free rx_ring->desc if xdp_rxq_info_reg() fails? Thank you! ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2 net-next v4] igc: avoid kernel warning when changing RX ring parameters 2022-01-17 14:57 ` Lennert Buytenhek @ 2022-01-17 15:57 ` Corinna Vinschen 0 siblings, 0 replies; 5+ messages in thread From: Corinna Vinschen @ 2022-01-17 15:57 UTC (permalink / raw) To: Lennert Buytenhek Cc: intel-wired-lan, netdev, Vinicius Costa Gomes, Alexander Lobakin On Jan 17 16:57, Lennert Buytenhek wrote: > On Fri, Jan 14, 2022 at 08:45:19PM +0100, Corinna Vinschen wrote: > > > Calling ethtool changing the RX ring parameters like this: > > > > $ ethtool -G eth0 rx 1024 > > > > on igc triggers kernel warnings like this: > > > > [ 225.198467] ------------[ cut here ]------------ > > [ 225.198473] Missing unregister, handled but fix driver > > [ 225.198485] WARNING: CPU: 7 PID: 959 at net/core/xdp.c:168 > > xdp_rxq_info_reg+0x79/0xd0 > > [...] > > @@ -534,10 +526,20 @@ int igc_setup_rx_resources(struct igc_ring *rx_ring) > > rx_ring->next_to_clean = 0; > > rx_ring->next_to_use = 0; > > > > + /* XDP RX-queue info */ > > + if (xdp_rxq_info_is_reg(&rx_ring->xdp_rxq)) > > + xdp_rxq_info_unreg(&rx_ring->xdp_rxq); > > + res = xdp_rxq_info_reg(&rx_ring->xdp_rxq, ndev, index, > > + rx_ring->q_vector->napi.napi_id); > > + if (res < 0) { > > + netdev_err(ndev, "Failed to register xdp_rxq index %u\n", > > + index); > > + goto err; > > + } > > + > > return 0; > > > > err: > > - xdp_rxq_info_unreg(&rx_ring->xdp_rxq); > > vfree(rx_ring->rx_buffer_info); > > rx_ring->rx_buffer_info = NULL; > > netdev_err(ndev, "Unable to allocate memory for Rx descriptor ring\n"); > > This patch fixes the warning... > > Tested-by: Lennert Buytenhek <buytenh@arista.com> > > ...but doesn't it now forget to free rx_ring->desc if xdp_rxq_info_reg() > fails? Uhm... I see what you mean. But then again, the same error was already present in igb. Looks like a call to dma_free_coherent is missing there, too. I'll prepare YA patch. Thanks, Corinna ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 2/2 net-next v4] igb: refactor XDP registration 2022-01-14 19:45 [PATCH 0/2 net-next v4] igb/igc: fix XDP registration Corinna Vinschen 2022-01-14 19:45 ` [PATCH 1/2 net-next v4] igc: avoid kernel warning when changing RX ring parameters Corinna Vinschen @ 2022-01-14 19:45 ` Corinna Vinschen 1 sibling, 0 replies; 5+ messages in thread From: Corinna Vinschen @ 2022-01-14 19:45 UTC (permalink / raw) To: intel-wired-lan, netdev, Vinicius Costa Gomes Cc: Lennert Buytenhek, Alexander Lobakin On changing the RX ring parameters igb uses a hack to avoid a warning when calling xdp_rxq_info_reg via igb_setup_rx_resources. It just clears the struct xdp_rxq_info content. Change this to unregister if we're already registered instead. Align code to the igc code. Fixes: 9cbc948b5a20c ("igb: add XDP support") Signed-off-by: Corinna Vinschen <vinschen@redhat.com> --- drivers/net/ethernet/intel/igb/igb_ethtool.c | 4 ---- drivers/net/ethernet/intel/igb/igb_main.c | 12 +++++++++--- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c index 51a2dcaf553d..2a5782063f4c 100644 --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c @@ -965,10 +965,6 @@ static int igb_set_ringparam(struct net_device *netdev, memcpy(&temp_ring[i], adapter->rx_ring[i], sizeof(struct igb_ring)); - /* Clear copied XDP RX-queue info */ - memset(&temp_ring[i].xdp_rxq, 0, - sizeof(temp_ring[i].xdp_rxq)); - temp_ring[i].count = new_rx_count; err = igb_setup_rx_resources(&temp_ring[i]); if (err) { diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c index 38ba92022cd4..cea89d301bfd 100644 --- a/drivers/net/ethernet/intel/igb/igb_main.c +++ b/drivers/net/ethernet/intel/igb/igb_main.c @@ -4352,7 +4352,7 @@ int igb_setup_rx_resources(struct igb_ring *rx_ring) { struct igb_adapter *adapter = netdev_priv(rx_ring->netdev); struct device *dev = rx_ring->dev; - int size; + int size, res; size = sizeof(struct igb_rx_buffer) * rx_ring->count; @@ -4376,9 +4376,15 @@ int igb_setup_rx_resources(struct igb_ring *rx_ring) rx_ring->xdp_prog = adapter->xdp_prog; /* XDP RX-queue info */ - if (xdp_rxq_info_reg(&rx_ring->xdp_rxq, rx_ring->netdev, - rx_ring->queue_index, 0) < 0) + if (xdp_rxq_info_is_reg(&rx_ring->xdp_rxq)) + xdp_rxq_info_unreg(&rx_ring->xdp_rxq); + res = xdp_rxq_info_reg(&rx_ring->xdp_rxq, rx_ring->netdev, + rx_ring->queue_index, 0); + if (res < 0) { + dev_err(dev, "Failed to register xdp_rxq index %u\n", + rx_ring->queue_index); goto err; + } return 0; -- 2.27.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-01-17 15:57 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-01-14 19:45 [PATCH 0/2 net-next v4] igb/igc: fix XDP registration Corinna Vinschen 2022-01-14 19:45 ` [PATCH 1/2 net-next v4] igc: avoid kernel warning when changing RX ring parameters Corinna Vinschen 2022-01-17 14:57 ` Lennert Buytenhek 2022-01-17 15:57 ` Corinna Vinschen 2022-01-14 19:45 ` [PATCH 2/2 net-next v4] igb: refactor XDP registration Corinna Vinschen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).