Re: [dpdk-dev] [PATCH v4 6/6] net/iavf: add watchdog for VFLR

["Nicolau, Radu" <radu.nicolau@intel.com>]

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.