[PATCH V1 net 0/4] Fixes for ENA driver

[PATCH V1 net 0/4] Fixes for ENA driver

[Shay Agroskin]

Hi all,
This series fixes some issues in the ENA driver:

- fix wrong data offset on machines that support rx offset
- work-around Intel iommu issue
- fix out of bound access when request id is wrong
- return error code if XDP TX xmit fails

Shay Agroskin (4):
  net: ena: handle bad request id in ena_netdev
  net: ena: set initial DMA width to avoid intel iommu issue
  net: ena: fix packet's addresses for rx_offset feature
  net: ena: return error code from ena_xdp_xmit_buff

 drivers/net/ethernet/amazon/ena/ena_eth_com.c |  3 +
 drivers/net/ethernet/amazon/ena/ena_netdev.c  | 82 +++++++++----------
 2 files changed, 41 insertions(+), 44 deletions(-)

-- 
2.17.1

[PATCH V1 net 1/4] net: ena: handle bad request id in ena_netdev

[Shay Agroskin]

After request id is checked in validate_rx_req_id() its value is still
used in the line
	rx_ring->free_ids[next_to_clean] =
					rx_ring->ena_bufs[i].req_id;
even if it was found to be out-of-bound for the array free_ids.

The patch moves the request id to an earlier stage in the napi routine and
makes sure its value isn't used if it's found out-of-bounds.

Fixes: 30623e1ed116 ("net: ena: avoid memory access violation by validating req_id properly")
Signed-off-by: Ido Segev <idose@amazon.com>
Signed-off-by: Shay Agroskin <shayagr@amazon.com>
---
 drivers/net/ethernet/amazon/ena/ena_eth_com.c |  3 ++
 drivers/net/ethernet/amazon/ena/ena_netdev.c  | 43 +++++--------------
 2 files changed, 14 insertions(+), 32 deletions(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_eth_com.c b/drivers/net/ethernet/amazon/ena/ena_eth_com.c
index ad30cacc1622..032ab9f20438 100644
--- a/drivers/net/ethernet/amazon/ena/ena_eth_com.c
+++ b/drivers/net/ethernet/amazon/ena/ena_eth_com.c
@@ -516,6 +516,7 @@ int ena_com_rx_pkt(struct ena_com_io_cq *io_cq,
 {
 	struct ena_com_rx_buf_info *ena_buf = &ena_rx_ctx->ena_bufs[0];
 	struct ena_eth_io_rx_cdesc_base *cdesc = NULL;
+	u16 q_depth = io_cq->q_depth;
 	u16 cdesc_idx = 0;
 	u16 nb_hw_desc;
 	u16 i = 0;
@@ -543,6 +544,8 @@ int ena_com_rx_pkt(struct ena_com_io_cq *io_cq,
 	do {
 		ena_buf[i].len = cdesc->length;
 		ena_buf[i].req_id = cdesc->req_id;
+		if (unlikely(ena_buf[i].req_id >= q_depth))
+			return -EIO;
 
 		if (++i >= nb_hw_desc)
 			break;
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index e8131dadc22c..574c2b5ba21e 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -789,24 +789,6 @@ static void ena_free_all_io_tx_resources(struct ena_adapter *adapter)
 					      adapter->num_io_queues);
 }
 
-static int validate_rx_req_id(struct ena_ring *rx_ring, u16 req_id)
-{
-	if (likely(req_id < rx_ring->ring_size))
-		return 0;
-
-	netif_err(rx_ring->adapter, rx_err, rx_ring->netdev,
-		  "Invalid rx req_id: %hu\n", req_id);
-
-	u64_stats_update_begin(&rx_ring->syncp);
-	rx_ring->rx_stats.bad_req_id++;
-	u64_stats_update_end(&rx_ring->syncp);
-
-	/* Trigger device reset */
-	rx_ring->adapter->reset_reason = ENA_REGS_RESET_INV_RX_REQ_ID;
-	set_bit(ENA_FLAG_TRIGGER_RESET, &rx_ring->adapter->flags);
-	return -EFAULT;
-}
-
 /* ena_setup_rx_resources - allocate I/O Rx resources (Descriptors)
  * @adapter: network interface device structure
  * @qid: queue index
@@ -1356,15 +1338,10 @@ static struct sk_buff *ena_rx_skb(struct ena_ring *rx_ring,
 	struct ena_rx_buffer *rx_info;
 	u16 len, req_id, buf = 0;
 	void *va;
-	int rc;
 
 	len = ena_bufs[buf].len;
 	req_id = ena_bufs[buf].req_id;
 
-	rc = validate_rx_req_id(rx_ring, req_id);
-	if (unlikely(rc < 0))
-		return NULL;
-
 	rx_info = &rx_ring->rx_buffer_info[req_id];
 
 	if (unlikely(!rx_info->page)) {
@@ -1440,10 +1417,6 @@ static struct sk_buff *ena_rx_skb(struct ena_ring *rx_ring,
 		len = ena_bufs[buf].len;
 		req_id = ena_bufs[buf].req_id;
 
-		rc = validate_rx_req_id(rx_ring, req_id);
-		if (unlikely(rc < 0))
-			return NULL;
-
 		rx_info = &rx_ring->rx_buffer_info[req_id];
 	} while (1);
 
@@ -1697,12 +1670,18 @@ static int ena_clean_rx_irq(struct ena_ring *rx_ring, struct napi_struct *napi,
 error:
 	adapter = netdev_priv(rx_ring->netdev);
 
-	u64_stats_update_begin(&rx_ring->syncp);
-	rx_ring->rx_stats.bad_desc_num++;
-	u64_stats_update_end(&rx_ring->syncp);
+	if (rc == -ENOSPC) {
+		u64_stats_update_begin(&rx_ring->syncp);
+		rx_ring->rx_stats.bad_desc_num++;
+		u64_stats_update_end(&rx_ring->syncp);
+		adapter->reset_reason = ENA_REGS_RESET_TOO_MANY_RX_DESCS;
+	} else {
+		u64_stats_update_begin(&rx_ring->syncp);
+		rx_ring->rx_stats.bad_req_id++;
+		u64_stats_update_end(&rx_ring->syncp);
+		adapter->reset_reason = ENA_REGS_RESET_INV_RX_REQ_ID;
+	}
 
-	/* Too many desc from the device. Trigger reset */
-	adapter->reset_reason = ENA_REGS_RESET_TOO_MANY_RX_DESCS;
 	set_bit(ENA_FLAG_TRIGGER_RESET, &adapter->flags);
 
 	return 0;
-- 
2.17.1

[PATCH V1 net 2/4] net: ena: set initial DMA width to avoid intel iommu issue

[Shay Agroskin]

The ENA driver uses the readless mechanism, which uses DMA, to find
out what the DMA mask is supposed to be.

If DMA is used without setting the dma_mask first, it causes the
Intel IOMMU driver to think that ENA is a 32-bit device and therefore
disables IOMMU passthrough permanently.

This patch sets the dma_mask to be ENA_MAX_PHYS_ADDR_SIZE_BITS=48
before readless initialization in
ena_device_init()->ena_com_mmio_reg_read_request_init(),
which is large enough to workaround the intel_iommu issue.

DMA mask is set again to the correct value after it's received from the
device after readless is initialized.

Fixes: 1738cd3ed342 ("net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)")
Signed-off-by: Mike Cui <mikecui@amazon.com>
Signed-off-by: Arthur Kiyanovski <akiyano@amazon.com>
Signed-off-by: Shay Agroskin <shayagr@amazon.com>
---
 drivers/net/ethernet/amazon/ena/ena_netdev.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index 574c2b5ba21e..854a22e692bf 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -4146,6 +4146,19 @@ static int ena_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		return rc;
 	}
 
+	rc = pci_set_dma_mask(pdev, DMA_BIT_MASK(ENA_MAX_PHYS_ADDR_SIZE_BITS));
+	if (rc) {
+		dev_err(&pdev->dev, "pci_set_dma_mask failed %d\n", rc);
+		goto err_disable_device;
+	}
+
+	rc = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(ENA_MAX_PHYS_ADDR_SIZE_BITS));
+	if (rc) {
+		dev_err(&pdev->dev, "err_pci_set_consistent_dma_mask failed %d\n",
+			rc);
+		goto err_disable_device;
+	}
+
 	pci_set_master(pdev);
 
 	ena_dev = vzalloc(sizeof(*ena_dev));
-- 
2.17.1

[PATCH V1 net 3/4] net: ena: fix packet's addresses for rx_offset feature

[Shay Agroskin]

This patch fixes two lines in which the rx_offset received by the device
wasn't taken into account:

- prefetch function:
	In our driver the copied data would reside in
	rx_info->page + rx_headroom + rx_offset

	so the prefetch function is changed accordingly.

- setting page_offset to zero for descriptors > 1:
	for every descriptor but the first, the rx_offset is zero. Hence
	the page_offset value should be set to rx_headroom.

	The previous implementation changed the value of rx_info after
	the descriptor was added to the SKB (essentially providing wrong
	page offset).

Fixes: 68f236df93a9 ("net: ena: add support for the rx offset feature")
Signed-off-by: Shay Agroskin <shayagr@amazon.com>
---
 drivers/net/ethernet/amazon/ena/ena_netdev.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index 854a22e692bf..f63ecc5bca3b 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -908,10 +908,14 @@ static void ena_free_all_io_rx_resources(struct ena_adapter *adapter)
 static int ena_alloc_rx_page(struct ena_ring *rx_ring,
 				    struct ena_rx_buffer *rx_info, gfp_t gfp)
 {
+	int headroom = rx_ring->rx_headroom;
 	struct ena_com_buf *ena_buf;
 	struct page *page;
 	dma_addr_t dma;
 
+	/* restore page offset value in case it has been changed by device */
+	rx_info->page_offset = headroom;
+
 	/* if previous allocated page is not used */
 	if (unlikely(rx_info->page))
 		return 0;
@@ -941,10 +945,9 @@ static int ena_alloc_rx_page(struct ena_ring *rx_ring,
 		  "Allocate page %p, rx_info %p\n", page, rx_info);
 
 	rx_info->page = page;
-	rx_info->page_offset = 0;
 	ena_buf = &rx_info->ena_buf;
-	ena_buf->paddr = dma + rx_ring->rx_headroom;
-	ena_buf->len = ENA_PAGE_SIZE - rx_ring->rx_headroom;
+	ena_buf->paddr = dma + headroom;
+	ena_buf->len = ENA_PAGE_SIZE - headroom;
 
 	return 0;
 }
@@ -1356,7 +1359,8 @@ static struct sk_buff *ena_rx_skb(struct ena_ring *rx_ring,
 
 	/* save virt address of first buffer */
 	va = page_address(rx_info->page) + rx_info->page_offset;
-	prefetch(va + NET_IP_ALIGN);
+
+	prefetch(va);
 
 	if (len <= rx_ring->rx_copybreak) {
 		skb = ena_alloc_skb(rx_ring, false);
@@ -1397,8 +1401,6 @@ static struct sk_buff *ena_rx_skb(struct ena_ring *rx_ring,
 
 		skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, rx_info->page,
 				rx_info->page_offset, len, ENA_PAGE_SIZE);
-		/* The offset is non zero only for the first buffer */
-		rx_info->page_offset = 0;
 
 		netif_dbg(rx_ring->adapter, rx_status, rx_ring->netdev,
 			  "RX skb updated. len %d. data_len %d\n",
@@ -1517,8 +1519,7 @@ static int ena_xdp_handle_buff(struct ena_ring *rx_ring, struct xdp_buff *xdp)
 	int ret;
 
 	rx_info = &rx_ring->rx_buffer_info[rx_ring->ena_bufs[0].req_id];
-	xdp->data = page_address(rx_info->page) +
-		rx_info->page_offset + rx_ring->rx_headroom;
+	xdp->data = page_address(rx_info->page) + rx_info->page_offset;
 	xdp_set_data_meta_invalid(xdp);
 	xdp->data_hard_start = page_address(rx_info->page);
 	xdp->data_end = xdp->data + rx_ring->ena_bufs[0].len;
@@ -1585,8 +1586,9 @@ static int ena_clean_rx_irq(struct ena_ring *rx_ring, struct napi_struct *napi,
 		if (unlikely(ena_rx_ctx.descs == 0))
 			break;
 
+		/* First descriptor might have an offset set by the device */
 		rx_info = &rx_ring->rx_buffer_info[rx_ring->ena_bufs[0].req_id];
-		rx_info->page_offset = ena_rx_ctx.pkt_offset;
+		rx_info->page_offset += ena_rx_ctx.pkt_offset;
 
 		netif_dbg(rx_ring->adapter, rx_status, rx_ring->netdev,
 			  "rx_poll: q %d got packet from ena. descs #: %d l3 proto %d l4 proto %d hash: %x\n",
-- 
2.17.1

[PATCH V1 net 4/4] net: ena: return error code from ena_xdp_xmit_buff

[Shay Agroskin]

The function mistakenly returns NETDEV_TX_OK regardless of the
transmission success. This patch fixes this behavior by returning the
error code from the function.

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

diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index f63ecc5bca3b..4f1b109ac1fc 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -284,9 +284,9 @@ static int ena_xdp_xmit_buff(struct net_device *dev,
 	struct ena_tx_buffer *tx_info;
 	struct ena_ring *xdp_ring;
 	u16 next_to_use, req_id;
-	int rc;
 	void *push_hdr;
 	u32 push_len;
+	int rc;
 
 	xdp_ring = &adapter->tx_ring[qid];
 	next_to_use = xdp_ring->next_to_use;
@@ -322,14 +322,14 @@ static int ena_xdp_xmit_buff(struct net_device *dev,
 	xdp_ring->tx_stats.doorbells++;
 	u64_stats_update_end(&xdp_ring->syncp);
 
-	return NETDEV_TX_OK;
+	return rc;
 
 error_unmap_dma:
 	ena_unmap_tx_buff(xdp_ring, tx_info);
 	tx_info->xdpf = NULL;
 error_drop_packet:
 	__free_page(tx_info->xdp_rx_page);
-	return NETDEV_TX_OK;
+	return rc;
 }
 
 static int ena_xdp_execute(struct ena_ring *rx_ring,
-- 
2.17.1

Re: [PATCH V1 net 2/4] net: ena: set initial DMA width to avoid intel iommu issue

[Heiner Kallweit]

Am 18.11.2020 um 22:59 schrieb Shay Agroskin:
> The ENA driver uses the readless mechanism, which uses DMA, to find
> out what the DMA mask is supposed to be.
> 
> If DMA is used without setting the dma_mask first, it causes the
> Intel IOMMU driver to think that ENA is a 32-bit device and therefore
> disables IOMMU passthrough permanently.
> 
> This patch sets the dma_mask to be ENA_MAX_PHYS_ADDR_SIZE_BITS=48
> before readless initialization in
> ena_device_init()->ena_com_mmio_reg_read_request_init(),
> which is large enough to workaround the intel_iommu issue.
> 
> DMA mask is set again to the correct value after it's received from the
> device after readless is initialized.
> 
> Fixes: 1738cd3ed342 ("net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)")
> Signed-off-by: Mike Cui <mikecui@amazon.com>
> Signed-off-by: Arthur Kiyanovski <akiyano@amazon.com>
> Signed-off-by: Shay Agroskin <shayagr@amazon.com>
> ---
>  drivers/net/ethernet/amazon/ena/ena_netdev.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> index 574c2b5ba21e..854a22e692bf 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> @@ -4146,6 +4146,19 @@ static int ena_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  		return rc;
>  	}
>  
> +	rc = pci_set_dma_mask(pdev, DMA_BIT_MASK(ENA_MAX_PHYS_ADDR_SIZE_BITS));
> +	if (rc) {
> +		dev_err(&pdev->dev, "pci_set_dma_mask failed %d\n", rc);
> +		goto err_disable_device;
> +	}
> +
> +	rc = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(ENA_MAX_PHYS_ADDR_SIZE_BITS));
> +	if (rc) {
> +		dev_err(&pdev->dev, "err_pci_set_consistent_dma_mask failed %d\n",
> +			rc);
> +		goto err_disable_device;
> +	}
> +
>  	pci_set_master(pdev);
>  
>  	ena_dev = vzalloc(sizeof(*ena_dev));
> 

The old pci_ dma wrappers are being phased out and shouldn't be used in
new code. See e.g. e059c6f340f6 ("tulip: switch from 'pci_' to 'dma_' API").
So better use:
dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(ENA_MAX_PHYS_ADDR_SIZE_BITS));

Re: [PATCH V1 net 2/4] net: ena: set initial DMA width to avoid intel iommu issue

[Heiner Kallweit]

Am 18.11.2020 um 23:35 schrieb Heiner Kallweit:
> Am 18.11.2020 um 22:59 schrieb Shay Agroskin:
>> The ENA driver uses the readless mechanism, which uses DMA, to find
>> out what the DMA mask is supposed to be.
>>
>> If DMA is used without setting the dma_mask first, it causes the
>> Intel IOMMU driver to think that ENA is a 32-bit device and therefore
>> disables IOMMU passthrough permanently.
>>
>> This patch sets the dma_mask to be ENA_MAX_PHYS_ADDR_SIZE_BITS=48
>> before readless initialization in
>> ena_device_init()->ena_com_mmio_reg_read_request_init(),
>> which is large enough to workaround the intel_iommu issue.
>>
>> DMA mask is set again to the correct value after it's received from the
>> device after readless is initialized.
>>
>> Fixes: 1738cd3ed342 ("net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)")
>> Signed-off-by: Mike Cui <mikecui@amazon.com>
>> Signed-off-by: Arthur Kiyanovski <akiyano@amazon.com>
>> Signed-off-by: Shay Agroskin <shayagr@amazon.com>
>> ---
>>  drivers/net/ethernet/amazon/ena/ena_netdev.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
>> index 574c2b5ba21e..854a22e692bf 100644
>> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
>> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
>> @@ -4146,6 +4146,19 @@ static int ena_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>  		return rc;
>>  	}
>>  
>> +	rc = pci_set_dma_mask(pdev, DMA_BIT_MASK(ENA_MAX_PHYS_ADDR_SIZE_BITS));
>> +	if (rc) {
>> +		dev_err(&pdev->dev, "pci_set_dma_mask failed %d\n", rc);
>> +		goto err_disable_device;
>> +	}
>> +
>> +	rc = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(ENA_MAX_PHYS_ADDR_SIZE_BITS));
>> +	if (rc) {
>> +		dev_err(&pdev->dev, "err_pci_set_consistent_dma_mask failed %d\n",
>> +			rc);
>> +		goto err_disable_device;
>> +	}
>> +
>>  	pci_set_master(pdev);
>>  
>>  	ena_dev = vzalloc(sizeof(*ena_dev));
>>
> 
> The old pci_ dma wrappers are being phased out and shouldn't be used in
> new code. See e.g. e059c6f340f6 ("tulip: switch from 'pci_' to 'dma_' API").
> So better use:
> dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(ENA_MAX_PHYS_ADDR_SIZE_BITS));
> 

However instead of dev_err(&pdev->dev, ..) you could use pci_err(pdev, ..).

Re: [PATCH V1 net 2/4] net: ena: set initial DMA width to avoid intel iommu issue

[Shay Agroskin]

Heiner Kallweit <hkallweit1@gmail.com> writes:

> Am 18.11.2020 um 23:35 schrieb Heiner Kallweit:
>> Am 18.11.2020 um 22:59 schrieb Shay Agroskin:
>>> The ENA driver uses the readless mechanism, which uses DMA, to 
>>> find
>>> out what the DMA mask is supposed to be.
>>>
>>> If DMA is used without setting the dma_mask first, it causes 
>>> the
>>> Intel IOMMU driver to think that ENA is a 32-bit device and 
>>> therefore
>>> disables IOMMU passthrough permanently.
>>>
>>> This patch sets the dma_mask to be 
>>> ENA_MAX_PHYS_ADDR_SIZE_BITS=48
>>> before readless initialization in
>>> ena_device_init()->ena_com_mmio_reg_read_request_init(),
>>> which is large enough to workaround the intel_iommu issue.
>>>
>>> DMA mask is set again to the correct value after it's received 
>>> from the
>>> device after readless is initialized.
>>>
>>> Fixes: 1738cd3ed342 ("net: ena: Add a driver for Amazon 
>>> Elastic Network Adapters (ENA)")
>>> Signed-off-by: Mike Cui <mikecui@amazon.com>
>>> Signed-off-by: Arthur Kiyanovski <akiyano@amazon.com>
>>> Signed-off-by: Shay Agroskin <shayagr@amazon.com>
>>> ---
>>>  drivers/net/ethernet/amazon/ena/ena_netdev.c | 13 
>>>  +++++++++++++
>>>  1 file changed, 13 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c 
>>> b/drivers/net/ethernet/amazon/ena/ena_netdev.c
>>> index 574c2b5ba21e..854a22e692bf 100644
>>> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
>>> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
>>> @@ -4146,6 +4146,19 @@ static int ena_probe(struct pci_dev 
>>> *pdev, const struct pci_device_id *ent)
>>>  		return rc;
>>>  	}
>>>  
>>> +	rc = pci_set_dma_mask(pdev, 
>>> DMA_BIT_MASK(ENA_MAX_PHYS_ADDR_SIZE_BITS));
>>> +	if (rc) {
>>> +		dev_err(&pdev->dev, "pci_set_dma_mask failed 
>>> %d\n", rc);
>>> +		goto err_disable_device;
>>> +	}
>>> +
>>> +	rc = pci_set_consistent_dma_mask(pdev, 
>>> DMA_BIT_MASK(ENA_MAX_PHYS_ADDR_SIZE_BITS));
>>> +	if (rc) {
>>> +		dev_err(&pdev->dev, 
>>> "err_pci_set_consistent_dma_mask failed %d\n",
>>> +			rc);
>>> +		goto err_disable_device;
>>> +	}
>>> +
>>>  	pci_set_master(pdev);
>>>  
>>>  	ena_dev = vzalloc(sizeof(*ena_dev));
>>>
>> 
>> The old pci_ dma wrappers are being phased out and shouldn't be 
>> used in
>> new code. See e.g. e059c6f340f6 ("tulip: switch from 'pci_' to 
>> 'dma_' API").
>> So better use:
>> dma_set_mask_and_coherent(&pdev->dev, 
>> DMA_BIT_MASK(ENA_MAX_PHYS_ADDR_SIZE_BITS));

Thank you for reviewing these patches. We will switch to using 
dma_set_...() instead

>> 
>
> However instead of dev_err(&pdev->dev, ..) you could use 
> pci_err(pdev, ..).

I see that pci_err evaluates to dev_err. While I see how using 
pci_* log function helps code readability, I prefer using
dev_err here to keep the code consistent with the rest of the 
driver. We'll discuss changing all log functions in future patches 
to net-next if that's okay.

Re: [PATCH V1 net 2/4] net: ena: set initial DMA width to avoid intel iommu issue

[Heiner Kallweit]

Am 19.11.2020 um 20:18 schrieb Shay Agroskin:
> 
> Heiner Kallweit <hkallweit1@gmail.com> writes:
> 
>> Am 18.11.2020 um 23:35 schrieb Heiner Kallweit:
>>> Am 18.11.2020 um 22:59 schrieb Shay Agroskin:
>>>> The ENA driver uses the readless mechanism, which uses DMA, to find
>>>> out what the DMA mask is supposed to be.
>>>>
>>>> If DMA is used without setting the dma_mask first, it causes the
>>>> Intel IOMMU driver to think that ENA is a 32-bit device and therefore
>>>> disables IOMMU passthrough permanently.
>>>>
>>>> This patch sets the dma_mask to be ENA_MAX_PHYS_ADDR_SIZE_BITS=48
>>>> before readless initialization in
>>>> ena_device_init()->ena_com_mmio_reg_read_request_init(),
>>>> which is large enough to workaround the intel_iommu issue.
>>>>
>>>> DMA mask is set again to the correct value after it's received from the
>>>> device after readless is initialized.
>>>>
>>>> Fixes: 1738cd3ed342 ("net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)")
>>>> Signed-off-by: Mike Cui <mikecui@amazon.com>
>>>> Signed-off-by: Arthur Kiyanovski <akiyano@amazon.com>
>>>> Signed-off-by: Shay Agroskin <shayagr@amazon.com>
>>>> ---
>>>>  drivers/net/ethernet/amazon/ena/ena_netdev.c | 13  +++++++++++++
>>>>  1 file changed, 13 insertions(+)
>>>>
>>>> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
>>>> index 574c2b5ba21e..854a22e692bf 100644
>>>> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
>>>> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
>>>> @@ -4146,6 +4146,19 @@ static int ena_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>>>          return rc;
>>>>      }
>>>>  
>>>> +    rc = pci_set_dma_mask(pdev, DMA_BIT_MASK(ENA_MAX_PHYS_ADDR_SIZE_BITS));
>>>> +    if (rc) {
>>>> +        dev_err(&pdev->dev, "pci_set_dma_mask failed %d\n", rc);
>>>> +        goto err_disable_device;
>>>> +    }
>>>> +
>>>> +    rc = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(ENA_MAX_PHYS_ADDR_SIZE_BITS));
>>>> +    if (rc) {
>>>> +        dev_err(&pdev->dev, "err_pci_set_consistent_dma_mask failed %d\n",
>>>> +            rc);
>>>> +        goto err_disable_device;
>>>> +    }
>>>> +
>>>>      pci_set_master(pdev);
>>>>  
>>>>      ena_dev = vzalloc(sizeof(*ena_dev));
>>>>
>>>
>>> The old pci_ dma wrappers are being phased out and shouldn't be used in
>>> new code. See e.g. e059c6f340f6 ("tulip: switch from 'pci_' to 'dma_' API").
>>> So better use:
>>> dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(ENA_MAX_PHYS_ADDR_SIZE_BITS));
> 
> Thank you for reviewing these patches. We will switch to using dma_set_...() instead
> 
>>>
>>
>> However instead of dev_err(&pdev->dev, ..) you could use pci_err(pdev, ..).
> 
> I see that pci_err evaluates to dev_err. While I see how using pci_* log function helps code readability, I prefer using
> dev_err here to keep the code consistent with the rest of the driver. We'll discuss changing all log functions in future patches to net-next if that's okay.
> 
The comment was just meant to make you aware of pci_err(), there's no need
to switch to it.