[v4,6/6] net/iavf: add watchdog for VFLR
Checks
Commit Message
Add watchdog to iAVF PMD which support monitoring the VFLR register. If
the device is not already in reset then if a VF reset in progress is
detected then notfiy user through callback and set into reset state.
If the device is already in reset then poll for completion of reset.
Signed-off-by: Declan Doherty <declan.doherty@intel.com>
Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
---
drivers/net/iavf/iavf.h | 6 +++
drivers/net/iavf/iavf_ethdev.c | 97 ++++++++++++++++++++++++++++++++++
2 files changed, 103 insertions(+)
Comments
> -----Original Message-----
> From: Nicolau, Radu <radu.nicolau@intel.com>
> Sent: Friday, October 1, 2021 5:52 PM
> To: Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>
> Cc: dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>; Sinha, Abhijit
> <abhijit.sinha@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> Nicolau, Radu <radu.nicolau@intel.com>
> Subject: [PATCH v4 6/6] net/iavf: add watchdog for VFLR
>
> Add watchdog to iAVF PMD which support monitoring the VFLR register. If
> the device is not already in reset then if a VF reset in progress is
> detected then notfiy user through callback and set into reset state.
> If the device is already in reset then poll for completion of reset.
>
> Signed-off-by: Declan Doherty <declan.doherty@intel.com>
> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
> ---
> drivers/net/iavf/iavf.h | 6 +++
> drivers/net/iavf/iavf_ethdev.c | 97 ++++++++++++++++++++++++++++++++++
> 2 files changed, 103 insertions(+)
>
> diff --git a/drivers/net/iavf/iavf.h b/drivers/net/iavf/iavf.h
> index d5f574b4b3..4481d2e134 100644
> --- a/drivers/net/iavf/iavf.h
> +++ b/drivers/net/iavf/iavf.h
> @@ -212,6 +212,12 @@ struct iavf_info {
> int cmd_retval; /* return value of the cmd response from PF */
> uint8_t *aq_resp; /* buffer to store the adminq response from PF */
>
> + struct {
> + uint8_t enabled:1;
> + uint64_t period_us;
> + } watchdog;
> + /** iAVF watchdog configuration */
> +
> /* Event from pf */
> bool dev_closed;
> bool link_up;
> diff --git a/drivers/net/iavf/iavf_ethdev.c b/drivers/net/iavf/iavf_ethdev.c
> index aad6a28585..d02aa9c1c5 100644
> --- a/drivers/net/iavf/iavf_ethdev.c
> +++ b/drivers/net/iavf/iavf_ethdev.c
> @@ -24,6 +24,7 @@
> #include <rte_malloc.h>
> #include <rte_memzone.h>
> #include <rte_dev.h>
> +#include <rte_alarm.h>
>
> #include "iavf.h"
> #include "iavf_rxtx.h"
> @@ -239,6 +240,94 @@ iavf_tm_ops_get(struct rte_eth_dev *dev __rte_unused,
> return 0;
> }
>
> +
> +static int
> +iavf_vfr_inprogress(struct iavf_hw *hw)
> +{
> + int inprogress = 0;
> +
> + if ((IAVF_READ_REG(hw, IAVF_VFGEN_RSTAT) &
> + IAVF_VFGEN_RSTAT_VFR_STATE_MASK) ==
> + VIRTCHNL_VFR_INPROGRESS)
> + inprogress = 1;
> +
> + if (inprogress)
> + PMD_DRV_LOG(INFO, "Watchdog detected VFR in progress");
> +
> + return inprogress;
> +}
> +
> +static void
> +iavf_dev_watchdog(void *cb_arg)
> +{
> + struct iavf_adapter *adapter = cb_arg;
> + struct iavf_hw *hw = IAVF_DEV_PRIVATE_TO_HW(adapter);
> + int vfr_inprogress = 0, rc = 0;
> +
> + /* check if watchdog has been disabled since last call */
> + if (!adapter->vf.watchdog.enabled)
> + return;
> +
> + /* If in reset then poll vfr_inprogress register for completion */
> + if (adapter->vf.vf_reset) {
> + vfr_inprogress = iavf_vfr_inprogress(hw);
> +
> + if (!vfr_inprogress) {
> + PMD_DRV_LOG(INFO, "VF \"%s\" reset has completed",
> + adapter->eth_dev->data->name);
> + adapter->vf.vf_reset = false;
> + }
> + /* If not in reset then poll vfr_inprogress register for VFLR event */
> + } else {
> + vfr_inprogress = iavf_vfr_inprogress(hw);
> +
> + if (vfr_inprogress) {
> + PMD_DRV_LOG(INFO,
> + "VF \"%s\" reset event has been detected by watchdog",
> + adapter->eth_dev->data->name);
> +
> + /* enter reset state with VFLR event */
> + adapter->vf.vf_reset = true;
> +
> + rte_eth_dev_callback_process(adapter->eth_dev,
> + RTE_ETH_EVENT_INTR_RESET, NULL);
> + }
> + }
> +
> + /* re-alarm watchdog */
> + rc = rte_eal_alarm_set(adapter->vf.watchdog.period_us,
> + &iavf_dev_watchdog, cb_arg);
> +
> + if (rc)
> + PMD_DRV_LOG(ERR, "Failed \"%s\" to reset device watchdog alarm",
> + adapter->eth_dev->data->name);
> +}
> +
> +static void
> +iavf_dev_watchdog_enable(struct iavf_adapter *adapter, uint64_t period_us)
> +{
> + int rc;
> +
> + PMD_DRV_LOG(INFO, "Enabling device watchdog");
> +
> + adapter->vf.watchdog.enabled = 1;
> + adapter->vf.watchdog.period_us = period_us;
> +
> + rc = rte_eal_alarm_set(adapter->vf.watchdog.period_us,
> + &iavf_dev_watchdog, (void *)adapter);
> + if (rc)
> + PMD_DRV_LOG(ERR, "Failed to enabled device watchdog");
> +}
> +
> +static void
> +iavf_dev_watchdog_disable(struct iavf_adapter *adapter)
> +{
> + PMD_DRV_LOG(INFO, "Disabling device watchdog");
> +
> + adapter->vf.watchdog.enabled = 0;
> + adapter->vf.watchdog.period_us = 0;
> +}
> +
> static int
> iavf_set_mc_addr_list(struct rte_eth_dev *dev,
> struct rte_ether_addr *mc_addrs,
> @@ -2448,6 +2537,11 @@ iavf_dev_init(struct rte_eth_dev *eth_dev)
>
> iavf_default_rss_disable(adapter);
>
> +
> + /* Start device watchdog, set polling period to 500us */
> + iavf_dev_watchdog_enable(adapter, 500);
> +
Besides checking VFGEN_RSTAT, there is a process to handle VIRTCHNL_OP_EVENT from PF. What is the change for? Any scenario which VIRTCHNL_OP_EVENT doesn't cover?
And how is the 500us been determined?
On 10/4/2021 3:15 AM, Wu, Jingjing wrote:
>
>> -----Original Message-----
>> From: Nicolau, Radu <radu.nicolau@intel.com>
>> Sent: Friday, October 1, 2021 5:52 PM
>> To: Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>
>> Cc: dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>; Sinha, Abhijit
>> <abhijit.sinha@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Richardson, Bruce
>> <bruce.richardson@intel.com>; Ananyev, Konstantin <konstantin.ananyev@intel.com>;
>> Nicolau, Radu <radu.nicolau@intel.com>
>> Subject: [PATCH v4 6/6] net/iavf: add watchdog for VFLR
>>
>> Add watchdog to iAVF PMD which support monitoring the VFLR register. If
>> the device is not already in reset then if a VF reset in progress is
>> detected then notfiy user through callback and set into reset state.
>> If the device is already in reset then poll for completion of reset.
>>
>> Signed-off-by: Declan Doherty <declan.doherty@intel.com>
>> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
>> ---
>> drivers/net/iavf/iavf.h | 6 +++
>> drivers/net/iavf/iavf_ethdev.c | 97 ++++++++++++++++++++++++++++++++++
>> 2 files changed, 103 insertions(+)
>>
>> diff --git a/drivers/net/iavf/iavf.h b/drivers/net/iavf/iavf.h
>> index d5f574b4b3..4481d2e134 100644
>> --- a/drivers/net/iavf/iavf.h
>> +++ b/drivers/net/iavf/iavf.h
>> @@ -212,6 +212,12 @@ struct iavf_info {
>> int cmd_retval; /* return value of the cmd response from PF */
>> uint8_t *aq_resp; /* buffer to store the adminq response from PF */
>>
>> + struct {
>> + uint8_t enabled:1;
>> + uint64_t period_us;
>> + } watchdog;
>> + /** iAVF watchdog configuration */
>> +
>> /* Event from pf */
>> bool dev_closed;
>> bool link_up;
>> diff --git a/drivers/net/iavf/iavf_ethdev.c b/drivers/net/iavf/iavf_ethdev.c
>> index aad6a28585..d02aa9c1c5 100644
>> --- a/drivers/net/iavf/iavf_ethdev.c
>> +++ b/drivers/net/iavf/iavf_ethdev.c
>> @@ -24,6 +24,7 @@
>> #include <rte_malloc.h>
>> #include <rte_memzone.h>
>> #include <rte_dev.h>
>> +#include <rte_alarm.h>
>>
>> #include "iavf.h"
>> #include "iavf_rxtx.h"
>> @@ -239,6 +240,94 @@ iavf_tm_ops_get(struct rte_eth_dev *dev __rte_unused,
>> return 0;
>> }
>>
>> +
>> +static int
>> +iavf_vfr_inprogress(struct iavf_hw *hw)
>> +{
>> + int inprogress = 0;
>> +
>> + if ((IAVF_READ_REG(hw, IAVF_VFGEN_RSTAT) &
>> + IAVF_VFGEN_RSTAT_VFR_STATE_MASK) ==
>> + VIRTCHNL_VFR_INPROGRESS)
>> + inprogress = 1;
>> +
>> + if (inprogress)
>> + PMD_DRV_LOG(INFO, "Watchdog detected VFR in progress");
>> +
>> + return inprogress;
>> +}
>> +
>> +static void
>> +iavf_dev_watchdog(void *cb_arg)
>> +{
>> + struct iavf_adapter *adapter = cb_arg;
>> + struct iavf_hw *hw = IAVF_DEV_PRIVATE_TO_HW(adapter);
>> + int vfr_inprogress = 0, rc = 0;
>> +
>> + /* check if watchdog has been disabled since last call */
>> + if (!adapter->vf.watchdog.enabled)
>> + return;
>> +
>> + /* If in reset then poll vfr_inprogress register for completion */
>> + if (adapter->vf.vf_reset) {
>> + vfr_inprogress = iavf_vfr_inprogress(hw);
>> +
>> + if (!vfr_inprogress) {
>> + PMD_DRV_LOG(INFO, "VF \"%s\" reset has completed",
>> + adapter->eth_dev->data->name);
>> + adapter->vf.vf_reset = false;
>> + }
>> + /* If not in reset then poll vfr_inprogress register for VFLR event */
>> + } else {
>> + vfr_inprogress = iavf_vfr_inprogress(hw);
>> +
>> + if (vfr_inprogress) {
>> + PMD_DRV_LOG(INFO,
>> + "VF \"%s\" reset event has been detected by watchdog",
>> + adapter->eth_dev->data->name);
>> +
>> + /* enter reset state with VFLR event */
>> + adapter->vf.vf_reset = true;
>> +
>> + rte_eth_dev_callback_process(adapter->eth_dev,
>> + RTE_ETH_EVENT_INTR_RESET, NULL);
>> + }
>> + }
>> +
>> + /* re-alarm watchdog */
>> + rc = rte_eal_alarm_set(adapter->vf.watchdog.period_us,
>> + &iavf_dev_watchdog, cb_arg);
>> +
>> + if (rc)
>> + PMD_DRV_LOG(ERR, "Failed \"%s\" to reset device watchdog alarm",
>> + adapter->eth_dev->data->name);
>> +}
>> +
>> +static void
>> +iavf_dev_watchdog_enable(struct iavf_adapter *adapter, uint64_t period_us)
>> +{
>> + int rc;
>> +
>> + PMD_DRV_LOG(INFO, "Enabling device watchdog");
>> +
>> + adapter->vf.watchdog.enabled = 1;
>> + adapter->vf.watchdog.period_us = period_us;
>> +
>> + rc = rte_eal_alarm_set(adapter->vf.watchdog.period_us,
>> + &iavf_dev_watchdog, (void *)adapter);
>> + if (rc)
>> + PMD_DRV_LOG(ERR, "Failed to enabled device watchdog");
>> +}
>> +
>> +static void
>> +iavf_dev_watchdog_disable(struct iavf_adapter *adapter)
>> +{
>> + PMD_DRV_LOG(INFO, "Disabling device watchdog");
>> +
>> + adapter->vf.watchdog.enabled = 0;
>> + adapter->vf.watchdog.period_us = 0;
>> +}
>> +
>> static int
>> iavf_set_mc_addr_list(struct rte_eth_dev *dev,
>> struct rte_ether_addr *mc_addrs,
>> @@ -2448,6 +2537,11 @@ iavf_dev_init(struct rte_eth_dev *eth_dev)
>>
>> iavf_default_rss_disable(adapter);
>>
>> +
>> + /* Start device watchdog, set polling period to 500us */
>> + iavf_dev_watchdog_enable(adapter, 500);
>> +
> Besides checking VFGEN_RSTAT, there is a process to handle VIRTCHNL_OP_EVENT from PF. What is the change for? Any scenario which VIRTCHNL_OP_EVENT doesn't cover?
> And how is the 500us been determined?
Hi Jingjing, thanks for reviewing, I think this can be handled with the
VIRTCHNL_OP_EVENT with no need for a watchdog alarm, I will rework the
patch.
On 10/4/2021 12:18 PM, Nicolau, Radu wrote:
>
> On 10/4/2021 3:15 AM, Wu, Jingjing wrote:
>>
>>> -----Original Message-----
>>> From: Nicolau, Radu <radu.nicolau@intel.com>
>>> Sent: Friday, October 1, 2021 5:52 PM
>>> To: Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
>>> <beilei.xing@intel.com>
>>> Cc: dev@dpdk.org; Doherty, Declan <declan.doherty@intel.com>; Sinha,
>>> Abhijit
>>> <abhijit.sinha@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>;
>>> Richardson, Bruce
>>> <bruce.richardson@intel.com>; Ananyev, Konstantin
>>> <konstantin.ananyev@intel.com>;
>>> Nicolau, Radu <radu.nicolau@intel.com>
>>> Subject: [PATCH v4 6/6] net/iavf: add watchdog for VFLR
>>>
>>> Add watchdog to iAVF PMD which support monitoring the VFLR register. If
>>> the device is not already in reset then if a VF reset in progress is
>>> detected then notfiy user through callback and set into reset state.
>>> If the device is already in reset then poll for completion of reset.
>>>
>>> Signed-off-by: Declan Doherty <declan.doherty@intel.com>
>>> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
>>> ---
>>> drivers/net/iavf/iavf.h | 6 +++
>>> drivers/net/iavf/iavf_ethdev.c | 97
>>> ++++++++++++++++++++++++++++++++++
>>> 2 files changed, 103 insertions(+)
>>>
>>> ...
>>>
>> Besides checking VFGEN_RSTAT, there is a process to handle
>> VIRTCHNL_OP_EVENT from PF. What is the change for? Any scenario
>> which VIRTCHNL_OP_EVENT doesn't cover?
>> And how is the 500us been determined?
>
> Hi Jingjing, thanks for reviewing, I think this can be handled with
> the VIRTCHNL_OP_EVENT with no need for a watchdog alarm, I will
> rework the patch.
>
Hi Jingjing I went over this with Declan, the reason it was added is
that we can actually have a hardware initiated reset that may not
trigger an event; and also the kernel driver is implementing a similar
mechanism. The 500us seems indeed excessive I will update the patch to
use a configurable value with the default of 5ms, as the kernel driver does.
> > Besides checking VFGEN_RSTAT, there is a process to handle
> VIRTCHNL_OP_EVENT from PF. What is the change for? Any scenario which
> VIRTCHNL_OP_EVENT doesn't cover?
> > And how is the 500us been determined?
>
> Hi Jingjing, thanks for reviewing, I think this can be handled with the
> VIRTCHNL_OP_EVENT with no need for a watchdog alarm, I will rework the
> patch.
>
Hi, Radu, I saw the patch is reworked, but looks like watchdog is still there. So what is the scenario
VIRTCHNL_OP_EVENT doesn't cover?
On 10/8/2021 7:19 AM, Wu, Jingjing wrote:
>>> Besides checking VFGEN_RSTAT, there is a process to handle
>> VIRTCHNL_OP_EVENT from PF. What is the change for? Any scenario which
>> VIRTCHNL_OP_EVENT doesn't cover?
>>> And how is the 500us been determined?
>> Hi Jingjing, thanks for reviewing, I think this can be handled with the
>> VIRTCHNL_OP_EVENT with no need for a watchdog alarm, I will rework the
>> patch.
>>
> Hi, Radu, I saw the patch is reworked, but looks like watchdog is still there. So what is the scenario
> VIRTCHNL_OP_EVENT doesn't cover?
Hi Jingjing I went over this with Declan, the reason it was added is
that we can actually have a hardware initiated reset that may not
trigger an event; and also the kernel driver is implementing a similar
mechanism.
Would it be more acceptable to have it disabled by default?
@@ -212,6 +212,12 @@ struct iavf_info {
int cmd_retval; /* return value of the cmd response from PF */
uint8_t *aq_resp; /* buffer to store the adminq response from PF */
+ struct {
+ uint8_t enabled:1;
+ uint64_t period_us;
+ } watchdog;
+ /** iAVF watchdog configuration */
+
/* Event from pf */
bool dev_closed;
bool link_up;
@@ -24,6 +24,7 @@
#include <rte_malloc.h>
#include <rte_memzone.h>
#include <rte_dev.h>
+#include <rte_alarm.h>
#include "iavf.h"
#include "iavf_rxtx.h"
@@ -239,6 +240,94 @@ iavf_tm_ops_get(struct rte_eth_dev *dev __rte_unused,
return 0;
}
+
+static int
+iavf_vfr_inprogress(struct iavf_hw *hw)
+{
+ int inprogress = 0;
+
+ if ((IAVF_READ_REG(hw, IAVF_VFGEN_RSTAT) &
+ IAVF_VFGEN_RSTAT_VFR_STATE_MASK) ==
+ VIRTCHNL_VFR_INPROGRESS)
+ inprogress = 1;
+
+ if (inprogress)
+ PMD_DRV_LOG(INFO, "Watchdog detected VFR in progress");
+
+ return inprogress;
+}
+
+static void
+iavf_dev_watchdog(void *cb_arg)
+{
+ struct iavf_adapter *adapter = cb_arg;
+ struct iavf_hw *hw = IAVF_DEV_PRIVATE_TO_HW(adapter);
+ int vfr_inprogress = 0, rc = 0;
+
+ /* check if watchdog has been disabled since last call */
+ if (!adapter->vf.watchdog.enabled)
+ return;
+
+ /* If in reset then poll vfr_inprogress register for completion */
+ if (adapter->vf.vf_reset) {
+ vfr_inprogress = iavf_vfr_inprogress(hw);
+
+ if (!vfr_inprogress) {
+ PMD_DRV_LOG(INFO, "VF \"%s\" reset has completed",
+ adapter->eth_dev->data->name);
+ adapter->vf.vf_reset = false;
+ }
+ /* If not in reset then poll vfr_inprogress register for VFLR event */
+ } else {
+ vfr_inprogress = iavf_vfr_inprogress(hw);
+
+ if (vfr_inprogress) {
+ PMD_DRV_LOG(INFO,
+ "VF \"%s\" reset event has been detected by watchdog",
+ adapter->eth_dev->data->name);
+
+ /* enter reset state with VFLR event */
+ adapter->vf.vf_reset = true;
+
+ rte_eth_dev_callback_process(adapter->eth_dev,
+ RTE_ETH_EVENT_INTR_RESET, NULL);
+ }
+ }
+
+ /* re-alarm watchdog */
+ rc = rte_eal_alarm_set(adapter->vf.watchdog.period_us,
+ &iavf_dev_watchdog, cb_arg);
+
+ if (rc)
+ PMD_DRV_LOG(ERR, "Failed \"%s\" to reset device watchdog alarm",
+ adapter->eth_dev->data->name);
+}
+
+static void
+iavf_dev_watchdog_enable(struct iavf_adapter *adapter, uint64_t period_us)
+{
+ int rc;
+
+ PMD_DRV_LOG(INFO, "Enabling device watchdog");
+
+ adapter->vf.watchdog.enabled = 1;
+ adapter->vf.watchdog.period_us = period_us;
+
+ rc = rte_eal_alarm_set(adapter->vf.watchdog.period_us,
+ &iavf_dev_watchdog, (void *)adapter);
+ if (rc)
+ PMD_DRV_LOG(ERR, "Failed to enabled device watchdog");
+}
+
+static void
+iavf_dev_watchdog_disable(struct iavf_adapter *adapter)
+{
+ PMD_DRV_LOG(INFO, "Disabling device watchdog");
+
+ adapter->vf.watchdog.enabled = 0;
+ adapter->vf.watchdog.period_us = 0;
+}
+
static int
iavf_set_mc_addr_list(struct rte_eth_dev *dev,
struct rte_ether_addr *mc_addrs,
@@ -2448,6 +2537,11 @@ iavf_dev_init(struct rte_eth_dev *eth_dev)
iavf_default_rss_disable(adapter);
+
+ /* Start device watchdog, set polling period to 500us */
+ iavf_dev_watchdog_enable(adapter, 500);
+
+
return 0;
flow_init_err:
@@ -2527,6 +2621,9 @@ iavf_dev_close(struct rte_eth_dev *dev)
if (vf->vf_reset && !rte_pci_set_bus_master(pci_dev, true))
vf->vf_reset = false;
+ /* disable watchdog */
+ iavf_dev_watchdog_disable(adapter);
+
return ret;
}