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
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 <redacted>
---
drivers/net/ethernet/amazon/ena/ena_netdev.c | 1 +
1 file changed, 1 insertion(+)
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 <redacted>
---
drivers/net/ethernet/amazon/ena/ena_admin_defs.h | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
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 <redacted>
---
drivers/net/ethernet/amazon/ena/ena_netdev.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
@@ -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);
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 <redacted>
---
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(-)
@@ -749,6 +751,31 @@ struct ena_admin_feature_rss_ind_table {structena_admin_rss_ind_table_entryinline_entry;};+/* When hint value is 0, driver should use it's own predefined value */+structena_admin_ena_hw_hints{+/* value in ms */+u16mmio_read_timeout;++/* value in ms */+u16driver_watchdog_timeout;++/* Per packet tx completion timeout. value in ms */+u16missing_tx_completion_timeout;++u16missed_tx_completion_count_threshold_to_reset;++/* value in ms */+u16admin_completion_tx_timeout;++u16netdev_wd_timeout;++u16max_tx_sgl_size;++u16max_rx_sgl_size;++u16reserved[8];+};+structena_admin_get_feat_cmd{structena_admin_aq_common_descaq_common_descriptor;
@@ -508,15 +508,13 @@ static int ena_com_comp_status_to_errno(u8 comp_status)staticintena_com_wait_and_process_admin_cq_polling(structena_comp_ctx*comp_ctx,structena_com_admin_queue*admin_queue){-unsignedlongflags;-u32start_time;+unsignedlongflags,timeout;intret;-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 *comintret;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.*Theremightbe2kindsoferrors
@@ -600,12 +599,14 @@ static u32 ena_com_reg_bar_read32(struct ena_com_dev *ena_dev, u16 offset)structena_com_mmio_read*mmio_read=&ena_dev->mmio_read;volatilestructena_admin_ena_mmio_req_read_less_resp*read_resp=mmio_read->read_resp;-u32mmio_read_reg,ret;+u32mmio_read_reg,timeout,ret;unsignedlongflags;inti;might_sleep();+timeout=mmio_read->reg_read_to?:ENA_REG_READ_TIMEOUT;+/* If readless is disabled, perform regular read */if(!mmio_read->readless_supported)returnreadl(ena_dev->reg_bar+offset);
@@ -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+*commandisn'tsupportedsetdriverhintsto0+*/+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));+elseif(rc==-EPERM)+memset(&get_feat_ctx->hw_hints,0x0,+sizeof(get_feat_ctx->hw_hints));+else+returnrc;+return0;}
@@ -1842,6 +1857,14 @@ int ena_com_dev_reset(struct ena_com_dev *ena_dev)returnrc;}+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;+return0;}
@@ -232,6 +234,7 @@ struct ena_com_admin_queue {void*q_dmadev;spinlock_tq_lock;/* spinlock for the admin queue */structena_comp_ctx*comp_ctx;+u32completion_timeout;u16q_depth;structena_com_admin_cqcq;structena_com_admin_sqsq;
@@ -266,6 +269,7 @@ struct ena_com_aenq {structena_com_mmio_read{structena_admin_ena_mmio_req_read_less_resp*read_resp;dma_addr_tread_resp_dma_addr;+u32reg_read_to;/* in us */u16seq_num;boolreadless_supported;/* spin lock to ensure a single outstanding read */
@@ -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-*becountedtwice.-*/-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;}}}
@@ -146,7 +146,18 @@ struct ena_tx_buffer {u32tx_descs;/* num of buffers used by this skb */u32num_of_bufs;-/* Save the last jiffies to detect missing tx packets */++/* Used for detect missing tx packets to limit the number of prints */+u32print_once;+/* Save the last jiffies to detect missing tx packets+*+*setstononzerovalueonena_start_xmitandsettozeroon+*napiandtimer_Service_routine.+*+*whilethisvalueisnotprotectedbylock,+*agivenpacketisnotexpectedtobehandledbyena_start_xmit+*andbynapi/timer_serviceatthesametime.+*/unsignedlonglast_jiffies;structena_com_bufbufs[ENA_PKT_MAX_BUFS];}____cacheline_aligned;
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 <redacted>
---
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(-)
@@ -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+*theinterruptandreturn0soNAPIwon'treschedule+*/+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;++}elseif((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-*interruptunmask+/* Update numa and unmask the interrupt only when schedule+*fromtheinterruptcontext(vsfromsk_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,+*txintrdelayandinterruptunmask+*/+ena_com_update_intr_reg(&intr_reg,+rx_ring->smoothed_interval,+tx_ring->smoothed_interval,+true);++/* It is a shared MSI-X.+*TxandRxCQhavepointertoit.+*Soweuseoneofthemtoreachtheintrreg+*/+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.-*Soweuseoneofthemtoreachtheintrreg-*/-ena_com_unmask_intr(rx_ring->ena_com_io_cq,&intr_reg);ena_update_ring_numa_node(tx_ring,rx_ring);
@@ -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));+returnPTR_ERR(comp_ctx);}
@@ -1918,7 +1924,7 @@ int ena_com_set_dev_mtu(struct ena_com_dev *ena_dev, int mtu)intret;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;}
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 <redacted>
---
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(-)
@@ -2474,11 +2474,9 @@ int ena_com_set_host_attributes(struct ena_com_dev *ena_dev)intret;-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+*soena_comcan'tcheckifthefeatureissupported.+*/memset(&cmd,0x0,sizeof(cmd));admin_queue=&ena_dev->admin_queue;
@@ -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.-*Wewaitfor3secjusttobeonthesafeside.+*Wewaitfor6secjusttobeonthesafeside.*/-#define ENA_DEVICE_KALIVE_TIMEOUT (3 * HZ)+#define ENA_DEVICE_KALIVE_TIMEOUT (6 * HZ)#define ENA_MMIO_DISABLE_REG_READ BIT(0)
To allow irqbalance to better distribute the napi handler,
remove the smp affinity hint from the driver.
Signed-off-by: Netanel Belgazal <redacted>
---
drivers/net/ethernet/amazon/ena/ena_netdev.c | 4 ----
1 file changed, 4 deletions(-)
@@ -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);-returnrc;}
@@ -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);}returnrc;
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 <redacted>
---
drivers/net/ethernet/amazon/ena/ena_netdev.c | 46 ++++++++++++++++------------
1 file changed, 27 insertions(+), 19 deletions(-)
We intend to use those fields in the future.
Signed-off-by: Netanel Belgazal <redacted>
---
drivers/net/ethernet/amazon/ena/ena_admin_defs.h | 6 ++++++
1 file changed, 6 insertions(+)
@@ -329,7 +329,7 @@ static int ena_com_init_io_sq(struct ena_com_dev *ena_dev,size_tsize;intdev_node=0;-memset(&io_sq->desc_addr,0x0,sizeof(structena_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_tsize;intprev_node=0;-memset(&io_cq->cdesc_addr,0x0,sizeof(structena_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,u8direction;intret;-memset(&destroy_cmd,0x0,sizeof(structena_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,u8direction;intret;-memset(&create_cmd,0x0,sizeof(structena_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,structena_admin_acq_create_cq_resp_desccmd_completion;intret;-memset(&create_cmd,0x0,sizeof(structena_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,structena_admin_acq_destroy_cq_resp_descdestroy_resp;intret;-memset(&destroy_cmd,0x0,sizeof(structena_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;
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 <redacted>
---
drivers/net/ethernet/amazon/ena/ena_netdev.c | 56 +++++++++++++++++++++-------
1 file changed, 43 insertions(+), 13 deletions(-)
@@ -80,14 +80,18 @@ static void ena_tx_timeout(struct net_device *dev){structena_adapter*adapter=netdev_priv(dev);+/* Change the state of the device to trigger reset+*Checkthatwearenotinthemiddleoratriggeralready+*/++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);}staticvoidupdate_rx_ring_mtu(structena_adapter*adapter,intmtu)
@@ -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);return0;}
@@ -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)){+intrc;++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)structena_adapter*adapter=netdev_priv(netdev);inti;+/* Dont schedule NAPI if the driver is in the middle of reset+*ornetdevisdown.+*/++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)booldev_up,wd_state;intrc;+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();
@@ -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);
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 <redacted>
---
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(-)
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 <redacted>
---
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(-)
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 <redacted>
---
drivers/net/ethernet/amazon/ena/ena_netdev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Remove NETIF_F_NTUPLE from netdev->features.
The ENA device driver does not support ntuple filtering.
Signed-off-by: Netanel Belgazal <redacted>
---
drivers/net/ethernet/amazon/ena/ena_netdev.c | 1 -
1 file changed, 1 deletion(-)
From: David Miller <davem@davemloft.net> Date: 2016-12-05 02:39:35
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?
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 [off-list ref]
Date: Wed Aug 3 04:02:04 2016 -0400
qlcnic: Update version to 5.3.65
Signed-off-by: Manish Chopra [off-list ref]
Signed-off-by: David S. Miller [off-list ref]
[...]
commit ae33256c55d2fefcad8712e750b846461994a1af
Author: Bimmy Pujari [off-list ref]
Date: Mon Jun 20 09:10:39 2016 -0700
i40e/i40evf-bump version to 1.6.11
Signed-off-by: Bimmy Pujari [off-list ref]
Tested-by: Andrew Bowers [off-list ref]
Signed-off-by: Jeff Kirsher [off-list ref]
[...]
commit 5264cc63ba10ebfa0e54e3e641cce2656c7a60e8
Author: Jacob Keller [off-list ref]
Date: Tue Jun 7 16:09:02 2016 -0700
fm10k: bump version number
Signed-off-by: Jacob Keller [off-list ref]
Tested-by: Krishneil Singh [off-list ref]
Signed-off-by: Jeff Kirsher [off-list ref]
[...]
commit a58a3e68037647de78e3461194239a1104f76003
Author: Michael Chan [off-list ref]
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 [off-list ref]
Signed-off-by: David S. Miller [off-list ref]
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
From: Matt Wilson <hidden> Date: 2016-12-05 04:10:41
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.
From: Matt Wilson <hidden> Date: 2016-12-05 04:12:31
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().
From: Matt Wilson <hidden> Date: 2016-12-05 04:18:57
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
From: Matt Wilson <hidden> Date: 2016-12-05 04:25:12
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
From: Matt Wilson <hidden> Date: 2016-12-05 04:29:55
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
@@ -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);
From: Matt Wilson <hidden> Date: 2016-12-05 04:31:52
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
quoted hunk
In case the device does not support this capability, the driver
will use its own defines.
Signed-off-by: Netanel Belgazal <redacted>
---
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(-)
@@ -749,6 +751,31 @@ struct ena_admin_feature_rss_ind_table {structena_admin_rss_ind_table_entryinline_entry;};+/* When hint value is 0, driver should use it's own predefined value */+structena_admin_ena_hw_hints{+/* value in ms */+u16mmio_read_timeout;++/* value in ms */+u16driver_watchdog_timeout;++/* Per packet tx completion timeout. value in ms */+u16missing_tx_completion_timeout;++u16missed_tx_completion_count_threshold_to_reset;++/* value in ms */+u16admin_completion_tx_timeout;++u16netdev_wd_timeout;++u16max_tx_sgl_size;++u16max_rx_sgl_size;++u16reserved[8];+};+structena_admin_get_feat_cmd{structena_admin_aq_common_descaq_common_descriptor;
@@ -508,15 +508,13 @@ static int ena_com_comp_status_to_errno(u8 comp_status)staticintena_com_wait_and_process_admin_cq_polling(structena_comp_ctx*comp_ctx,structena_com_admin_queue*admin_queue){-unsignedlongflags;-u32start_time;+unsignedlongflags,timeout;intret;-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 *comintret;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.*Theremightbe2kindsoferrors
@@ -600,12 +599,14 @@ static u32 ena_com_reg_bar_read32(struct ena_com_dev *ena_dev, u16 offset)structena_com_mmio_read*mmio_read=&ena_dev->mmio_read;volatilestructena_admin_ena_mmio_req_read_less_resp*read_resp=mmio_read->read_resp;-u32mmio_read_reg,ret;+u32mmio_read_reg,timeout,ret;unsignedlongflags;inti;might_sleep();+timeout=mmio_read->reg_read_to?:ENA_REG_READ_TIMEOUT;+/* If readless is disabled, perform regular read */if(!mmio_read->readless_supported)returnreadl(ena_dev->reg_bar+offset);
@@ -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+*commandisn'tsupportedsetdriverhintsto0+*/+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));+elseif(rc==-EPERM)+memset(&get_feat_ctx->hw_hints,0x0,+sizeof(get_feat_ctx->hw_hints));+else+returnrc;+return0;}
@@ -1842,6 +1857,14 @@ int ena_com_dev_reset(struct ena_com_dev *ena_dev)returnrc;}+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;+return0;}
@@ -232,6 +234,7 @@ struct ena_com_admin_queue {void*q_dmadev;spinlock_tq_lock;/* spinlock for the admin queue */structena_comp_ctx*comp_ctx;+u32completion_timeout;u16q_depth;structena_com_admin_cqcq;structena_com_admin_sqsq;
@@ -266,6 +269,7 @@ struct ena_com_aenq {structena_com_mmio_read{structena_admin_ena_mmio_req_read_less_resp*read_resp;dma_addr_tread_resp_dma_addr;+u32reg_read_to;/* in us */u16seq_num;boolreadless_supported;/* spin lock to ensure a single outstanding read */
@@ -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-*becountedtwice.-*/-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;}}}
@@ -146,7 +146,18 @@ struct ena_tx_buffer {u32tx_descs;/* num of buffers used by this skb */u32num_of_bufs;-/* Save the last jiffies to detect missing tx packets */++/* Used for detect missing tx packets to limit the number of prints */+u32print_once;+/* Save the last jiffies to detect missing tx packets+*+*setstononzerovalueonena_start_xmitandsettozeroon+*napiandtimer_Service_routine.+*+*whilethisvalueisnotprotectedbylock,+*agivenpacketisnotexpectedtobehandledbyena_start_xmit+*andbynapi/timer_serviceatthesametime.+*/unsignedlonglast_jiffies;structena_com_bufbufs[ENA_PKT_MAX_BUFS];}____cacheline_aligned;
@@ -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)
From: Eric Dumazet <hidden> Date: 2016-12-05 05:58:43
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 <redacted>
---
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 :
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.
On Sun, Dec 04, 2016 at 03:19:20PM +0200, Netanel Belgazal wrote:
quoted
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.
On Sun, Dec 04, 2016 at 03:19:21PM +0200, Netanel Belgazal wrote:
quoted
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().
On Sun, Dec 04, 2016 at 03:19:22PM +0200, Netanel Belgazal wrote:
quoted
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.
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.
Removed to a different patch (will be submitted in a new patchset)
quoted
#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)
On Sun, 2016-12-04 at 15:19 +0200, Netanel Belgazal wrote:
quoted
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 <redacted>
---
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.
quoted hunk
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 :
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.
On Sun, Dec 04, 2016 at 03:19:25PM +0200, Netanel Belgazal wrote:
quoted
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.
quoted
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.
On Sun, Dec 04, 2016 at 03:19:24PM +0200, Netanel Belgazal wrote:
quoted
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
quoted
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
quoted
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.
@@ -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);
On Sun, Dec 04, 2016 at 03:19:26PM +0200, Netanel Belgazal wrote:
quoted
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
quoted
In case the device does not support this capability, the driver
will use its own defines.
Signed-off-by: Netanel Belgazal <redacted>
---
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(-)
@@ -749,6 +751,31 @@ struct ena_admin_feature_rss_ind_table {structena_admin_rss_ind_table_entryinline_entry;};+/* When hint value is 0, driver should use it's own predefined value */+structena_admin_ena_hw_hints{+/* value in ms */+u16mmio_read_timeout;++/* value in ms */+u16driver_watchdog_timeout;++/* Per packet tx completion timeout. value in ms */+u16missing_tx_completion_timeout;++u16missed_tx_completion_count_threshold_to_reset;++/* value in ms */+u16admin_completion_tx_timeout;++u16netdev_wd_timeout;++u16max_tx_sgl_size;++u16max_rx_sgl_size;++u16reserved[8];+};+structena_admin_get_feat_cmd{structena_admin_aq_common_descaq_common_descriptor;
@@ -508,15 +508,13 @@ static int ena_com_comp_status_to_errno(u8 comp_status)staticintena_com_wait_and_process_admin_cq_polling(structena_comp_ctx*comp_ctx,structena_com_admin_queue*admin_queue){-unsignedlongflags;-u32start_time;+unsignedlongflags,timeout;intret;-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 *comintret;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.*Theremightbe2kindsoferrors
@@ -600,12 +599,14 @@ static u32 ena_com_reg_bar_read32(struct ena_com_dev *ena_dev, u16 offset)structena_com_mmio_read*mmio_read=&ena_dev->mmio_read;volatilestructena_admin_ena_mmio_req_read_less_resp*read_resp=mmio_read->read_resp;-u32mmio_read_reg,ret;+u32mmio_read_reg,timeout,ret;unsignedlongflags;inti;might_sleep();+timeout=mmio_read->reg_read_to?:ENA_REG_READ_TIMEOUT;+/* If readless is disabled, perform regular read */if(!mmio_read->readless_supported)returnreadl(ena_dev->reg_bar+offset);
@@ -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+*commandisn'tsupportedsetdriverhintsto0+*/+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));+elseif(rc==-EPERM)+memset(&get_feat_ctx->hw_hints,0x0,+sizeof(get_feat_ctx->hw_hints));+else+returnrc;+return0;}
@@ -1842,6 +1857,14 @@ int ena_com_dev_reset(struct ena_com_dev *ena_dev)returnrc;}+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;+return0;}
@@ -232,6 +234,7 @@ struct ena_com_admin_queue {void*q_dmadev;spinlock_tq_lock;/* spinlock for the admin queue */structena_comp_ctx*comp_ctx;+u32completion_timeout;u16q_depth;structena_com_admin_cqcq;structena_com_admin_sqsq;
@@ -266,6 +269,7 @@ struct ena_com_aenq {structena_com_mmio_read{structena_admin_ena_mmio_req_read_less_resp*read_resp;dma_addr_tread_resp_dma_addr;+u32reg_read_to;/* in us */u16seq_num;boolreadless_supported;/* spin lock to ensure a single outstanding read */
@@ -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-*becountedtwice.-*/-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;}}}
@@ -146,7 +146,18 @@ struct ena_tx_buffer {u32tx_descs;/* num of buffers used by this skb */u32num_of_bufs;-/* Save the last jiffies to detect missing tx packets */++/* Used for detect missing tx packets to limit the number of prints */+u32print_once;+/* Save the last jiffies to detect missing tx packets+*+*setstononzerovalueonena_start_xmitandsettozeroon+*napiandtimer_Service_routine.+*+*whilethisvalueisnotprotectedbylock,+*agivenpacketisnotexpectedtobehandledbyena_start_xmit+*andbynapi/timer_serviceatthesametime.+*/unsignedlonglast_jiffies;structena_com_bufbufs[ENA_PKT_MAX_BUFS];}____cacheline_aligned;