* [Intel-wired-lan] [PATCH 0/3 net-next v5] igb/igc: fix XDP registration
@ 2022-01-17 18:29 ` Corinna Vinschen
0 siblings, 0 replies; 16+ messages in thread
From: Corinna Vinschen @ 2022-01-17 18:29 UTC (permalink / raw)
To: intel-wired-lan
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.
v5: add missing dma_free_coherent calls in error case.
Corinna Vinschen (2):
igc: avoid kernel warning when changing RX ring parameters
igb: refactor XDP registration
igb/igc: RX queues: fix DMA leak in error case
drivers/net/ethernet/intel/igb/igb_ethtool.c | 4 ----
drivers/net/ethernet/intel/igb/igb_main.c | 13 +++++++++---
drivers/net/ethernet/intel/igc/igc_main.c | 21 +++++++++++---------
3 files changed, 22 insertions(+), 16 deletions(-)
--
2.27.0
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH 1/3 net-next v5] igc: avoid kernel warning when changing RX ring parameters
2022-01-17 18:29 ` [Intel-wired-lan] " Corinna Vinschen
@ 2022-01-17 18:29 ` Corinna Vinschen
-1 siblings, 0 replies; 16+ messages in thread
From: Corinna Vinschen @ 2022-01-17 18:29 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] 16+ messages in thread* [Intel-wired-lan] [PATCH 1/3 net-next v5] igc: avoid kernel warning when changing RX ring parameters
@ 2022-01-17 18:29 ` Corinna Vinschen
0 siblings, 0 replies; 16+ messages in thread
From: Corinna Vinschen @ 2022-01-17 18:29 UTC (permalink / raw)
To: intel-wired-lan
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] 16+ messages in thread
* [PATCH 2/3 net-next v5] igb: refactor XDP registration
2022-01-17 18:29 ` [Intel-wired-lan] " Corinna Vinschen
@ 2022-01-17 18:29 ` Corinna Vinschen
-1 siblings, 0 replies; 16+ messages in thread
From: Corinna Vinschen @ 2022-01-17 18:29 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] 16+ messages in thread* [Intel-wired-lan] [PATCH 2/3 net-next v5] igb: refactor XDP registration
@ 2022-01-17 18:29 ` Corinna Vinschen
0 siblings, 0 replies; 16+ messages in thread
From: Corinna Vinschen @ 2022-01-17 18:29 UTC (permalink / raw)
To: intel-wired-lan
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] 16+ messages in thread* Re: [PATCH 2/3 net-next v5] igb: refactor XDP registration
2022-01-17 18:29 ` [Intel-wired-lan] " Corinna Vinschen
@ 2022-01-18 15:05 ` Alexander Lobakin
-1 siblings, 0 replies; 16+ messages in thread
From: Alexander Lobakin @ 2022-01-18 15:05 UTC (permalink / raw)
To: Corinna Vinschen
Cc: Alexander Lobakin, intel-wired-lan, netdev, Vinicius Costa Gomes,
Lennert Buytenhek
From: Corinna Vinschen <vinschen@redhat.com>
Date: Mon, 17 Jan 2022 19:29:14 +0100
> 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;
Error path always returns -ENOMEM, even in this case, and reports
that it failed to allocate memory for rings. Handle this correctly
and return `res` instead and without one more error message?
> + }
As I mentioned a bit above, `res` is unused here as an error code,
only to test the value on < 0. Does it make sense to add a new
variable?
>
> return 0;
>
> --
> 2.27.0
Thanks,
Al
^ permalink raw reply [flat|nested] 16+ messages in thread* [Intel-wired-lan] [PATCH 2/3 net-next v5] igb: refactor XDP registration
@ 2022-01-18 15:05 ` Alexander Lobakin
0 siblings, 0 replies; 16+ messages in thread
From: Alexander Lobakin @ 2022-01-18 15:05 UTC (permalink / raw)
To: intel-wired-lan
From: Corinna Vinschen <vinschen@redhat.com>
Date: Mon, 17 Jan 2022 19:29:14 +0100
> 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;
Error path always returns -ENOMEM, even in this case, and reports
that it failed to allocate memory for rings. Handle this correctly
and return `res` instead and without one more error message?
> + }
As I mentioned a bit above, `res` is unused here as an error code,
only to test the value on < 0. Does it make sense to add a new
variable?
>
> return 0;
>
> --
> 2.27.0
Thanks,
Al
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 2/3 net-next v5] igb: refactor XDP registration
2022-01-18 15:05 ` [Intel-wired-lan] " Alexander Lobakin
@ 2022-01-19 6:08 ` Corinna Vinschen
-1 siblings, 0 replies; 16+ messages in thread
From: Corinna Vinschen @ 2022-01-19 6:08 UTC (permalink / raw)
To: Alexander Lobakin
Cc: intel-wired-lan, netdev, Vinicius Costa Gomes, Lennert Buytenhek
On Jan 18 16:05, Alexander Lobakin wrote:
> From: Corinna Vinschen <vinschen@redhat.com>
> Date: Mon, 17 Jan 2022 19:29:14 +0100
>
> > 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;
>
> Error path always returns -ENOMEM, even in this case, and reports
> that it failed to allocate memory for rings. Handle this correctly
> and return `res` instead and without one more error message?
In that case, it makes sense to revert the code to the way igc did it,
rather then trying to do as igb did it.
I. e., for both drivers, call xdp_rxq_info_is_reg before the first
allocation took place, and just return immediately from there if it
fails. Everything else complicates the code unnecessarily.
> As I mentioned a bit above, `res` is unused here as an error code,
> only to test the value on < 0. Does it make sense to add a new
> variable?
Following my above sugggestion, res will be used as error code, so
it should stay.
I'll provide a matching patchset later today.
Thanks,
Corinna
^ permalink raw reply [flat|nested] 16+ messages in thread* [Intel-wired-lan] [PATCH 2/3 net-next v5] igb: refactor XDP registration
@ 2022-01-19 6:08 ` Corinna Vinschen
0 siblings, 0 replies; 16+ messages in thread
From: Corinna Vinschen @ 2022-01-19 6:08 UTC (permalink / raw)
To: intel-wired-lan
On Jan 18 16:05, Alexander Lobakin wrote:
> From: Corinna Vinschen <vinschen@redhat.com>
> Date: Mon, 17 Jan 2022 19:29:14 +0100
>
> > 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;
>
> Error path always returns -ENOMEM, even in this case, and reports
> that it failed to allocate memory for rings. Handle this correctly
> and return `res` instead and without one more error message?
In that case, it makes sense to revert the code to the way igc did it,
rather then trying to do as igb did it.
I. e., for both drivers, call xdp_rxq_info_is_reg before the first
allocation took place, and just return immediately from there if it
fails. Everything else complicates the code unnecessarily.
> As I mentioned a bit above, `res` is unused here as an error code,
> only to test the value on < 0. Does it make sense to add a new
> variable?
Following my above sugggestion, res will be used as error code, so
it should stay.
I'll provide a matching patchset later today.
Thanks,
Corinna
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/3 net-next v5] igb/igc: RX queues: fix DMA leak in error case
2022-01-17 18:29 ` [Intel-wired-lan] " Corinna Vinschen
@ 2022-01-17 18:29 ` Corinna Vinschen
-1 siblings, 0 replies; 16+ messages in thread
From: Corinna Vinschen @ 2022-01-17 18:29 UTC (permalink / raw)
To: intel-wired-lan, netdev, Vinicius Costa Gomes
Cc: Lennert Buytenhek, Alexander Lobakin
When setting up the rx queues, igb and igc neglect to free DMA memory
in error case. Add matching dma_free_coherent calls.
Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
---
drivers/net/ethernet/intel/igb/igb_main.c | 1 +
drivers/net/ethernet/intel/igc/igc_main.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index cea89d301bfd..343568d4ff7f 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -4389,6 +4389,7 @@ int igb_setup_rx_resources(struct igb_ring *rx_ring)
return 0;
err:
+ dma_free_coherent(dev, rx_ring->size, rx_ring->desc, rx_ring->dma);
vfree(rx_ring->rx_buffer_info);
rx_ring->rx_buffer_info = NULL;
dev_err(dev, "Unable to allocate memory for the Rx descriptor ring\n");
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 56ed0f1463e5..f323cec0b74f 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -540,6 +540,7 @@ int igc_setup_rx_resources(struct igc_ring *rx_ring)
return 0;
err:
+ dma_free_coherent(dev, rx_ring->size, rx_ring->desc, rx_ring->dma);
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] 16+ messages in thread
* [Intel-wired-lan] [PATCH 3/3 net-next v5] igb/igc: RX queues: fix DMA leak in error case
@ 2022-01-17 18:29 ` Corinna Vinschen
0 siblings, 0 replies; 16+ messages in thread
From: Corinna Vinschen @ 2022-01-17 18:29 UTC (permalink / raw)
To: intel-wired-lan
When setting up the rx queues, igb and igc neglect to free DMA memory
in error case. Add matching dma_free_coherent calls.
Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
---
drivers/net/ethernet/intel/igb/igb_main.c | 1 +
drivers/net/ethernet/intel/igc/igc_main.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index cea89d301bfd..343568d4ff7f 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -4389,6 +4389,7 @@ int igb_setup_rx_resources(struct igb_ring *rx_ring)
return 0;
err:
+ dma_free_coherent(dev, rx_ring->size, rx_ring->desc, rx_ring->dma);
vfree(rx_ring->rx_buffer_info);
rx_ring->rx_buffer_info = NULL;
dev_err(dev, "Unable to allocate memory for the Rx descriptor ring\n");
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 56ed0f1463e5..f323cec0b74f 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -540,6 +540,7 @@ int igc_setup_rx_resources(struct igc_ring *rx_ring)
return 0;
err:
+ dma_free_coherent(dev, rx_ring->size, rx_ring->desc, rx_ring->dma);
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] 16+ messages in thread
* Re: [PATCH 3/3 net-next v5] igb/igc: RX queues: fix DMA leak in error case
2022-01-17 18:29 ` [Intel-wired-lan] " Corinna Vinschen
@ 2022-01-18 6:01 ` Lennert Buytenhek
-1 siblings, 0 replies; 16+ messages in thread
From: Lennert Buytenhek @ 2022-01-18 6:01 UTC (permalink / raw)
To: Corinna Vinschen
Cc: intel-wired-lan, netdev, Vinicius Costa Gomes, Alexander Lobakin
On Mon, Jan 17, 2022 at 07:29:15PM +0100, Corinna Vinschen wrote:
> When setting up the rx queues, igb and igc neglect to free DMA memory
> in error case. Add matching dma_free_coherent calls.
>
> Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
> ---
> drivers/net/ethernet/intel/igb/igb_main.c | 1 +
> drivers/net/ethernet/intel/igc/igc_main.c | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index cea89d301bfd..343568d4ff7f 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -4389,6 +4389,7 @@ int igb_setup_rx_resources(struct igb_ring *rx_ring)
> return 0;
>
> err:
> + dma_free_coherent(dev, rx_ring->size, rx_ring->desc, rx_ring->dma);
> vfree(rx_ring->rx_buffer_info);
> rx_ring->rx_buffer_info = NULL;
> dev_err(dev, "Unable to allocate memory for the Rx descriptor ring\n");
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 56ed0f1463e5..f323cec0b74f 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -540,6 +540,7 @@ int igc_setup_rx_resources(struct igc_ring *rx_ring)
> return 0;
>
> err:
> + dma_free_coherent(dev, rx_ring->size, rx_ring->desc, rx_ring->dma);
> vfree(rx_ring->rx_buffer_info);
> rx_ring->rx_buffer_info = NULL;
> netdev_err(ndev, "Unable to allocate memory for Rx descriptor ring\n");
If the vzalloc() call in igc_setup_rx_resources() fails, and we jump
to 'err' before dma_alloc_coherent() was reached, won't dma_free_coherent()
be called inadvertently here?
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Intel-wired-lan] [PATCH 3/3 net-next v5] igb/igc: RX queues: fix DMA leak in error case
@ 2022-01-18 6:01 ` Lennert Buytenhek
0 siblings, 0 replies; 16+ messages in thread
From: Lennert Buytenhek @ 2022-01-18 6:01 UTC (permalink / raw)
To: intel-wired-lan
On Mon, Jan 17, 2022 at 07:29:15PM +0100, Corinna Vinschen wrote:
> When setting up the rx queues, igb and igc neglect to free DMA memory
> in error case. Add matching dma_free_coherent calls.
>
> Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
> ---
> drivers/net/ethernet/intel/igb/igb_main.c | 1 +
> drivers/net/ethernet/intel/igc/igc_main.c | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index cea89d301bfd..343568d4ff7f 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -4389,6 +4389,7 @@ int igb_setup_rx_resources(struct igb_ring *rx_ring)
> return 0;
>
> err:
> + dma_free_coherent(dev, rx_ring->size, rx_ring->desc, rx_ring->dma);
> vfree(rx_ring->rx_buffer_info);
> rx_ring->rx_buffer_info = NULL;
> dev_err(dev, "Unable to allocate memory for the Rx descriptor ring\n");
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 56ed0f1463e5..f323cec0b74f 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -540,6 +540,7 @@ int igc_setup_rx_resources(struct igc_ring *rx_ring)
> return 0;
>
> err:
> + dma_free_coherent(dev, rx_ring->size, rx_ring->desc, rx_ring->dma);
> vfree(rx_ring->rx_buffer_info);
> rx_ring->rx_buffer_info = NULL;
> netdev_err(ndev, "Unable to allocate memory for Rx descriptor ring\n");
If the vzalloc() call in igc_setup_rx_resources() fails, and we jump
to 'err' before dma_alloc_coherent() was reached, won't dma_free_coherent()
be called inadvertently here?
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3 net-next v5] igb/igc: RX queues: fix DMA leak in error case
2022-01-18 6:01 ` [Intel-wired-lan] " Lennert Buytenhek
@ 2022-01-18 8:26 ` Corinna Vinschen
-1 siblings, 0 replies; 16+ messages in thread
From: Corinna Vinschen @ 2022-01-18 8:26 UTC (permalink / raw)
To: Lennert Buytenhek
Cc: intel-wired-lan, netdev, Vinicius Costa Gomes, Alexander Lobakin
On Jan 18 08:01, Lennert Buytenhek wrote:
> On Mon, Jan 17, 2022 at 07:29:15PM +0100, Corinna Vinschen wrote:
>
> > When setting up the rx queues, igb and igc neglect to free DMA memory
> > in error case. Add matching dma_free_coherent calls.
> >
> > Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
> > ---
> > drivers/net/ethernet/intel/igb/igb_main.c | 1 +
> > drivers/net/ethernet/intel/igc/igc_main.c | 1 +
> > 2 files changed, 2 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> > index cea89d301bfd..343568d4ff7f 100644
> > --- a/drivers/net/ethernet/intel/igb/igb_main.c
> > +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> > @@ -4389,6 +4389,7 @@ int igb_setup_rx_resources(struct igb_ring *rx_ring)
> > return 0;
> >
> > err:
> > + dma_free_coherent(dev, rx_ring->size, rx_ring->desc, rx_ring->dma);
> > vfree(rx_ring->rx_buffer_info);
> > rx_ring->rx_buffer_info = NULL;
> > dev_err(dev, "Unable to allocate memory for the Rx descriptor ring\n");
> > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> > index 56ed0f1463e5..f323cec0b74f 100644
> > --- a/drivers/net/ethernet/intel/igc/igc_main.c
> > +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> > @@ -540,6 +540,7 @@ int igc_setup_rx_resources(struct igc_ring *rx_ring)
> > return 0;
> >
> > err:
> > + dma_free_coherent(dev, rx_ring->size, rx_ring->desc, rx_ring->dma);
> > vfree(rx_ring->rx_buffer_info);
> > rx_ring->rx_buffer_info = NULL;
> > netdev_err(ndev, "Unable to allocate memory for Rx descriptor ring\n");
>
> If the vzalloc() call in igc_setup_rx_resources() fails, and we jump
> to 'err' before dma_alloc_coherent() was reached, won't dma_free_coherent()
> be called inadvertently here?
These calls all check for a NULL pointer themselves.
Corinna
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Intel-wired-lan] [PATCH 3/3 net-next v5] igb/igc: RX queues: fix DMA leak in error case
@ 2022-01-18 8:26 ` Corinna Vinschen
0 siblings, 0 replies; 16+ messages in thread
From: Corinna Vinschen @ 2022-01-18 8:26 UTC (permalink / raw)
To: intel-wired-lan
On Jan 18 08:01, Lennert Buytenhek wrote:
> On Mon, Jan 17, 2022 at 07:29:15PM +0100, Corinna Vinschen wrote:
>
> > When setting up the rx queues, igb and igc neglect to free DMA memory
> > in error case. Add matching dma_free_coherent calls.
> >
> > Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
> > ---
> > drivers/net/ethernet/intel/igb/igb_main.c | 1 +
> > drivers/net/ethernet/intel/igc/igc_main.c | 1 +
> > 2 files changed, 2 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> > index cea89d301bfd..343568d4ff7f 100644
> > --- a/drivers/net/ethernet/intel/igb/igb_main.c
> > +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> > @@ -4389,6 +4389,7 @@ int igb_setup_rx_resources(struct igb_ring *rx_ring)
> > return 0;
> >
> > err:
> > + dma_free_coherent(dev, rx_ring->size, rx_ring->desc, rx_ring->dma);
> > vfree(rx_ring->rx_buffer_info);
> > rx_ring->rx_buffer_info = NULL;
> > dev_err(dev, "Unable to allocate memory for the Rx descriptor ring\n");
> > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> > index 56ed0f1463e5..f323cec0b74f 100644
> > --- a/drivers/net/ethernet/intel/igc/igc_main.c
> > +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> > @@ -540,6 +540,7 @@ int igc_setup_rx_resources(struct igc_ring *rx_ring)
> > return 0;
> >
> > err:
> > + dma_free_coherent(dev, rx_ring->size, rx_ring->desc, rx_ring->dma);
> > vfree(rx_ring->rx_buffer_info);
> > rx_ring->rx_buffer_info = NULL;
> > netdev_err(ndev, "Unable to allocate memory for Rx descriptor ring\n");
>
> If the vzalloc() call in igc_setup_rx_resources() fails, and we jump
> to 'err' before dma_alloc_coherent() was reached, won't dma_free_coherent()
> be called inadvertently here?
These calls all check for a NULL pointer themselves.
Corinna
^ permalink raw reply [flat|nested] 16+ messages in thread