All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v1 net 0/4] ENA driver bug fixes
@ 2024-04-10  9:13 darinzon
  2024-04-10  9:13 ` [PATCH v1 net 1/4] net: ena: Fix potential sign extension issue darinzon
                   ` (4 more replies)
  0 siblings, 5 replies; 10+ messages in thread
From: darinzon @ 2024-04-10  9:13 UTC (permalink / raw)
  To: David Miller, Jakub Kicinski, netdev
  Cc: David Arinzon, Woodhouse, David, Machulsky, Zorik, Matushevsky,
	Alexander, Saeed Bshara, Wilson, Matt, Liguori, Anthony, Bshara,
	Nafea, Belgazal, Netanel, Saidi, Ali, Herrenschmidt, Benjamin,
	Kiyanovski, Arthur, Dagan, Noam, Agroskin, Shay, Itzko, Shahar,
	Abboud, Osama, Ostrovsky, Evgeny, Tabachnik, Ofir,
	Netanel Belgazal, Sameeh Jubran

From: David Arinzon <darinzon@amazon.com>

This patchset contains multiple bug fixes for the
ENA driver.

David Arinzon (4):
  net: ena: Fix potential sign extension issue
  net: ena: Wrong missing IO completions check order
  net: ena: Fix incorrect descriptor free behavior
  net: ena: Set tx_info->xdpf value to NULL

 drivers/net/ethernet/amazon/ena/ena_com.c    |  2 +-
 drivers/net/ethernet/amazon/ena/ena_netdev.c | 35 +++++++++++++-------
 drivers/net/ethernet/amazon/ena/ena_xdp.c    |  4 ++-
 3 files changed, 27 insertions(+), 14 deletions(-)

-- 
2.40.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v1 net 1/4] net: ena: Fix potential sign extension issue
  2024-04-10  9:13 [PATCH v1 net 0/4] ENA driver bug fixes darinzon
@ 2024-04-10  9:13 ` darinzon
  2024-04-10 21:49   ` Nelson, Shannon
  2024-04-10  9:13 ` [PATCH v1 net 2/4] net: ena: Wrong missing IO completions check order darinzon
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 10+ messages in thread
From: darinzon @ 2024-04-10  9:13 UTC (permalink / raw)
  To: David Miller, Jakub Kicinski, netdev
  Cc: David Arinzon, Woodhouse, David, Machulsky, Zorik, Matushevsky,
	Alexander, Saeed Bshara, Wilson, Matt, Liguori, Anthony, Bshara,
	Nafea, Belgazal, Netanel, Saidi, Ali, Herrenschmidt, Benjamin,
	Kiyanovski, Arthur, Dagan, Noam, Agroskin, Shay, Itzko, Shahar,
	Abboud, Osama, Ostrovsky, Evgeny, Tabachnik, Ofir,
	Netanel Belgazal, Sameeh Jubran

From: David Arinzon <darinzon@amazon.com>

Small unsigned types are promoted to larger signed types in
the case of multiplication, the result of which may overflow.
In case the result of such a multiplication has its MSB
turned on, it will be sign extended with '1's.
This changes the multiplication result.

Code example of the phenomenon:
-------------------------------
u16 x, y;
size_t z1, z2;

x = y = 0xffff;
printk("x=%x y=%x\n",x,y);

z1 = x*y;
z2 = (size_t)x*y;

printk("z1=%lx z2=%lx\n", z1, z2);

Output:
-------
x=ffff y=ffff
z1=fffffffffffe0001 z2=fffe0001

The expected result of ffff*ffff is fffe0001, and without the
explicit casting to avoid the unwanted sign extension we got
fffffffffffe0001.

This commit adds an explicit casting to avoid the sign extension
issue.

Fixes: 689b2bdaaa14 ("net: ena: add functions for handling Low Latency Queues in ena_com")
Signed-off-by: Arthur Kiyanovski <akiyano@amazon.com>
Signed-off-by: David Arinzon <darinzon@amazon.com>
---
 drivers/net/ethernet/amazon/ena/ena_com.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c b/drivers/net/ethernet/amazon/ena/ena_com.c
index 9e9e4a03..2d8a66ea 100644
--- a/drivers/net/ethernet/amazon/ena/ena_com.c
+++ b/drivers/net/ethernet/amazon/ena/ena_com.c
@@ -351,7 +351,7 @@ static int ena_com_init_io_sq(struct ena_com_dev *ena_dev,
 			ENA_COM_BOUNCE_BUFFER_CNTRL_CNT;
 		io_sq->bounce_buf_ctrl.next_to_use = 0;
 
-		size = io_sq->bounce_buf_ctrl.buffer_size *
+		size = (size_t)io_sq->bounce_buf_ctrl.buffer_size *
 			io_sq->bounce_buf_ctrl.buffers_num;
 
 		dev_node = dev_to_node(ena_dev->dmadev);
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v1 net 2/4] net: ena: Wrong missing IO completions check order
  2024-04-10  9:13 [PATCH v1 net 0/4] ENA driver bug fixes darinzon
  2024-04-10  9:13 ` [PATCH v1 net 1/4] net: ena: Fix potential sign extension issue darinzon
@ 2024-04-10  9:13 ` darinzon
  2024-04-10 21:51   ` Nelson, Shannon
  2024-04-10  9:13 ` [PATCH v1 net 3/4] net: ena: Fix incorrect descriptor free behavior darinzon
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 10+ messages in thread
From: darinzon @ 2024-04-10  9:13 UTC (permalink / raw)
  To: David Miller, Jakub Kicinski, netdev
  Cc: David Arinzon, Woodhouse, David, Machulsky, Zorik, Matushevsky,
	Alexander, Saeed Bshara, Wilson, Matt, Liguori, Anthony, Bshara,
	Nafea, Belgazal, Netanel, Saidi, Ali, Herrenschmidt, Benjamin,
	Kiyanovski, Arthur, Dagan, Noam, Agroskin, Shay, Itzko, Shahar,
	Abboud, Osama, Ostrovsky, Evgeny, Tabachnik, Ofir,
	Netanel Belgazal, Sameeh Jubran, Amit Bernstein

From: David Arinzon <darinzon@amazon.com>

Missing IO completions check is called every second (HZ jiffies).
This commit fixes several issues with this check:

1. Duplicate queues check:
   Max of 4 queues are scanned on each check due to monitor budget.
   Once reaching the budget, this check exits under the assumption that
   the next check will continue to scan the remainder of the queues,
   but in practice, next check will first scan the last already scanned
   queue which is not necessary and may cause the full queue scan to
   last a couple of seconds longer.
   The fix is to start every check with the next queue to scan.
   For example, on 8 IO queues:
   Bug: [0,1,2,3], [3,4,5,6], [6,7]
   Fix: [0,1,2,3], [4,5,6,7]

2. Unbalanced queues check:
   In case the number of active IO queues is not a multiple of budget,
   there will be checks which don't utilize the full budget
   because the full scan exits when reaching the last queue id.
   The fix is to run every TX completion check with exact queue budget
   regardless of the queue id.
   For example, on 7 IO queues:
   Bug: [0,1,2,3], [4,5,6], [0,1,2,3]
   Fix: [0,1,2,3], [4,5,6,0], [1,2,3,4]
   The budget may be lowered in case the number of IO queues is less
   than the budget (4) to make sure there are no duplicate queues on
   the same check.
   For example, on 3 IO queues:
   Bug: [0,1,2,0], [1,2,0,1]
   Fix: [0,1,2], [0,1,2]

Fixes: 1738cd3ed342 ("net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)")
Signed-off-by: Amit Bernstein <amitbern@amazon.com>
Signed-off-by: David Arinzon <darinzon@amazon.com>
---
 drivers/net/ethernet/amazon/ena/ena_netdev.c | 21 +++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index 09e7da1a..59befc0f 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -3481,10 +3481,11 @@ static void check_for_missing_completions(struct ena_adapter *adapter)
 {
 	struct ena_ring *tx_ring;
 	struct ena_ring *rx_ring;
-	int i, budget, rc;
+	int qid, budget, rc;
 	int io_queue_count;
 
 	io_queue_count = adapter->xdp_num_queues + adapter->num_io_queues;
+
 	/* Make sure the driver doesn't turn the device in other process */
 	smp_rmb();
 
@@ -3497,27 +3498,29 @@ static void check_for_missing_completions(struct ena_adapter *adapter)
 	if (adapter->missing_tx_completion_to == ENA_HW_HINTS_NO_TIMEOUT)
 		return;
 
-	budget = ENA_MONITORED_TX_QUEUES;
+	budget = min_t(u32, io_queue_count, ENA_MONITORED_TX_QUEUES);
 
-	for (i = adapter->last_monitored_tx_qid; i < io_queue_count; i++) {
-		tx_ring = &adapter->tx_ring[i];
-		rx_ring = &adapter->rx_ring[i];
+	qid = adapter->last_monitored_tx_qid;
+
+	while (budget) {
+		qid = (qid + 1) % io_queue_count;
+
+		tx_ring = &adapter->tx_ring[qid];
+		rx_ring = &adapter->rx_ring[qid];
 
 		rc = check_missing_comp_in_tx_queue(adapter, tx_ring);
 		if (unlikely(rc))
 			return;
 
-		rc =  !ENA_IS_XDP_INDEX(adapter, i) ?
+		rc =  !ENA_IS_XDP_INDEX(adapter, qid) ?
 			check_for_rx_interrupt_queue(adapter, rx_ring) : 0;
 		if (unlikely(rc))
 			return;
 
 		budget--;
-		if (!budget)
-			break;
 	}
 
-	adapter->last_monitored_tx_qid = i % io_queue_count;
+	adapter->last_monitored_tx_qid = qid;
 }
 
 /* trigger napi schedule after 2 consecutive detections */
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v1 net 3/4] net: ena: Fix incorrect descriptor free behavior
  2024-04-10  9:13 [PATCH v1 net 0/4] ENA driver bug fixes darinzon
  2024-04-10  9:13 ` [PATCH v1 net 1/4] net: ena: Fix potential sign extension issue darinzon
  2024-04-10  9:13 ` [PATCH v1 net 2/4] net: ena: Wrong missing IO completions check order darinzon
@ 2024-04-10  9:13 ` darinzon
  2024-04-10 21:52   ` Nelson, Shannon
  2024-04-10  9:13 ` [PATCH v1 net 4/4] net: ena: Set tx_info->xdpf value to NULL darinzon
  2024-04-11  9:40 ` [PATCH v1 net 0/4] ENA driver bug fixes patchwork-bot+netdevbpf
  4 siblings, 1 reply; 10+ messages in thread
From: darinzon @ 2024-04-10  9:13 UTC (permalink / raw)
  To: David Miller, Jakub Kicinski, netdev
  Cc: David Arinzon, Woodhouse, David, Machulsky, Zorik, Matushevsky,
	Alexander, Saeed Bshara, Wilson, Matt, Liguori, Anthony, Bshara,
	Nafea, Belgazal, Netanel, Saidi, Ali, Herrenschmidt, Benjamin,
	Kiyanovski, Arthur, Dagan, Noam, Agroskin, Shay, Itzko, Shahar,
	Abboud, Osama, Ostrovsky, Evgeny, Tabachnik, Ofir,
	Netanel Belgazal, Sameeh Jubran

From: David Arinzon <darinzon@amazon.com>

ENA has two types of TX queues:
- queues which only process TX packets arriving from the network stack
- queues which only process TX packets forwarded to it by XDP_REDIRECT
  or XDP_TX instructions

The ena_free_tx_bufs() cycles through all descriptors in a TX queue
and unmaps + frees every descriptor that hasn't been acknowledged yet
by the device (uncompleted TX transactions).
The function assumes that the processed TX queue is necessarily from
the first category listed above and ends up using napi_consume_skb()
for descriptors belonging to an XDP specific queue.

This patch solves a bug in which, in case of a VF reset, the
descriptors aren't freed correctly, leading to crashes.

Fixes: 548c4940b9f1 ("net: ena: Implement XDP_TX action")
Signed-off-by: Shay Agroskin <shayagr@amazon.com>
Signed-off-by: David Arinzon <darinzon@amazon.com>
---
 drivers/net/ethernet/amazon/ena/ena_netdev.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index 59befc0f..be5acfa4 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -718,8 +718,11 @@ void ena_unmap_tx_buff(struct ena_ring *tx_ring,
 static void ena_free_tx_bufs(struct ena_ring *tx_ring)
 {
 	bool print_once = true;
+	bool is_xdp_ring;
 	u32 i;
 
+	is_xdp_ring = ENA_IS_XDP_INDEX(tx_ring->adapter, tx_ring->qid);
+
 	for (i = 0; i < tx_ring->ring_size; i++) {
 		struct ena_tx_buffer *tx_info = &tx_ring->tx_buffer_info[i];
 
@@ -739,10 +742,15 @@ static void ena_free_tx_bufs(struct ena_ring *tx_ring)
 
 		ena_unmap_tx_buff(tx_ring, tx_info);
 
-		dev_kfree_skb_any(tx_info->skb);
+		if (is_xdp_ring)
+			xdp_return_frame(tx_info->xdpf);
+		else
+			dev_kfree_skb_any(tx_info->skb);
 	}
-	netdev_tx_reset_queue(netdev_get_tx_queue(tx_ring->netdev,
-						  tx_ring->qid));
+
+	if (!is_xdp_ring)
+		netdev_tx_reset_queue(netdev_get_tx_queue(tx_ring->netdev,
+							  tx_ring->qid));
 }
 
 static void ena_free_all_tx_bufs(struct ena_adapter *adapter)
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v1 net 4/4] net: ena: Set tx_info->xdpf value to NULL
  2024-04-10  9:13 [PATCH v1 net 0/4] ENA driver bug fixes darinzon
                   ` (2 preceding siblings ...)
  2024-04-10  9:13 ` [PATCH v1 net 3/4] net: ena: Fix incorrect descriptor free behavior darinzon
@ 2024-04-10  9:13 ` darinzon
  2024-04-10 21:53   ` Nelson, Shannon
  2024-04-11  9:40 ` [PATCH v1 net 0/4] ENA driver bug fixes patchwork-bot+netdevbpf
  4 siblings, 1 reply; 10+ messages in thread
From: darinzon @ 2024-04-10  9:13 UTC (permalink / raw)
  To: David Miller, Jakub Kicinski, netdev
  Cc: David Arinzon, Woodhouse, David, Machulsky, Zorik, Matushevsky,
	Alexander, Saeed Bshara, Wilson, Matt, Liguori, Anthony, Bshara,
	Nafea, Belgazal, Netanel, Saidi, Ali, Herrenschmidt, Benjamin,
	Kiyanovski, Arthur, Dagan, Noam, Agroskin, Shay, Itzko, Shahar,
	Abboud, Osama, Ostrovsky, Evgeny, Tabachnik, Ofir,
	Netanel Belgazal, Sameeh Jubran

From: David Arinzon <darinzon@amazon.com>

The patch mentioned in the `Fixes` tag removed the explicit assignment
of tx_info->xdpf to NULL with the justification that there's no need
to set tx_info->xdpf to NULL and tx_info->num_of_bufs to 0 in case
of a mapping error. Both values won't be used once the mapping function
returns an error, and their values would be overridden by the next
transmitted packet.

While both values do indeed get overridden in the next transmission
call, the value of tx_info->xdpf is also used to check whether a TX
descriptor's transmission has been completed (i.e. a completion for it
was polled).

An example scenario:
1. Mapping failed, tx_info->xdpf wasn't set to NULL
2. A VF reset occurred leading to IO resource destruction and
   a call to ena_free_tx_bufs() function
3. Although the descriptor whose mapping failed was freed by the
   transmission function, it still passes the check
     if (!tx_info->skb)

   (skb and xdp_frame are in a union)
4. The xdp_frame associated with the descriptor is freed twice

This patch returns the assignment of NULL to tx_info->xdpf to make the
cleaning function knows that the descriptor is already freed.

Fixes: 504fd6a5390c ("net: ena: fix DMA mapping function issues in XDP")
Signed-off-by: Shay Agroskin <shayagr@amazon.com>
Signed-off-by: David Arinzon <darinzon@amazon.com>
---
 drivers/net/ethernet/amazon/ena/ena_xdp.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_xdp.c b/drivers/net/ethernet/amazon/ena/ena_xdp.c
index 337c435d..5b175e7e 100644
--- a/drivers/net/ethernet/amazon/ena/ena_xdp.c
+++ b/drivers/net/ethernet/amazon/ena/ena_xdp.c
@@ -89,7 +89,7 @@ int ena_xdp_xmit_frame(struct ena_ring *tx_ring,
 
 	rc = ena_xdp_tx_map_frame(tx_ring, tx_info, xdpf, &ena_tx_ctx);
 	if (unlikely(rc))
-		return rc;
+		goto err;
 
 	ena_tx_ctx.req_id = req_id;
 
@@ -112,7 +112,9 @@ int ena_xdp_xmit_frame(struct ena_ring *tx_ring,
 
 error_unmap_dma:
 	ena_unmap_tx_buff(tx_ring, tx_info);
+err:
 	tx_info->xdpf = NULL;
+
 	return rc;
 }
 
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 net 1/4] net: ena: Fix potential sign extension issue
  2024-04-10  9:13 ` [PATCH v1 net 1/4] net: ena: Fix potential sign extension issue darinzon
@ 2024-04-10 21:49   ` Nelson, Shannon
  0 siblings, 0 replies; 10+ messages in thread
From: Nelson, Shannon @ 2024-04-10 21:49 UTC (permalink / raw)
  To: darinzon, David Miller, Jakub Kicinski, netdev
  Cc: Woodhouse, David, Machulsky, Zorik, Matushevsky, Alexander,
	Saeed Bshara, Wilson, Matt, Liguori, Anthony, Bshara, Nafea,
	Belgazal, Netanel, Saidi, Ali, Herrenschmidt, Benjamin,
	Kiyanovski, Arthur, Dagan, Noam, Agroskin, Shay, Itzko, Shahar,
	Abboud, Osama, Ostrovsky, Evgeny, Tabachnik, Ofir,
	Netanel Belgazal, Sameeh Jubran

On 4/10/2024 2:13 AM, darinzon@amazon.com wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> From: David Arinzon <darinzon@amazon.com>
> 
> Small unsigned types are promoted to larger signed types in
> the case of multiplication, the result of which may overflow.
> In case the result of such a multiplication has its MSB
> turned on, it will be sign extended with '1's.
> This changes the multiplication result.
> 
> Code example of the phenomenon:
> -------------------------------
> u16 x, y;
> size_t z1, z2;
> 
> x = y = 0xffff;
> printk("x=%x y=%x\n",x,y);
> 
> z1 = x*y;
> z2 = (size_t)x*y;
> 
> printk("z1=%lx z2=%lx\n", z1, z2);
> 
> Output:
> -------
> x=ffff y=ffff
> z1=fffffffffffe0001 z2=fffe0001
> 
> The expected result of ffff*ffff is fffe0001, and without the
> explicit casting to avoid the unwanted sign extension we got
> fffffffffffe0001.
> 
> This commit adds an explicit casting to avoid the sign extension
> issue.
> 
> Fixes: 689b2bdaaa14 ("net: ena: add functions for handling Low Latency Queues in ena_com")
> Signed-off-by: Arthur Kiyanovski <akiyano@amazon.com>
> Signed-off-by: David Arinzon <darinzon@amazon.com>

Reviewed-by: Shannon Nelson <shannon.nelson@amd.com>

> ---
>   drivers/net/ethernet/amazon/ena/ena_com.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c b/drivers/net/ethernet/amazon/ena/ena_com.c
> index 9e9e4a03..2d8a66ea 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_com.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_com.c
> @@ -351,7 +351,7 @@ static int ena_com_init_io_sq(struct ena_com_dev *ena_dev,
>                          ENA_COM_BOUNCE_BUFFER_CNTRL_CNT;
>                  io_sq->bounce_buf_ctrl.next_to_use = 0;
> 
> -               size = io_sq->bounce_buf_ctrl.buffer_size *
> +               size = (size_t)io_sq->bounce_buf_ctrl.buffer_size *
>                          io_sq->bounce_buf_ctrl.buffers_num;
> 
>                  dev_node = dev_to_node(ena_dev->dmadev);
> --
> 2.40.1
> 
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 net 2/4] net: ena: Wrong missing IO completions check order
  2024-04-10  9:13 ` [PATCH v1 net 2/4] net: ena: Wrong missing IO completions check order darinzon
@ 2024-04-10 21:51   ` Nelson, Shannon
  0 siblings, 0 replies; 10+ messages in thread
From: Nelson, Shannon @ 2024-04-10 21:51 UTC (permalink / raw)
  To: darinzon, David Miller, Jakub Kicinski, netdev
  Cc: Woodhouse, David, Machulsky, Zorik, Matushevsky, Alexander,
	Saeed Bshara, Wilson, Matt, Liguori, Anthony, Bshara, Nafea,
	Belgazal, Netanel, Saidi, Ali, Herrenschmidt, Benjamin,
	Kiyanovski, Arthur, Dagan, Noam, Agroskin, Shay, Itzko, Shahar,
	Abboud, Osama, Ostrovsky, Evgeny, Tabachnik, Ofir,
	Netanel Belgazal, Sameeh Jubran, Amit Bernstein

On 4/10/2024 2:13 AM, darinzon@amazon.com wrote:
> From: David Arinzon <darinzon@amazon.com>
> 
> Missing IO completions check is called every second (HZ jiffies).
> This commit fixes several issues with this check:
> 
> 1. Duplicate queues check:
>     Max of 4 queues are scanned on each check due to monitor budget.
>     Once reaching the budget, this check exits under the assumption that
>     the next check will continue to scan the remainder of the queues,
>     but in practice, next check will first scan the last already scanned
>     queue which is not necessary and may cause the full queue scan to
>     last a couple of seconds longer.
>     The fix is to start every check with the next queue to scan.
>     For example, on 8 IO queues:
>     Bug: [0,1,2,3], [3,4,5,6], [6,7]
>     Fix: [0,1,2,3], [4,5,6,7]
> 
> 2. Unbalanced queues check:
>     In case the number of active IO queues is not a multiple of budget,
>     there will be checks which don't utilize the full budget
>     because the full scan exits when reaching the last queue id.
>     The fix is to run every TX completion check with exact queue budget
>     regardless of the queue id.
>     For example, on 7 IO queues:
>     Bug: [0,1,2,3], [4,5,6], [0,1,2,3]
>     Fix: [0,1,2,3], [4,5,6,0], [1,2,3,4]
>     The budget may be lowered in case the number of IO queues is less
>     than the budget (4) to make sure there are no duplicate queues on
>     the same check.
>     For example, on 3 IO queues:
>     Bug: [0,1,2,0], [1,2,0,1]
>     Fix: [0,1,2], [0,1,2]
> 
> Fixes: 1738cd3ed342 ("net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)")
> Signed-off-by: Amit Bernstein <amitbern@amazon.com>
> Signed-off-by: David Arinzon <darinzon@amazon.com>


Reviewed-by: Shannon Nelson <shannon.nelson@amd.com>


> ---
>   drivers/net/ethernet/amazon/ena/ena_netdev.c | 21 +++++++++++---------
>   1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> index 09e7da1a..59befc0f 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> @@ -3481,10 +3481,11 @@ static void check_for_missing_completions(struct ena_adapter *adapter)
>   {
>          struct ena_ring *tx_ring;
>          struct ena_ring *rx_ring;
> -       int i, budget, rc;
> +       int qid, budget, rc;
>          int io_queue_count;
> 
>          io_queue_count = adapter->xdp_num_queues + adapter->num_io_queues;
> +
>          /* Make sure the driver doesn't turn the device in other process */
>          smp_rmb();
> 
> @@ -3497,27 +3498,29 @@ static void check_for_missing_completions(struct ena_adapter *adapter)
>          if (adapter->missing_tx_completion_to == ENA_HW_HINTS_NO_TIMEOUT)
>                  return;
> 
> -       budget = ENA_MONITORED_TX_QUEUES;
> +       budget = min_t(u32, io_queue_count, ENA_MONITORED_TX_QUEUES);
> 
> -       for (i = adapter->last_monitored_tx_qid; i < io_queue_count; i++) {
> -               tx_ring = &adapter->tx_ring[i];
> -               rx_ring = &adapter->rx_ring[i];
> +       qid = adapter->last_monitored_tx_qid;
> +
> +       while (budget) {
> +               qid = (qid + 1) % io_queue_count;
> +
> +               tx_ring = &adapter->tx_ring[qid];
> +               rx_ring = &adapter->rx_ring[qid];
> 
>                  rc = check_missing_comp_in_tx_queue(adapter, tx_ring);
>                  if (unlikely(rc))
>                          return;
> 
> -               rc =  !ENA_IS_XDP_INDEX(adapter, i) ?
> +               rc =  !ENA_IS_XDP_INDEX(adapter, qid) ?
>                          check_for_rx_interrupt_queue(adapter, rx_ring) : 0;
>                  if (unlikely(rc))
>                          return;
> 
>                  budget--;
> -               if (!budget)
> -                       break;
>          }
> 
> -       adapter->last_monitored_tx_qid = i % io_queue_count;
> +       adapter->last_monitored_tx_qid = qid;
>   }
> 
>   /* trigger napi schedule after 2 consecutive detections */
> --
> 2.40.1
> 
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 net 3/4] net: ena: Fix incorrect descriptor free behavior
  2024-04-10  9:13 ` [PATCH v1 net 3/4] net: ena: Fix incorrect descriptor free behavior darinzon
@ 2024-04-10 21:52   ` Nelson, Shannon
  0 siblings, 0 replies; 10+ messages in thread
From: Nelson, Shannon @ 2024-04-10 21:52 UTC (permalink / raw)
  To: darinzon, David Miller, Jakub Kicinski, netdev
  Cc: Woodhouse, David, Machulsky, Zorik, Matushevsky, Alexander,
	Saeed Bshara, Wilson, Matt, Liguori, Anthony, Bshara, Nafea,
	Belgazal, Netanel, Saidi, Ali, Herrenschmidt, Benjamin,
	Kiyanovski, Arthur, Dagan, Noam, Agroskin, Shay, Itzko, Shahar,
	Abboud, Osama, Ostrovsky, Evgeny, Tabachnik, Ofir,
	Netanel Belgazal, Sameeh Jubran

On 4/10/2024 2:13 AM, darinzon@amazon.com wrote:
> From: David Arinzon <darinzon@amazon.com>
> 
> ENA has two types of TX queues:
> - queues which only process TX packets arriving from the network stack
> - queues which only process TX packets forwarded to it by XDP_REDIRECT
>    or XDP_TX instructions
> 
> The ena_free_tx_bufs() cycles through all descriptors in a TX queue
> and unmaps + frees every descriptor that hasn't been acknowledged yet
> by the device (uncompleted TX transactions).
> The function assumes that the processed TX queue is necessarily from
> the first category listed above and ends up using napi_consume_skb()
> for descriptors belonging to an XDP specific queue.
> 
> This patch solves a bug in which, in case of a VF reset, the
> descriptors aren't freed correctly, leading to crashes.
> 
> Fixes: 548c4940b9f1 ("net: ena: Implement XDP_TX action")
> Signed-off-by: Shay Agroskin <shayagr@amazon.com>
> Signed-off-by: David Arinzon <darinzon@amazon.com>


Reviewed-by: Shannon Nelson <shannon.nelson@amd.com>


> ---
>   drivers/net/ethernet/amazon/ena/ena_netdev.c | 14 +++++++++++---
>   1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> index 59befc0f..be5acfa4 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> @@ -718,8 +718,11 @@ void ena_unmap_tx_buff(struct ena_ring *tx_ring,
>   static void ena_free_tx_bufs(struct ena_ring *tx_ring)
>   {
>          bool print_once = true;
> +       bool is_xdp_ring;
>          u32 i;
> 
> +       is_xdp_ring = ENA_IS_XDP_INDEX(tx_ring->adapter, tx_ring->qid);
> +
>          for (i = 0; i < tx_ring->ring_size; i++) {
>                  struct ena_tx_buffer *tx_info = &tx_ring->tx_buffer_info[i];
> 
> @@ -739,10 +742,15 @@ static void ena_free_tx_bufs(struct ena_ring *tx_ring)
> 
>                  ena_unmap_tx_buff(tx_ring, tx_info);
> 
> -               dev_kfree_skb_any(tx_info->skb);
> +               if (is_xdp_ring)
> +                       xdp_return_frame(tx_info->xdpf);
> +               else
> +                       dev_kfree_skb_any(tx_info->skb);
>          }
> -       netdev_tx_reset_queue(netdev_get_tx_queue(tx_ring->netdev,
> -                                                 tx_ring->qid));
> +
> +       if (!is_xdp_ring)
> +               netdev_tx_reset_queue(netdev_get_tx_queue(tx_ring->netdev,
> +                                                         tx_ring->qid));
>   }
> 
>   static void ena_free_all_tx_bufs(struct ena_adapter *adapter)
> --
> 2.40.1
> 
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 net 4/4] net: ena: Set tx_info->xdpf value to NULL
  2024-04-10  9:13 ` [PATCH v1 net 4/4] net: ena: Set tx_info->xdpf value to NULL darinzon
@ 2024-04-10 21:53   ` Nelson, Shannon
  0 siblings, 0 replies; 10+ messages in thread
From: Nelson, Shannon @ 2024-04-10 21:53 UTC (permalink / raw)
  To: darinzon, David Miller, Jakub Kicinski, netdev
  Cc: Woodhouse, David, Machulsky, Zorik, Matushevsky, Alexander,
	Saeed Bshara, Wilson, Matt, Liguori, Anthony, Bshara, Nafea,
	Belgazal, Netanel, Saidi, Ali, Herrenschmidt, Benjamin,
	Kiyanovski, Arthur, Dagan, Noam, Agroskin, Shay, Itzko, Shahar,
	Abboud, Osama, Ostrovsky, Evgeny, Tabachnik, Ofir,
	Netanel Belgazal, Sameeh Jubran

On 4/10/2024 2:13 AM, darinzon@amazon.com wrote:
> From: David Arinzon <darinzon@amazon.com>
> 
> The patch mentioned in the `Fixes` tag removed the explicit assignment
> of tx_info->xdpf to NULL with the justification that there's no need
> to set tx_info->xdpf to NULL and tx_info->num_of_bufs to 0 in case
> of a mapping error. Both values won't be used once the mapping function
> returns an error, and their values would be overridden by the next
> transmitted packet.
> 
> While both values do indeed get overridden in the next transmission
> call, the value of tx_info->xdpf is also used to check whether a TX
> descriptor's transmission has been completed (i.e. a completion for it
> was polled).
> 
> An example scenario:
> 1. Mapping failed, tx_info->xdpf wasn't set to NULL
> 2. A VF reset occurred leading to IO resource destruction and
>     a call to ena_free_tx_bufs() function
> 3. Although the descriptor whose mapping failed was freed by the
>     transmission function, it still passes the check
>       if (!tx_info->skb)
> 
>     (skb and xdp_frame are in a union)
> 4. The xdp_frame associated with the descriptor is freed twice
> 
> This patch returns the assignment of NULL to tx_info->xdpf to make the
> cleaning function knows that the descriptor is already freed.
> 
> Fixes: 504fd6a5390c ("net: ena: fix DMA mapping function issues in XDP")
> Signed-off-by: Shay Agroskin <shayagr@amazon.com>
> Signed-off-by: David Arinzon <darinzon@amazon.com>


Reviewed-by: Shannon Nelson <shannon.nelson@amd.com>


> ---
>   drivers/net/ethernet/amazon/ena/ena_xdp.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/amazon/ena/ena_xdp.c b/drivers/net/ethernet/amazon/ena/ena_xdp.c
> index 337c435d..5b175e7e 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_xdp.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_xdp.c
> @@ -89,7 +89,7 @@ int ena_xdp_xmit_frame(struct ena_ring *tx_ring,
> 
>          rc = ena_xdp_tx_map_frame(tx_ring, tx_info, xdpf, &ena_tx_ctx);
>          if (unlikely(rc))
> -               return rc;
> +               goto err;
> 
>          ena_tx_ctx.req_id = req_id;
> 
> @@ -112,7 +112,9 @@ int ena_xdp_xmit_frame(struct ena_ring *tx_ring,
> 
>   error_unmap_dma:
>          ena_unmap_tx_buff(tx_ring, tx_info);
> +err:
>          tx_info->xdpf = NULL;
> +
>          return rc;
>   }
> 
> --
> 2.40.1
> 
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 net 0/4] ENA driver bug fixes
  2024-04-10  9:13 [PATCH v1 net 0/4] ENA driver bug fixes darinzon
                   ` (3 preceding siblings ...)
  2024-04-10  9:13 ` [PATCH v1 net 4/4] net: ena: Set tx_info->xdpf value to NULL darinzon
@ 2024-04-11  9:40 ` patchwork-bot+netdevbpf
  4 siblings, 0 replies; 10+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-04-11  9:40 UTC (permalink / raw)
  To: Arinzon, David
  Cc: davem, kuba, netdev, dwmw, zorik, matua, saeedb, msw, aliguori,
	nafea, netanel, alisaidi, benh, akiyano, ndagan, shayagr, itzko,
	osamaabb, evostrov, ofirt, netanel, sameehj

Hello:

This series was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Wed, 10 Apr 2024 09:13:54 +0000 you wrote:
> From: David Arinzon <darinzon@amazon.com>
> 
> This patchset contains multiple bug fixes for the
> ENA driver.
> 
> David Arinzon (4):
>   net: ena: Fix potential sign extension issue
>   net: ena: Wrong missing IO completions check order
>   net: ena: Fix incorrect descriptor free behavior
>   net: ena: Set tx_info->xdpf value to NULL
> 
> [...]

Here is the summary with links:
  - [v1,net,1/4] net: ena: Fix potential sign extension issue
    https://git.kernel.org/netdev/net/c/713a85195aad
  - [v1,net,2/4] net: ena: Wrong missing IO completions check order
    https://git.kernel.org/netdev/net/c/f7e417180665
  - [v1,net,3/4] net: ena: Fix incorrect descriptor free behavior
    https://git.kernel.org/netdev/net/c/bf02d9fe0063
  - [v1,net,4/4] net: ena: Set tx_info->xdpf value to NULL
    https://git.kernel.org/netdev/net/c/36a1ca01f045

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2024-04-11  9:40 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-10  9:13 [PATCH v1 net 0/4] ENA driver bug fixes darinzon
2024-04-10  9:13 ` [PATCH v1 net 1/4] net: ena: Fix potential sign extension issue darinzon
2024-04-10 21:49   ` Nelson, Shannon
2024-04-10  9:13 ` [PATCH v1 net 2/4] net: ena: Wrong missing IO completions check order darinzon
2024-04-10 21:51   ` Nelson, Shannon
2024-04-10  9:13 ` [PATCH v1 net 3/4] net: ena: Fix incorrect descriptor free behavior darinzon
2024-04-10 21:52   ` Nelson, Shannon
2024-04-10  9:13 ` [PATCH v1 net 4/4] net: ena: Set tx_info->xdpf value to NULL darinzon
2024-04-10 21:53   ` Nelson, Shannon
2024-04-11  9:40 ` [PATCH v1 net 0/4] ENA driver bug fixes patchwork-bot+netdevbpf

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.