[PATCH V2 net 00/20] Increase ENA driver version to 1.1.2

[PATCH V2 net 00/20] Increase ENA driver version to 1.1.2

[Netanel Belgazal]

Changes between V1 and V2:
* reorder the patches so the bug fixes will appear first.
* fix the commit message of removing a tuple filter. The first patch
stated mistakenly that it removes RFS.
* add another bug fix (fix RSS default hash configuration).
* split the driver's version increase to a dedicated patch.
* add this patchset description.

This patchset contains mainly bug fixes.
Most of them are critical for the driver and system functionality.
In addition to the bug fixes, this patchset also introduces some minor
Improvements listed below.

Bug fixes:
net/ena: remove ntuple filter support from device feature list
net/ena: fix error handling when probe fails
net/ena: fix queues number calculation
net/ena: fix ethtool RSS flow configuration
net/ena: fix RSS default hash configuration
net/ena: fix NULL dereference when removing the driver after device
reset faild
net/ena: refactor ena_get_stats64 to be atomic context safe
net/ena: add hardware hints capability to the driver
net/ena: fix potential access to freed memory during device reset
net/ena: remove redundant logic in napi callback for busy poll mode
net/ena: use READ_ONCE to access completion descriptors
net/ena: reduce the severity of ena printouts
net/ena: change driver's default timeouts
net/ena: change condition for host attribute configuration
net/ena: change sizeof() argument to be the type pointer

Other improvments:
net/ena: change sizeof() argument to be the type pointer
net/ena: use napi_schedule_irqoff when possible
net/ena: add IPv6 extended protocols to ena_admin_flow_hash_proto
net/ena: remove affinity hint from the driver
net/ena: restructure skb allocation
net/ena: increase driver version to 1.1.2

Netanel Belgazal (20):
  net/ena: remove ntuple filter support from device feature list
  net/ena: fix error handling when probe fails
  net/ena: fix queues number calculation
  net/ena: fix ethtool RSS flow configuration
  net/ena: fix RSS default hash configuration
  net/ena: fix NULL dereference when removing the driver after device
    reset faild
  net/ena: refactor ena_get_stats64 to be atomic context safe
  net/ena: add hardware hints capability to the driver
  net/ena: fix potential access to freed memory during device reset
  net/ena: remove redundant logic in napi callback for busy poll mode
  net/ena: use READ_ONCE to access completion descriptors
  net/ena: reduce the severity of ena printouts
  net/ena: change driver's default timeouts
  net/ena: change condition for host attribute configuration
  net/ena: change sizeof() argument to be the type pointer
  net/ena: use napi_schedule_irqoff when possible
  net/ena: add IPv6 extended protocols to ena_admin_flow_hash_proto
  net/ena: remove affinity hint from the driver
  net/ena: restructure skb allocation
  net/ena: increase driver version to 1.1.2

 drivers/net/ethernet/amazon/ena/ena_admin_defs.h |  57 +++-
 drivers/net/ethernet/amazon/ena/ena_com.c        |  98 ++++---
 drivers/net/ethernet/amazon/ena/ena_com.h        |   6 +
 drivers/net/ethernet/amazon/ena/ena_eth_com.c    |   8 +-
 drivers/net/ethernet/amazon/ena/ena_ethtool.c    |   1 -
 drivers/net/ethernet/amazon/ena/ena_netdev.c     | 326 ++++++++++++++++-------
 drivers/net/ethernet/amazon/ena/ena_netdev.h     |  30 ++-
 drivers/net/ethernet/amazon/ena/ena_regs_defs.h  |   2 +
 8 files changed, 385 insertions(+), 143 deletions(-)

-- 
2.7.4

[PATCH V2 net 01/20] net/ena: remove ntuple filter support from device feature list

[Netanel Belgazal]

Remove NETIF_F_NTUPLE from netdev->features.
The ENA device driver does not support ntuple filtering.

Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.com>
---
 drivers/net/ethernet/amazon/ena/ena_netdev.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index bfeaec5..33a760e 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -2729,7 +2729,6 @@ static void ena_set_dev_offloads(struct ena_com_dev_get_features_ctx *feat,
 	netdev->features =
 		dev_features |
 		NETIF_F_SG |
-		NETIF_F_NTUPLE |
 		NETIF_F_RXHASH |
 		NETIF_F_HIGHDMA;
 
-- 
2.7.4

Re: [PATCH V2 net 01/20] net/ena: remove ntuple filter support from device feature list

[Matt Wilson]

On Sun, Dec 04, 2016 at 03:19:19PM +0200, Netanel Belgazal wrote:
> Remove NETIF_F_NTUPLE from netdev->features.
> The ENA device driver does not support ntuple filtering.
> 
> Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.com>

Reviewed-by: Matt Wilson <msw@amazon.com>

> ---
>  drivers/net/ethernet/amazon/ena/ena_netdev.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> index bfeaec5..33a760e 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> @@ -2729,7 +2729,6 @@ static void ena_set_dev_offloads(struct ena_com_dev_get_features_ctx *feat,
>  	netdev->features =
>  		dev_features |
>  		NETIF_F_SG |
> -		NETIF_F_NTUPLE |
>  		NETIF_F_RXHASH |
>  		NETIF_F_HIGHDMA;
>  

[PATCH V2 net 02/20] net/ena: fix error handling when probe fails

[Netanel Belgazal]

When driver fails in probe, it will release all resources, including
adapter.
In case of probe failure, ena_remove should not try to free the adapter
resources.

Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.com>
---
 drivers/net/ethernet/amazon/ena/ena_netdev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index 33a760e..397c9bc 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -3054,6 +3054,7 @@ static int ena_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 err_free_region:
 	ena_release_bars(ena_dev, pdev);
 err_free_ena_dev:
+	pci_set_drvdata(pdev, NULL);
 	vfree(ena_dev);
 err_disable_device:
 	pci_disable_device(pdev);
-- 
2.7.4

Re: [PATCH V2 net 02/20] net/ena: fix error handling when probe fails

[Matt Wilson]

On Sun, Dec 04, 2016 at 03:19:20PM +0200, Netanel Belgazal wrote:
> When driver fails in probe, it will release all resources, including
> adapter.
> In case of probe failure, ena_remove should not try to free the adapter
> resources.

Please word wrap your commit message around 75 columns.

> Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.com>

Reviewed-by: Matt Wilson <msw@amazon.com>

> ---
>  drivers/net/ethernet/amazon/ena/ena_netdev.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> index 33a760e..397c9bc 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> @@ -3054,6 +3054,7 @@ static int ena_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  err_free_region:
>  	ena_release_bars(ena_dev, pdev);
>  err_free_ena_dev:
> +	pci_set_drvdata(pdev, NULL);
>  	vfree(ena_dev);
>  err_disable_device:
>  	pci_disable_device(pdev);

Re: [PATCH V2 net 02/20] net/ena: fix error handling when probe fails

[Netanel Belgazal]

On 12/05/2016 06:09 AM, Matt Wilson wrote:
> On Sun, Dec 04, 2016 at 03:19:20PM +0200, Netanel Belgazal wrote:
>> When driver fails in probe, it will release all resources, including
>> adapter.
>> In case of probe failure, ena_remove should not try to free the adapter
>> resources.
> Please word wrap your commit message around 75 columns.

OK

>
>> Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.com>
> Reviewed-by: Matt Wilson <msw@amazon.com>
>
>> ---
>>   drivers/net/ethernet/amazon/ena/ena_netdev.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
>> index 33a760e..397c9bc 100644
>> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
>> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
>> @@ -3054,6 +3054,7 @@ static int ena_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>   err_free_region:
>>   	ena_release_bars(ena_dev, pdev);
>>   err_free_ena_dev:
>> +	pci_set_drvdata(pdev, NULL);
>>   	vfree(ena_dev);
>>   err_disable_device:
>>   	pci_disable_device(pdev);

[PATCH V2 net 03/20] net/ena: fix queues number calculation

[Netanel Belgazal]

The ENA driver tries to open a queue per vCPU.
To determine how many vCPUs the instance have it uses num_possible_cpus
while it should have use num_online_cpus instead.

Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.com>
---
 drivers/net/ethernet/amazon/ena/ena_netdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index 397c9bc..224302c 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -2667,7 +2667,7 @@ static int ena_calc_io_queue_num(struct pci_dev *pdev,
 		io_sq_num = get_feat_ctx->max_queues.max_sq_num;
 	}
 
-	io_queue_num = min_t(int, num_possible_cpus(), ENA_MAX_NUM_IO_QUEUES);
+	io_queue_num = min_t(int, num_online_cpus(), ENA_MAX_NUM_IO_QUEUES);
 	io_queue_num = min_t(int, io_queue_num, io_sq_num);
 	io_queue_num = min_t(int, io_queue_num,
 			     get_feat_ctx->max_queues.max_cq_num);
-- 
2.7.4

Re: [PATCH V2 net 03/20] net/ena: fix queues number calculation

[Matt Wilson]

On Sun, Dec 04, 2016 at 03:19:21PM +0200, Netanel Belgazal wrote:
> The ENA driver tries to open a queue per vCPU.
> To determine how many vCPUs the instance have it uses num_possible_cpus
> while it should have use num_online_cpus instead.

use () when referring to functions: num_possible_cpus(), num_online_cpus().

> Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.com>

Reviewed-by: Matt Wilson <msw@amazon.com>

> ---
>  drivers/net/ethernet/amazon/ena/ena_netdev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> index 397c9bc..224302c 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> @@ -2667,7 +2667,7 @@ static int ena_calc_io_queue_num(struct pci_dev *pdev,
>  		io_sq_num = get_feat_ctx->max_queues.max_sq_num;
>  	}
>  
> -	io_queue_num = min_t(int, num_possible_cpus(), ENA_MAX_NUM_IO_QUEUES);
> +	io_queue_num = min_t(int, num_online_cpus(), ENA_MAX_NUM_IO_QUEUES);
>  	io_queue_num = min_t(int, io_queue_num, io_sq_num);
>  	io_queue_num = min_t(int, io_queue_num,
>  			     get_feat_ctx->max_queues.max_cq_num);

Re: [PATCH V2 net 03/20] net/ena: fix queues number calculation

[Netanel Belgazal]

On 12/05/2016 06:11 AM, Matt Wilson wrote:
> On Sun, Dec 04, 2016 at 03:19:21PM +0200, Netanel Belgazal wrote:
>> The ENA driver tries to open a queue per vCPU.
>> To determine how many vCPUs the instance have it uses num_possible_cpus
>> while it should have use num_online_cpus instead.
> use () when referring to functions: num_possible_cpus(), num_online_cpus().

Ack
>
>> Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.com>
> Reviewed-by: Matt Wilson <msw@amazon.com>
>
>> ---
>>   drivers/net/ethernet/amazon/ena/ena_netdev.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
>> index 397c9bc..224302c 100644
>> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
>> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
>> @@ -2667,7 +2667,7 @@ static int ena_calc_io_queue_num(struct pci_dev *pdev,
>>   		io_sq_num = get_feat_ctx->max_queues.max_sq_num;
>>   	}
>>   
>> -	io_queue_num = min_t(int, num_possible_cpus(), ENA_MAX_NUM_IO_QUEUES);
>> +	io_queue_num = min_t(int, num_online_cpus(), ENA_MAX_NUM_IO_QUEUES);
>>   	io_queue_num = min_t(int, io_queue_num, io_sq_num);
>>   	io_queue_num = min_t(int, io_queue_num,
>>   			     get_feat_ctx->max_queues.max_cq_num);

[PATCH V2 net 04/20] net/ena: fix ethtool RSS flow configuration

[Netanel Belgazal]

ena_flow_data_to_flow_hash and ena_flow_hash_to_flow_type
treat the ena_flow_hash_to_flow_type enum as power of two values.

Change the values of ena_admin_flow_hash_fields to be power of two values.

Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.com>
---
 drivers/net/ethernet/amazon/ena/ena_admin_defs.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
index a46e749..f48c886 100644
--- a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
+++ b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
@@ -631,22 +631,22 @@ enum ena_admin_flow_hash_proto {
 /* RSS flow hash fields */
 enum ena_admin_flow_hash_fields {
 	/* Ethernet Dest Addr */
-	ENA_ADMIN_RSS_L2_DA	= 0,
+	ENA_ADMIN_RSS_L2_DA	= 0x1,
 
 	/* Ethernet Src Addr */
-	ENA_ADMIN_RSS_L2_SA	= 1,
+	ENA_ADMIN_RSS_L2_SA	= 0x2,
 
 	/* ipv4/6 Dest Addr */
-	ENA_ADMIN_RSS_L3_DA	= 2,
+	ENA_ADMIN_RSS_L3_DA	= 0x4,
 
 	/* ipv4/6 Src Addr */
-	ENA_ADMIN_RSS_L3_SA	= 5,
+	ENA_ADMIN_RSS_L3_SA	= 0x8,
 
 	/* tcp/udp Dest Port */
-	ENA_ADMIN_RSS_L4_DP	= 6,
+	ENA_ADMIN_RSS_L4_DP	= 0x10,
 
 	/* tcp/udp Src Port */
-	ENA_ADMIN_RSS_L4_SP	= 7,
+	ENA_ADMIN_RSS_L4_SP	= 0x20,
 };
 
 struct ena_admin_proto_input {
-- 
2.7.4

Re: [PATCH V2 net 04/20] net/ena: fix ethtool RSS flow configuration

[Matt Wilson]

On Sun, Dec 04, 2016 at 03:19:22PM +0200, Netanel Belgazal wrote:
> ena_flow_data_to_flow_hash and ena_flow_hash_to_flow_type
> treat the ena_flow_hash_to_flow_type enum as power of two values.
> 
> Change the values of ena_admin_flow_hash_fields to be power of two values.

Then I generally prefer BIT(0), BIT(1), BIT(2), etc.

Also it would be helpful to include some comments about the
consequences of the current state of the code.

--msw

> Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.com>
> ---
>  drivers/net/ethernet/amazon/ena/ena_admin_defs.h | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
> index a46e749..f48c886 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
> +++ b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
> @@ -631,22 +631,22 @@ enum ena_admin_flow_hash_proto {
>  /* RSS flow hash fields */
>  enum ena_admin_flow_hash_fields {
>  	/* Ethernet Dest Addr */
> -	ENA_ADMIN_RSS_L2_DA	= 0,
> +	ENA_ADMIN_RSS_L2_DA	= 0x1,
>  
>  	/* Ethernet Src Addr */
> -	ENA_ADMIN_RSS_L2_SA	= 1,
> +	ENA_ADMIN_RSS_L2_SA	= 0x2,
>  
>  	/* ipv4/6 Dest Addr */
> -	ENA_ADMIN_RSS_L3_DA	= 2,
> +	ENA_ADMIN_RSS_L3_DA	= 0x4,
>  
>  	/* ipv4/6 Src Addr */
> -	ENA_ADMIN_RSS_L3_SA	= 5,
> +	ENA_ADMIN_RSS_L3_SA	= 0x8,
>  
>  	/* tcp/udp Dest Port */
> -	ENA_ADMIN_RSS_L4_DP	= 6,
> +	ENA_ADMIN_RSS_L4_DP	= 0x10,
>  
>  	/* tcp/udp Src Port */
> -	ENA_ADMIN_RSS_L4_SP	= 7,
> +	ENA_ADMIN_RSS_L4_SP	= 0x20,
>  };
>  
>  struct ena_admin_proto_input {

Re: [PATCH V2 net 04/20] net/ena: fix ethtool RSS flow configuration

[Netanel Belgazal]

On 12/05/2016 06:18 AM, Matt Wilson wrote:
> On Sun, Dec 04, 2016 at 03:19:22PM +0200, Netanel Belgazal wrote:
>> ena_flow_data_to_flow_hash and ena_flow_hash_to_flow_type
>> treat the ena_flow_hash_to_flow_type enum as power of two values.
>>
>> Change the values of ena_admin_flow_hash_fields to be power of two values.
> Then I generally prefer BIT(0), BIT(1), BIT(2), etc.

I'll use BIT(x)

>
> Also it would be helpful to include some comments about the
> consequences of the current state of the code.

I'll add explanation.

>
> --msw
>
>> Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.com>
>> ---
>>   drivers/net/ethernet/amazon/ena/ena_admin_defs.h | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
>> index a46e749..f48c886 100644
>> --- a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
>> +++ b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
>> @@ -631,22 +631,22 @@ enum ena_admin_flow_hash_proto {
>>   /* RSS flow hash fields */
>>   enum ena_admin_flow_hash_fields {
>>   	/* Ethernet Dest Addr */
>> -	ENA_ADMIN_RSS_L2_DA	= 0,
>> +	ENA_ADMIN_RSS_L2_DA	= 0x1,
>>   
>>   	/* Ethernet Src Addr */
>> -	ENA_ADMIN_RSS_L2_SA	= 1,
>> +	ENA_ADMIN_RSS_L2_SA	= 0x2,
>>   
>>   	/* ipv4/6 Dest Addr */
>> -	ENA_ADMIN_RSS_L3_DA	= 2,
>> +	ENA_ADMIN_RSS_L3_DA	= 0x4,
>>   
>>   	/* ipv4/6 Src Addr */
>> -	ENA_ADMIN_RSS_L3_SA	= 5,
>> +	ENA_ADMIN_RSS_L3_SA	= 0x8,
>>   
>>   	/* tcp/udp Dest Port */
>> -	ENA_ADMIN_RSS_L4_DP	= 6,
>> +	ENA_ADMIN_RSS_L4_DP	= 0x10,
>>   
>>   	/* tcp/udp Src Port */
>> -	ENA_ADMIN_RSS_L4_SP	= 7,
>> +	ENA_ADMIN_RSS_L4_SP	= 0x20,
>>   };
>>   
>>   struct ena_admin_proto_input {

[PATCH V2 net 05/20] net/ena: fix RSS default hash configuration

[Netanel Belgazal]

ENA default hash configure IPv4_frag hash twice instead of
configure non ip packets.

Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.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 3066d9c..46aad3a 100644
--- a/drivers/net/ethernet/amazon/ena/ena_com.c
+++ b/drivers/net/ethernet/amazon/ena/ena_com.c
@@ -2184,7 +2184,7 @@ int ena_com_set_default_hash_ctrl(struct ena_com_dev *ena_dev)
 	hash_ctrl->selected_fields[ENA_ADMIN_RSS_IP4_FRAG].fields =
 		ENA_ADMIN_RSS_L3_SA | ENA_ADMIN_RSS_L3_DA;
 
-	hash_ctrl->selected_fields[ENA_ADMIN_RSS_IP4_FRAG].fields =
+	hash_ctrl->selected_fields[ENA_ADMIN_RSS_NOT_IP].fields =
 		ENA_ADMIN_RSS_L2_DA | ENA_ADMIN_RSS_L2_SA;
 
 	for (i = 0; i < ENA_ADMIN_RSS_PROTO_NUM; i++) {
-- 
2.7.4

Re: [PATCH V2 net 05/20] net/ena: fix RSS default hash configuration

[Matt Wilson]

On Sun, Dec 04, 2016 at 03:19:23PM +0200, Netanel Belgazal wrote:
> ENA default hash configure IPv4_frag hash twice instead of

configure -> configures. You may want to include "erroneously". What
is the consequence of this bug?

> configure non ip packets.

configuring non-IP packets.

--msw

> Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.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 3066d9c..46aad3a 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_com.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_com.c
> @@ -2184,7 +2184,7 @@ int ena_com_set_default_hash_ctrl(struct ena_com_dev *ena_dev)
>  	hash_ctrl->selected_fields[ENA_ADMIN_RSS_IP4_FRAG].fields =
>  		ENA_ADMIN_RSS_L3_SA | ENA_ADMIN_RSS_L3_DA;
>  
> -	hash_ctrl->selected_fields[ENA_ADMIN_RSS_IP4_FRAG].fields =
> +	hash_ctrl->selected_fields[ENA_ADMIN_RSS_NOT_IP].fields =
>  		ENA_ADMIN_RSS_L2_DA | ENA_ADMIN_RSS_L2_SA;
>  
>  	for (i = 0; i < ENA_ADMIN_RSS_PROTO_NUM; i++) {

Re: [PATCH V2 net 05/20] net/ena: fix RSS default hash configuration

[Netanel Belgazal]

On 12/05/2016 06:20 AM, Matt Wilson wrote:
> On Sun, Dec 04, 2016 at 03:19:23PM +0200, Netanel Belgazal wrote:
>> ENA default hash configure IPv4_frag hash twice instead of
> configure -> configures. You may want to include "erroneously". What
> is the consequence of this bug?
I'll fix and I'll add explain the effect of this bug.

>
>> configure non ip packets.
> configuring non-IP packets.

OK.
>
> --msw
>
>> Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.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 3066d9c..46aad3a 100644
>> --- a/drivers/net/ethernet/amazon/ena/ena_com.c
>> +++ b/drivers/net/ethernet/amazon/ena/ena_com.c
>> @@ -2184,7 +2184,7 @@ int ena_com_set_default_hash_ctrl(struct ena_com_dev *ena_dev)
>>   	hash_ctrl->selected_fields[ENA_ADMIN_RSS_IP4_FRAG].fields =
>>   		ENA_ADMIN_RSS_L3_SA | ENA_ADMIN_RSS_L3_DA;
>>   
>> -	hash_ctrl->selected_fields[ENA_ADMIN_RSS_IP4_FRAG].fields =
>> +	hash_ctrl->selected_fields[ENA_ADMIN_RSS_NOT_IP].fields =
>>   		ENA_ADMIN_RSS_L2_DA | ENA_ADMIN_RSS_L2_SA;
>>   
>>   	for (i = 0; i < ENA_ADMIN_RSS_PROTO_NUM; i++) {

[PATCH V2 net 06/20] net/ena: fix NULL dereference when removing the driver after device reset faild

[Netanel Belgazal]

If for some reason the device stop responding and the device reset failed
to recover the device, the mmio register read datastructure will not be
reinitialized.
On driver removal, the driver will also tries to reset the device
but this time the mmio data structure will be NULL.

To solve this issue perform the device reset in the remove function only if
the device is runnig.

Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.com>
---
 drivers/net/ethernet/amazon/ena/ena_netdev.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index 224302c..ad5f78f 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -2516,6 +2516,8 @@ static void ena_fw_reset_device(struct work_struct *work)
 err:
 	rtnl_unlock();
 
+	clear_bit(ENA_FLAG_DEVICE_RUNNING, &adapter->flags);
+
 	dev_err(&pdev->dev,
 		"Reset attempt failed. Can not reset the device\n");
 }
@@ -3126,7 +3128,9 @@ static void ena_remove(struct pci_dev *pdev)
 
 	cancel_work_sync(&adapter->resume_io_task);
 
-	ena_com_dev_reset(ena_dev);
+	/* Reset the device only if the device is running. */
+	if (test_bit(ENA_FLAG_DEVICE_RUNNING, &adapter->flags))
+		ena_com_dev_reset(ena_dev);
 
 	ena_free_mgmnt_irq(adapter);
 
-- 
2.7.4

Re: [PATCH V2 net 06/20] net/ena: fix NULL dereference when removing the driver after device reset faild

[Matt Wilson]

On Sun, Dec 04, 2016 at 03:19:24PM +0200, Netanel Belgazal wrote:
> If for some reason the device stop responding and the device reset failed
> to recover the device, the mmio register read datastructure will not be
> reinitialized.

If for some reason the device stops responding, and the device reset
fails to recover the device, the MMIO register read data structure
will not be reinitialized.

> On driver removal, the driver will also tries to reset the device
> but this time the mmio data structure will be NULL.

On driver removal, the driver will also try to reset the device, but
this time the MMIO data structure will be NULL.

> To solve this issue perform the device reset in the remove function only if
> the device is runnig.

To solve this issue, perform the device reset in the remove function
only if the device is running.

Do you have an example of the NULL pointer dereference that you can
paste in? It can be helpful for those searching for a fix for a bug
they've experienced.

--msw

> Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.com>
> ---
>  drivers/net/ethernet/amazon/ena/ena_netdev.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> index 224302c..ad5f78f 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> @@ -2516,6 +2516,8 @@ static void ena_fw_reset_device(struct work_struct *work)
>  err:
>  	rtnl_unlock();
>  
> +	clear_bit(ENA_FLAG_DEVICE_RUNNING, &adapter->flags);
> +
>  	dev_err(&pdev->dev,
>  		"Reset attempt failed. Can not reset the device\n");
>  }
> @@ -3126,7 +3128,9 @@ static void ena_remove(struct pci_dev *pdev)
>  
>  	cancel_work_sync(&adapter->resume_io_task);
>  
> -	ena_com_dev_reset(ena_dev);
> +	/* Reset the device only if the device is running. */
> +	if (test_bit(ENA_FLAG_DEVICE_RUNNING, &adapter->flags))
> +		ena_com_dev_reset(ena_dev);
>  
>  	ena_free_mgmnt_irq(adapter);
>  

Re: [PATCH V2 net 06/20] net/ena: fix NULL dereference when removing the driver after device reset faild

[Netanel Belgazal]

On 12/05/2016 06:29 AM, Matt Wilson wrote:
> On Sun, Dec 04, 2016 at 03:19:24PM +0200, Netanel Belgazal wrote:
>> If for some reason the device stop responding and the device reset failed
>> to recover the device, the mmio register read datastructure will not be
>> reinitialized.
> If for some reason the device stops responding, and the device reset
> fails to recover the device, the MMIO register read data structure
> will not be reinitialized.

OK
>
>> On driver removal, the driver will also tries to reset the device
>> but this time the mmio data structure will be NULL.
> On driver removal, the driver will also try to reset the device, but
> this time the MMIO data structure will be NULL.
OK
>> To solve this issue perform the device reset in the remove function only if
>> the device is runnig.
> To solve this issue, perform the device reset in the remove function
> only if the device is running.
>
> Do you have an example of the NULL pointer dereference that you can
> paste in? It can be helpful for those searching for a fix for a bug
> they've experienced.
I'll add a crash dump.

> --msw
>
>> Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.com>
>> ---
>>   drivers/net/ethernet/amazon/ena/ena_netdev.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
>> index 224302c..ad5f78f 100644
>> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
>> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
>> @@ -2516,6 +2516,8 @@ static void ena_fw_reset_device(struct work_struct *work)
>>   err:
>>   	rtnl_unlock();
>>   
>> +	clear_bit(ENA_FLAG_DEVICE_RUNNING, &adapter->flags);
>> +
>>   	dev_err(&pdev->dev,
>>   		"Reset attempt failed. Can not reset the device\n");
>>   }
>> @@ -3126,7 +3128,9 @@ static void ena_remove(struct pci_dev *pdev)
>>   
>>   	cancel_work_sync(&adapter->resume_io_task);
>>   
>> -	ena_com_dev_reset(ena_dev);
>> +	/* Reset the device only if the device is running. */
>> +	if (test_bit(ENA_FLAG_DEVICE_RUNNING, &adapter->flags))
>> +		ena_com_dev_reset(ena_dev);
>>   
>>   	ena_free_mgmnt_irq(adapter);
>>   

[PATCH V2 net 07/20] net/ena: refactor ena_get_stats64 to be atomic context safe

[Netanel Belgazal]

ndo_get_stat64 can be called from atomic context.
However the current implementation sends an admin command to retrieve
the statistics from the device.
This admin commands uses sleep.

Refactor the implementation of ena_get_stats64 to take the
{rx,tx}bytes/cnt from the driver's inner counters
and to take the rx drops counter
from the asynchronous keep alive (heart bit) event.

Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.com>
---
 drivers/net/ethernet/amazon/ena/ena_admin_defs.h |  8 ++++
 drivers/net/ethernet/amazon/ena/ena_netdev.c     | 57 +++++++++++++++++-------
 drivers/net/ethernet/amazon/ena/ena_netdev.h     |  1 +
 3 files changed, 51 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
index f48c886..6d70bf5 100644
--- a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
+++ b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
@@ -873,6 +873,14 @@ struct ena_admin_aenq_link_change_desc {
 	u32 flags;
 };
 
+struct ena_admin_aenq_keep_alive_desc {
+	struct ena_admin_aenq_common_desc aenq_common_desc;
+
+	u32 rx_drops_low;
+
+	u32 rx_drops_high;
+};
+
 struct ena_admin_ena_mmio_req_read_less_resp {
 	u16 req_id;
 
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index ad5f78f..962ffb5 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -2176,28 +2176,46 @@ static struct rtnl_link_stats64 *ena_get_stats64(struct net_device *netdev,
 						 struct rtnl_link_stats64 *stats)
 {
 	struct ena_adapter *adapter = netdev_priv(netdev);
-	struct ena_admin_basic_stats ena_stats;
-	int rc;
+	struct ena_ring *rx_ring, *tx_ring;
+	unsigned int start;
+	u64 rx_drops;
+	int i;
 
 	if (!test_bit(ENA_FLAG_DEV_UP, &adapter->flags))
 		return NULL;
 
-	rc = ena_com_get_dev_basic_stats(adapter->ena_dev, &ena_stats);
-	if (rc)
-		return NULL;
+	for (i = 0; i < adapter->num_queues; i++) {
+		u64 bytes, packets;
+
+		tx_ring = &adapter->tx_ring[i];
+
+		do {
+			start = u64_stats_fetch_begin_irq(&tx_ring->syncp);
+			packets = tx_ring->tx_stats.cnt;
+			bytes = tx_ring->tx_stats.bytes;
+		} while (u64_stats_fetch_retry_irq(&tx_ring->syncp, start));
+
+		stats->tx_packets += packets;
+		stats->tx_bytes += bytes;
 
-	stats->tx_bytes = ((u64)ena_stats.tx_bytes_high << 32) |
-		ena_stats.tx_bytes_low;
-	stats->rx_bytes = ((u64)ena_stats.rx_bytes_high << 32) |
-		ena_stats.rx_bytes_low;
+		rx_ring = &adapter->rx_ring[i];
+
+		do {
+			start = u64_stats_fetch_begin_irq(&rx_ring->syncp);
+			packets = rx_ring->rx_stats.cnt;
+			bytes = rx_ring->rx_stats.bytes;
+		} while (u64_stats_fetch_retry_irq(&rx_ring->syncp, start));
 
-	stats->rx_packets = ((u64)ena_stats.rx_pkts_high << 32) |
-		ena_stats.rx_pkts_low;
-	stats->tx_packets = ((u64)ena_stats.tx_pkts_high << 32) |
-		ena_stats.tx_pkts_low;
+		stats->rx_packets += packets;
+		stats->rx_bytes += bytes;
+	}
+
+	do {
+		start = u64_stats_fetch_begin_irq(&adapter->syncp);
+		rx_drops = adapter->dev_stats.rx_drops;
+	} while (u64_stats_fetch_retry_irq(&adapter->syncp, start));
 
-	stats->rx_dropped = ((u64)ena_stats.rx_drops_high << 32) |
-		ena_stats.rx_drops_low;
+	stats->rx_dropped = rx_drops;
 
 	stats->multicast = 0;
 	stats->collisions = 0;
@@ -3221,8 +3239,17 @@ static void ena_keep_alive_wd(void *adapter_data,
 			      struct ena_admin_aenq_entry *aenq_e)
 {
 	struct ena_adapter *adapter = (struct ena_adapter *)adapter_data;
+	struct ena_admin_aenq_keep_alive_desc *desc;
+	u64 rx_drops;
 
+	desc = (struct ena_admin_aenq_keep_alive_desc *)aenq_e;
 	adapter->last_keep_alive_jiffies = jiffies;
+
+	rx_drops = ((u64)desc->rx_drops_high << 32) | desc->rx_drops_low;
+
+	u64_stats_update_begin(&adapter->syncp);
+	adapter->dev_stats.rx_drops = rx_drops;
+	u64_stats_update_end(&adapter->syncp);
 }
 
 static void ena_notification(void *adapter_data,
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.h b/drivers/net/ethernet/amazon/ena/ena_netdev.h
index 69d7e9e..f0ddc11 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.h
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.h
@@ -241,6 +241,7 @@ struct ena_stats_dev {
 	u64 interface_up;
 	u64 interface_down;
 	u64 admin_q_pause;
+	u64 rx_drops;
 };
 
 enum ena_flags_t {
-- 
2.7.4

Re: [PATCH V2 net 07/20] net/ena: refactor ena_get_stats64 to be atomic context safe

[Matt Wilson]

On Sun, Dec 04, 2016 at 03:19:25PM +0200, Netanel Belgazal wrote:
> ndo_get_stat64 can be called from atomic context.
> However the current implementation sends an admin command to retrieve
> the statistics from the device.
> This admin commands uses sleep.

Suggest some comment edits:

ndo_get_stat64() can be called from atomic context, but the current
implementation sends an admin command to retrieve the statistics from
the device. This admin command can sleep.

> Refactor the implementation of ena_get_stats64 to take the
> {rx,tx}bytes/cnt from the driver's inner counters
> and to take the rx drops counter
> from the asynchronous keep alive (heart bit) event.

This patch re-factors the implementation of ena_get_stats64() to use
the {rx,tx}bytes/count from the driver's inner counters, and to obtain
the rx drop counter from the asynchronous keep alive (heart bit)
event.

--msw

> Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.com>
> ---
>  drivers/net/ethernet/amazon/ena/ena_admin_defs.h |  8 ++++
>  drivers/net/ethernet/amazon/ena/ena_netdev.c     | 57 +++++++++++++++++-------
>  drivers/net/ethernet/amazon/ena/ena_netdev.h     |  1 +
>  3 files changed, 51 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
> index f48c886..6d70bf5 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
> +++ b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
> @@ -873,6 +873,14 @@ struct ena_admin_aenq_link_change_desc {
>  	u32 flags;
>  };
>  
> +struct ena_admin_aenq_keep_alive_desc {
> +	struct ena_admin_aenq_common_desc aenq_common_desc;
> +
> +	u32 rx_drops_low;
> +
> +	u32 rx_drops_high;
> +};
> +
>  struct ena_admin_ena_mmio_req_read_less_resp {
>  	u16 req_id;
>  
> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> index ad5f78f..962ffb5 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> @@ -2176,28 +2176,46 @@ static struct rtnl_link_stats64 *ena_get_stats64(struct net_device *netdev,
>  						 struct rtnl_link_stats64 *stats)
>  {
>  	struct ena_adapter *adapter = netdev_priv(netdev);
> -	struct ena_admin_basic_stats ena_stats;
> -	int rc;
> +	struct ena_ring *rx_ring, *tx_ring;
> +	unsigned int start;
> +	u64 rx_drops;
> +	int i;
>  
>  	if (!test_bit(ENA_FLAG_DEV_UP, &adapter->flags))
>  		return NULL;
>  
> -	rc = ena_com_get_dev_basic_stats(adapter->ena_dev, &ena_stats);
> -	if (rc)
> -		return NULL;
> +	for (i = 0; i < adapter->num_queues; i++) {
> +		u64 bytes, packets;
> +
> +		tx_ring = &adapter->tx_ring[i];
> +
> +		do {
> +			start = u64_stats_fetch_begin_irq(&tx_ring->syncp);
> +			packets = tx_ring->tx_stats.cnt;
> +			bytes = tx_ring->tx_stats.bytes;
> +		} while (u64_stats_fetch_retry_irq(&tx_ring->syncp, start));
> +
> +		stats->tx_packets += packets;
> +		stats->tx_bytes += bytes;
>  
> -	stats->tx_bytes = ((u64)ena_stats.tx_bytes_high << 32) |
> -		ena_stats.tx_bytes_low;
> -	stats->rx_bytes = ((u64)ena_stats.rx_bytes_high << 32) |
> -		ena_stats.rx_bytes_low;
> +		rx_ring = &adapter->rx_ring[i];
> +
> +		do {
> +			start = u64_stats_fetch_begin_irq(&rx_ring->syncp);
> +			packets = rx_ring->rx_stats.cnt;
> +			bytes = rx_ring->rx_stats.bytes;
> +		} while (u64_stats_fetch_retry_irq(&rx_ring->syncp, start));
>  
> -	stats->rx_packets = ((u64)ena_stats.rx_pkts_high << 32) |
> -		ena_stats.rx_pkts_low;
> -	stats->tx_packets = ((u64)ena_stats.tx_pkts_high << 32) |
> -		ena_stats.tx_pkts_low;
> +		stats->rx_packets += packets;
> +		stats->rx_bytes += bytes;
> +	}
> +
> +	do {
> +		start = u64_stats_fetch_begin_irq(&adapter->syncp);
> +		rx_drops = adapter->dev_stats.rx_drops;
> +	} while (u64_stats_fetch_retry_irq(&adapter->syncp, start));
>  
> -	stats->rx_dropped = ((u64)ena_stats.rx_drops_high << 32) |
> -		ena_stats.rx_drops_low;
> +	stats->rx_dropped = rx_drops;
>  
>  	stats->multicast = 0;
>  	stats->collisions = 0;
> @@ -3221,8 +3239,17 @@ static void ena_keep_alive_wd(void *adapter_data,
>  			      struct ena_admin_aenq_entry *aenq_e)
>  {
>  	struct ena_adapter *adapter = (struct ena_adapter *)adapter_data;
> +	struct ena_admin_aenq_keep_alive_desc *desc;
> +	u64 rx_drops;
>  
> +	desc = (struct ena_admin_aenq_keep_alive_desc *)aenq_e;
>  	adapter->last_keep_alive_jiffies = jiffies;
> +
> +	rx_drops = ((u64)desc->rx_drops_high << 32) | desc->rx_drops_low;
> +
> +	u64_stats_update_begin(&adapter->syncp);
> +	adapter->dev_stats.rx_drops = rx_drops;
> +	u64_stats_update_end(&adapter->syncp);
>  }
>  
>  static void ena_notification(void *adapter_data,
> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.h b/drivers/net/ethernet/amazon/ena/ena_netdev.h
> index 69d7e9e..f0ddc11 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.h
> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.h
> @@ -241,6 +241,7 @@ struct ena_stats_dev {
>  	u64 interface_up;
>  	u64 interface_down;
>  	u64 admin_q_pause;
> +	u64 rx_drops;
>  };
>  
>  enum ena_flags_t {

Re: [PATCH V2 net 07/20] net/ena: refactor ena_get_stats64 to be atomic context safe

[Netanel Belgazal]

On 12/05/2016 06:24 AM, Matt Wilson wrote:
> On Sun, Dec 04, 2016 at 03:19:25PM +0200, Netanel Belgazal wrote:
>> ndo_get_stat64 can be called from atomic context.
>> However the current implementation sends an admin command to retrieve
>> the statistics from the device.
>> This admin commands uses sleep.
> Suggest some comment edits:
>
> ndo_get_stat64() can be called from atomic context, but the current
> implementation sends an admin command to retrieve the statistics from
> the device. This admin command can sleep.
>
>> Refactor the implementation of ena_get_stats64 to take the
>> {rx,tx}bytes/cnt from the driver's inner counters
>> and to take the rx drops counter
>> from the asynchronous keep alive (heart bit) event.
> This patch re-factors the implementation of ena_get_stats64() to use
> the {rx,tx}bytes/count from the driver's inner counters, and to obtain
> the rx drop counter from the asynchronous keep alive (heart bit)
> event.
Applied
> --msw
>
>> Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.com>
>> ---
>>   drivers/net/ethernet/amazon/ena/ena_admin_defs.h |  8 ++++
>>   drivers/net/ethernet/amazon/ena/ena_netdev.c     | 57 +++++++++++++++++-------
>>   drivers/net/ethernet/amazon/ena/ena_netdev.h     |  1 +
>>   3 files changed, 51 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
>> index f48c886..6d70bf5 100644
>> --- a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
>> +++ b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
>> @@ -873,6 +873,14 @@ struct ena_admin_aenq_link_change_desc {
>>   	u32 flags;
>>   };
>>   
>> +struct ena_admin_aenq_keep_alive_desc {
>> +	struct ena_admin_aenq_common_desc aenq_common_desc;
>> +
>> +	u32 rx_drops_low;
>> +
>> +	u32 rx_drops_high;
>> +};
>> +
>>   struct ena_admin_ena_mmio_req_read_less_resp {
>>   	u16 req_id;
>>   
>> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
>> index ad5f78f..962ffb5 100644
>> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
>> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
>> @@ -2176,28 +2176,46 @@ static struct rtnl_link_stats64 *ena_get_stats64(struct net_device *netdev,
>>   						 struct rtnl_link_stats64 *stats)
>>   {
>>   	struct ena_adapter *adapter = netdev_priv(netdev);
>> -	struct ena_admin_basic_stats ena_stats;
>> -	int rc;
>> +	struct ena_ring *rx_ring, *tx_ring;
>> +	unsigned int start;
>> +	u64 rx_drops;
>> +	int i;
>>   
>>   	if (!test_bit(ENA_FLAG_DEV_UP, &adapter->flags))
>>   		return NULL;
>>   
>> -	rc = ena_com_get_dev_basic_stats(adapter->ena_dev, &ena_stats);
>> -	if (rc)
>> -		return NULL;
>> +	for (i = 0; i < adapter->num_queues; i++) {
>> +		u64 bytes, packets;
>> +
>> +		tx_ring = &adapter->tx_ring[i];
>> +
>> +		do {
>> +			start = u64_stats_fetch_begin_irq(&tx_ring->syncp);
>> +			packets = tx_ring->tx_stats.cnt;
>> +			bytes = tx_ring->tx_stats.bytes;
>> +		} while (u64_stats_fetch_retry_irq(&tx_ring->syncp, start));
>> +
>> +		stats->tx_packets += packets;
>> +		stats->tx_bytes += bytes;
>>   
>> -	stats->tx_bytes = ((u64)ena_stats.tx_bytes_high << 32) |
>> -		ena_stats.tx_bytes_low;
>> -	stats->rx_bytes = ((u64)ena_stats.rx_bytes_high << 32) |
>> -		ena_stats.rx_bytes_low;
>> +		rx_ring = &adapter->rx_ring[i];
>> +
>> +		do {
>> +			start = u64_stats_fetch_begin_irq(&rx_ring->syncp);
>> +			packets = rx_ring->rx_stats.cnt;
>> +			bytes = rx_ring->rx_stats.bytes;
>> +		} while (u64_stats_fetch_retry_irq(&rx_ring->syncp, start));
>>   
>> -	stats->rx_packets = ((u64)ena_stats.rx_pkts_high << 32) |
>> -		ena_stats.rx_pkts_low;
>> -	stats->tx_packets = ((u64)ena_stats.tx_pkts_high << 32) |
>> -		ena_stats.tx_pkts_low;
>> +		stats->rx_packets += packets;
>> +		stats->rx_bytes += bytes;
>> +	}
>> +
>> +	do {
>> +		start = u64_stats_fetch_begin_irq(&adapter->syncp);
>> +		rx_drops = adapter->dev_stats.rx_drops;
>> +	} while (u64_stats_fetch_retry_irq(&adapter->syncp, start));
>>   
>> -	stats->rx_dropped = ((u64)ena_stats.rx_drops_high << 32) |
>> -		ena_stats.rx_drops_low;
>> +	stats->rx_dropped = rx_drops;
>>   
>>   	stats->multicast = 0;
>>   	stats->collisions = 0;
>> @@ -3221,8 +3239,17 @@ static void ena_keep_alive_wd(void *adapter_data,
>>   			      struct ena_admin_aenq_entry *aenq_e)
>>   {
>>   	struct ena_adapter *adapter = (struct ena_adapter *)adapter_data;
>> +	struct ena_admin_aenq_keep_alive_desc *desc;
>> +	u64 rx_drops;
>>   
>> +	desc = (struct ena_admin_aenq_keep_alive_desc *)aenq_e;
>>   	adapter->last_keep_alive_jiffies = jiffies;
>> +
>> +	rx_drops = ((u64)desc->rx_drops_high << 32) | desc->rx_drops_low;
>> +
>> +	u64_stats_update_begin(&adapter->syncp);
>> +	adapter->dev_stats.rx_drops = rx_drops;
>> +	u64_stats_update_end(&adapter->syncp);
>>   }
>>   
>>   static void ena_notification(void *adapter_data,
>> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.h b/drivers/net/ethernet/amazon/ena/ena_netdev.h
>> index 69d7e9e..f0ddc11 100644
>> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.h
>> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.h
>> @@ -241,6 +241,7 @@ struct ena_stats_dev {
>>   	u64 interface_up;
>>   	u64 interface_down;
>>   	u64 admin_q_pause;
>> +	u64 rx_drops;
>>   };
>>   
>>   enum ena_flags_t {

[PATCH V2 net 08/20] net/ena: add hardware hints capability to the driver

[Netanel Belgazal]

The ENA device can update the ena driver about the desire timeouts.
The hardware hints are transmitted as Asynchronous event to the driver.

In case the device does not support this capability, the driver
will use its own defines.

Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.com>
---
 drivers/net/ethernet/amazon/ena/ena_admin_defs.h | 31 +++++++++
 drivers/net/ethernet/amazon/ena/ena_com.c        | 41 ++++++++---
 drivers/net/ethernet/amazon/ena/ena_com.h        |  5 ++
 drivers/net/ethernet/amazon/ena/ena_ethtool.c    |  1 -
 drivers/net/ethernet/amazon/ena/ena_netdev.c     | 86 +++++++++++++++++++-----
 drivers/net/ethernet/amazon/ena/ena_netdev.h     | 19 +++++-
 drivers/net/ethernet/amazon/ena/ena_regs_defs.h  |  2 +
 7 files changed, 157 insertions(+), 28 deletions(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
index 6d70bf5..35ae511 100644
--- a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
+++ b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
@@ -70,6 +70,8 @@ enum ena_admin_aq_feature_id {
 
 	ENA_ADMIN_MAX_QUEUES_NUM		= 2,
 
+	ENA_ADMIN_HW_HINTS			= 3,
+
 	ENA_ADMIN_RSS_HASH_FUNCTION		= 10,
 
 	ENA_ADMIN_STATELESS_OFFLOAD_CONFIG	= 11,
@@ -749,6 +751,31 @@ struct ena_admin_feature_rss_ind_table {
 	struct ena_admin_rss_ind_table_entry inline_entry;
 };
 
+/* When hint value is 0, driver should use it's own predefined value */
+struct ena_admin_ena_hw_hints {
+	/* value in ms */
+	u16 mmio_read_timeout;
+
+	/* value in ms */
+	u16 driver_watchdog_timeout;
+
+	/* Per packet tx completion timeout. value in ms */
+	u16 missing_tx_completion_timeout;
+
+	u16 missed_tx_completion_count_threshold_to_reset;
+
+	/* value in ms */
+	u16 admin_completion_tx_timeout;
+
+	u16 netdev_wd_timeout;
+
+	u16 max_tx_sgl_size;
+
+	u16 max_rx_sgl_size;
+
+	u16 reserved[8];
+};
+
 struct ena_admin_get_feat_cmd {
 	struct ena_admin_aq_common_desc aq_common_descriptor;
 
@@ -782,6 +809,8 @@ struct ena_admin_get_feat_resp {
 		struct ena_admin_feature_rss_ind_table ind_table;
 
 		struct ena_admin_feature_intr_moder_desc intr_moderation;
+
+		struct ena_admin_ena_hw_hints hw_hints;
 	} u;
 };
 
@@ -857,6 +886,8 @@ enum ena_admin_aenq_notification_syndrom {
 	ENA_ADMIN_SUSPEND	= 0,
 
 	ENA_ADMIN_RESUME	= 1,
+
+	ENA_ADMIN_UPDATE_HINTS	= 2,
 };
 
 struct ena_admin_aenq_entry {
diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c b/drivers/net/ethernet/amazon/ena/ena_com.c
index 46aad3a..f1e4f04 100644
--- a/drivers/net/ethernet/amazon/ena/ena_com.c
+++ b/drivers/net/ethernet/amazon/ena/ena_com.c
@@ -508,15 +508,13 @@ static int ena_com_comp_status_to_errno(u8 comp_status)
 static int ena_com_wait_and_process_admin_cq_polling(struct ena_comp_ctx *comp_ctx,
 						     struct ena_com_admin_queue *admin_queue)
 {
-	unsigned long flags;
-	u32 start_time;
+	unsigned long flags, timeout;
 	int ret;
 
-	start_time = ((u32)jiffies_to_usecs(jiffies));
+	timeout = jiffies + usecs_to_jiffies(admin_queue->completion_timeout);
 
 	while (comp_ctx->status == ENA_CMD_SUBMITTED) {
-		if ((((u32)jiffies_to_usecs(jiffies)) - start_time) >
-		    ADMIN_CMD_TIMEOUT_US) {
+		if (time_is_before_jiffies(timeout)) {
 			pr_err("Wait for completion (polling) timeout\n");
 			/* ENA didn't have any completion */
 			spin_lock_irqsave(&admin_queue->q_lock, flags);
@@ -560,7 +558,8 @@ static int ena_com_wait_and_process_admin_cq_interrupts(struct ena_comp_ctx *com
 	int ret;
 
 	wait_for_completion_timeout(&comp_ctx->wait_event,
-				    usecs_to_jiffies(ADMIN_CMD_TIMEOUT_US));
+				    usecs_to_jiffies(
+					    admin_queue->completion_timeout));
 
 	/* In case the command wasn't completed find out the root cause.
 	 * There might be 2 kinds of errors
@@ -600,12 +599,14 @@ static u32 ena_com_reg_bar_read32(struct ena_com_dev *ena_dev, u16 offset)
 	struct ena_com_mmio_read *mmio_read = &ena_dev->mmio_read;
 	volatile struct ena_admin_ena_mmio_req_read_less_resp *read_resp =
 		mmio_read->read_resp;
-	u32 mmio_read_reg, ret;
+	u32 mmio_read_reg, timeout, ret;
 	unsigned long flags;
 	int i;
 
 	might_sleep();
 
+	timeout = mmio_read->reg_read_to ? : ENA_REG_READ_TIMEOUT;
+
 	/* If readless is disabled, perform regular read */
 	if (!mmio_read->readless_supported)
 		return readl(ena_dev->reg_bar + offset);
@@ -626,14 +627,14 @@ static u32 ena_com_reg_bar_read32(struct ena_com_dev *ena_dev, u16 offset)
 
 	writel(mmio_read_reg, ena_dev->reg_bar + ENA_REGS_MMIO_REG_READ_OFF);
 
-	for (i = 0; i < ENA_REG_READ_TIMEOUT; i++) {
+	for (i = 0; i < timeout; i++) {
 		if (read_resp->req_id == mmio_read->seq_num)
 			break;
 
 		udelay(1);
 	}
 
-	if (unlikely(i == ENA_REG_READ_TIMEOUT)) {
+	if (unlikely(i == timeout)) {
 		pr_err("reading reg failed for timeout. expected: req id[%hu] offset[%hu] actual: req id[%hu] offset[%hu]\n",
 		       mmio_read->seq_num, offset, read_resp->req_id,
 		       read_resp->reg_off);
@@ -1717,6 +1718,20 @@ int ena_com_get_dev_attr_feat(struct ena_com_dev *ena_dev,
 	memcpy(&get_feat_ctx->offload, &get_resp.u.offload,
 	       sizeof(get_resp.u.offload));
 
+	/* Driver hints isn't mandatory admin command. So in case the
+	 * command isn't supported set driver hints to 0
+	 */
+	rc = ena_com_get_feature(ena_dev, &get_resp, ENA_ADMIN_HW_HINTS);
+
+	if (!rc)
+		memcpy(&get_feat_ctx->hw_hints, &get_resp.u.hw_hints,
+		       sizeof(get_resp.u.hw_hints));
+	else if (rc == -EPERM)
+		memset(&get_feat_ctx->hw_hints, 0x0,
+		       sizeof(get_feat_ctx->hw_hints));
+	else
+		return rc;
+
 	return 0;
 }
 
@@ -1842,6 +1857,14 @@ int ena_com_dev_reset(struct ena_com_dev *ena_dev)
 		return rc;
 	}
 
+	timeout = (cap & ENA_REGS_CAPS_ADMIN_CMD_TO_MASK) >>
+		ENA_REGS_CAPS_ADMIN_CMD_TO_SHIFT;
+	if (timeout)
+		/* the resolution of timeout reg is 100ms */
+		ena_dev->admin_queue.completion_timeout = timeout * 100000;
+	else
+		ena_dev->admin_queue.completion_timeout = ADMIN_CMD_TIMEOUT_US;
+
 	return 0;
 }
 
diff --git a/drivers/net/ethernet/amazon/ena/ena_com.h b/drivers/net/ethernet/amazon/ena/ena_com.h
index 509d7b8..6883ee5 100644
--- a/drivers/net/ethernet/amazon/ena/ena_com.h
+++ b/drivers/net/ethernet/amazon/ena/ena_com.h
@@ -96,6 +96,8 @@
 #define ENA_INTR_MODER_LEVEL_STRIDE			2
 #define ENA_INTR_BYTE_COUNT_NOT_SUPPORTED		0xFFFFFF
 
+#define ENA_HW_HINTS_NO_TIMEOUT				0xFFFF
+
 enum ena_intr_moder_level {
 	ENA_INTR_MODER_LOWEST = 0,
 	ENA_INTR_MODER_LOW,
@@ -232,6 +234,7 @@ struct ena_com_admin_queue {
 	void *q_dmadev;
 	spinlock_t q_lock; /* spinlock for the admin queue */
 	struct ena_comp_ctx *comp_ctx;
+	u32 completion_timeout;
 	u16 q_depth;
 	struct ena_com_admin_cq cq;
 	struct ena_com_admin_sq sq;
@@ -266,6 +269,7 @@ struct ena_com_aenq {
 struct ena_com_mmio_read {
 	struct ena_admin_ena_mmio_req_read_less_resp *read_resp;
 	dma_addr_t read_resp_dma_addr;
+	u32 reg_read_to; /* in us */
 	u16 seq_num;
 	bool readless_supported;
 	/* spin lock to ensure a single outstanding read */
@@ -335,6 +339,7 @@ struct ena_com_dev_get_features_ctx {
 	struct ena_admin_device_attr_feature_desc dev_attr;
 	struct ena_admin_feature_aenq_desc aenq;
 	struct ena_admin_feature_offload_desc offload;
+	struct ena_admin_ena_hw_hints hw_hints;
 };
 
 struct ena_com_create_io_ctx {
diff --git a/drivers/net/ethernet/amazon/ena/ena_ethtool.c b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
index 67b2338f..a1fbfc2 100644
--- a/drivers/net/ethernet/amazon/ena/ena_ethtool.c
+++ b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
@@ -80,7 +80,6 @@ static const struct ena_stats ena_stats_tx_strings[] = {
 	ENA_STAT_TX_ENTRY(tx_poll),
 	ENA_STAT_TX_ENTRY(doorbells),
 	ENA_STAT_TX_ENTRY(prepare_ctx_err),
-	ENA_STAT_TX_ENTRY(missing_tx_comp),
 	ENA_STAT_TX_ENTRY(bad_req_id),
 };
 
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index 962ffb5..0b718ee 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -1981,6 +1981,7 @@ static netdev_tx_t ena_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	tx_info->tx_descs = nb_hw_desc;
 	tx_info->last_jiffies = jiffies;
+	tx_info->print_once = 0;
 
 	tx_ring->next_to_use = ENA_TX_RING_IDX_NEXT(next_to_use,
 		tx_ring->ring_size);
@@ -2554,33 +2555,34 @@ static void check_for_missing_tx_completions(struct ena_adapter *adapter)
 	if (!test_bit(ENA_FLAG_DEV_UP, &adapter->flags))
 		return;
 
+	if (adapter->missing_tx_completion_to == ENA_HW_HINTS_NO_TIMEOUT)
+		return;
+
 	budget = ENA_MONITORED_TX_QUEUES;
 
 	for (i = adapter->last_monitored_tx_qid; i < adapter->num_queues; i++) {
 		tx_ring = &adapter->tx_ring[i];
 
+		missed_tx = 0;
+
 		for (j = 0; j < tx_ring->ring_size; j++) {
 			tx_buf = &tx_ring->tx_buffer_info[j];
 			last_jiffies = tx_buf->last_jiffies;
-			if (unlikely(last_jiffies && time_is_before_jiffies(last_jiffies + TX_TIMEOUT))) {
-				netif_notice(adapter, tx_err, adapter->netdev,
-					     "Found a Tx that wasn't completed on time, qid %d, index %d.\n",
-					     tx_ring->qid, j);
+			if (unlikely(last_jiffies && time_is_before_jiffies(last_jiffies + adapter->missing_tx_completion_to))) {
+				if (!tx_buf->print_once)
+					netif_notice(adapter, tx_err, adapter->netdev,
+						     "Found a Tx that wasn't completed on time, qid %d, index %d.\n",
+						     tx_ring->qid, j);
 
-				u64_stats_update_begin(&tx_ring->syncp);
-				missed_tx = tx_ring->tx_stats.missing_tx_comp++;
-				u64_stats_update_end(&tx_ring->syncp);
+				tx_buf->print_once = 1;
+				missed_tx++;
 
-				/* Clear last jiffies so the lost buffer won't
-				 * be counted twice.
-				 */
-				tx_buf->last_jiffies = 0;
-
-				if (unlikely(missed_tx > MAX_NUM_OF_TIMEOUTED_PACKETS)) {
+				if (unlikely(missed_tx > adapter->missing_tx_completion_threshold)) {
 					netif_err(adapter, tx_err, adapter->netdev,
 						  "The number of lost tx completion is above the threshold (%d > %d). Reset the device\n",
-						  missed_tx, MAX_NUM_OF_TIMEOUTED_PACKETS);
+						  missed_tx, adapter->missing_tx_completion_threshold);
 					set_bit(ENA_FLAG_TRIGGER_RESET, &adapter->flags);
+					return;
 				}
 			}
 		}
@@ -2601,8 +2603,11 @@ static void check_for_missing_keep_alive(struct ena_adapter *adapter)
 	if (!adapter->wd_state)
 		return;
 
-	keep_alive_expired = round_jiffies(adapter->last_keep_alive_jiffies
-					   + ENA_DEVICE_KALIVE_TIMEOUT);
+	if (adapter->keep_alive_timeout == ENA_HW_HINTS_NO_TIMEOUT)
+		return;
+
+	keep_alive_expired = round_jiffies(adapter->last_keep_alive_jiffies +
+					   adapter->keep_alive_timeout);
 	if (unlikely(time_is_before_jiffies(keep_alive_expired))) {
 		netif_err(adapter, drv, adapter->netdev,
 			  "Keep alive watchdog timeout.\n");
@@ -2625,6 +2630,44 @@ static void check_for_admin_com_state(struct ena_adapter *adapter)
 	}
 }
 
+static void ena_update_hints(struct ena_adapter *adapter,
+			     struct ena_admin_ena_hw_hints *hints)
+{
+	struct net_device *netdev = adapter->netdev;
+
+	if (hints->admin_completion_tx_timeout)
+		adapter->ena_dev->admin_queue.completion_timeout =
+			hints->admin_completion_tx_timeout * 1000;
+
+	if (hints->mmio_read_timeout)
+		/* convert to usec */
+		adapter->ena_dev->mmio_read.reg_read_to =
+			hints->mmio_read_timeout * 1000;
+
+	if (hints->missed_tx_completion_count_threshold_to_reset)
+		adapter->missing_tx_completion_threshold =
+			hints->missed_tx_completion_count_threshold_to_reset;
+
+	if (hints->missing_tx_completion_timeout) {
+		if (hints->missing_tx_completion_timeout == ENA_HW_HINTS_NO_TIMEOUT)
+			adapter->missing_tx_completion_to = ENA_HW_HINTS_NO_TIMEOUT;
+		else
+			adapter->missing_tx_completion_to =
+				msecs_to_jiffies(hints->missing_tx_completion_timeout);
+	}
+
+	if (hints->netdev_wd_timeout)
+		netdev->watchdog_timeo = msecs_to_jiffies(hints->netdev_wd_timeout);
+
+	if (hints->driver_watchdog_timeout) {
+		if (hints->driver_watchdog_timeout == ENA_HW_HINTS_NO_TIMEOUT)
+			adapter->keep_alive_timeout = ENA_HW_HINTS_NO_TIMEOUT;
+		else
+			adapter->keep_alive_timeout =
+				msecs_to_jiffies(hints->driver_watchdog_timeout);
+	}
+}
+
 static void ena_update_host_info(struct ena_admin_host_info *host_info,
 				 struct net_device *netdev)
 {
@@ -3036,6 +3079,11 @@ static int ena_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	INIT_WORK(&adapter->reset_task, ena_fw_reset_device);
 
 	adapter->last_keep_alive_jiffies = jiffies;
+	adapter->keep_alive_timeout = ENA_DEVICE_KALIVE_TIMEOUT;
+	adapter->missing_tx_completion_to = TX_TIMEOUT;
+	adapter->missing_tx_completion_threshold = MAX_NUM_OF_TIMEOUTED_PACKETS;
+
+	ena_update_hints(adapter, &get_feat_ctx.hw_hints);
 
 	init_timer(&adapter->timer_service);
 	adapter->timer_service.expires = round_jiffies(jiffies + HZ);
@@ -3256,6 +3304,7 @@ static void ena_notification(void *adapter_data,
 			     struct ena_admin_aenq_entry *aenq_e)
 {
 	struct ena_adapter *adapter = (struct ena_adapter *)adapter_data;
+	struct ena_admin_ena_hw_hints *hints;
 
 	WARN(aenq_e->aenq_common_desc.group != ENA_ADMIN_NOTIFICATION,
 	     "Invalid group(%x) expected %x\n",
@@ -3273,6 +3322,11 @@ static void ena_notification(void *adapter_data,
 	case ENA_ADMIN_RESUME:
 		queue_work(ena_wq, &adapter->resume_io_task);
 		break;
+	case ENA_ADMIN_UPDATE_HINTS:
+		hints = (struct ena_admin_ena_hw_hints *)
+			(&aenq_e->inline_data_w4);
+		ena_update_hints(adapter, hints);
+		break;
 	default:
 		netif_err(adapter, drv, adapter->netdev,
 			  "Invalid aenq notification link state %d\n",
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.h b/drivers/net/ethernet/amazon/ena/ena_netdev.h
index f0ddc11..2897fab 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.h
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.h
@@ -146,7 +146,18 @@ struct ena_tx_buffer {
 	u32 tx_descs;
 	/* num of buffers used by this skb */
 	u32 num_of_bufs;
-	/* Save the last jiffies to detect missing tx packets */
+
+	/* Used for detect missing tx packets to limit the number of prints */
+	u32 print_once;
+	/* Save the last jiffies to detect missing tx packets
+	 *
+	 * sets to non zero value on ena_start_xmit and set to zero on
+	 * napi and timer_Service_routine.
+	 *
+	 * while this value is not protected by lock,
+	 * a given packet is not expected to be handled by ena_start_xmit
+	 * and by napi/timer_service at the same time.
+	 */
 	unsigned long last_jiffies;
 	struct ena_com_buf bufs[ENA_PKT_MAX_BUFS];
 } ____cacheline_aligned;
@@ -170,7 +181,6 @@ struct ena_stats_tx {
 	u64 napi_comp;
 	u64 tx_poll;
 	u64 doorbells;
-	u64 missing_tx_comp;
 	u64 bad_req_id;
 };
 
@@ -270,6 +280,8 @@ struct ena_adapter {
 	struct msix_entry *msix_entries;
 	int msix_vecs;
 
+	u32 missing_tx_completion_threshold;
+
 	u32 tx_usecs, rx_usecs; /* interrupt moderation */
 	u32 tx_frames, rx_frames; /* interrupt moderation */
 
@@ -283,6 +295,9 @@ struct ena_adapter {
 
 	u8 mac_addr[ETH_ALEN];
 
+	unsigned long keep_alive_timeout;
+	unsigned long missing_tx_completion_to;
+
 	char name[ENA_NAME_MAX_LEN];
 
 	unsigned long flags;
diff --git a/drivers/net/ethernet/amazon/ena/ena_regs_defs.h b/drivers/net/ethernet/amazon/ena/ena_regs_defs.h
index 26097a2..c3891c5 100644
--- a/drivers/net/ethernet/amazon/ena/ena_regs_defs.h
+++ b/drivers/net/ethernet/amazon/ena/ena_regs_defs.h
@@ -78,6 +78,8 @@
 #define ENA_REGS_CAPS_RESET_TIMEOUT_MASK		0x3e
 #define ENA_REGS_CAPS_DMA_ADDR_WIDTH_SHIFT		8
 #define ENA_REGS_CAPS_DMA_ADDR_WIDTH_MASK		0xff00
+#define ENA_REGS_CAPS_ADMIN_CMD_TO_SHIFT		16
+#define ENA_REGS_CAPS_ADMIN_CMD_TO_MASK		0xf0000
 
 /* aq_caps register */
 #define ENA_REGS_AQ_CAPS_AQ_DEPTH_MASK		0xffff
-- 
2.7.4

Re: [PATCH V2 net 08/20] net/ena: add hardware hints capability to the driver

[Matt Wilson]

On Sun, Dec 04, 2016 at 03:19:26PM +0200, Netanel Belgazal wrote:
> The ENA device can update the ena driver about the desire timeouts.
> The hardware hints are transmitted as Asynchronous event to the driver.

This is really a new feature, not a bugfix - correct? If it is a new
feature, submit it separately. If the built-in defaults need to be
changed, submit that as a bugfix.

--msw

> In case the device does not support this capability, the driver
> will use its own defines.
> 
> Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.com>
> ---
>  drivers/net/ethernet/amazon/ena/ena_admin_defs.h | 31 +++++++++
>  drivers/net/ethernet/amazon/ena/ena_com.c        | 41 ++++++++---
>  drivers/net/ethernet/amazon/ena/ena_com.h        |  5 ++
>  drivers/net/ethernet/amazon/ena/ena_ethtool.c    |  1 -
>  drivers/net/ethernet/amazon/ena/ena_netdev.c     | 86 +++++++++++++++++++-----
>  drivers/net/ethernet/amazon/ena/ena_netdev.h     | 19 +++++-
>  drivers/net/ethernet/amazon/ena/ena_regs_defs.h  |  2 +
>  7 files changed, 157 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
> index 6d70bf5..35ae511 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
> +++ b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
> @@ -70,6 +70,8 @@ enum ena_admin_aq_feature_id {
>  
>  	ENA_ADMIN_MAX_QUEUES_NUM		= 2,
>  
> +	ENA_ADMIN_HW_HINTS			= 3,
> +
>  	ENA_ADMIN_RSS_HASH_FUNCTION		= 10,
>  
>  	ENA_ADMIN_STATELESS_OFFLOAD_CONFIG	= 11,
> @@ -749,6 +751,31 @@ struct ena_admin_feature_rss_ind_table {
>  	struct ena_admin_rss_ind_table_entry inline_entry;
>  };
>  
> +/* When hint value is 0, driver should use it's own predefined value */
> +struct ena_admin_ena_hw_hints {
> +	/* value in ms */
> +	u16 mmio_read_timeout;
> +
> +	/* value in ms */
> +	u16 driver_watchdog_timeout;
> +
> +	/* Per packet tx completion timeout. value in ms */
> +	u16 missing_tx_completion_timeout;
> +
> +	u16 missed_tx_completion_count_threshold_to_reset;
> +
> +	/* value in ms */
> +	u16 admin_completion_tx_timeout;
> +
> +	u16 netdev_wd_timeout;
> +
> +	u16 max_tx_sgl_size;
> +
> +	u16 max_rx_sgl_size;
> +
> +	u16 reserved[8];
> +};
> +
>  struct ena_admin_get_feat_cmd {
>  	struct ena_admin_aq_common_desc aq_common_descriptor;
>  
> @@ -782,6 +809,8 @@ struct ena_admin_get_feat_resp {
>  		struct ena_admin_feature_rss_ind_table ind_table;
>  
>  		struct ena_admin_feature_intr_moder_desc intr_moderation;
> +
> +		struct ena_admin_ena_hw_hints hw_hints;
>  	} u;
>  };
>  
> @@ -857,6 +886,8 @@ enum ena_admin_aenq_notification_syndrom {
>  	ENA_ADMIN_SUSPEND	= 0,
>  
>  	ENA_ADMIN_RESUME	= 1,
> +
> +	ENA_ADMIN_UPDATE_HINTS	= 2,
>  };
>  
>  struct ena_admin_aenq_entry {
> diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c b/drivers/net/ethernet/amazon/ena/ena_com.c
> index 46aad3a..f1e4f04 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_com.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_com.c
> @@ -508,15 +508,13 @@ static int ena_com_comp_status_to_errno(u8 comp_status)
>  static int ena_com_wait_and_process_admin_cq_polling(struct ena_comp_ctx *comp_ctx,
>  						     struct ena_com_admin_queue *admin_queue)
>  {
> -	unsigned long flags;
> -	u32 start_time;
> +	unsigned long flags, timeout;
>  	int ret;
>  
> -	start_time = ((u32)jiffies_to_usecs(jiffies));
> +	timeout = jiffies + usecs_to_jiffies(admin_queue->completion_timeout);
>  
>  	while (comp_ctx->status == ENA_CMD_SUBMITTED) {
> -		if ((((u32)jiffies_to_usecs(jiffies)) - start_time) >
> -		    ADMIN_CMD_TIMEOUT_US) {
> +		if (time_is_before_jiffies(timeout)) {
>  			pr_err("Wait for completion (polling) timeout\n");
>  			/* ENA didn't have any completion */
>  			spin_lock_irqsave(&admin_queue->q_lock, flags);
> @@ -560,7 +558,8 @@ static int ena_com_wait_and_process_admin_cq_interrupts(struct ena_comp_ctx *com
>  	int ret;
>  
>  	wait_for_completion_timeout(&comp_ctx->wait_event,
> -				    usecs_to_jiffies(ADMIN_CMD_TIMEOUT_US));
> +				    usecs_to_jiffies(
> +					    admin_queue->completion_timeout));
>  
>  	/* In case the command wasn't completed find out the root cause.
>  	 * There might be 2 kinds of errors
> @@ -600,12 +599,14 @@ static u32 ena_com_reg_bar_read32(struct ena_com_dev *ena_dev, u16 offset)
>  	struct ena_com_mmio_read *mmio_read = &ena_dev->mmio_read;
>  	volatile struct ena_admin_ena_mmio_req_read_less_resp *read_resp =
>  		mmio_read->read_resp;
> -	u32 mmio_read_reg, ret;
> +	u32 mmio_read_reg, timeout, ret;
>  	unsigned long flags;
>  	int i;
>  
>  	might_sleep();
>  
> +	timeout = mmio_read->reg_read_to ? : ENA_REG_READ_TIMEOUT;
> +
>  	/* If readless is disabled, perform regular read */
>  	if (!mmio_read->readless_supported)
>  		return readl(ena_dev->reg_bar + offset);
> @@ -626,14 +627,14 @@ static u32 ena_com_reg_bar_read32(struct ena_com_dev *ena_dev, u16 offset)
>  
>  	writel(mmio_read_reg, ena_dev->reg_bar + ENA_REGS_MMIO_REG_READ_OFF);
>  
> -	for (i = 0; i < ENA_REG_READ_TIMEOUT; i++) {
> +	for (i = 0; i < timeout; i++) {
>  		if (read_resp->req_id == mmio_read->seq_num)
>  			break;
>  
>  		udelay(1);
>  	}
>  
> -	if (unlikely(i == ENA_REG_READ_TIMEOUT)) {
> +	if (unlikely(i == timeout)) {
>  		pr_err("reading reg failed for timeout. expected: req id[%hu] offset[%hu] actual: req id[%hu] offset[%hu]\n",
>  		       mmio_read->seq_num, offset, read_resp->req_id,
>  		       read_resp->reg_off);
> @@ -1717,6 +1718,20 @@ int ena_com_get_dev_attr_feat(struct ena_com_dev *ena_dev,
>  	memcpy(&get_feat_ctx->offload, &get_resp.u.offload,
>  	       sizeof(get_resp.u.offload));
>  
> +	/* Driver hints isn't mandatory admin command. So in case the
> +	 * command isn't supported set driver hints to 0
> +	 */
> +	rc = ena_com_get_feature(ena_dev, &get_resp, ENA_ADMIN_HW_HINTS);
> +
> +	if (!rc)
> +		memcpy(&get_feat_ctx->hw_hints, &get_resp.u.hw_hints,
> +		       sizeof(get_resp.u.hw_hints));
> +	else if (rc == -EPERM)
> +		memset(&get_feat_ctx->hw_hints, 0x0,
> +		       sizeof(get_feat_ctx->hw_hints));
> +	else
> +		return rc;
> +
>  	return 0;
>  }
>  
> @@ -1842,6 +1857,14 @@ int ena_com_dev_reset(struct ena_com_dev *ena_dev)
>  		return rc;
>  	}
>  
> +	timeout = (cap & ENA_REGS_CAPS_ADMIN_CMD_TO_MASK) >>
> +		ENA_REGS_CAPS_ADMIN_CMD_TO_SHIFT;
> +	if (timeout)
> +		/* the resolution of timeout reg is 100ms */
> +		ena_dev->admin_queue.completion_timeout = timeout * 100000;
> +	else
> +		ena_dev->admin_queue.completion_timeout = ADMIN_CMD_TIMEOUT_US;
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/net/ethernet/amazon/ena/ena_com.h b/drivers/net/ethernet/amazon/ena/ena_com.h
> index 509d7b8..6883ee5 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_com.h
> +++ b/drivers/net/ethernet/amazon/ena/ena_com.h
> @@ -96,6 +96,8 @@
>  #define ENA_INTR_MODER_LEVEL_STRIDE			2
>  #define ENA_INTR_BYTE_COUNT_NOT_SUPPORTED		0xFFFFFF
>  
> +#define ENA_HW_HINTS_NO_TIMEOUT				0xFFFF
> +
>  enum ena_intr_moder_level {
>  	ENA_INTR_MODER_LOWEST = 0,
>  	ENA_INTR_MODER_LOW,
> @@ -232,6 +234,7 @@ struct ena_com_admin_queue {
>  	void *q_dmadev;
>  	spinlock_t q_lock; /* spinlock for the admin queue */
>  	struct ena_comp_ctx *comp_ctx;
> +	u32 completion_timeout;
>  	u16 q_depth;
>  	struct ena_com_admin_cq cq;
>  	struct ena_com_admin_sq sq;
> @@ -266,6 +269,7 @@ struct ena_com_aenq {
>  struct ena_com_mmio_read {
>  	struct ena_admin_ena_mmio_req_read_less_resp *read_resp;
>  	dma_addr_t read_resp_dma_addr;
> +	u32 reg_read_to; /* in us */
>  	u16 seq_num;
>  	bool readless_supported;
>  	/* spin lock to ensure a single outstanding read */
> @@ -335,6 +339,7 @@ struct ena_com_dev_get_features_ctx {
>  	struct ena_admin_device_attr_feature_desc dev_attr;
>  	struct ena_admin_feature_aenq_desc aenq;
>  	struct ena_admin_feature_offload_desc offload;
> +	struct ena_admin_ena_hw_hints hw_hints;
>  };
>  
>  struct ena_com_create_io_ctx {
> diff --git a/drivers/net/ethernet/amazon/ena/ena_ethtool.c b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
> index 67b2338f..a1fbfc2 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_ethtool.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
> @@ -80,7 +80,6 @@ static const struct ena_stats ena_stats_tx_strings[] = {
>  	ENA_STAT_TX_ENTRY(tx_poll),
>  	ENA_STAT_TX_ENTRY(doorbells),
>  	ENA_STAT_TX_ENTRY(prepare_ctx_err),
> -	ENA_STAT_TX_ENTRY(missing_tx_comp),
>  	ENA_STAT_TX_ENTRY(bad_req_id),
>  };
>  
> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> index 962ffb5..0b718ee 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> @@ -1981,6 +1981,7 @@ static netdev_tx_t ena_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  
>  	tx_info->tx_descs = nb_hw_desc;
>  	tx_info->last_jiffies = jiffies;
> +	tx_info->print_once = 0;
>  
>  	tx_ring->next_to_use = ENA_TX_RING_IDX_NEXT(next_to_use,
>  		tx_ring->ring_size);
> @@ -2554,33 +2555,34 @@ static void check_for_missing_tx_completions(struct ena_adapter *adapter)
>  	if (!test_bit(ENA_FLAG_DEV_UP, &adapter->flags))
>  		return;
>  
> +	if (adapter->missing_tx_completion_to == ENA_HW_HINTS_NO_TIMEOUT)
> +		return;
> +
>  	budget = ENA_MONITORED_TX_QUEUES;
>  
>  	for (i = adapter->last_monitored_tx_qid; i < adapter->num_queues; i++) {
>  		tx_ring = &adapter->tx_ring[i];
>  
> +		missed_tx = 0;
> +
>  		for (j = 0; j < tx_ring->ring_size; j++) {
>  			tx_buf = &tx_ring->tx_buffer_info[j];
>  			last_jiffies = tx_buf->last_jiffies;
> -			if (unlikely(last_jiffies && time_is_before_jiffies(last_jiffies + TX_TIMEOUT))) {
> -				netif_notice(adapter, tx_err, adapter->netdev,
> -					     "Found a Tx that wasn't completed on time, qid %d, index %d.\n",
> -					     tx_ring->qid, j);
> +			if (unlikely(last_jiffies && time_is_before_jiffies(last_jiffies + adapter->missing_tx_completion_to))) {
> +				if (!tx_buf->print_once)
> +					netif_notice(adapter, tx_err, adapter->netdev,
> +						     "Found a Tx that wasn't completed on time, qid %d, index %d.\n",
> +						     tx_ring->qid, j);
>  
> -				u64_stats_update_begin(&tx_ring->syncp);
> -				missed_tx = tx_ring->tx_stats.missing_tx_comp++;
> -				u64_stats_update_end(&tx_ring->syncp);
> +				tx_buf->print_once = 1;
> +				missed_tx++;
>  
> -				/* Clear last jiffies so the lost buffer won't
> -				 * be counted twice.
> -				 */
> -				tx_buf->last_jiffies = 0;
> -
> -				if (unlikely(missed_tx > MAX_NUM_OF_TIMEOUTED_PACKETS)) {
> +				if (unlikely(missed_tx > adapter->missing_tx_completion_threshold)) {
>  					netif_err(adapter, tx_err, adapter->netdev,
>  						  "The number of lost tx completion is above the threshold (%d > %d). Reset the device\n",
> -						  missed_tx, MAX_NUM_OF_TIMEOUTED_PACKETS);
> +						  missed_tx, adapter->missing_tx_completion_threshold);
>  					set_bit(ENA_FLAG_TRIGGER_RESET, &adapter->flags);
> +					return;
>  				}
>  			}
>  		}
> @@ -2601,8 +2603,11 @@ static void check_for_missing_keep_alive(struct ena_adapter *adapter)
>  	if (!adapter->wd_state)
>  		return;
>  
> -	keep_alive_expired = round_jiffies(adapter->last_keep_alive_jiffies
> -					   + ENA_DEVICE_KALIVE_TIMEOUT);
> +	if (adapter->keep_alive_timeout == ENA_HW_HINTS_NO_TIMEOUT)
> +		return;
> +
> +	keep_alive_expired = round_jiffies(adapter->last_keep_alive_jiffies +
> +					   adapter->keep_alive_timeout);
>  	if (unlikely(time_is_before_jiffies(keep_alive_expired))) {
>  		netif_err(adapter, drv, adapter->netdev,
>  			  "Keep alive watchdog timeout.\n");
> @@ -2625,6 +2630,44 @@ static void check_for_admin_com_state(struct ena_adapter *adapter)
>  	}
>  }
>  
> +static void ena_update_hints(struct ena_adapter *adapter,
> +			     struct ena_admin_ena_hw_hints *hints)
> +{
> +	struct net_device *netdev = adapter->netdev;
> +
> +	if (hints->admin_completion_tx_timeout)
> +		adapter->ena_dev->admin_queue.completion_timeout =
> +			hints->admin_completion_tx_timeout * 1000;
> +
> +	if (hints->mmio_read_timeout)
> +		/* convert to usec */
> +		adapter->ena_dev->mmio_read.reg_read_to =
> +			hints->mmio_read_timeout * 1000;
> +
> +	if (hints->missed_tx_completion_count_threshold_to_reset)
> +		adapter->missing_tx_completion_threshold =
> +			hints->missed_tx_completion_count_threshold_to_reset;
> +
> +	if (hints->missing_tx_completion_timeout) {
> +		if (hints->missing_tx_completion_timeout == ENA_HW_HINTS_NO_TIMEOUT)
> +			adapter->missing_tx_completion_to = ENA_HW_HINTS_NO_TIMEOUT;
> +		else
> +			adapter->missing_tx_completion_to =
> +				msecs_to_jiffies(hints->missing_tx_completion_timeout);
> +	}
> +
> +	if (hints->netdev_wd_timeout)
> +		netdev->watchdog_timeo = msecs_to_jiffies(hints->netdev_wd_timeout);
> +
> +	if (hints->driver_watchdog_timeout) {
> +		if (hints->driver_watchdog_timeout == ENA_HW_HINTS_NO_TIMEOUT)
> +			adapter->keep_alive_timeout = ENA_HW_HINTS_NO_TIMEOUT;
> +		else
> +			adapter->keep_alive_timeout =
> +				msecs_to_jiffies(hints->driver_watchdog_timeout);
> +	}
> +}
> +
>  static void ena_update_host_info(struct ena_admin_host_info *host_info,
>  				 struct net_device *netdev)
>  {
> @@ -3036,6 +3079,11 @@ static int ena_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	INIT_WORK(&adapter->reset_task, ena_fw_reset_device);
>  
>  	adapter->last_keep_alive_jiffies = jiffies;
> +	adapter->keep_alive_timeout = ENA_DEVICE_KALIVE_TIMEOUT;
> +	adapter->missing_tx_completion_to = TX_TIMEOUT;
> +	adapter->missing_tx_completion_threshold = MAX_NUM_OF_TIMEOUTED_PACKETS;
> +
> +	ena_update_hints(adapter, &get_feat_ctx.hw_hints);
>  
>  	init_timer(&adapter->timer_service);
>  	adapter->timer_service.expires = round_jiffies(jiffies + HZ);
> @@ -3256,6 +3304,7 @@ static void ena_notification(void *adapter_data,
>  			     struct ena_admin_aenq_entry *aenq_e)
>  {
>  	struct ena_adapter *adapter = (struct ena_adapter *)adapter_data;
> +	struct ena_admin_ena_hw_hints *hints;
>  
>  	WARN(aenq_e->aenq_common_desc.group != ENA_ADMIN_NOTIFICATION,
>  	     "Invalid group(%x) expected %x\n",
> @@ -3273,6 +3322,11 @@ static void ena_notification(void *adapter_data,
>  	case ENA_ADMIN_RESUME:
>  		queue_work(ena_wq, &adapter->resume_io_task);
>  		break;
> +	case ENA_ADMIN_UPDATE_HINTS:
> +		hints = (struct ena_admin_ena_hw_hints *)
> +			(&aenq_e->inline_data_w4);
> +		ena_update_hints(adapter, hints);
> +		break;
>  	default:
>  		netif_err(adapter, drv, adapter->netdev,
>  			  "Invalid aenq notification link state %d\n",
> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.h b/drivers/net/ethernet/amazon/ena/ena_netdev.h
> index f0ddc11..2897fab 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.h
> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.h
> @@ -146,7 +146,18 @@ struct ena_tx_buffer {
>  	u32 tx_descs;
>  	/* num of buffers used by this skb */
>  	u32 num_of_bufs;
> -	/* Save the last jiffies to detect missing tx packets */
> +
> +	/* Used for detect missing tx packets to limit the number of prints */
> +	u32 print_once;
> +	/* Save the last jiffies to detect missing tx packets
> +	 *
> +	 * sets to non zero value on ena_start_xmit and set to zero on
> +	 * napi and timer_Service_routine.
> +	 *
> +	 * while this value is not protected by lock,
> +	 * a given packet is not expected to be handled by ena_start_xmit
> +	 * and by napi/timer_service at the same time.
> +	 */
>  	unsigned long last_jiffies;
>  	struct ena_com_buf bufs[ENA_PKT_MAX_BUFS];
>  } ____cacheline_aligned;
> @@ -170,7 +181,6 @@ struct ena_stats_tx {
>  	u64 napi_comp;
>  	u64 tx_poll;
>  	u64 doorbells;
> -	u64 missing_tx_comp;
>  	u64 bad_req_id;
>  };
>  
> @@ -270,6 +280,8 @@ struct ena_adapter {
>  	struct msix_entry *msix_entries;
>  	int msix_vecs;
>  
> +	u32 missing_tx_completion_threshold;
> +
>  	u32 tx_usecs, rx_usecs; /* interrupt moderation */
>  	u32 tx_frames, rx_frames; /* interrupt moderation */
>  
> @@ -283,6 +295,9 @@ struct ena_adapter {
>  
>  	u8 mac_addr[ETH_ALEN];
>  
> +	unsigned long keep_alive_timeout;
> +	unsigned long missing_tx_completion_to;
> +
>  	char name[ENA_NAME_MAX_LEN];
>  
>  	unsigned long flags;
> diff --git a/drivers/net/ethernet/amazon/ena/ena_regs_defs.h b/drivers/net/ethernet/amazon/ena/ena_regs_defs.h
> index 26097a2..c3891c5 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_regs_defs.h
> +++ b/drivers/net/ethernet/amazon/ena/ena_regs_defs.h
> @@ -78,6 +78,8 @@
>  #define ENA_REGS_CAPS_RESET_TIMEOUT_MASK		0x3e
>  #define ENA_REGS_CAPS_DMA_ADDR_WIDTH_SHIFT		8
>  #define ENA_REGS_CAPS_DMA_ADDR_WIDTH_MASK		0xff00
> +#define ENA_REGS_CAPS_ADMIN_CMD_TO_SHIFT		16
> +#define ENA_REGS_CAPS_ADMIN_CMD_TO_MASK		0xf0000
>  
>  /* aq_caps register */
>  #define ENA_REGS_AQ_CAPS_AQ_DEPTH_MASK		0xffff

Re: [PATCH V2 net 08/20] net/ena: add hardware hints capability to the driver

[Netanel Belgazal]

On 12/05/2016 06:31 AM, Matt Wilson wrote:
> On Sun, Dec 04, 2016 at 03:19:26PM +0200, Netanel Belgazal wrote:
>> The ENA device can update the ena driver about the desire timeouts.
>> The hardware hints are transmitted as Asynchronous event to the driver.
> This is really a new feature, not a bugfix - correct? If it is a new
> feature, submit it separately. If the built-in defaults need to be
> changed, submit that as a bugfix.

I'll submit this patch as a new feature.
There is a patch that sets the new defaults.

> --msw
>
>> In case the device does not support this capability, the driver
>> will use its own defines.
>>
>> Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.com>
>> ---
>>   drivers/net/ethernet/amazon/ena/ena_admin_defs.h | 31 +++++++++
>>   drivers/net/ethernet/amazon/ena/ena_com.c        | 41 ++++++++---
>>   drivers/net/ethernet/amazon/ena/ena_com.h        |  5 ++
>>   drivers/net/ethernet/amazon/ena/ena_ethtool.c    |  1 -
>>   drivers/net/ethernet/amazon/ena/ena_netdev.c     | 86 +++++++++++++++++++-----
>>   drivers/net/ethernet/amazon/ena/ena_netdev.h     | 19 +++++-
>>   drivers/net/ethernet/amazon/ena/ena_regs_defs.h  |  2 +
>>   7 files changed, 157 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
>> index 6d70bf5..35ae511 100644
>> --- a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
>> +++ b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
>> @@ -70,6 +70,8 @@ enum ena_admin_aq_feature_id {
>>   
>>   	ENA_ADMIN_MAX_QUEUES_NUM		= 2,
>>   
>> +	ENA_ADMIN_HW_HINTS			= 3,
>> +
>>   	ENA_ADMIN_RSS_HASH_FUNCTION		= 10,
>>   
>>   	ENA_ADMIN_STATELESS_OFFLOAD_CONFIG	= 11,
>> @@ -749,6 +751,31 @@ struct ena_admin_feature_rss_ind_table {
>>   	struct ena_admin_rss_ind_table_entry inline_entry;
>>   };
>>   
>> +/* When hint value is 0, driver should use it's own predefined value */
>> +struct ena_admin_ena_hw_hints {
>> +	/* value in ms */
>> +	u16 mmio_read_timeout;
>> +
>> +	/* value in ms */
>> +	u16 driver_watchdog_timeout;
>> +
>> +	/* Per packet tx completion timeout. value in ms */
>> +	u16 missing_tx_completion_timeout;
>> +
>> +	u16 missed_tx_completion_count_threshold_to_reset;
>> +
>> +	/* value in ms */
>> +	u16 admin_completion_tx_timeout;
>> +
>> +	u16 netdev_wd_timeout;
>> +
>> +	u16 max_tx_sgl_size;
>> +
>> +	u16 max_rx_sgl_size;
>> +
>> +	u16 reserved[8];
>> +};
>> +
>>   struct ena_admin_get_feat_cmd {
>>   	struct ena_admin_aq_common_desc aq_common_descriptor;
>>   
>> @@ -782,6 +809,8 @@ struct ena_admin_get_feat_resp {
>>   		struct ena_admin_feature_rss_ind_table ind_table;
>>   
>>   		struct ena_admin_feature_intr_moder_desc intr_moderation;
>> +
>> +		struct ena_admin_ena_hw_hints hw_hints;
>>   	} u;
>>   };
>>   
>> @@ -857,6 +886,8 @@ enum ena_admin_aenq_notification_syndrom {
>>   	ENA_ADMIN_SUSPEND	= 0,
>>   
>>   	ENA_ADMIN_RESUME	= 1,
>> +
>> +	ENA_ADMIN_UPDATE_HINTS	= 2,
>>   };
>>   
>>   struct ena_admin_aenq_entry {
>> diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c b/drivers/net/ethernet/amazon/ena/ena_com.c
>> index 46aad3a..f1e4f04 100644
>> --- a/drivers/net/ethernet/amazon/ena/ena_com.c
>> +++ b/drivers/net/ethernet/amazon/ena/ena_com.c
>> @@ -508,15 +508,13 @@ static int ena_com_comp_status_to_errno(u8 comp_status)
>>   static int ena_com_wait_and_process_admin_cq_polling(struct ena_comp_ctx *comp_ctx,
>>   						     struct ena_com_admin_queue *admin_queue)
>>   {
>> -	unsigned long flags;
>> -	u32 start_time;
>> +	unsigned long flags, timeout;
>>   	int ret;
>>   
>> -	start_time = ((u32)jiffies_to_usecs(jiffies));
>> +	timeout = jiffies + usecs_to_jiffies(admin_queue->completion_timeout);
>>   
>>   	while (comp_ctx->status == ENA_CMD_SUBMITTED) {
>> -		if ((((u32)jiffies_to_usecs(jiffies)) - start_time) >
>> -		    ADMIN_CMD_TIMEOUT_US) {
>> +		if (time_is_before_jiffies(timeout)) {
>>   			pr_err("Wait for completion (polling) timeout\n");
>>   			/* ENA didn't have any completion */
>>   			spin_lock_irqsave(&admin_queue->q_lock, flags);
>> @@ -560,7 +558,8 @@ static int ena_com_wait_and_process_admin_cq_interrupts(struct ena_comp_ctx *com
>>   	int ret;
>>   
>>   	wait_for_completion_timeout(&comp_ctx->wait_event,
>> -				    usecs_to_jiffies(ADMIN_CMD_TIMEOUT_US));
>> +				    usecs_to_jiffies(
>> +					    admin_queue->completion_timeout));
>>   
>>   	/* In case the command wasn't completed find out the root cause.
>>   	 * There might be 2 kinds of errors
>> @@ -600,12 +599,14 @@ static u32 ena_com_reg_bar_read32(struct ena_com_dev *ena_dev, u16 offset)
>>   	struct ena_com_mmio_read *mmio_read = &ena_dev->mmio_read;
>>   	volatile struct ena_admin_ena_mmio_req_read_less_resp *read_resp =
>>   		mmio_read->read_resp;
>> -	u32 mmio_read_reg, ret;
>> +	u32 mmio_read_reg, timeout, ret;
>>   	unsigned long flags;
>>   	int i;
>>   
>>   	might_sleep();
>>   
>> +	timeout = mmio_read->reg_read_to ? : ENA_REG_READ_TIMEOUT;
>> +
>>   	/* If readless is disabled, perform regular read */
>>   	if (!mmio_read->readless_supported)
>>   		return readl(ena_dev->reg_bar + offset);
>> @@ -626,14 +627,14 @@ static u32 ena_com_reg_bar_read32(struct ena_com_dev *ena_dev, u16 offset)
>>   
>>   	writel(mmio_read_reg, ena_dev->reg_bar + ENA_REGS_MMIO_REG_READ_OFF);
>>   
>> -	for (i = 0; i < ENA_REG_READ_TIMEOUT; i++) {
>> +	for (i = 0; i < timeout; i++) {
>>   		if (read_resp->req_id == mmio_read->seq_num)
>>   			break;
>>   
>>   		udelay(1);
>>   	}
>>   
>> -	if (unlikely(i == ENA_REG_READ_TIMEOUT)) {
>> +	if (unlikely(i == timeout)) {
>>   		pr_err("reading reg failed for timeout. expected: req id[%hu] offset[%hu] actual: req id[%hu] offset[%hu]\n",
>>   		       mmio_read->seq_num, offset, read_resp->req_id,
>>   		       read_resp->reg_off);
>> @@ -1717,6 +1718,20 @@ int ena_com_get_dev_attr_feat(struct ena_com_dev *ena_dev,
>>   	memcpy(&get_feat_ctx->offload, &get_resp.u.offload,
>>   	       sizeof(get_resp.u.offload));
>>   
>> +	/* Driver hints isn't mandatory admin command. So in case the
>> +	 * command isn't supported set driver hints to 0
>> +	 */
>> +	rc = ena_com_get_feature(ena_dev, &get_resp, ENA_ADMIN_HW_HINTS);
>> +
>> +	if (!rc)
>> +		memcpy(&get_feat_ctx->hw_hints, &get_resp.u.hw_hints,
>> +		       sizeof(get_resp.u.hw_hints));
>> +	else if (rc == -EPERM)
>> +		memset(&get_feat_ctx->hw_hints, 0x0,
>> +		       sizeof(get_feat_ctx->hw_hints));
>> +	else
>> +		return rc;
>> +
>>   	return 0;
>>   }
>>   
>> @@ -1842,6 +1857,14 @@ int ena_com_dev_reset(struct ena_com_dev *ena_dev)
>>   		return rc;
>>   	}
>>   
>> +	timeout = (cap & ENA_REGS_CAPS_ADMIN_CMD_TO_MASK) >>
>> +		ENA_REGS_CAPS_ADMIN_CMD_TO_SHIFT;
>> +	if (timeout)
>> +		/* the resolution of timeout reg is 100ms */
>> +		ena_dev->admin_queue.completion_timeout = timeout * 100000;
>> +	else
>> +		ena_dev->admin_queue.completion_timeout = ADMIN_CMD_TIMEOUT_US;
>> +
>>   	return 0;
>>   }
>>   
>> diff --git a/drivers/net/ethernet/amazon/ena/ena_com.h b/drivers/net/ethernet/amazon/ena/ena_com.h
>> index 509d7b8..6883ee5 100644
>> --- a/drivers/net/ethernet/amazon/ena/ena_com.h
>> +++ b/drivers/net/ethernet/amazon/ena/ena_com.h
>> @@ -96,6 +96,8 @@
>>   #define ENA_INTR_MODER_LEVEL_STRIDE			2
>>   #define ENA_INTR_BYTE_COUNT_NOT_SUPPORTED		0xFFFFFF
>>   
>> +#define ENA_HW_HINTS_NO_TIMEOUT				0xFFFF
>> +
>>   enum ena_intr_moder_level {
>>   	ENA_INTR_MODER_LOWEST = 0,
>>   	ENA_INTR_MODER_LOW,
>> @@ -232,6 +234,7 @@ struct ena_com_admin_queue {
>>   	void *q_dmadev;
>>   	spinlock_t q_lock; /* spinlock for the admin queue */
>>   	struct ena_comp_ctx *comp_ctx;
>> +	u32 completion_timeout;
>>   	u16 q_depth;
>>   	struct ena_com_admin_cq cq;
>>   	struct ena_com_admin_sq sq;
>> @@ -266,6 +269,7 @@ struct ena_com_aenq {
>>   struct ena_com_mmio_read {
>>   	struct ena_admin_ena_mmio_req_read_less_resp *read_resp;
>>   	dma_addr_t read_resp_dma_addr;
>> +	u32 reg_read_to; /* in us */
>>   	u16 seq_num;
>>   	bool readless_supported;
>>   	/* spin lock to ensure a single outstanding read */
>> @@ -335,6 +339,7 @@ struct ena_com_dev_get_features_ctx {
>>   	struct ena_admin_device_attr_feature_desc dev_attr;
>>   	struct ena_admin_feature_aenq_desc aenq;
>>   	struct ena_admin_feature_offload_desc offload;
>> +	struct ena_admin_ena_hw_hints hw_hints;
>>   };
>>   
>>   struct ena_com_create_io_ctx {
>> diff --git a/drivers/net/ethernet/amazon/ena/ena_ethtool.c b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
>> index 67b2338f..a1fbfc2 100644
>> --- a/drivers/net/ethernet/amazon/ena/ena_ethtool.c
>> +++ b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
>> @@ -80,7 +80,6 @@ static const struct ena_stats ena_stats_tx_strings[] = {
>>   	ENA_STAT_TX_ENTRY(tx_poll),
>>   	ENA_STAT_TX_ENTRY(doorbells),
>>   	ENA_STAT_TX_ENTRY(prepare_ctx_err),
>> -	ENA_STAT_TX_ENTRY(missing_tx_comp),
>>   	ENA_STAT_TX_ENTRY(bad_req_id),
>>   };
>>   
>> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
>> index 962ffb5..0b718ee 100644
>> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
>> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
>> @@ -1981,6 +1981,7 @@ static netdev_tx_t ena_start_xmit(struct sk_buff *skb, struct net_device *dev)
>>   
>>   	tx_info->tx_descs = nb_hw_desc;
>>   	tx_info->last_jiffies = jiffies;
>> +	tx_info->print_once = 0;
>>   
>>   	tx_ring->next_to_use = ENA_TX_RING_IDX_NEXT(next_to_use,
>>   		tx_ring->ring_size);
>> @@ -2554,33 +2555,34 @@ static void check_for_missing_tx_completions(struct ena_adapter *adapter)
>>   	if (!test_bit(ENA_FLAG_DEV_UP, &adapter->flags))
>>   		return;
>>   
>> +	if (adapter->missing_tx_completion_to == ENA_HW_HINTS_NO_TIMEOUT)
>> +		return;
>> +
>>   	budget = ENA_MONITORED_TX_QUEUES;
>>   
>>   	for (i = adapter->last_monitored_tx_qid; i < adapter->num_queues; i++) {
>>   		tx_ring = &adapter->tx_ring[i];
>>   
>> +		missed_tx = 0;
>> +
>>   		for (j = 0; j < tx_ring->ring_size; j++) {
>>   			tx_buf = &tx_ring->tx_buffer_info[j];
>>   			last_jiffies = tx_buf->last_jiffies;
>> -			if (unlikely(last_jiffies && time_is_before_jiffies(last_jiffies + TX_TIMEOUT))) {
>> -				netif_notice(adapter, tx_err, adapter->netdev,
>> -					     "Found a Tx that wasn't completed on time, qid %d, index %d.\n",
>> -					     tx_ring->qid, j);
>> +			if (unlikely(last_jiffies && time_is_before_jiffies(last_jiffies + adapter->missing_tx_completion_to))) {
>> +				if (!tx_buf->print_once)
>> +					netif_notice(adapter, tx_err, adapter->netdev,
>> +						     "Found a Tx that wasn't completed on time, qid %d, index %d.\n",
>> +						     tx_ring->qid, j);
>>   
>> -				u64_stats_update_begin(&tx_ring->syncp);
>> -				missed_tx = tx_ring->tx_stats.missing_tx_comp++;
>> -				u64_stats_update_end(&tx_ring->syncp);
>> +				tx_buf->print_once = 1;
>> +				missed_tx++;
>>   
>> -				/* Clear last jiffies so the lost buffer won't
>> -				 * be counted twice.
>> -				 */
>> -				tx_buf->last_jiffies = 0;
>> -
>> -				if (unlikely(missed_tx > MAX_NUM_OF_TIMEOUTED_PACKETS)) {
>> +				if (unlikely(missed_tx > adapter->missing_tx_completion_threshold)) {
>>   					netif_err(adapter, tx_err, adapter->netdev,
>>   						  "The number of lost tx completion is above the threshold (%d > %d). Reset the device\n",
>> -						  missed_tx, MAX_NUM_OF_TIMEOUTED_PACKETS);
>> +						  missed_tx, adapter->missing_tx_completion_threshold);
>>   					set_bit(ENA_FLAG_TRIGGER_RESET, &adapter->flags);
>> +					return;
>>   				}
>>   			}
>>   		}
>> @@ -2601,8 +2603,11 @@ static void check_for_missing_keep_alive(struct ena_adapter *adapter)
>>   	if (!adapter->wd_state)
>>   		return;
>>   
>> -	keep_alive_expired = round_jiffies(adapter->last_keep_alive_jiffies
>> -					   + ENA_DEVICE_KALIVE_TIMEOUT);
>> +	if (adapter->keep_alive_timeout == ENA_HW_HINTS_NO_TIMEOUT)
>> +		return;
>> +
>> +	keep_alive_expired = round_jiffies(adapter->last_keep_alive_jiffies +
>> +					   adapter->keep_alive_timeout);
>>   	if (unlikely(time_is_before_jiffies(keep_alive_expired))) {
>>   		netif_err(adapter, drv, adapter->netdev,
>>   			  "Keep alive watchdog timeout.\n");
>> @@ -2625,6 +2630,44 @@ static void check_for_admin_com_state(struct ena_adapter *adapter)
>>   	}
>>   }
>>   
>> +static void ena_update_hints(struct ena_adapter *adapter,
>> +			     struct ena_admin_ena_hw_hints *hints)
>> +{
>> +	struct net_device *netdev = adapter->netdev;
>> +
>> +	if (hints->admin_completion_tx_timeout)
>> +		adapter->ena_dev->admin_queue.completion_timeout =
>> +			hints->admin_completion_tx_timeout * 1000;
>> +
>> +	if (hints->mmio_read_timeout)
>> +		/* convert to usec */
>> +		adapter->ena_dev->mmio_read.reg_read_to =
>> +			hints->mmio_read_timeout * 1000;
>> +
>> +	if (hints->missed_tx_completion_count_threshold_to_reset)
>> +		adapter->missing_tx_completion_threshold =
>> +			hints->missed_tx_completion_count_threshold_to_reset;
>> +
>> +	if (hints->missing_tx_completion_timeout) {
>> +		if (hints->missing_tx_completion_timeout == ENA_HW_HINTS_NO_TIMEOUT)
>> +			adapter->missing_tx_completion_to = ENA_HW_HINTS_NO_TIMEOUT;
>> +		else
>> +			adapter->missing_tx_completion_to =
>> +				msecs_to_jiffies(hints->missing_tx_completion_timeout);
>> +	}
>> +
>> +	if (hints->netdev_wd_timeout)
>> +		netdev->watchdog_timeo = msecs_to_jiffies(hints->netdev_wd_timeout);
>> +
>> +	if (hints->driver_watchdog_timeout) {
>> +		if (hints->driver_watchdog_timeout == ENA_HW_HINTS_NO_TIMEOUT)
>> +			adapter->keep_alive_timeout = ENA_HW_HINTS_NO_TIMEOUT;
>> +		else
>> +			adapter->keep_alive_timeout =
>> +				msecs_to_jiffies(hints->driver_watchdog_timeout);
>> +	}
>> +}
>> +
>>   static void ena_update_host_info(struct ena_admin_host_info *host_info,
>>   				 struct net_device *netdev)
>>   {
>> @@ -3036,6 +3079,11 @@ static int ena_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>   	INIT_WORK(&adapter->reset_task, ena_fw_reset_device);
>>   
>>   	adapter->last_keep_alive_jiffies = jiffies;
>> +	adapter->keep_alive_timeout = ENA_DEVICE_KALIVE_TIMEOUT;
>> +	adapter->missing_tx_completion_to = TX_TIMEOUT;
>> +	adapter->missing_tx_completion_threshold = MAX_NUM_OF_TIMEOUTED_PACKETS;
>> +
>> +	ena_update_hints(adapter, &get_feat_ctx.hw_hints);
>>   
>>   	init_timer(&adapter->timer_service);
>>   	adapter->timer_service.expires = round_jiffies(jiffies + HZ);
>> @@ -3256,6 +3304,7 @@ static void ena_notification(void *adapter_data,
>>   			     struct ena_admin_aenq_entry *aenq_e)
>>   {
>>   	struct ena_adapter *adapter = (struct ena_adapter *)adapter_data;
>> +	struct ena_admin_ena_hw_hints *hints;
>>   
>>   	WARN(aenq_e->aenq_common_desc.group != ENA_ADMIN_NOTIFICATION,
>>   	     "Invalid group(%x) expected %x\n",
>> @@ -3273,6 +3322,11 @@ static void ena_notification(void *adapter_data,
>>   	case ENA_ADMIN_RESUME:
>>   		queue_work(ena_wq, &adapter->resume_io_task);
>>   		break;
>> +	case ENA_ADMIN_UPDATE_HINTS:
>> +		hints = (struct ena_admin_ena_hw_hints *)
>> +			(&aenq_e->inline_data_w4);
>> +		ena_update_hints(adapter, hints);
>> +		break;
>>   	default:
>>   		netif_err(adapter, drv, adapter->netdev,
>>   			  "Invalid aenq notification link state %d\n",
>> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.h b/drivers/net/ethernet/amazon/ena/ena_netdev.h
>> index f0ddc11..2897fab 100644
>> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.h
>> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.h
>> @@ -146,7 +146,18 @@ struct ena_tx_buffer {
>>   	u32 tx_descs;
>>   	/* num of buffers used by this skb */
>>   	u32 num_of_bufs;
>> -	/* Save the last jiffies to detect missing tx packets */
>> +
>> +	/* Used for detect missing tx packets to limit the number of prints */
>> +	u32 print_once;
>> +	/* Save the last jiffies to detect missing tx packets
>> +	 *
>> +	 * sets to non zero value on ena_start_xmit and set to zero on
>> +	 * napi and timer_Service_routine.
>> +	 *
>> +	 * while this value is not protected by lock,
>> +	 * a given packet is not expected to be handled by ena_start_xmit
>> +	 * and by napi/timer_service at the same time.
>> +	 */
>>   	unsigned long last_jiffies;
>>   	struct ena_com_buf bufs[ENA_PKT_MAX_BUFS];
>>   } ____cacheline_aligned;
>> @@ -170,7 +181,6 @@ struct ena_stats_tx {
>>   	u64 napi_comp;
>>   	u64 tx_poll;
>>   	u64 doorbells;
>> -	u64 missing_tx_comp;
>>   	u64 bad_req_id;
>>   };
>>   
>> @@ -270,6 +280,8 @@ struct ena_adapter {
>>   	struct msix_entry *msix_entries;
>>   	int msix_vecs;
>>   
>> +	u32 missing_tx_completion_threshold;
>> +
>>   	u32 tx_usecs, rx_usecs; /* interrupt moderation */
>>   	u32 tx_frames, rx_frames; /* interrupt moderation */
>>   
>> @@ -283,6 +295,9 @@ struct ena_adapter {
>>   
>>   	u8 mac_addr[ETH_ALEN];
>>   
>> +	unsigned long keep_alive_timeout;
>> +	unsigned long missing_tx_completion_to;
>> +
>>   	char name[ENA_NAME_MAX_LEN];
>>   
>>   	unsigned long flags;
>> diff --git a/drivers/net/ethernet/amazon/ena/ena_regs_defs.h b/drivers/net/ethernet/amazon/ena/ena_regs_defs.h
>> index 26097a2..c3891c5 100644
>> --- a/drivers/net/ethernet/amazon/ena/ena_regs_defs.h
>> +++ b/drivers/net/ethernet/amazon/ena/ena_regs_defs.h
>> @@ -78,6 +78,8 @@
>>   #define ENA_REGS_CAPS_RESET_TIMEOUT_MASK		0x3e
>>   #define ENA_REGS_CAPS_DMA_ADDR_WIDTH_SHIFT		8
>>   #define ENA_REGS_CAPS_DMA_ADDR_WIDTH_MASK		0xff00
>> +#define ENA_REGS_CAPS_ADMIN_CMD_TO_SHIFT		16
>> +#define ENA_REGS_CAPS_ADMIN_CMD_TO_MASK		0xf0000
>>   
>>   /* aq_caps register */
>>   #define ENA_REGS_AQ_CAPS_AQ_DEPTH_MASK		0xffff

[PATCH V2 net 09/20] net/ena: fix potential access to freed memory during device reset

[Netanel Belgazal]

If the ena driver detects that the device is not behave as expected,
it tries to reset the device.
The reset flow calls ena_down, which will frees all the resources
the driver allocates and then it will reset the device.

This flow can cause memory corruption if the device is still writes
to the driver's memory space.
To overcome this potential race, move the reset before the device
resources are freed.

Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.com>
---
 drivers/net/ethernet/amazon/ena/ena_netdev.c | 56 +++++++++++++++++++++-------
 1 file changed, 43 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index 0b718ee..bb7eeea 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -80,14 +80,18 @@ static void ena_tx_timeout(struct net_device *dev)
 {
 	struct ena_adapter *adapter = netdev_priv(dev);
 
+	/* Change the state of the device to trigger reset
+	 * Check that we are not in the middle or a trigger already
+	 */
+
+	if (test_and_set_bit(ENA_FLAG_TRIGGER_RESET, &adapter->flags))
+		return;
+
 	u64_stats_update_begin(&adapter->syncp);
 	adapter->dev_stats.tx_timeout++;
 	u64_stats_update_end(&adapter->syncp);
 
 	netif_err(adapter, tx_err, dev, "Transmit time out\n");
-
-	/* Change the state of the device to trigger reset */
-	set_bit(ENA_FLAG_TRIGGER_RESET, &adapter->flags);
 }
 
 static void update_rx_ring_mtu(struct ena_adapter *adapter, int mtu)
@@ -1116,7 +1120,8 @@ static int ena_io_poll(struct napi_struct *napi, int budget)
 
 	tx_budget = tx_ring->ring_size / ENA_TX_POLL_BUDGET_DIVIDER;
 
-	if (!test_bit(ENA_FLAG_DEV_UP, &tx_ring->adapter->flags)) {
+	if (!test_bit(ENA_FLAG_DEV_UP, &tx_ring->adapter->flags) ||
+	    test_bit(ENA_FLAG_TRIGGER_RESET, &tx_ring->adapter->flags)) {
 		napi_complete_done(napi, 0);
 		return 0;
 	}
@@ -1705,12 +1710,22 @@ static void ena_down(struct ena_adapter *adapter)
 	adapter->dev_stats.interface_down++;
 	u64_stats_update_end(&adapter->syncp);
 
-	/* After this point the napi handler won't enable the tx queue */
-	ena_napi_disable_all(adapter);
 	netif_carrier_off(adapter->netdev);
 	netif_tx_disable(adapter->netdev);
 
+	/* After this point the napi handler won't enable the tx queue */
+	ena_napi_disable_all(adapter);
+
 	/* After destroy the queue there won't be any new interrupts */
+
+	if (test_bit(ENA_FLAG_TRIGGER_RESET, &adapter->flags)) {
+		int rc;
+
+		rc = ena_com_dev_reset(adapter->ena_dev);
+		if (rc)
+			dev_err(&adapter->pdev->dev, "Device reset failed\n");
+	}
+
 	ena_destroy_all_io_queues(adapter);
 
 	ena_disable_io_intr_sync(adapter);
@@ -2073,6 +2088,14 @@ static void ena_netpoll(struct net_device *netdev)
 	struct ena_adapter *adapter = netdev_priv(netdev);
 	int i;
 
+	/* Dont schedule NAPI if the driver is in the middle of reset
+	 * or netdev is down.
+	 */
+
+	if (!test_bit(ENA_FLAG_DEV_UP, &adapter->flags) ||
+	    test_bit(ENA_FLAG_TRIGGER_RESET, &adapter->flags))
+		return;
+
 	for (i = 0; i < adapter->num_queues; i++)
 		napi_schedule(&adapter->ena_napi[i].napi);
 }
@@ -2459,6 +2482,14 @@ static void ena_fw_reset_device(struct work_struct *work)
 	bool dev_up, wd_state;
 	int rc;
 
+	if (unlikely(!test_bit(ENA_FLAG_TRIGGER_RESET, &adapter->flags))) {
+		dev_err(&pdev->dev,
+			"device reset schedule while reset bit is off\n");
+		return;
+	}
+
+	netif_carrier_off(netdev);
+
 	del_timer_sync(&adapter->timer_service);
 
 	rtnl_lock();
@@ -2472,12 +2503,6 @@ static void ena_fw_reset_device(struct work_struct *work)
 	 */
 	ena_close(netdev);
 
-	rc = ena_com_dev_reset(ena_dev);
-	if (rc) {
-		dev_err(&pdev->dev, "Device reset failed\n");
-		goto err;
-	}
-
 	ena_free_mgmnt_irq(adapter);
 
 	ena_disable_msix(adapter);
@@ -2490,6 +2515,8 @@ static void ena_fw_reset_device(struct work_struct *work)
 
 	ena_com_mmio_reg_read_request_destroy(ena_dev);
 
+	clear_bit(ENA_FLAG_TRIGGER_RESET, &adapter->flags);
+
 	/* Finish with the destroy part. Start the init part */
 
 	rc = ena_device_init(ena_dev, adapter->pdev, &get_feat_ctx, &wd_state);
@@ -2555,6 +2582,9 @@ static void check_for_missing_tx_completions(struct ena_adapter *adapter)
 	if (!test_bit(ENA_FLAG_DEV_UP, &adapter->flags))
 		return;
 
+	if (test_bit(ENA_FLAG_TRIGGER_RESET, &adapter->flags))
+		return;
+
 	if (adapter->missing_tx_completion_to == ENA_HW_HINTS_NO_TIMEOUT)
 		return;
 
@@ -2696,7 +2726,7 @@ static void ena_timer_service(unsigned long data)
 	if (host_info)
 		ena_update_host_info(host_info, adapter->netdev);
 
-	if (unlikely(test_and_clear_bit(ENA_FLAG_TRIGGER_RESET, &adapter->flags))) {
+	if (unlikely(test_bit(ENA_FLAG_TRIGGER_RESET, &adapter->flags))) {
 		netif_err(adapter, drv, adapter->netdev,
 			  "Trigger reset is on\n");
 		ena_dump_stats_to_dmesg(adapter);
-- 
2.7.4

[PATCH V2 net 10/20] net/ena: remove redundant logic in napi callback for busy poll mode

[Netanel Belgazal]

sk_busy_loop can call the napi callback few million times a sec.
For each call there is unmask interrupt.
We want to reduce the number of unmasks.

Add an atomic variable that will tell the napi handler if
it was called from irq context or not.
Unmask the interrupt only from irq context.

A schenario where the driver left with missed unmask isn't feasible.
when ena_intr_msix_io is called the driver have 2 options:
1)Before napi completes and call napi_complete_done
2)After calling napi_complete_done

In the former case the napi will unmask the interrupt as needed.
In the latter case napi_complete_done will remove napi from the schedule
list so napi will be rescheduled (by ena_intr_msix_io) and interrupt
will be unmasked as desire in the 2nd napi call.

Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.com>
---
 drivers/net/ethernet/amazon/ena/ena_netdev.c | 46 +++++++++++++++++++---------
 drivers/net/ethernet/amazon/ena/ena_netdev.h |  1 +
 2 files changed, 32 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index bb7eeea..5625007 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -1129,26 +1129,41 @@ static int ena_io_poll(struct napi_struct *napi, int budget)
 	tx_work_done = ena_clean_tx_irq(tx_ring, tx_budget);
 	rx_work_done = ena_clean_rx_irq(rx_ring, napi, budget);
 
-	if ((budget > rx_work_done) && (tx_budget > tx_work_done)) {
+	/* If the device is about to reset or down, avoid unmask
+	 * the interrupt and return 0 so NAPI won't reschedule
+	 */
+	if (unlikely(!test_bit(ENA_FLAG_DEV_UP, &tx_ring->adapter->flags) ||
+		     test_bit(ENA_FLAG_TRIGGER_RESET, &tx_ring->adapter->flags))) {
+		napi_complete_done(napi, 0);
+		ret = 0;
+
+	} else if ((budget > rx_work_done) && (tx_budget > tx_work_done)) {
 		napi_complete_done(napi, rx_work_done);
 
 		napi_comp_call = 1;
-		/* Tx and Rx share the same interrupt vector */
-		if (ena_com_get_adaptive_moderation_enabled(rx_ring->ena_dev))
-			ena_adjust_intr_moderation(rx_ring, tx_ring);
-
-		/* Update intr register: rx intr delay, tx intr delay and
-		 * interrupt unmask
+		/* Update numa and unmask the interrupt only when schedule
+		 * from the interrupt context (vs from sk_busy_loop)
 		 */
-		ena_com_update_intr_reg(&intr_reg,
-					rx_ring->smoothed_interval,
-					tx_ring->smoothed_interval,
-					true);
+		if (atomic_cmpxchg(&ena_napi->unmask_interrupt, 1, 0)) {
+			/* Tx and Rx share the same interrupt vector */
+			if (ena_com_get_adaptive_moderation_enabled(rx_ring->ena_dev))
+				ena_adjust_intr_moderation(rx_ring, tx_ring);
+
+			/* Update intr register: rx intr delay,
+			 * tx intr delay and interrupt unmask
+			 */
+			ena_com_update_intr_reg(&intr_reg,
+						rx_ring->smoothed_interval,
+						tx_ring->smoothed_interval,
+						true);
+
+			/* It is a shared MSI-X.
+			 * Tx and Rx CQ have pointer to it.
+			 * So we use one of them to reach the intr reg
+			 */
+			ena_com_unmask_intr(rx_ring->ena_com_io_cq, &intr_reg);
+		}
 
-		/* It is a shared MSI-X. Tx and Rx CQ have pointer to it.
-		 * So we use one of them to reach the intr reg
-		 */
-		ena_com_unmask_intr(rx_ring->ena_com_io_cq, &intr_reg);
 
 		ena_update_ring_numa_node(tx_ring, rx_ring);
 
@@ -1186,6 +1201,7 @@ static irqreturn_t ena_intr_msix_io(int irq, void *data)
 {
 	struct ena_napi *ena_napi = data;
 
+	atomic_set(&ena_napi->unmask_interrupt, 1);
 	napi_schedule(&ena_napi->napi);
 
 	return IRQ_HANDLED;
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.h b/drivers/net/ethernet/amazon/ena/ena_netdev.h
index 2897fab..c081fd3 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.h
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.h
@@ -135,6 +135,7 @@ struct ena_napi {
 	struct napi_struct napi ____cacheline_aligned;
 	struct ena_ring *tx_ring;
 	struct ena_ring *rx_ring;
+	atomic_t unmask_interrupt;
 	u32 qid;
 };
 
-- 
2.7.4

Re: [PATCH V2 net 10/20] net/ena: remove redundant logic in napi callback for busy poll mode

[Eric Dumazet]

On Sun, 2016-12-04 at 15:19 +0200, Netanel Belgazal wrote:
> sk_busy_loop can call the napi callback few million times a sec.
> For each call there is unmask interrupt.
> We want to reduce the number of unmasks.
> 
> Add an atomic variable that will tell the napi handler if
> it was called from irq context or not.
> Unmask the interrupt only from irq context.
> 
> A schenario where the driver left with missed unmask isn't feasible.
> when ena_intr_msix_io is called the driver have 2 options:
> 1)Before napi completes and call napi_complete_done
> 2)After calling napi_complete_done
> 
> In the former case the napi will unmask the interrupt as needed.
> In the latter case napi_complete_done will remove napi from the schedule
> list so napi will be rescheduled (by ena_intr_msix_io) and interrupt
> will be unmasked as desire in the 2nd napi call.
> 
> Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.com>
> ---


This looks very complicated to me.

I guess you missed the recent patches that happened on net-next ?

2e713283751f494596655d9125c168aeb913f71d net/mlx4_en: use napi_complete_done() return value
364b6055738b4c752c30ccaaf25c624e69d76195 net: busy-poll: return busypolling status to drivers
21cb84c48ca0619181106f0f44f3802a989de024 net: busy-poll: remove need_resched() from sk_can_busy_loop()
217f6974368188fd8bd7804bf5a036aa5762c5e4 net: busy-poll: allow preemption in sk_busy_loop()

napi_complete_done() return code can be used by a driver,
no need to add yet another atomic operation in fast path.

Anyway, this looks wrong :

@@ -1186,6 +1201,7 @@ static irqreturn_t ena_intr_msix_io(int irq, void *data)
 {
        struct ena_napi *ena_napi = data;
 
+       atomic_set(&ena_napi->unmask_interrupt, 1);
        napi_schedule(&ena_napi->napi);
 
You probably wanted :

if (napi_schedule_prep(n)) {
	atomic_set(&ena_napi->unmask_interrupt, 1);
	__napi_schedule(n);
}



Please rework this napi poll using core infrastructure.

busypoll logic should be centralized, not reimplemented in different ways in a driver.

Thanks.

Re: [PATCH V2 net 10/20] net/ena: remove redundant logic in napi callback for busy poll mode

[Netanel Belgazal]

On 12/05/2016 07:45 AM, Eric Dumazet wrote:
> On Sun, 2016-12-04 at 15:19 +0200, Netanel Belgazal wrote:
>> sk_busy_loop can call the napi callback few million times a sec.
>> For each call there is unmask interrupt.
>> We want to reduce the number of unmasks.
>>
>> Add an atomic variable that will tell the napi handler if
>> it was called from irq context or not.
>> Unmask the interrupt only from irq context.
>>
>> A schenario where the driver left with missed unmask isn't feasible.
>> when ena_intr_msix_io is called the driver have 2 options:
>> 1)Before napi completes and call napi_complete_done
>> 2)After calling napi_complete_done
>>
>> In the former case the napi will unmask the interrupt as needed.
>> In the latter case napi_complete_done will remove napi from the schedule
>> list so napi will be rescheduled (by ena_intr_msix_io) and interrupt
>> will be unmasked as desire in the 2nd napi call.
>>
>> Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.com>
>> ---
>
> This looks very complicated to me.
>
> I guess you missed the recent patches that happened on net-next ?

You are correct.
I didn't see the patches.
It is much better to use the napi_complete_done() return value.
I'll rework my patch.

>
> 2e713283751f494596655d9125c168aeb913f71d net/mlx4_en: use napi_complete_done() return value
> 364b6055738b4c752c30ccaaf25c624e69d76195 net: busy-poll: return busypolling status to drivers
> 21cb84c48ca0619181106f0f44f3802a989de024 net: busy-poll: remove need_resched() from sk_can_busy_loop()
> 217f6974368188fd8bd7804bf5a036aa5762c5e4 net: busy-poll: allow preemption in sk_busy_loop()
>
> napi_complete_done() return code can be used by a driver,
> no need to add yet another atomic operation in fast path.
>
> Anyway, this looks wrong :
>
> @@ -1186,6 +1201,7 @@ static irqreturn_t ena_intr_msix_io(int irq, void *data)
>   {
>          struct ena_napi *ena_napi = data;
>   
> +       atomic_set(&ena_napi->unmask_interrupt, 1);
>          napi_schedule(&ena_napi->napi);
>   
> You probably wanted :
>
> if (napi_schedule_prep(n)) {
> 	atomic_set(&ena_napi->unmask_interrupt, 1);
> 	__napi_schedule(n);
> }
>
>
>
> Please rework this napi poll using core infrastructure.
>
> busypoll logic should be centralized, not reimplemented in different ways in a driver.
>
> Thanks.
>
>
>

Re: [PATCH V2 net 10/20] net/ena: remove redundant logic in napi callback for busy poll mode

[Eric Dumazet]

On Mon, 2016-12-05 at 20:29 +0200, Netanel Belgazal wrote:

> You are correct.
> I didn't see the patches.
> It is much better to use the napi_complete_done() return value.
> I'll rework my patch.

Excellent, please CC me on this particular work on the future.

Thanks.

[PATCH V2 net 11/20] net/ena: use READ_ONCE to access completion descriptors

[Netanel Belgazal]

Completion descriptors are accessed from the driver and from the device.
To avoid reading the old value, use READ_ONCE macro.

Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.com>
---
 drivers/net/ethernet/amazon/ena/ena_com.h     | 1 +
 drivers/net/ethernet/amazon/ena/ena_eth_com.c | 8 ++++----
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_com.h b/drivers/net/ethernet/amazon/ena/ena_com.h
index 6883ee5..f8cdce0 100644
--- a/drivers/net/ethernet/amazon/ena/ena_com.h
+++ b/drivers/net/ethernet/amazon/ena/ena_com.h
@@ -33,6 +33,7 @@
 #ifndef ENA_COM
 #define ENA_COM
 
+#include <linux/compiler.h>
 #include <linux/delay.h>
 #include <linux/dma-mapping.h>
 #include <linux/gfp.h>
diff --git a/drivers/net/ethernet/amazon/ena/ena_eth_com.c b/drivers/net/ethernet/amazon/ena/ena_eth_com.c
index 539c536..f999305 100644
--- a/drivers/net/ethernet/amazon/ena/ena_eth_com.c
+++ b/drivers/net/ethernet/amazon/ena/ena_eth_com.c
@@ -45,7 +45,7 @@ static inline struct ena_eth_io_rx_cdesc_base *ena_com_get_next_rx_cdesc(
 	cdesc = (struct ena_eth_io_rx_cdesc_base *)(io_cq->cdesc_addr.virt_addr
 			+ (head_masked * io_cq->cdesc_entry_size_in_bytes));
 
-	desc_phase = (cdesc->status & ENA_ETH_IO_RX_CDESC_BASE_PHASE_MASK) >>
+	desc_phase = (READ_ONCE(cdesc->status) & ENA_ETH_IO_RX_CDESC_BASE_PHASE_MASK) >>
 			ENA_ETH_IO_RX_CDESC_BASE_PHASE_SHIFT;
 
 	if (desc_phase != expected_phase)
@@ -141,7 +141,7 @@ static inline u16 ena_com_cdesc_rx_pkt_get(struct ena_com_io_cq *io_cq,
 
 		ena_com_cq_inc_head(io_cq);
 		count++;
-		last = (cdesc->status & ENA_ETH_IO_RX_CDESC_BASE_LAST_MASK) >>
+		last = (READ_ONCE(cdesc->status) & ENA_ETH_IO_RX_CDESC_BASE_LAST_MASK) >>
 			ENA_ETH_IO_RX_CDESC_BASE_LAST_SHIFT;
 	} while (!last);
 
@@ -489,13 +489,13 @@ int ena_com_tx_comp_req_id_get(struct ena_com_io_cq *io_cq, u16 *req_id)
 	 * expected, it mean that the device still didn't update
 	 * this completion.
 	 */
-	cdesc_phase = cdesc->flags & ENA_ETH_IO_TX_CDESC_PHASE_MASK;
+	cdesc_phase = READ_ONCE(cdesc->flags) & ENA_ETH_IO_TX_CDESC_PHASE_MASK;
 	if (cdesc_phase != expected_phase)
 		return -EAGAIN;
 
 	ena_com_cq_inc_head(io_cq);
 
-	*req_id = cdesc->req_id;
+	*req_id = READ_ONCE(cdesc->req_id);
 
 	return 0;
 }
-- 
2.7.4

[PATCH V2 net 12/20] net/ena: reduce the severity of ena printouts

[Netanel Belgazal]

Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.com>
---
 drivers/net/ethernet/amazon/ena/ena_com.c    | 27 +++++++++++++++++----------
 drivers/net/ethernet/amazon/ena/ena_netdev.c | 14 +++++++++++---
 2 files changed, 28 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c b/drivers/net/ethernet/amazon/ena/ena_com.c
index f1e4f04..4147d6e 100644
--- a/drivers/net/ethernet/amazon/ena/ena_com.c
+++ b/drivers/net/ethernet/amazon/ena/ena_com.c
@@ -785,7 +785,7 @@ static int ena_com_get_feature_ex(struct ena_com_dev *ena_dev,
 	int ret;
 
 	if (!ena_com_check_supported_feature_id(ena_dev, feature_id)) {
-		pr_info("Feature %d isn't supported\n", feature_id);
+		pr_debug("Feature %d isn't supported\n", feature_id);
 		return -EPERM;
 	}
 
@@ -1127,7 +1127,13 @@ int ena_com_execute_admin_command(struct ena_com_admin_queue *admin_queue,
 	comp_ctx = ena_com_submit_admin_cmd(admin_queue, cmd, cmd_size,
 					    comp, comp_size);
 	if (unlikely(IS_ERR(comp_ctx))) {
-		pr_err("Failed to submit command [%ld]\n", PTR_ERR(comp_ctx));
+		if (comp_ctx == ERR_PTR(-ENODEV))
+			pr_debug("Failed to submit command [%ld]\n",
+				 PTR_ERR(comp_ctx));
+		else
+			pr_err("Failed to submit command [%ld]\n",
+			       PTR_ERR(comp_ctx));
+
 		return PTR_ERR(comp_ctx);
 	}
 
@@ -1918,7 +1924,7 @@ int ena_com_set_dev_mtu(struct ena_com_dev *ena_dev, int mtu)
 	int ret;
 
 	if (!ena_com_check_supported_feature_id(ena_dev, ENA_ADMIN_MTU)) {
-		pr_info("Feature %d isn't supported\n", ENA_ADMIN_MTU);
+		pr_debug("Feature %d isn't supported\n", ENA_ADMIN_MTU);
 		return -EPERM;
 	}
 
@@ -1971,8 +1977,8 @@ int ena_com_set_hash_function(struct ena_com_dev *ena_dev)
 
 	if (!ena_com_check_supported_feature_id(ena_dev,
 						ENA_ADMIN_RSS_HASH_FUNCTION)) {
-		pr_info("Feature %d isn't supported\n",
-			ENA_ADMIN_RSS_HASH_FUNCTION);
+		pr_debug("Feature %d isn't supported\n",
+			 ENA_ADMIN_RSS_HASH_FUNCTION);
 		return -EPERM;
 	}
 
@@ -2135,7 +2141,8 @@ int ena_com_set_hash_ctrl(struct ena_com_dev *ena_dev)
 
 	if (!ena_com_check_supported_feature_id(ena_dev,
 						ENA_ADMIN_RSS_HASH_INPUT)) {
-		pr_info("Feature %d isn't supported\n", ENA_ADMIN_RSS_HASH_INPUT);
+		pr_debug("Feature %d isn't supported\n",
+			 ENA_ADMIN_RSS_HASH_INPUT);
 		return -EPERM;
 	}
 
@@ -2293,8 +2300,8 @@ int ena_com_indirect_table_set(struct ena_com_dev *ena_dev)
 
 	if (!ena_com_check_supported_feature_id(
 		    ena_dev, ENA_ADMIN_RSS_REDIRECTION_TABLE_CONFIG)) {
-		pr_info("Feature %d isn't supported\n",
-			ENA_ADMIN_RSS_REDIRECTION_TABLE_CONFIG);
+		pr_debug("Feature %d isn't supported\n",
+			 ENA_ADMIN_RSS_REDIRECTION_TABLE_CONFIG);
 		return -EPERM;
 	}
 
@@ -2565,8 +2572,8 @@ int ena_com_init_interrupt_moderation(struct ena_com_dev *ena_dev)
 
 	if (rc) {
 		if (rc == -EPERM) {
-			pr_info("Feature %d isn't supported\n",
-				ENA_ADMIN_INTERRUPT_MODERATION);
+			pr_debug("Feature %d isn't supported\n",
+				 ENA_ADMIN_INTERRUPT_MODERATION);
 			rc = 0;
 		} else {
 			pr_err("Failed to get interrupt moderation admin cmd. rc: %d\n",
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index 5625007..7ae1fce 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -570,6 +570,7 @@ static void ena_free_all_rx_bufs(struct ena_adapter *adapter)
  */
 static void ena_free_tx_bufs(struct ena_ring *tx_ring)
 {
+	bool print_once = true;
 	u32 i;
 
 	for (i = 0; i < tx_ring->ring_size; i++) {
@@ -581,9 +582,16 @@ static void ena_free_tx_bufs(struct ena_ring *tx_ring)
 		if (!tx_info->skb)
 			continue;
 
-		netdev_notice(tx_ring->netdev,
-			      "free uncompleted tx skb qid %d idx 0x%x\n",
-			      tx_ring->qid, i);
+		if (print_once) {
+			netdev_notice(tx_ring->netdev,
+				      "free uncompleted tx skb qid %d idx 0x%x\n",
+				      tx_ring->qid, i);
+			print_once = false;
+		} else {
+			netdev_dbg(tx_ring->netdev,
+				   "free uncompleted tx skb qid %d idx 0x%x\n",
+				   tx_ring->qid, i);
+		}
 
 		ena_buf = tx_info->bufs;
 		dma_unmap_single(tx_ring->dev,
-- 
2.7.4

[PATCH V2 net 13/20] net/ena: change driver's default timeouts

[Netanel Belgazal]

Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.com>
---
 drivers/net/ethernet/amazon/ena/ena_com.c    | 4 ++--
 drivers/net/ethernet/amazon/ena/ena_netdev.h | 7 ++++---
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c b/drivers/net/ethernet/amazon/ena/ena_com.c
index 4147d6e..a550c8a 100644
--- a/drivers/net/ethernet/amazon/ena/ena_com.c
+++ b/drivers/net/ethernet/amazon/ena/ena_com.c
@@ -36,9 +36,9 @@
 /*****************************************************************************/
 
 /* Timeout in micro-sec */
-#define ADMIN_CMD_TIMEOUT_US (1000000)
+#define ADMIN_CMD_TIMEOUT_US (3000000)
 
-#define ENA_ASYNC_QUEUE_DEPTH 4
+#define ENA_ASYNC_QUEUE_DEPTH 16
 #define ENA_ADMIN_QUEUE_DEPTH 32
 
 #define MIN_ENA_VER (((ENA_COMMON_SPEC_VERSION_MAJOR) << \
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.h b/drivers/net/ethernet/amazon/ena/ena_netdev.h
index c081fd3..ed42e07 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.h
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.h
@@ -39,6 +39,7 @@
 #include <linux/interrupt.h>
 #include <linux/netdevice.h>
 #include <linux/skbuff.h>
+#include <linux/u64_stats_sync.h>
 
 #include "ena_com.h"
 #include "ena_eth_com.h"
@@ -100,7 +101,7 @@
 /* Number of queues to check for missing queues per timer service */
 #define ENA_MONITORED_TX_QUEUES	4
 /* Max timeout packets before device reset */
-#define MAX_NUM_OF_TIMEOUTED_PACKETS 32
+#define MAX_NUM_OF_TIMEOUTED_PACKETS 128
 
 #define ENA_TX_RING_IDX_NEXT(idx, ring_size) (((idx) + 1) & ((ring_size) - 1))
 
@@ -116,9 +117,9 @@
 #define ENA_IO_IRQ_IDX(q)		(ENA_IO_IRQ_FIRST_IDX + (q))
 
 /* ENA device should send keep alive msg every 1 sec.
- * We wait for 3 sec just to be on the safe side.
+ * We wait for 6 sec just to be on the safe side.
  */
-#define ENA_DEVICE_KALIVE_TIMEOUT	(3 * HZ)
+#define ENA_DEVICE_KALIVE_TIMEOUT	(6 * HZ)
 
 #define ENA_MMIO_DISABLE_REG_READ	BIT(0)
 
-- 
2.7.4

Re: [PATCH V2 net 13/20] net/ena: change driver's default timeouts

[Matt Wilson]

On Sun, Dec 04, 2016 at 03:19:31PM +0200, Netanel Belgazal wrote:

... because? (they turned out to be too aggressive, I believe.)

> Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.com>
> ---
>  drivers/net/ethernet/amazon/ena/ena_com.c    | 4 ++--
>  drivers/net/ethernet/amazon/ena/ena_netdev.h | 7 ++++---
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c b/drivers/net/ethernet/amazon/ena/ena_com.c
> index 4147d6e..a550c8a 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_com.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_com.c
> @@ -36,9 +36,9 @@
>  /*****************************************************************************/
>  
>  /* Timeout in micro-sec */
> -#define ADMIN_CMD_TIMEOUT_US (1000000)
> +#define ADMIN_CMD_TIMEOUT_US (3000000)
>  
> -#define ENA_ASYNC_QUEUE_DEPTH 4
> +#define ENA_ASYNC_QUEUE_DEPTH 16

Why is this changed at the same time?

>  #define ENA_ADMIN_QUEUE_DEPTH 32
>  
>  #define MIN_ENA_VER (((ENA_COMMON_SPEC_VERSION_MAJOR) << \
> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.h b/drivers/net/ethernet/amazon/ena/ena_netdev.h
> index c081fd3..ed42e07 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.h
> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.h
> @@ -39,6 +39,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/netdevice.h>
>  #include <linux/skbuff.h>
> +#include <linux/u64_stats_sync.h>

This change seems unrelated.

>  #include "ena_com.h"
>  #include "ena_eth_com.h"
> @@ -100,7 +101,7 @@
>  /* Number of queues to check for missing queues per timer service */
>  #define ENA_MONITORED_TX_QUEUES	4
>  /* Max timeout packets before device reset */
> -#define MAX_NUM_OF_TIMEOUTED_PACKETS 32
> +#define MAX_NUM_OF_TIMEOUTED_PACKETS 128
>  
>  #define ENA_TX_RING_IDX_NEXT(idx, ring_size) (((idx) + 1) & ((ring_size) - 1))
>  
> @@ -116,9 +117,9 @@
>  #define ENA_IO_IRQ_IDX(q)		(ENA_IO_IRQ_FIRST_IDX + (q))
>  
>  /* ENA device should send keep alive msg every 1 sec.
> - * We wait for 3 sec just to be on the safe side.
> + * We wait for 6 sec just to be on the safe side.
>   */
> -#define ENA_DEVICE_KALIVE_TIMEOUT	(3 * HZ)
> +#define ENA_DEVICE_KALIVE_TIMEOUT	(6 * HZ)
>  
>  #define ENA_MMIO_DISABLE_REG_READ	BIT(0)
>  

Re: [PATCH V2 net 13/20] net/ena: change driver's default timeouts

[Netanel Belgazal]

On 12/05/2016 06:35 AM, Matt Wilson wrote:
> On Sun, Dec 04, 2016 at 03:19:31PM +0200, Netanel Belgazal wrote:
>
> ... because? (they turned out to be too aggressive, I believe.)

Yes, The timeout were too aggressive on some specific machines.

>> Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.com>
>> ---
>>   drivers/net/ethernet/amazon/ena/ena_com.c    | 4 ++--
>>   drivers/net/ethernet/amazon/ena/ena_netdev.h | 7 ++++---
>>   2 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c b/drivers/net/ethernet/amazon/ena/ena_com.c
>> index 4147d6e..a550c8a 100644
>> --- a/drivers/net/ethernet/amazon/ena/ena_com.c
>> +++ b/drivers/net/ethernet/amazon/ena/ena_com.c
>> @@ -36,9 +36,9 @@
>>   /*****************************************************************************/
>>   
>>   /* Timeout in micro-sec */
>> -#define ADMIN_CMD_TIMEOUT_US (1000000)
>> +#define ADMIN_CMD_TIMEOUT_US (3000000)
>>   
>> -#define ENA_ASYNC_QUEUE_DEPTH 4
>> +#define ENA_ASYNC_QUEUE_DEPTH 16
> Why is this changed at the same time?

It related to the too aggressive thresholds.
On some heavy loaded system we reached to a state where the AENQ
was full so the driver missed some events.


>
>>   #define ENA_ADMIN_QUEUE_DEPTH 32
>>   
>>   #define MIN_ENA_VER (((ENA_COMMON_SPEC_VERSION_MAJOR) << \
>> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.h b/drivers/net/ethernet/amazon/ena/ena_netdev.h
>> index c081fd3..ed42e07 100644
>> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.h
>> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.h
>> @@ -39,6 +39,7 @@
>>   #include <linux/interrupt.h>
>>   #include <linux/netdevice.h>
>>   #include <linux/skbuff.h>
>> +#include <linux/u64_stats_sync.h>
> This change seems unrelated.

Removed to a different patch (will be submitted in a new patchset)
>
>>   #include "ena_com.h"
>>   #include "ena_eth_com.h"
>> @@ -100,7 +101,7 @@
>>   /* Number of queues to check for missing queues per timer service */
>>   #define ENA_MONITORED_TX_QUEUES	4
>>   /* Max timeout packets before device reset */
>> -#define MAX_NUM_OF_TIMEOUTED_PACKETS 32
>> +#define MAX_NUM_OF_TIMEOUTED_PACKETS 128
>>   
>>   #define ENA_TX_RING_IDX_NEXT(idx, ring_size) (((idx) + 1) & ((ring_size) - 1))
>>   
>> @@ -116,9 +117,9 @@
>>   #define ENA_IO_IRQ_IDX(q)		(ENA_IO_IRQ_FIRST_IDX + (q))
>>   
>>   /* ENA device should send keep alive msg every 1 sec.
>> - * We wait for 3 sec just to be on the safe side.
>> + * We wait for 6 sec just to be on the safe side.
>>    */
>> -#define ENA_DEVICE_KALIVE_TIMEOUT	(3 * HZ)
>> +#define ENA_DEVICE_KALIVE_TIMEOUT	(6 * HZ)
>>   
>>   #define ENA_MMIO_DISABLE_REG_READ	BIT(0)
>>   

[PATCH V2 net 14/20] net/ena: change condition for host attribute configuration

[Netanel Belgazal]

Move the host info config to be the first admin command that is executed.
This change require the driver to remove the 'feature check'
from host info configuration flow.
The check is removed since the supported features bitmask field
is retrieved only after calling ENA_ADMIN_DEVICE_ATTRIBUTES admin command.

If set host info is not supported an error will be returned by the device.

Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.com>
---
 drivers/net/ethernet/amazon/ena/ena_com.c    | 8 +++-----
 drivers/net/ethernet/amazon/ena/ena_netdev.c | 5 +++--
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c b/drivers/net/ethernet/amazon/ena/ena_com.c
index a550c8a..9deb0dd 100644
--- a/drivers/net/ethernet/amazon/ena/ena_com.c
+++ b/drivers/net/ethernet/amazon/ena/ena_com.c
@@ -2474,11 +2474,9 @@ int ena_com_set_host_attributes(struct ena_com_dev *ena_dev)
 
 	int ret;
 
-	if (!ena_com_check_supported_feature_id(ena_dev,
-						ENA_ADMIN_HOST_ATTR_CONFIG)) {
-		pr_warn("Set host attribute isn't supported\n");
-		return -EPERM;
-	}
+	/* Host attribute config is called before ena_com_get_dev_attr_feat
+	 * so ena_com can't check if the feature is supported.
+	 */
 
 	memset(&cmd, 0x0, sizeof(cmd));
 	admin_queue = &ena_dev->admin_queue;
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index 7ae1fce..8c1e14b 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -2426,6 +2426,8 @@ static int ena_device_init(struct ena_com_dev *ena_dev, struct pci_dev *pdev,
 	 */
 	ena_com_set_admin_polling_mode(ena_dev, true);
 
+	ena_config_host_info(ena_dev);
+
 	/* Get Device Attributes*/
 	rc = ena_com_get_dev_attr_feat(ena_dev, get_feat_ctx);
 	if (rc) {
@@ -2450,11 +2452,10 @@ static int ena_device_init(struct ena_com_dev *ena_dev, struct pci_dev *pdev,
 
 	*wd_state = !!(aenq_groups & BIT(ENA_ADMIN_KEEP_ALIVE));
 
-	ena_config_host_info(ena_dev);
-
 	return 0;
 
 err_admin_init:
+	ena_com_delete_host_info(ena_dev);
 	ena_com_admin_destroy(ena_dev);
 err_mmio_read_less:
 	ena_com_mmio_reg_read_request_destroy(ena_dev);
-- 
2.7.4

[PATCH V2 net 15/20] net/ena: change sizeof() argument to be the type pointer

[Netanel Belgazal]

Instead of using:
memset(ptr, 0x0, sizeof(struct ...))
use:
memset(ptr, 0x0, sizeor(*ptr))

Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.com>
---
 drivers/net/ethernet/amazon/ena/ena_com.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c b/drivers/net/ethernet/amazon/ena/ena_com.c
index 9deb0dd..f2e167b 100644
--- a/drivers/net/ethernet/amazon/ena/ena_com.c
+++ b/drivers/net/ethernet/amazon/ena/ena_com.c
@@ -329,7 +329,7 @@ static int ena_com_init_io_sq(struct ena_com_dev *ena_dev,
 	size_t size;
 	int dev_node = 0;
 
-	memset(&io_sq->desc_addr, 0x0, sizeof(struct ena_com_io_desc_addr));
+	memset(&io_sq->desc_addr, 0x0, sizeof(io_sq->desc_addr));
 
 	io_sq->desc_entry_size =
 		(io_sq->direction == ENA_COM_IO_QUEUE_DIRECTION_TX) ?
@@ -383,7 +383,7 @@ static int ena_com_init_io_cq(struct ena_com_dev *ena_dev,
 	size_t size;
 	int prev_node = 0;
 
-	memset(&io_cq->cdesc_addr, 0x0, sizeof(struct ena_com_io_desc_addr));
+	memset(&io_cq->cdesc_addr, 0x0, sizeof(io_cq->cdesc_addr));
 
 	/* Use the basic completion descriptor for Rx */
 	io_cq->cdesc_entry_size_in_bytes =
@@ -681,7 +681,7 @@ static int ena_com_destroy_io_sq(struct ena_com_dev *ena_dev,
 	u8 direction;
 	int ret;
 
-	memset(&destroy_cmd, 0x0, sizeof(struct ena_admin_aq_destroy_sq_cmd));
+	memset(&destroy_cmd, 0x0, sizeof(destroy_cmd));
 
 	if (io_sq->direction == ENA_COM_IO_QUEUE_DIRECTION_TX)
 		direction = ENA_ADMIN_SQ_DIRECTION_TX;
@@ -963,7 +963,7 @@ static int ena_com_create_io_sq(struct ena_com_dev *ena_dev,
 	u8 direction;
 	int ret;
 
-	memset(&create_cmd, 0x0, sizeof(struct ena_admin_aq_create_sq_cmd));
+	memset(&create_cmd, 0x0, sizeof(create_cmd));
 
 	create_cmd.aq_common_descriptor.opcode = ENA_ADMIN_CREATE_SQ;
 
@@ -1155,7 +1155,7 @@ int ena_com_create_io_cq(struct ena_com_dev *ena_dev,
 	struct ena_admin_acq_create_cq_resp_desc cmd_completion;
 	int ret;
 
-	memset(&create_cmd, 0x0, sizeof(struct ena_admin_aq_create_cq_cmd));
+	memset(&create_cmd, 0x0, sizeof(create_cmd));
 
 	create_cmd.aq_common_descriptor.opcode = ENA_ADMIN_CREATE_CQ;
 
@@ -1263,7 +1263,7 @@ int ena_com_destroy_io_cq(struct ena_com_dev *ena_dev,
 	struct ena_admin_acq_destroy_cq_resp_desc destroy_resp;
 	int ret;
 
-	memset(&destroy_cmd, 0x0, sizeof(struct ena_admin_aq_destroy_sq_cmd));
+	memset(&destroy_cmd, 0x0, sizeof(destroy_cmd));
 
 	destroy_cmd.cq_idx = io_cq->idx;
 	destroy_cmd.aq_common_descriptor.opcode = ENA_ADMIN_DESTROY_CQ;
@@ -1613,8 +1613,8 @@ int ena_com_create_io_queue(struct ena_com_dev *ena_dev,
 	io_sq = &ena_dev->io_sq_queues[ctx->qid];
 	io_cq = &ena_dev->io_cq_queues[ctx->qid];
 
-	memset(io_sq, 0x0, sizeof(struct ena_com_io_sq));
-	memset(io_cq, 0x0, sizeof(struct ena_com_io_cq));
+	memset(io_sq, 0x0, sizeof(*io_sq));
+	memset(io_cq, 0x0, sizeof(*io_cq));
 
 	/* Init CQ */
 	io_cq->q_depth = ctx->queue_size;
-- 
2.7.4

[PATCH V2 net 16/20] net/ena: use napi_schedule_irqoff when possible

[Netanel Belgazal]

Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.com>
---
 drivers/net/ethernet/amazon/ena/ena_netdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index 8c1e14b..6c49529 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -1210,7 +1210,7 @@ static irqreturn_t ena_intr_msix_io(int irq, void *data)
 	struct ena_napi *ena_napi = data;
 
 	atomic_set(&ena_napi->unmask_interrupt, 1);
-	napi_schedule(&ena_napi->napi);
+	napi_schedule_irqoff(&ena_napi->napi);
 
 	return IRQ_HANDLED;
 }
-- 
2.7.4

[PATCH V2 net 17/20] net/ena: add IPv6 extended protocols to ena_admin_flow_hash_proto

[Netanel Belgazal]

We intend to use those fields in the future.

Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.com>
---
 drivers/net/ethernet/amazon/ena/ena_admin_defs.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
index 35ae511..92bba08 100644
--- a/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
+++ b/drivers/net/ethernet/amazon/ena/ena_admin_defs.h
@@ -627,6 +627,12 @@ enum ena_admin_flow_hash_proto {
 
 	ENA_ADMIN_RSS_NOT_IP	= 7,
 
+	/* TCPv6 with extension header */
+	ENA_ADMIN_RSS_TCP6_EX	= 8,
+
+	/* IPv6 with extension header */
+	ENA_ADMIN_RSS_IP6_EX	= 9,
+
 	ENA_ADMIN_RSS_PROTO_NUM	= 16,
 };
 
-- 
2.7.4

[PATCH V2 net 18/20] net/ena: remove affinity hint from the driver

[Netanel Belgazal]

To allow irqbalance to better distribute the napi handler,
remove the smp affinity hint from the driver.

Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.com>
---
 drivers/net/ethernet/amazon/ena/ena_netdev.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index 6c49529..ee80472 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -1323,8 +1323,6 @@ static int ena_request_mgmnt_irq(struct ena_adapter *adapter)
 		  "set affinity hint of mgmnt irq.to 0x%lx (irq vector: %d)\n",
 		  irq->affinity_hint_mask.bits[0], irq->vector);
 
-	irq_set_affinity_hint(irq->vector, &irq->affinity_hint_mask);
-
 	return rc;
 }
 
@@ -1354,8 +1352,6 @@ static int ena_request_io_irq(struct ena_adapter *adapter)
 		netif_dbg(adapter, ifup, adapter->netdev,
 			  "set affinity hint of irq. index %d to 0x%lx (irq vector: %d)\n",
 			  i, irq->affinity_hint_mask.bits[0], irq->vector);
-
-		irq_set_affinity_hint(irq->vector, &irq->affinity_hint_mask);
 	}
 
 	return rc;
-- 
2.7.4

[PATCH V2 net 19/20] net/ena: restructure skb allocation

[Netanel Belgazal]

To increase readability, refactor skb allocation to dedicated function
This change does not impact the performance since the compiler optimize
the code and elimitate the if condition.

Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.com>
---
 drivers/net/ethernet/amazon/ena/ena_netdev.c | 46 ++++++++++++++++------------
 1 file changed, 27 insertions(+), 19 deletions(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index ee80472..8d42960 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -787,6 +787,28 @@ static int ena_clean_tx_irq(struct ena_ring *tx_ring, u32 budget)
 	return tx_pkts;
 }
 
+static struct sk_buff *ena_alloc_skb(struct ena_ring *rx_ring, bool frags)
+{
+	struct sk_buff *skb;
+
+	if (frags)
+		skb = napi_get_frags(rx_ring->napi);
+	else
+		skb = netdev_alloc_skb_ip_align(rx_ring->netdev,
+						rx_ring->rx_copybreak);
+
+	if (unlikely(!skb)) {
+		u64_stats_update_begin(&rx_ring->syncp);
+		rx_ring->rx_stats.skb_alloc_fail++;
+		u64_stats_update_end(&rx_ring->syncp);
+		netif_err(rx_ring->adapter, rx_err, rx_ring->netdev,
+			  "Failed to allocate skb. frags: %d\n", frags);
+		return NULL;
+	}
+
+	return skb;
+}
+
 static struct sk_buff *ena_rx_skb(struct ena_ring *rx_ring,
 				  struct ena_com_rx_buf_info *ena_bufs,
 				  u32 descs,
@@ -795,8 +817,7 @@ static struct sk_buff *ena_rx_skb(struct ena_ring *rx_ring,
 	struct sk_buff *skb;
 	struct ena_rx_buffer *rx_info =
 		&rx_ring->rx_buffer_info[*next_to_clean];
-	u32 len;
-	u32 buf = 0;
+	u32 len, buf = 0;
 	void *va;
 
 	len = ena_bufs[0].len;
@@ -815,16 +836,9 @@ static struct sk_buff *ena_rx_skb(struct ena_ring *rx_ring,
 	prefetch(va + NET_IP_ALIGN);
 
 	if (len <= rx_ring->rx_copybreak) {
-		skb = netdev_alloc_skb_ip_align(rx_ring->netdev,
-						rx_ring->rx_copybreak);
-		if (unlikely(!skb)) {
-			u64_stats_update_begin(&rx_ring->syncp);
-			rx_ring->rx_stats.skb_alloc_fail++;
-			u64_stats_update_end(&rx_ring->syncp);
-			netif_err(rx_ring->adapter, rx_err, rx_ring->netdev,
-				  "Failed to allocate skb\n");
+		skb = ena_alloc_skb(rx_ring, false);
+		if (unlikely(!skb))
 			return NULL;
-		}
 
 		netif_dbg(rx_ring->adapter, rx_status, rx_ring->netdev,
 			  "rx allocated small packet. len %d. data_len %d\n",
@@ -848,15 +862,9 @@ static struct sk_buff *ena_rx_skb(struct ena_ring *rx_ring,
 		return skb;
 	}
 
-	skb = napi_get_frags(rx_ring->napi);
-	if (unlikely(!skb)) {
-		netif_dbg(rx_ring->adapter, rx_status, rx_ring->netdev,
-			  "Failed allocating skb\n");
-		u64_stats_update_begin(&rx_ring->syncp);
-		rx_ring->rx_stats.skb_alloc_fail++;
-		u64_stats_update_end(&rx_ring->syncp);
+	skb = ena_alloc_skb(rx_ring, true);
+	if (unlikely(!skb))
 		return NULL;
-	}
 
 	do {
 		dma_unmap_page(rx_ring->dev,
-- 
2.7.4

[PATCH V2 net 20/20] net/ena: increase driver version to 1.1.2

[Netanel Belgazal]

Signed-off-by: Netanel Belgazal <netanel@annapurnalabs.com>
---
 drivers/net/ethernet/amazon/ena/ena_netdev.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.h b/drivers/net/ethernet/amazon/ena/ena_netdev.h
index ed42e07..de1e5ac 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.h
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.h
@@ -45,7 +45,7 @@
 #include "ena_eth_com.h"
 
 #define DRV_MODULE_VER_MAJOR	1
-#define DRV_MODULE_VER_MINOR	0
+#define DRV_MODULE_VER_MINOR	1
 #define DRV_MODULE_VER_SUBMINOR 2
 
 #define DRV_MODULE_NAME		"ena"
-- 
2.7.4

Re: [PATCH V2 net 00/20] Increase ENA driver version to 1.1.2

[David Miller]

It is not appropriate to submit so many patches at one time.

Please keep your patch series to no more than about a dozen
at a time.

Also, group your changes logically and tie an appropriately
descriptive cover letter.

"Increase driver version to X.Y.Z" tells the reader absolutely
nothing.  Someone reading that Subject line in the GIT logs
will have no idea what the overall purpose of the patch series
is and what it accomplishes.

You really need to describe the high level purpose of the patch set.
Is it adding a new feature?  What is that feature?  Why are you
adding that feature?  How is that feature implemented?  Why is
it implemented that way?

Re: [PATCH V2 net 00/20] Increase ENA driver version to 1.1.2

[Matt Wilson]

On Sun, Dec 04, 2016 at 09:37:43PM -0500, David Miller wrote:
> 
> It is not appropriate to submit so many patches at one time.

Indeed, https://www.kernel.org/doc/Documentation/SubmittingPatches
recommends submitting no more than 15 or so at once.

> Please keep your patch series to no more than about a dozen
> at a time.

How about 15 from SubmittingPatches? The first 15 in the series are
all important bugfixes. Should Netanel resubmit a series with just the
bugfixes and a new cover letter? Or are you willing to consider the
first 15 of this series as posted?

> Also, group your changes logically and tie an appropriately
> descriptive cover letter.
> 
> "Increase driver version to X.Y.Z" tells the reader absolutely
> nothing.  Someone reading that Subject line in the GIT logs
> will have no idea what the overall purpose of the patch series
> is and what it accomplishes.

You're right, the cover letter subject needs to be better. There is
only one commit submitted with the subject "increase driver version to
1.1.2." - Patch 20/20. It is logically like:

commit b8b2372de9cc00d5ed667c7b8db29b6cfbf037f5
Author: Manish Chopra <manish.chopra@qlogic.com>
Date:   Wed Aug 3 04:02:04 2016 -0400

    qlcnic: Update version to 5.3.65
    
    Signed-off-by: Manish Chopra <manish.chopra@qlogic.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

[...]

commit ae33256c55d2fefcad8712e750b846461994a1af
Author: Bimmy Pujari <bimmy.pujari@intel.com>
Date:   Mon Jun 20 09:10:39 2016 -0700

    i40e/i40evf-bump version to 1.6.11
    
    Signed-off-by: Bimmy Pujari <bimmy.pujari@intel.com>
    Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
    Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

[...]

commit 5264cc63ba10ebfa0e54e3e641cce2656c7a60e8
Author: Jacob Keller <jacob.e.keller@intel.com>
Date:   Tue Jun 7 16:09:02 2016 -0700

    fm10k: bump version number
    
    Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
    Tested-by: Krishneil Singh <Krishneil.k.singh@intel.com>
    Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

[...]

commit a58a3e68037647de78e3461194239a1104f76003
Author: Michael Chan <michael.chan@broadcom.com>
Date:   Fri Jul 1 18:46:20 2016 -0400

    bnxt_en: Update firmware spec. to 1.3.0.
    
    And update driver version to 1.3.0.
    
    Signed-off-by: Michael Chan <michael.chan@broadcom.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

> You really need to describe the high level purpose of the patch set.
> Is it adding a new feature?  What is that feature?  Why are you
> adding that feature?  How is that feature implemented?  Why is
> it implemented that way?

The priority is to get bug fixes to the ENA driver in 4.9. Let's focus
on the first 15.

--msw