[dpdk-dev] [RFC PATCH 0/3] librte_ethdev: error recovery support

[dpdk-dev] [RFC PATCH 0/3] librte_ethdev: error recovery support

[Kalesh A P]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

This patch adds support for recovery event in rte_eth_event framework.
FW error and FW reset conditions would be managed by PMD. Driver uses
RTE_ETH_EVENT_INTR_RESET event to notify the applications about the
FW reset or error. In such cases, PMD would need recovery events to
notify application about PMD has recovered from FW reset or FW error.

Kalesh AP (3):
  librte_ethdev: support device recovery event
  net/bnxt: notify applications about device reset
  app/testpmd: handle device recovery event

 app/test-pmd/testpmd.c         |  7 ++++++-
 drivers/net/bnxt/bnxt_cpr.c    |  3 +++
 drivers/net/bnxt/bnxt_ethdev.c | 10 ++++++++++
 lib/librte_ethdev/rte_ethdev.h |  1 +
 4 files changed, 20 insertions(+), 1 deletion(-)

-- 
2.10.1

[dpdk-dev] [RFC PATCH 1/3] librte_ethdev: support device recovery event

[Kalesh A P]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

Adding support for recovery event in rte_eth_event framework.
FW error and FW reset conditions would be managed by PMD.
In such cases, PMD would need recovery events to notify application
about PMD has recovered from FW reset or FW error.

Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Signed-off-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
---
 lib/librte_ethdev/rte_ethdev.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index d1a593a..0989f41 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -3015,6 +3015,7 @@ enum rte_eth_event_type {
 	RTE_ETH_EVENT_NEW,      /**< port is probed */
 	RTE_ETH_EVENT_DESTROY,  /**< port is released */
 	RTE_ETH_EVENT_IPSEC,    /**< IPsec offload related event */
+	RTE_ETH_EVENT_RECOVERED, /**< port recovered from an error */
 	RTE_ETH_EVENT_MAX       /**< max value of this enum */
 };
 
-- 
2.10.1

[dpdk-dev] [RFC PATCH 2/3] net/bnxt: notify applications about device reset

[Kalesh A P]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

When the driver receives RESET_NOTIFY async event from FW or detected
an error condition, it should update the application that FW is going
to reset. Once the driver recoveres from the reset, update the reset
recovery status to application as well.

The recovery process is transparent to the application as the driver
itself tries to recover from FW reset or FW error conditions.

Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Signed-off-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
---
 drivers/net/bnxt/bnxt_cpr.c    |  3 +++
 drivers/net/bnxt/bnxt_ethdev.c | 10 ++++++++++
 2 files changed, 13 insertions(+)

diff --git a/drivers/net/bnxt/bnxt_cpr.c b/drivers/net/bnxt/bnxt_cpr.c
index bb316b9..02eeeb9 100644
--- a/drivers/net/bnxt/bnxt_cpr.c
+++ b/drivers/net/bnxt/bnxt_cpr.c
@@ -76,6 +76,9 @@ void bnxt_handle_async_event(struct bnxt *bp,
 		PMD_DRV_LOG(INFO, "Port conn async event\n");
 		break;
 	case HWRM_ASYNC_EVENT_CMPL_EVENT_ID_RESET_NOTIFY:
+		_rte_eth_dev_callback_process(bp->eth_dev,
+					      RTE_ETH_EVENT_INTR_RESET,
+					      NULL);
 		event_data = rte_le_to_cpu_32(async_cmp->event_data1);
 		/* timestamp_lo/hi values are in units of 100ms */
 		bp->fw_reset_max_msecs = async_cmp->timestamp_hi ?
diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
index 2ef1169..93e67b1 100644
--- a/drivers/net/bnxt/bnxt_ethdev.c
+++ b/drivers/net/bnxt/bnxt_ethdev.c
@@ -4021,11 +4021,17 @@ static void bnxt_dev_recover(void *arg)
 		goto err;
 
 	PMD_DRV_LOG(INFO, "Recovered from FW reset\n");
+	_rte_eth_dev_callback_process(bp->eth_dev,
+				      RTE_ETH_EVENT_RECOVERED,
+				      NULL);
 	return;
 err:
 	bp->flags |= BNXT_FLAG_FATAL_ERROR;
 	bnxt_uninit_resources(bp, false);
 	PMD_DRV_LOG(ERR, "Failed to recover from FW reset\n");
+	_rte_eth_dev_callback_process(bp->eth_dev,
+				      RTE_ETH_EVENT_INTR_RMV,
+				      NULL);
 }
 
 void bnxt_dev_reset_and_resume(void *arg)
@@ -4160,6 +4166,10 @@ static void bnxt_check_fw_health(void *arg)
 	bp->flags |= BNXT_FLAG_FATAL_ERROR;
 	bp->flags |= BNXT_FLAG_FW_RESET;
 
+	_rte_eth_dev_callback_process(bp->eth_dev,
+				      RTE_ETH_EVENT_INTR_RESET,
+				      NULL);
+
 	PMD_DRV_LOG(ERR, "Detected FW dead condition\n");
 
 	if (bnxt_is_master_func(bp))
-- 
2.10.1

[dpdk-dev] [RFC PATCH 3/3] app/testpmd: handle device recovery event

[Kalesh A P]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

Added code to handle device recovery event in testpmd.
This is an indication from PMD that it has recovered from a FW
reset or FW error condition.

Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Reviewed-by: Ajit Kumar Khaparde <ajit.khaparde@broadcom.com>
---
 app/test-pmd/testpmd.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index f9f4cd1..d1f063e 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -375,6 +375,7 @@ static const char * const eth_event_desc[] = {
 	[RTE_ETH_EVENT_INTR_RMV] = "device removal",
 	[RTE_ETH_EVENT_NEW] = "device probed",
 	[RTE_ETH_EVENT_DESTROY] = "device released",
+	[RTE_ETH_EVENT_RECOVERED] = "device recovered",
 	[RTE_ETH_EVENT_MAX] = NULL,
 };
 
@@ -388,7 +389,8 @@ uint32_t event_print_mask = (UINT32_C(1) << RTE_ETH_EVENT_UNKNOWN) |
 			    (UINT32_C(1) << RTE_ETH_EVENT_INTR_RESET) |
 			    (UINT32_C(1) << RTE_ETH_EVENT_IPSEC) |
 			    (UINT32_C(1) << RTE_ETH_EVENT_MACSEC) |
-			    (UINT32_C(1) << RTE_ETH_EVENT_INTR_RMV);
+			    (UINT32_C(1) << RTE_ETH_EVENT_INTR_RMV) |
+			    (UINT32_C(1) << RTE_ETH_EVENT_RECOVERED);
 /*
  * Decide if all memory are locked for performance.
  */
@@ -2920,6 +2922,9 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
 				rmv_port_callback, (void *)(intptr_t)port_id))
 			fprintf(stderr, "Could not set up deferred device removal\n");
 		break;
+	case RTE_ETH_EVENT_RECOVERED:
+		printf("Please recreate any rules offloaded prior to this\n");
+		break;
 	default:
 		break;
 	}
-- 
2.10.1

Re: [dpdk-dev] [RFC PATCH 0/3] librte_ethdev: error recovery support

[Thomas Monjalon]

22/01/2020 11:16, Kalesh A P:
> From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> 
> This patch adds support for recovery event in rte_eth_event framework.
> FW error and FW reset conditions would be managed by PMD. Driver uses

"Driver"? THE driver? :)

> RTE_ETH_EVENT_INTR_RESET event to notify the applications about the
> FW reset or error.

Which drivers doe that?

> In such cases, PMD would need recovery events to
> notify application about PMD has recovered from FW reset or FW error.

Sorry I don't understand. You said application is notified of any error.
But the PMD can recover from this error? So what is the error at the end?
If the error is recovered why notifying the application?

Re: [dpdk-dev] [RFC PATCH 1/3] librte_ethdev: support device recovery event

[Thomas Monjalon]

22/01/2020 11:16, Kalesh A P:
> From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> 
> Adding support for recovery event in rte_eth_event framework.
> FW error and FW reset conditions would be managed by PMD.
> In such cases, PMD would need recovery events to notify application
> about PMD has recovered from FW reset or FW error.
> 
> Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> Signed-off-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
> ---
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -3015,6 +3015,7 @@ enum rte_eth_event_type {
>  	RTE_ETH_EVENT_NEW,      /**< port is probed */
>  	RTE_ETH_EVENT_DESTROY,  /**< port is released */
>  	RTE_ETH_EVENT_IPSEC,    /**< IPsec offload related event */
> +	RTE_ETH_EVENT_RECOVERED, /**< port recovered from an error */

What the application is supposed to do when receiving such event?

Re: [dpdk-dev] [RFC PATCH 0/3] librte_ethdev: error recovery support

[Kalesh Anakkur Purayil]

Hi Thomas,

On Wed, Mar 11, 2020 at 6:49 PM Thomas Monjalon <thomas@monjalon.net> wrote:

> 22/01/2020 11:16, Kalesh A P:
> > From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> >
> > This patch adds support for recovery event in rte_eth_event framework.
> > FW error and FW reset conditions would be managed by PMD. Driver uses
>
> "Driver"? THE driver? :)
>
> > RTE_ETH_EVENT_INTR_RESET event to notify the applications about the
> > FW reset or error.
>
> Which drivers doe that?
>
[Kalesh]: Second patch in this series implements this behavior in bnxt PMD.
Error recovery is a new feature added in bnxt PMD in 19.11. This change is
needed to support error recovery functionality.

>
> > In such cases, PMD would need recovery events to
> > notify application about PMD has recovered from FW reset or FW error.
>
> Sorry I don't understand. You said application is notified of any error.
> But the PMD can recover from this error? So what is the error at the end?
> If the error is recovered why notifying the application?
>
[Kalesh] : Let me give you some insight on this.

The error recovery solution is a protocol implemented between firmware and
bnxt PMD to recover from the fatal errors without a system reboot. There is
an alarm thread which constantly monitors the health of the firmware and
initiates a recovery when needed.

There are two scenarios here:

1. Hardware or firmware encountered an error which firmware detected.
Firmware is in operational status here. In this case, firmware can reset
the chip and notify the driver about the reset.
2. Hardware or firmware encountered an error but firmware is dead/hung.
Firmware is not in operational status. In this case, the only possible way
to recover the adapter is through host driver(bnxt PMD).

In both cases, bnxt PMD reinitializes with the FW again after the reset.
During that recovery process, data path will be halted and any control path
operation would fail. So, bnxt PMD has to notify the application about this
reset/error event to prevent any activities from application during this
time.

-- 
Regards,
Kalesh A P

Re: [dpdk-dev] [RFC PATCH 1/3] librte_ethdev: support device recovery event

[Kalesh Anakkur Purayil]

Hi Thomas,

On Wed, Mar 11, 2020 at 6:50 PM Thomas Monjalon <thomas@monjalon.net> wrote:

> 22/01/2020 11:16, Kalesh A P:
> > From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> >
> > Adding support for recovery event in rte_eth_event framework.
> > FW error and FW reset conditions would be managed by PMD.
> > In such cases, PMD would need recovery events to notify application
> > about PMD has recovered from FW reset or FW error.
> >
> > Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> > Signed-off-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> > Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
> > ---
> > --- a/lib/librte_ethdev/rte_ethdev.h
> > +++ b/lib/librte_ethdev/rte_ethdev.h
> > @@ -3015,6 +3015,7 @@ enum rte_eth_event_type {
> >       RTE_ETH_EVENT_NEW,      /**< port is probed */
> >       RTE_ETH_EVENT_DESTROY,  /**< port is released */
> >       RTE_ETH_EVENT_IPSEC,    /**< IPsec offload related event */
> > +     RTE_ETH_EVENT_RECOVERED, /**< port recovered from an error */
>
> What the application is supposed to do when receiving such event?
>
[Kalesh]:  RTE_ETH_EVENT_RECOVERED event is an indication to the
application that PMD has recovered from the reset and is functional
again(i.e control path and data path is up again). On receiving this event
from PMD, application has to re-create any flow rules created prior to the
reset.

-- 
Regards,
Kalesh A P

Re: [dpdk-dev] [RFC PATCH 1/3] librte_ethdev: support device recovery event

[Thomas Monjalon]

12/03/2020 04:31, Kalesh Anakkur Purayil:
> On Wed, Mar 11, 2020 at 6:50 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > 22/01/2020 11:16, Kalesh A P:
> > > From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> > >
> > > Adding support for recovery event in rte_eth_event framework.
> > > FW error and FW reset conditions would be managed by PMD.
> > > In such cases, PMD would need recovery events to notify application
> > > about PMD has recovered from FW reset or FW error.
> > >
> > > Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> > > Signed-off-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> > > Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
> > > ---
> > > --- a/lib/librte_ethdev/rte_ethdev.h
> > > +++ b/lib/librte_ethdev/rte_ethdev.h
> > > @@ -3015,6 +3015,7 @@ enum rte_eth_event_type {
> > >       RTE_ETH_EVENT_NEW,      /**< port is probed */
> > >       RTE_ETH_EVENT_DESTROY,  /**< port is released */
> > >       RTE_ETH_EVENT_IPSEC,    /**< IPsec offload related event */
> > > +     RTE_ETH_EVENT_RECOVERED, /**< port recovered from an error */
> >
> > What the application is supposed to do when receiving such event?
> >
> [Kalesh]:  RTE_ETH_EVENT_RECOVERED event is an indication to the
> application that PMD has recovered from the reset and is functional
> again(i.e control path and data path is up again). On receiving this event
> from PMD, application has to re-create any flow rules created prior to the
> reset.

How the application knows that flow rules were resetted?
Is there any other configuration resetted?
These informations must be explicit in the doxygen comments.

Re: [dpdk-dev] [RFC PATCH 0/3] librte_ethdev: error recovery support

[Thomas Monjalon]

12/03/2020 04:25, Kalesh Anakkur Purayil:
> Hi Thomas,
> 
> On Wed, Mar 11, 2020 at 6:49 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> 
> > 22/01/2020 11:16, Kalesh A P:
> > > From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> > >
> > > This patch adds support for recovery event in rte_eth_event framework.
> > > FW error and FW reset conditions would be managed by PMD. Driver uses
> >
> > "Driver"? THE driver? :)
> >
> > > RTE_ETH_EVENT_INTR_RESET event to notify the applications about the
> > > FW reset or error.
> >
> > Which drivers doe that?
> >
> [Kalesh]: Second patch in this series implements this behavior in bnxt PMD.
> Error recovery is a new feature added in bnxt PMD in 19.11. This change is
> needed to support error recovery functionality.
> 
> >
> > > In such cases, PMD would need recovery events to
> > > notify application about PMD has recovered from FW reset or FW error.
> >
> > Sorry I don't understand. You said application is notified of any error.
> > But the PMD can recover from this error? So what is the error at the end?
> > If the error is recovered why notifying the application?
> >
> [Kalesh] : Let me give you some insight on this.
> 
> The error recovery solution is a protocol implemented between firmware and
> bnxt PMD to recover from the fatal errors without a system reboot. There is
> an alarm thread which constantly monitors the health of the firmware and
> initiates a recovery when needed.
> 
> There are two scenarios here:
> 
> 1. Hardware or firmware encountered an error which firmware detected.
> Firmware is in operational status here. In this case, firmware can reset
> the chip and notify the driver about the reset.
> 2. Hardware or firmware encountered an error but firmware is dead/hung.
> Firmware is not in operational status. In this case, the only possible way
> to recover the adapter is through host driver(bnxt PMD).
> 
> In both cases, bnxt PMD reinitializes with the FW again after the reset.
> During that recovery process, data path will be halted and any control path
> operation would fail. So, bnxt PMD has to notify the application about this
> reset/error event to prevent any activities from application during this
> time.

I think you are changing the meaning of the reset event.
It was described like this:
RTE_ETH_EVENT_INTR_RESET,
            /**< reset interrupt event, sent to VF on PF reset */

Please update this description as well.

Of course, we'll need approval from other PMD maintainers
to accept the new recovery API.

Re: [dpdk-dev] [RFC PATCH 0/3] librte_ethdev: error recovery support

[Ferruh Yigit]

On 3/12/2020 7:34 AM, Thomas Monjalon wrote:
> 12/03/2020 04:25, Kalesh Anakkur Purayil:
>> Hi Thomas,
>>
>> On Wed, Mar 11, 2020 at 6:49 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>>
>>> 22/01/2020 11:16, Kalesh A P:
>>>> From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
>>>>
>>>> This patch adds support for recovery event in rte_eth_event framework.
>>>> FW error and FW reset conditions would be managed by PMD. Driver uses
>>>
>>> "Driver"? THE driver? :)
>>>
>>>> RTE_ETH_EVENT_INTR_RESET event to notify the applications about the
>>>> FW reset or error.
>>>
>>> Which drivers doe that?
>>>
>> [Kalesh]: Second patch in this series implements this behavior in bnxt PMD.
>> Error recovery is a new feature added in bnxt PMD in 19.11. This change is
>> needed to support error recovery functionality.
>>
>>>
>>>> In such cases, PMD would need recovery events to
>>>> notify application about PMD has recovered from FW reset or FW error.
>>>
>>> Sorry I don't understand. You said application is notified of any error.
>>> But the PMD can recover from this error? So what is the error at the end?
>>> If the error is recovered why notifying the application?
>>>
>> [Kalesh] : Let me give you some insight on this.
>>
>> The error recovery solution is a protocol implemented between firmware and
>> bnxt PMD to recover from the fatal errors without a system reboot. There is
>> an alarm thread which constantly monitors the health of the firmware and
>> initiates a recovery when needed.
>>
>> There are two scenarios here:
>>
>> 1. Hardware or firmware encountered an error which firmware detected.
>> Firmware is in operational status here. In this case, firmware can reset
>> the chip and notify the driver about the reset.
>> 2. Hardware or firmware encountered an error but firmware is dead/hung.
>> Firmware is not in operational status. In this case, the only possible way
>> to recover the adapter is through host driver(bnxt PMD).
>>
>> In both cases, bnxt PMD reinitializes with the FW again after the reset.
>> During that recovery process, data path will be halted and any control path
>> operation would fail. So, bnxt PMD has to notify the application about this
>> reset/error event to prevent any activities from application during this
>> time.
> 
> I think you are changing the meaning of the reset event.
> It was described like this:
> RTE_ETH_EVENT_INTR_RESET,
>             /**< reset interrupt event, sent to VF on PF reset */
> 
> Please update this description as well.
> 
> Of course, we'll need approval from other PMD maintainers
> to accept the new recovery API.
> 

Hi Kalesh,

Is this RFC still relevant/valid?

[dpdk-dev] [RFC V2 0/3] librte_ethdev: error recovery support

[Kalesh A P]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

The error recovery solution is a protocol implemented between firmware
and bnxt PMD to recover from the fatal errors without a system reboot.
There is an alarm thread which constantly monitors the health of the
firmware and initiates a recovery when needed.

There are two scenarios here:

1. Hardware or firmware encountered an error which firmware detected.
   Firmware is in operational status here. In this case, firmware can
   reset the chip and notify the driver about the reset.
2. Hardware or firmware encountered an error but firmware is dead/hung.
   Firmware is not in operational status. In this case, the only possible
   way to recover the adapter is through host driver(bnxt PMD).

In both cases, bnxt PMD reinitializes with the FW again after the reset.
During that recovery process, data path will be halted and any control path
operation would fail. So, the PMD has to notify the application about this
reset/error event to prevent any activities from the application while
the PMD is recovering from the error.

This patch set adds support for the reset and recovery event in
the rte_eth_event framework. FW error and FW reset conditions would be
managed by the PMD. Driver uses RTE_ETH_EVENT_RESET event to notify
the applications about the FW reset or error. In such cases,
PMD use the RTE_ETH_EVENT_RECOVERED event to notify application about
PMD has recovered from FW reset or FW error.

V2: Added a new event RTE_ETH_EVENT_RESET instead of using the
RTE_ETH_EVENT_INTR_RESET to notify applications about device reset.

Kalesh AP (3):
  ethdev: support device reset and recovery events
  net/bnxt: notify applications about device reset/recovery
  app/testpmd: handle device recovery event

 app/test-pmd/testpmd.c         | 6 +++++-
 drivers/net/bnxt/bnxt_cpr.c    | 3 +++
 drivers/net/bnxt/bnxt_ethdev.c | 9 +++++++++
 lib/librte_ethdev/rte_ethdev.h | 2 ++
 4 files changed, 19 insertions(+), 1 deletion(-)

-- 
2.10.1

[dpdk-dev] [RFC V2 1/3] ethdev: support device reset and recovery events

[Kalesh A P]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

Adding support for device reset and recovery events in the
rte_eth_event framework. FW error and FW reset conditions would be
managed internally by PMD without needing application intervention.
In such cases, PMD would need reset/recovery events to notify application
that PMD is undergoing a reset.

Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
---
 lib/librte_ethdev/rte_ethdev.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index 9759f13..b9dd14c 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -3207,6 +3207,8 @@ enum rte_eth_event_type {
 	RTE_ETH_EVENT_DESTROY,  /**< port is released */
 	RTE_ETH_EVENT_IPSEC,    /**< IPsec offload related event */
 	RTE_ETH_EVENT_FLOW_AGED,/**< New aged-out flows is detected */
+	RTE_ETH_EVENT_RESET,    /**< port resetting from an error */
+	RTE_ETH_EVENT_RECOVERED, /**< port recovered from an error */
 	RTE_ETH_EVENT_MAX       /**< max value of this enum */
 };
 
-- 
2.10.1

[dpdk-dev] [RFC V2 2/3] net/bnxt: notify applications about device reset/recovery

[Kalesh A P]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

When the driver receives RESET_NOTIFY async event from FW or detected
an error condition, it should update the application that FW is going
to reset. Once the driver recoveres from the reset, update the reset
recovery status to application as well.

The recovery process is transparent to the application as the driver
itself tries to recover from FW reset or FW error conditions.

Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Signed-off-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
---
 drivers/net/bnxt/bnxt_cpr.c    | 3 +++
 drivers/net/bnxt/bnxt_ethdev.c | 9 +++++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/net/bnxt/bnxt_cpr.c b/drivers/net/bnxt/bnxt_cpr.c
index 8311e26..f2fddf7 100644
--- a/drivers/net/bnxt/bnxt_cpr.c
+++ b/drivers/net/bnxt/bnxt_cpr.c
@@ -129,6 +129,9 @@ void bnxt_handle_async_event(struct bnxt *bp,
 			bp->flags |= BNXT_FLAG_FATAL_ERROR;
 			return;
 		}
+		_rte_eth_dev_callback_process(bp->eth_dev,
+					      RTE_ETH_EVENT_RESET,
+					      NULL);
 
 		event_data = rte_le_to_cpu_32(async_cmp->event_data1);
 		/* timestamp_lo/hi values are in units of 100ms */
diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
index b99c712..609bf46 100644
--- a/drivers/net/bnxt/bnxt_ethdev.c
+++ b/drivers/net/bnxt/bnxt_ethdev.c
@@ -4566,6 +4566,9 @@ static void bnxt_dev_recover(void *arg)
 		goto err_start;
 
 	PMD_DRV_LOG(INFO, "Recovered from FW reset\n");
+	_rte_eth_dev_callback_process(bp->eth_dev,
+				      RTE_ETH_EVENT_RECOVERED,
+				      NULL);
 	return;
 err_start:
 	bnxt_dev_stop_op(bp->eth_dev);
@@ -4573,6 +4576,9 @@ static void bnxt_dev_recover(void *arg)
 	bp->flags |= BNXT_FLAG_FATAL_ERROR;
 	bnxt_uninit_resources(bp, false);
 	PMD_DRV_LOG(ERR, "Failed to recover from FW reset\n");
+	_rte_eth_dev_callback_process(bp->eth_dev,
+				      RTE_ETH_EVENT_INTR_RMV,
+				      NULL);
 }
 
 void bnxt_dev_reset_and_resume(void *arg)
@@ -4708,6 +4714,9 @@ static void bnxt_check_fw_health(void *arg)
 	bp->flags |= BNXT_FLAG_FW_RESET;
 
 	PMD_DRV_LOG(ERR, "Detected FW dead condition\n");
+	_rte_eth_dev_callback_process(bp->eth_dev,
+				      RTE_ETH_EVENT_RESET,
+				      NULL);
 
 	if (bnxt_is_master_func(bp))
 		wait_msec = info->master_func_wait_period;
-- 
2.10.1

[dpdk-dev] [RFC V2 3/3] app/testpmd: handle device recovery event

[Kalesh A P]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

Added code to handle device reset and recovery event in testpmd.
This is an indication from the PMD that device has resetted and
recovered error condition.

Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Reviewed-by: Ajit Kumar Khaparde <ajit.khaparde@broadcom.com>
---
 app/test-pmd/testpmd.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index fe6450c..1c8fb46 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -380,6 +380,8 @@ static const char * const eth_event_desc[] = {
 	[RTE_ETH_EVENT_NEW] = "device probed",
 	[RTE_ETH_EVENT_DESTROY] = "device released",
 	[RTE_ETH_EVENT_FLOW_AGED] = "flow aged",
+	[RTE_ETH_EVENT_RESET] = "device reset",
+	[RTE_ETH_EVENT_RECOVERED] = "device recovery",
 	[RTE_ETH_EVENT_MAX] = NULL,
 };
 
@@ -394,7 +396,9 @@ uint32_t event_print_mask = (UINT32_C(1) << RTE_ETH_EVENT_UNKNOWN) |
 			    (UINT32_C(1) << RTE_ETH_EVENT_IPSEC) |
 			    (UINT32_C(1) << RTE_ETH_EVENT_MACSEC) |
 			    (UINT32_C(1) << RTE_ETH_EVENT_INTR_RMV) |
-			    (UINT32_C(1) << RTE_ETH_EVENT_FLOW_AGED);
+			    (UINT32_C(1) << RTE_ETH_EVENT_FLOW_AGED) |
+			    (UINT32_C(1) << RTE_ETH_EVENT_RESET) |
+			    (UINT32_C(1) << RTE_ETH_EVENT_RECOVERED);
 /*
  * Decide if all memory are locked for performance.
  */
-- 
2.10.1

[dpdk-dev] [RFC PATCH v2 0/3] librte_ethdev: error recovery support

[Kalesh A P]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

The error recovery solution is a protocol implemented between firmware
and bnxt PMD to recover from the fatal errors without a system reboot.
There is an alarm thread which constantly monitors the health of the
firmware and initiates a recovery when needed.

There are two scenarios here:

1. Hardware or firmware encountered an error which firmware detected.
   Firmware is in operational status here. In this case, firmware can
   reset the chip and notify the driver about the reset.
2. Hardware or firmware encountered an error but firmware is dead/hung.
   Firmware is not in operational status. In this case, the only possible
   way to recover the adapter is through host driver(bnxt PMD).

In both cases, bnxt PMD reinitializes with the FW again after the reset.
During that recovery process, data path will be halted and any control path
operation would fail. So, the PMD has to notify the application about this
reset/error event to prevent any activities from the application while
the PMD is recovering from the error.

This patch set adds support for the reset and recovery event in
the rte_eth_event framework. FW error and FW reset conditions would be
managed by the PMD. Driver uses RTE_ETH_EVENT_RESET event to notify
the applications about the FW reset or error. In such cases,
PMD use the RTE_ETH_EVENT_RECOVERED event to notify application about
PMD has recovered from FW reset or FW error.

V2: Added a new event RTE_ETH_EVENT_RESET instead of using the
RTE_ETH_EVENT_INTR_RESET to notify applications about device reset.

Kalesh AP (3):
  ethdev: support device reset and recovery events
  net/bnxt: notify applications about device reset/recovery
  app/testpmd: handle device recovery event

 app/test-pmd/testpmd.c         | 6 +++++-
 drivers/net/bnxt/bnxt_cpr.c    | 3 +++
 drivers/net/bnxt/bnxt_ethdev.c | 9 +++++++++
 lib/librte_ethdev/rte_ethdev.h | 2 ++
 4 files changed, 19 insertions(+), 1 deletion(-)

-- 
2.10.1

[dpdk-dev] [RFC PATCH v2 1/3] ethdev: support device reset and recovery events

[Kalesh A P]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

Adding support for device reset and recovery events in the
rte_eth_event framework. FW error and FW reset conditions would be
managed internally by PMD without needing application intervention.
In such cases, PMD would need reset/recovery events to notify application
that PMD is undergoing a reset.

Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
---
 lib/librte_ethdev/rte_ethdev.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index 9759f13..b9dd14c 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -3207,6 +3207,8 @@ enum rte_eth_event_type {
 	RTE_ETH_EVENT_DESTROY,  /**< port is released */
 	RTE_ETH_EVENT_IPSEC,    /**< IPsec offload related event */
 	RTE_ETH_EVENT_FLOW_AGED,/**< New aged-out flows is detected */
+	RTE_ETH_EVENT_RESET,    /**< port resetting from an error */
+	RTE_ETH_EVENT_RECOVERED, /**< port recovered from an error */
 	RTE_ETH_EVENT_MAX       /**< max value of this enum */
 };
 
-- 
2.10.1

[dpdk-dev] [RFC PATCH v2 2/3] net/bnxt: notify applications about device reset/recovery

[Kalesh A P]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

When the driver receives RESET_NOTIFY async event from FW or detected
an error condition, it should update the application that FW is going
to reset. Once the driver recoveres from the reset, update the reset
recovery status to application as well.

The recovery process is transparent to the application as the driver
itself tries to recover from FW reset or FW error conditions.

Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Signed-off-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
---
 drivers/net/bnxt/bnxt_cpr.c    | 3 +++
 drivers/net/bnxt/bnxt_ethdev.c | 9 +++++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/net/bnxt/bnxt_cpr.c b/drivers/net/bnxt/bnxt_cpr.c
index 8311e26..f2fddf7 100644
--- a/drivers/net/bnxt/bnxt_cpr.c
+++ b/drivers/net/bnxt/bnxt_cpr.c
@@ -129,6 +129,9 @@ void bnxt_handle_async_event(struct bnxt *bp,
 			bp->flags |= BNXT_FLAG_FATAL_ERROR;
 			return;
 		}
+		_rte_eth_dev_callback_process(bp->eth_dev,
+					      RTE_ETH_EVENT_RESET,
+					      NULL);
 
 		event_data = rte_le_to_cpu_32(async_cmp->event_data1);
 		/* timestamp_lo/hi values are in units of 100ms */
diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
index b99c712..609bf46 100644
--- a/drivers/net/bnxt/bnxt_ethdev.c
+++ b/drivers/net/bnxt/bnxt_ethdev.c
@@ -4566,6 +4566,9 @@ static void bnxt_dev_recover(void *arg)
 		goto err_start;
 
 	PMD_DRV_LOG(INFO, "Recovered from FW reset\n");
+	_rte_eth_dev_callback_process(bp->eth_dev,
+				      RTE_ETH_EVENT_RECOVERED,
+				      NULL);
 	return;
 err_start:
 	bnxt_dev_stop_op(bp->eth_dev);
@@ -4573,6 +4576,9 @@ static void bnxt_dev_recover(void *arg)
 	bp->flags |= BNXT_FLAG_FATAL_ERROR;
 	bnxt_uninit_resources(bp, false);
 	PMD_DRV_LOG(ERR, "Failed to recover from FW reset\n");
+	_rte_eth_dev_callback_process(bp->eth_dev,
+				      RTE_ETH_EVENT_INTR_RMV,
+				      NULL);
 }
 
 void bnxt_dev_reset_and_resume(void *arg)
@@ -4708,6 +4714,9 @@ static void bnxt_check_fw_health(void *arg)
 	bp->flags |= BNXT_FLAG_FW_RESET;
 
 	PMD_DRV_LOG(ERR, "Detected FW dead condition\n");
+	_rte_eth_dev_callback_process(bp->eth_dev,
+				      RTE_ETH_EVENT_RESET,
+				      NULL);
 
 	if (bnxt_is_master_func(bp))
 		wait_msec = info->master_func_wait_period;
-- 
2.10.1

[dpdk-dev] [RFC PATCH v2 3/3] app/testpmd: handle device recovery event

[Kalesh A P]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

Added code to handle device reset and recovery event in testpmd.
This is an indication from the PMD that device has resetted and
recovered error condition.

Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Reviewed-by: Ajit Kumar Khaparde <ajit.khaparde@broadcom.com>
---
 app/test-pmd/testpmd.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index fe6450c..1c8fb46 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -380,6 +380,8 @@ static const char * const eth_event_desc[] = {
 	[RTE_ETH_EVENT_NEW] = "device probed",
 	[RTE_ETH_EVENT_DESTROY] = "device released",
 	[RTE_ETH_EVENT_FLOW_AGED] = "flow aged",
+	[RTE_ETH_EVENT_RESET] = "device reset",
+	[RTE_ETH_EVENT_RECOVERED] = "device recovery",
 	[RTE_ETH_EVENT_MAX] = NULL,
 };
 
@@ -394,7 +396,9 @@ uint32_t event_print_mask = (UINT32_C(1) << RTE_ETH_EVENT_UNKNOWN) |
 			    (UINT32_C(1) << RTE_ETH_EVENT_IPSEC) |
 			    (UINT32_C(1) << RTE_ETH_EVENT_MACSEC) |
 			    (UINT32_C(1) << RTE_ETH_EVENT_INTR_RMV) |
-			    (UINT32_C(1) << RTE_ETH_EVENT_FLOW_AGED);
+			    (UINT32_C(1) << RTE_ETH_EVENT_FLOW_AGED) |
+			    (UINT32_C(1) << RTE_ETH_EVENT_RESET) |
+			    (UINT32_C(1) << RTE_ETH_EVENT_RECOVERED);
 /*
  * Decide if all memory are locked for performance.
  */
-- 
2.10.1

[dpdk-dev] [RFC PATCH v3 0/3] librte_ethdev: error recovery support

[Kalesh A P]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

The error recovery solution is a protocol implemented between firmware
and bnxt PMD to recover from the fatal errors without a system reboot.
There is an alarm thread which constantly monitors the health of the
firmware and initiates a recovery when needed.

There are two scenarios here:

1. Hardware or firmware encountered an error which firmware detected.
   Firmware is in operational status here. In this case, firmware can
   reset the chip and notify the driver about the reset.
2. Hardware or firmware encountered an error but firmware is dead/hung.
   Firmware is not in operational status. In this case, the only possible
   way to recover the adapter is through host driver(bnxt PMD).

In both cases, bnxt PMD reinitializes with the FW again after the reset.
During that recovery process, data path will be halted and any control path
operation would fail. So, the PMD has to notify the application about this
reset/error event to prevent any activities from the application while
the PMD is recovering from the error.

This patch set adds support for the reset and recovery event in
the rte_eth_event framework. FW error and FW reset conditions would be
managed by the PMD. Driver uses RTE_ETH_EVENT_RESET event to notify
the applications about the FW reset or error. In such cases,
PMD use the RTE_ETH_EVENT_RECOVERED event to notify application about
PMD has recovered from FW reset or FW error.

V3: fixed a typo in commit log.
V2: Added a new event RTE_ETH_EVENT_RESET instead of using the
RTE_ETH_EVENT_INTR_RESET to notify applications about device reset.

Kalesh AP (3):
  ethdev: support device reset and recovery events
  net/bnxt: notify applications about device reset/recovery
  app/testpmd: handle device recovery event

 app/test-pmd/testpmd.c         | 6 +++++-
 drivers/net/bnxt/bnxt_cpr.c    | 3 +++
 drivers/net/bnxt/bnxt_ethdev.c | 9 +++++++++
 lib/librte_ethdev/rte_ethdev.h | 2 ++
 4 files changed, 19 insertions(+), 1 deletion(-)

-- 
2.10.1

[dpdk-dev] [RFC PATCH v3 1/3] ethdev: support device reset and recovery events

[Kalesh A P]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

Adding support for device reset and recovery events in the
rte_eth_event framework. FW error and FW reset conditions would be
managed internally by PMD without needing application intervention.
In such cases, PMD would need reset/recovery events to notify application
that PMD is undergoing a reset.

Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
---
 lib/librte_ethdev/rte_ethdev.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index 9759f13..b9dd14c 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -3207,6 +3207,8 @@ enum rte_eth_event_type {
 	RTE_ETH_EVENT_DESTROY,  /**< port is released */
 	RTE_ETH_EVENT_IPSEC,    /**< IPsec offload related event */
 	RTE_ETH_EVENT_FLOW_AGED,/**< New aged-out flows is detected */
+	RTE_ETH_EVENT_RESET,    /**< port resetting from an error */
+	RTE_ETH_EVENT_RECOVERED, /**< port recovered from an error */
 	RTE_ETH_EVENT_MAX       /**< max value of this enum */
 };
 
-- 
2.10.1

[dpdk-dev] [RFC PATCH v3 2/3] net/bnxt: notify applications about device reset/recovery

[Kalesh A P]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

When the driver receives RESET_NOTIFY async event from FW or detected
an error condition, it should update the application that FW is going
to reset. Once the driver recoveres from the reset, update the reset
recovery status to application as well.

The recovery process is transparent to the application as the driver
itself tries to recover from FW reset or FW error conditions.

Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Signed-off-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
---
 drivers/net/bnxt/bnxt_cpr.c    | 3 +++
 drivers/net/bnxt/bnxt_ethdev.c | 9 +++++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/net/bnxt/bnxt_cpr.c b/drivers/net/bnxt/bnxt_cpr.c
index 8311e26..f2fddf7 100644
--- a/drivers/net/bnxt/bnxt_cpr.c
+++ b/drivers/net/bnxt/bnxt_cpr.c
@@ -129,6 +129,9 @@ void bnxt_handle_async_event(struct bnxt *bp,
 			bp->flags |= BNXT_FLAG_FATAL_ERROR;
 			return;
 		}
+		_rte_eth_dev_callback_process(bp->eth_dev,
+					      RTE_ETH_EVENT_RESET,
+					      NULL);
 
 		event_data = rte_le_to_cpu_32(async_cmp->event_data1);
 		/* timestamp_lo/hi values are in units of 100ms */
diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
index b99c712..609bf46 100644
--- a/drivers/net/bnxt/bnxt_ethdev.c
+++ b/drivers/net/bnxt/bnxt_ethdev.c
@@ -4566,6 +4566,9 @@ static void bnxt_dev_recover(void *arg)
 		goto err_start;
 
 	PMD_DRV_LOG(INFO, "Recovered from FW reset\n");
+	_rte_eth_dev_callback_process(bp->eth_dev,
+				      RTE_ETH_EVENT_RECOVERED,
+				      NULL);
 	return;
 err_start:
 	bnxt_dev_stop_op(bp->eth_dev);
@@ -4573,6 +4576,9 @@ static void bnxt_dev_recover(void *arg)
 	bp->flags |= BNXT_FLAG_FATAL_ERROR;
 	bnxt_uninit_resources(bp, false);
 	PMD_DRV_LOG(ERR, "Failed to recover from FW reset\n");
+	_rte_eth_dev_callback_process(bp->eth_dev,
+				      RTE_ETH_EVENT_INTR_RMV,
+				      NULL);
 }
 
 void bnxt_dev_reset_and_resume(void *arg)
@@ -4708,6 +4714,9 @@ static void bnxt_check_fw_health(void *arg)
 	bp->flags |= BNXT_FLAG_FW_RESET;
 
 	PMD_DRV_LOG(ERR, "Detected FW dead condition\n");
+	_rte_eth_dev_callback_process(bp->eth_dev,
+				      RTE_ETH_EVENT_RESET,
+				      NULL);
 
 	if (bnxt_is_master_func(bp))
 		wait_msec = info->master_func_wait_period;
-- 
2.10.1

[dpdk-dev] [RFC PATCH v3 3/3] app/testpmd: handle device recovery event

[Kalesh A P]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

Added code to handle device reset and recovery event in testpmd.
This is an indication from the PMD that device has reset and
recovered error condition.

Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Reviewed-by: Ajit Kumar Khaparde <ajit.khaparde@broadcom.com>
---
 app/test-pmd/testpmd.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index fe6450c..1c8fb46 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -380,6 +380,8 @@ static const char * const eth_event_desc[] = {
 	[RTE_ETH_EVENT_NEW] = "device probed",
 	[RTE_ETH_EVENT_DESTROY] = "device released",
 	[RTE_ETH_EVENT_FLOW_AGED] = "flow aged",
+	[RTE_ETH_EVENT_RESET] = "device reset",
+	[RTE_ETH_EVENT_RECOVERED] = "device recovery",
 	[RTE_ETH_EVENT_MAX] = NULL,
 };
 
@@ -394,7 +396,9 @@ uint32_t event_print_mask = (UINT32_C(1) << RTE_ETH_EVENT_UNKNOWN) |
 			    (UINT32_C(1) << RTE_ETH_EVENT_IPSEC) |
 			    (UINT32_C(1) << RTE_ETH_EVENT_MACSEC) |
 			    (UINT32_C(1) << RTE_ETH_EVENT_INTR_RMV) |
-			    (UINT32_C(1) << RTE_ETH_EVENT_FLOW_AGED);
+			    (UINT32_C(1) << RTE_ETH_EVENT_FLOW_AGED) |
+			    (UINT32_C(1) << RTE_ETH_EVENT_RESET) |
+			    (UINT32_C(1) << RTE_ETH_EVENT_RECOVERED);
 /*
  * Decide if all memory are locked for performance.
  */
-- 
2.10.1

Re: [dpdk-dev] [RFC PATCH v3 1/3] ethdev: support device reset and recovery events

[Thomas Monjalon]

Hi,

Please use --cc-cmd devtools/get-maintainer.sh so all relevant
maintainers are Cc'ed. Adding Andrew.

> From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> 
> Adding support for device reset and recovery events in the
> rte_eth_event framework. FW error and FW reset conditions would be
> managed internally by PMD without needing application intervention.
> In such cases, PMD would need reset/recovery events to notify application
> that PMD is undergoing a reset.

We already have this event:
	
    RTE_ETH_EVENT_INTR_RESET,
            /**< reset interrupt event, sent to VF on PF reset */

I don't know why "INTR" is in the name of this event,
and I think it does not need to be restricted to VF.
The application does not need to know whether the reset
is caused by the PF, the FW or the HW.
That's why I think you could share the same event.

> +       RTE_ETH_EVENT_RESET,    /**< port resetting from an error */
> +       RTE_ETH_EVENT_RECOVERED, /**< port recovered from an error */

You ignored my previous comments:
"
What the application is supposed to do when receiving such event?
How the application knows that flow rules were resetted?
Is there any other configuration resetted?
These informations must be explicit in the doxygen comments.
"

Re: [dpdk-dev] [RFC PATCH v3 1/3] ethdev: support device reset and recovery events

[Kalesh Anakkur Purayil]

Hi Thomas,

Please see my response inline.

Regards,
Kalesh

On Wed, Sep 30, 2020 at 1:21 PM Thomas Monjalon <thomas@monjalon.net> wrote:

> Hi,
>
> Please use --cc-cmd devtools/get-maintainer.sh so all relevant
> maintainers are Cc'ed. Adding Andrew.
>
[Kalesh]: Thank you. Sure, I will follow next time.

>
> > From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> >
> > Adding support for device reset and recovery events in the
> > rte_eth_event framework. FW error and FW reset conditions would be
> > managed internally by PMD without needing application intervention.
> > In such cases, PMD would need reset/recovery events to notify application
> > that PMD is undergoing a reset.
>
> We already have this event:
>
>     RTE_ETH_EVENT_INTR_RESET,
>             /**< reset interrupt event, sent to VF on PF reset */
>
> I don't know why "INTR" is in the name of this event,
> and I think it does not need to be restricted to VF.
> The application does not need to know whether the reset
> is caused by the PF, the FW or the HW.
> That's why I think you could share the same event.
>

[Kalesh]: Yes. As you mentioned, this event is used for some other purpose.
I did not want to break the existing usage/purpose of this event.
For example, upon receiving the RTE_ETH_EVENT_INTR_RESET event OVS
application invokes rte_eth_dev_reset() to reset the port.
The aim here is to recover from the device error condition without the
intervention of Applications. PMD itself will recover from the error using
the protocol with FW.

>
> > +       RTE_ETH_EVENT_RESET,    /**< port resetting from an error */
> > +       RTE_ETH_EVENT_RECOVERED, /**< port recovered from an error */
>
> You ignored my previous comments:
> "
> What the application is supposed to do when receiving such event?
> How the application knows that flow rules were resetted?
> Is there any other configuration resetted?
> These informations must be explicit in the doxygen comments.
> "
>
[Kalesh]: Sorry, I missed it.
I am not sure what you meant by "These information must be explicit in the
doxygen comments ".
Could you please elaborate a little how to/where to put these details?


-- 
Regards,
Kalesh A P

Re: [dpdk-dev] [RFC PATCH v3 1/3] ethdev: support device reset and recovery events

[Thomas Monjalon]

30/09/2020 10:35, Kalesh Anakkur Purayil:
> On Wed, Sep 30, 2020 at 1:21 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > > From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> > >
> > > Adding support for device reset and recovery events in the
> > > rte_eth_event framework. FW error and FW reset conditions would be
> > > managed internally by PMD without needing application intervention.
> > > In such cases, PMD would need reset/recovery events to notify application
> > > that PMD is undergoing a reset.
> >
> > We already have this event:
> >
> >     RTE_ETH_EVENT_INTR_RESET,
> >             /**< reset interrupt event, sent to VF on PF reset */
> >
> > I don't know why "INTR" is in the name of this event,
> > and I think it does not need to be restricted to VF.
> > The application does not need to know whether the reset
> > is caused by the PF, the FW or the HW.
> > That's why I think you could share the same event.
> >
> 
> [Kalesh]: Yes. As you mentioned, this event is used for some other purpose.
> I did not want to break the existing usage/purpose of this event.
> For example, upon receiving the RTE_ETH_EVENT_INTR_RESET event OVS
> application invokes rte_eth_dev_reset() to reset the port.
> The aim here is to recover from the device error condition without the
> intervention of Applications. PMD itself will recover from the error using
> the protocol with FW.
> 
> >
> > > +       RTE_ETH_EVENT_RESET,    /**< port resetting from an error */
> > > +       RTE_ETH_EVENT_RECOVERED, /**< port recovered from an error */
> >
> > You ignored my previous comments:
> > "
> > What the application is supposed to do when receiving such event?
> > How the application knows that flow rules were resetted?
> > Is there any other configuration resetted?
> > These informations must be explicit in the doxygen comments.
> > "
> >
> [Kalesh]: Sorry, I missed it.
> I am not sure what you meant by "These information must be explicit in the
> doxygen comments ".
> Could you please elaborate a little how to/where to put these details?

/** is the start of a doxygen comment.
This is the place (in the .h file) to explain to application
developer what to do with the event.
The code + the comments is what we call "the API".

You should complete the description of RTE_ETH_EVENT_INTR_RESET
as well: the need for calling rte_eth_dev_reset() was not explicit.

[dpdk-dev] [RFC PATCH v4 0/3] librte_ethdev: error recovery support

[Kalesh A P]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

The error recovery solution is a protocol implemented between firmware
and bnxt PMD to recover from the fatal errors without a system reboot.
There is an alarm thread which constantly monitors the health of the
firmware and initiates a recovery when needed.

There are two scenarios here:

1. Hardware or firmware encountered an error which firmware detected.
   Firmware is in operational status here. In this case, firmware can
   reset the chip and notify the driver about the reset.
2. Hardware or firmware encountered an error but firmware is dead/hung.
   Firmware is not in operational status. In this case, the only possible
   way to recover the adapter is through host driver(bnxt PMD).

In both cases, bnxt PMD reinitializes with the FW again after the reset.
During that recovery process, data path will be halted and any control path
operation would fail. So, the PMD has to notify the application about this
reset/error event to prevent any activities from the application while
the PMD is recovering from the error.

This patch set adds support for the reset and recovery event in
the rte_eth_event framework. FW error and FW reset conditions would be
managed by the PMD. Driver uses RTE_ETH_EVENT_RESET event to notify
the applications about the FW reset or error. In such cases,
PMD use the RTE_ETH_EVENT_RECOVERED event to notify application about
PMD has recovered from FW reset or FW error.

v4: Added doxygen comments about new events.
V3: Fixed a typo in commit log.
V2: Added a new event RTE_ETH_EVENT_RESET instead of using the
RTE_ETH_EVENT_INTR_RESET to notify applications about device reset.

Kalesh AP (3):
  ethdev: support device reset and recovery events
  net/bnxt: notify applications about device reset/recovery
  app/testpmd: handle device recovery event

 app/test-pmd/testpmd.c         |  6 +++++-
 drivers/net/bnxt/bnxt_cpr.c    |  3 +++
 drivers/net/bnxt/bnxt_ethdev.c |  9 +++++++++
 lib/librte_ethdev/rte_ethdev.h | 17 +++++++++++++++++
 4 files changed, 34 insertions(+), 1 deletion(-)

-- 
2.10.1

[dpdk-dev] [RFC PATCH v4 1/3] ethdev: support device reset and recovery events

[Kalesh A P]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

Adding support for device reset and recovery events in the
rte_eth_event framework. FW error and FW reset conditions would be
managed internally by PMD without needing application intervention.
In such cases, PMD would need reset/recovery events to notify application
that PMD is undergoing a reset.

Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
---
 lib/librte_ethdev/rte_ethdev.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index 9759f13..c0c90b7 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -3207,6 +3207,23 @@ enum rte_eth_event_type {
 	RTE_ETH_EVENT_DESTROY,  /**< port is released */
 	RTE_ETH_EVENT_IPSEC,    /**< IPsec offload related event */
 	RTE_ETH_EVENT_FLOW_AGED,/**< New aged-out flows is detected */
+	RTE_ETH_EVENT_RESET,
+			/**< port resetting from an error
+			 *
+			 * PMD detected a FW reset or error condition.
+			 * PMD will try to recover from the error.
+			 * Data path will be halted and Control path operations
+			 * would fail at this time.
+			 */
+	RTE_ETH_EVENT_RECOVERED,
+			/**< port recovered from an error
+			 *
+			 * PMD has recovered from the error condition.
+			 * Control path and Data path are up now.
+			 * Since the device undergone a reset, flow rules
+			 * offloaded prior to the reset will be lost and
+			 * the application has to recreate the rules again.
+			 */
 	RTE_ETH_EVENT_MAX       /**< max value of this enum */
 };
 
-- 
2.10.1

[dpdk-dev] [RFC PATCH v4 2/3] net/bnxt: notify applications about device reset/recovery

[Kalesh A P]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

When the driver receives RESET_NOTIFY async event from FW or detected
an error condition, it should update the application that FW is going
to reset. Once the driver recoveres from the reset, update the reset
recovery status to application as well.

The recovery process is transparent to the application as the driver
itself tries to recover from FW reset or FW error conditions.

Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Signed-off-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
---
 drivers/net/bnxt/bnxt_cpr.c    | 3 +++
 drivers/net/bnxt/bnxt_ethdev.c | 9 +++++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/net/bnxt/bnxt_cpr.c b/drivers/net/bnxt/bnxt_cpr.c
index 8311e26..3b19ef7 100644
--- a/drivers/net/bnxt/bnxt_cpr.c
+++ b/drivers/net/bnxt/bnxt_cpr.c
@@ -129,6 +129,9 @@ void bnxt_handle_async_event(struct bnxt *bp,
 			bp->flags |= BNXT_FLAG_FATAL_ERROR;
 			return;
 		}
+		rte_eth_dev_callback_process(bp->eth_dev,
+					     RTE_ETH_EVENT_RESET,
+					     NULL);
 
 		event_data = rte_le_to_cpu_32(async_cmp->event_data1);
 		/* timestamp_lo/hi values are in units of 100ms */
diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
index b99c712..9a47bfd 100644
--- a/drivers/net/bnxt/bnxt_ethdev.c
+++ b/drivers/net/bnxt/bnxt_ethdev.c
@@ -4566,6 +4566,9 @@ static void bnxt_dev_recover(void *arg)
 		goto err_start;
 
 	PMD_DRV_LOG(INFO, "Recovered from FW reset\n");
+	rte_eth_dev_callback_process(bp->eth_dev,
+				     RTE_ETH_EVENT_RECOVERED,
+				     NULL);
 	return;
 err_start:
 	bnxt_dev_stop_op(bp->eth_dev);
@@ -4573,6 +4576,9 @@ static void bnxt_dev_recover(void *arg)
 	bp->flags |= BNXT_FLAG_FATAL_ERROR;
 	bnxt_uninit_resources(bp, false);
 	PMD_DRV_LOG(ERR, "Failed to recover from FW reset\n");
+	rte_eth_dev_callback_process(bp->eth_dev,
+				     RTE_ETH_EVENT_INTR_RMV,
+				     NULL);
 }
 
 void bnxt_dev_reset_and_resume(void *arg)
@@ -4708,6 +4714,9 @@ static void bnxt_check_fw_health(void *arg)
 	bp->flags |= BNXT_FLAG_FW_RESET;
 
 	PMD_DRV_LOG(ERR, "Detected FW dead condition\n");
+	rte_eth_dev_callback_process(bp->eth_dev,
+				     RTE_ETH_EVENT_RESET,
+				     NULL);
 
 	if (bnxt_is_master_func(bp))
 		wait_msec = info->master_func_wait_period;
-- 
2.10.1

[dpdk-dev] [RFC PATCH v4 3/3] app/testpmd: handle device recovery event

[Kalesh A P]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

Added code to handle device reset and recovery event in testpmd.
This is an indication from the PMD that device has reset and
recovered error condition.

Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Reviewed-by: Ajit Kumar Khaparde <ajit.khaparde@broadcom.com>
---
 app/test-pmd/testpmd.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index fe6450c..1c8fb46 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -380,6 +380,8 @@ static const char * const eth_event_desc[] = {
 	[RTE_ETH_EVENT_NEW] = "device probed",
 	[RTE_ETH_EVENT_DESTROY] = "device released",
 	[RTE_ETH_EVENT_FLOW_AGED] = "flow aged",
+	[RTE_ETH_EVENT_RESET] = "device reset",
+	[RTE_ETH_EVENT_RECOVERED] = "device recovery",
 	[RTE_ETH_EVENT_MAX] = NULL,
 };
 
@@ -394,7 +396,9 @@ uint32_t event_print_mask = (UINT32_C(1) << RTE_ETH_EVENT_UNKNOWN) |
 			    (UINT32_C(1) << RTE_ETH_EVENT_IPSEC) |
 			    (UINT32_C(1) << RTE_ETH_EVENT_MACSEC) |
 			    (UINT32_C(1) << RTE_ETH_EVENT_INTR_RMV) |
-			    (UINT32_C(1) << RTE_ETH_EVENT_FLOW_AGED);
+			    (UINT32_C(1) << RTE_ETH_EVENT_FLOW_AGED) |
+			    (UINT32_C(1) << RTE_ETH_EVENT_RESET) |
+			    (UINT32_C(1) << RTE_ETH_EVENT_RECOVERED);
 /*
  * Decide if all memory are locked for performance.
  */
-- 
2.10.1

Re: [dpdk-dev] [RFC PATCH v4 3/3] app/testpmd: handle device recovery event

[Ophir Munk]

Hi Kalesh,
Please find a few comments.
The name you gave to the event (EVENT_RESET) is very close to an already existing one: "EVENT_INTR_RESET".
But they are different.
EVENT_INTR_RESET originates from a port reset. It requires application reaction. It is widely used. It is documented in *.rst files.
EVENT_RESET originates from FW error (or maybe any error). It requires no application reaction (PMD manages by itself). It is not documented.
I therefore suggest renaming it (maybe EVENT_ERR_RECOVERING) and please document it in *.rst files.
More comments below:

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Kalesh A P
> Sent: Wednesday, September 30, 2020 3:33 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [RFC PATCH v4 3/3] app/testpmd: handle device recovery
> event
> 
> From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> 
> Added code to handle device reset and recovery event in testpmd.
> This is an indication from the PMD that device has reset and recovered error
> condition.
> 
> Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> Reviewed-by: Ajit Kumar Khaparde <ajit.khaparde@broadcom.com>
> ---
>  app/test-pmd/testpmd.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> fe6450c..1c8fb46 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -380,6 +380,8 @@ static const char * const eth_event_desc[] = {
>  	[RTE_ETH_EVENT_NEW] = "device probed",
>  	[RTE_ETH_EVENT_DESTROY] = "device released",
>  	[RTE_ETH_EVENT_FLOW_AGED] = "flow aged",
> +	[RTE_ETH_EVENT_RESET] = "device reset",

"device reset" is similar to the existing "reset" string. Can you suggest a different one? Maybe "error under recovery" ?

> +	[RTE_ETH_EVENT_RECOVERED] = "device recovery",

Wouldn't you prefer "device recovered" ?

>  	[RTE_ETH_EVENT_MAX] = NULL,
>  };
> 
> @@ -394,7 +396,9 @@ uint32_t event_print_mask = (UINT32_C(1) <<
> RTE_ETH_EVENT_UNKNOWN) |
>  			    (UINT32_C(1) << RTE_ETH_EVENT_IPSEC) |
>  			    (UINT32_C(1) << RTE_ETH_EVENT_MACSEC) |
>  			    (UINT32_C(1) << RTE_ETH_EVENT_INTR_RMV) |
> -			    (UINT32_C(1) << RTE_ETH_EVENT_FLOW_AGED);
> +			    (UINT32_C(1) << RTE_ETH_EVENT_FLOW_AGED) |
> +			    (UINT32_C(1) << RTE_ETH_EVENT_RESET) |
> +			    (UINT32_C(1) << RTE_ETH_EVENT_RECOVERED);
>  /*
>   * Decide if all memory are locked for performance.
>   */
> --
> 2.10.1

Re: [dpdk-dev] [RFC PATCH v4 3/3] app/testpmd: handle device recovery event

[Kalesh Anakkur Purayil]

Hi Ophir,

Thank you for the comments. I will address them in the next version.

I will push these changes as Patches next time and not as an RFC. Hope that
is OK.

Regards,
Kalesh

On Tue, Oct 6, 2020 at 10:55 PM Ophir Munk <ophirmu@nvidia.com> wrote:

> Hi Kalesh,
> Please find a few comments.
> The name you gave to the event (EVENT_RESET) is very close to an already
> existing one: "EVENT_INTR_RESET".
> But they are different.
> EVENT_INTR_RESET originates from a port reset. It requires application
> reaction. It is widely used. It is documented in *.rst files.
> EVENT_RESET originates from FW error (or maybe any error). It requires no
> application reaction (PMD manages by itself). It is not documented.
> I therefore suggest renaming it (maybe EVENT_ERR_RECOVERING) and please
> document it in *.rst files.
> More comments below:
>
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Kalesh A P
> > Sent: Wednesday, September 30, 2020 3:33 PM
> > To: dev@dpdk.org
> > Subject: [dpdk-dev] [RFC PATCH v4 3/3] app/testpmd: handle device
> recovery
> > event
> >
> > From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> >
> > Added code to handle device reset and recovery event in testpmd.
> > This is an indication from the PMD that device has reset and recovered
> error
> > condition.
> >
> > Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> > Reviewed-by: Ajit Kumar Khaparde <ajit.khaparde@broadcom.com>
> > ---
> >  app/test-pmd/testpmd.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > fe6450c..1c8fb46 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -380,6 +380,8 @@ static const char * const eth_event_desc[] = {
> >       [RTE_ETH_EVENT_NEW] = "device probed",
> >       [RTE_ETH_EVENT_DESTROY] = "device released",
> >       [RTE_ETH_EVENT_FLOW_AGED] = "flow aged",
> > +     [RTE_ETH_EVENT_RESET] = "device reset",
>
> "device reset" is similar to the existing "reset" string. Can you suggest
> a different one? Maybe "error under recovery" ?
>
> > +     [RTE_ETH_EVENT_RECOVERED] = "device recovery",
>
> Wouldn't you prefer "device recovered" ?
>
> >       [RTE_ETH_EVENT_MAX] = NULL,
> >  };
> >
> > @@ -394,7 +396,9 @@ uint32_t event_print_mask = (UINT32_C(1) <<
> > RTE_ETH_EVENT_UNKNOWN) |
> >                           (UINT32_C(1) << RTE_ETH_EVENT_IPSEC) |
> >                           (UINT32_C(1) << RTE_ETH_EVENT_MACSEC) |
> >                           (UINT32_C(1) << RTE_ETH_EVENT_INTR_RMV) |
> > -                         (UINT32_C(1) << RTE_ETH_EVENT_FLOW_AGED);
> > +                         (UINT32_C(1) << RTE_ETH_EVENT_FLOW_AGED) |
> > +                         (UINT32_C(1) << RTE_ETH_EVENT_RESET) |
> > +                         (UINT32_C(1) << RTE_ETH_EVENT_RECOVERED);
> >  /*
> >   * Decide if all memory are locked for performance.
> >   */
> > --
> > 2.10.1
>
>

-- 
Regards,
Kalesh A P

Re: [dpdk-dev] [RFC PATCH v4 3/3] app/testpmd: handle device recovery event

[Ophir Munk]

Adding Ferruh and Ajit

From: Kalesh Anakkur Purayil <kalesh-anakkur.purayil@broadcom.com>
Sent: Wednesday, October 7, 2020 7:47 AM
To: Ophir Munk <ophirmu@nvidia.com>
Cc: dev@dpdk.org; NBU-Contact-Thomas Monjalon <thomas@monjalon.net>
Subject: Re: [dpdk-dev] [RFC PATCH v4 3/3] app/testpmd: handle device recovery event

Hi Ophir,

Thank you for the comments. I will address them in the next version.

I will push these changes as Patches next time and not as an RFC. Hope that is OK.

Regards,
Kalesh

On Tue, Oct 6, 2020 at 10:55 PM Ophir Munk <ophirmu@nvidia.com<mailto:ophirmu@nvidia.com>> wrote:
Hi Kalesh,
Please find a few comments.
The name you gave to the event (EVENT_RESET) is very close to an already existing one: "EVENT_INTR_RESET".
But they are different.
EVENT_INTR_RESET originates from a port reset. It requires application reaction. It is widely used. It is documented in *.rst files.
EVENT_RESET originates from FW error (or maybe any error). It requires no application reaction (PMD manages by itself). It is not documented.
I therefore suggest renaming it (maybe EVENT_ERR_RECOVERING) and please document it in *.rst files.
More comments below:

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org<mailto:dev-bounces@dpdk.org>> On Behalf Of Kalesh A P
> Sent: Wednesday, September 30, 2020 3:33 PM
> To: dev@dpdk.org<mailto:dev@dpdk.org>
> Subject: [dpdk-dev] [RFC PATCH v4 3/3] app/testpmd: handle device recovery
> event
>
> From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com<mailto:kalesh-anakkur.purayil@broadcom.com>>
>
> Added code to handle device reset and recovery event in testpmd.
> This is an indication from the PMD that device has reset and recovered error
> condition.
>
> Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com<mailto:kalesh-anakkur.purayil@broadcom.com>>
> Reviewed-by: Ajit Kumar Khaparde <ajit.khaparde@broadcom.com<mailto:ajit.khaparde@broadcom.com>>
> ---
>  app/test-pmd/testpmd.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> fe6450c..1c8fb46 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -380,6 +380,8 @@ static const char * const eth_event_desc[] = {
>       [RTE_ETH_EVENT_NEW] = "device probed",
>       [RTE_ETH_EVENT_DESTROY] = "device released",
>       [RTE_ETH_EVENT_FLOW_AGED] = "flow aged",
> +     [RTE_ETH_EVENT_RESET] = "device reset",

"device reset" is similar to the existing "reset" string. Can you suggest a different one? Maybe "error under recovery" ?

> +     [RTE_ETH_EVENT_RECOVERED] = "device recovery",

Wouldn't you prefer "device recovered" ?

>       [RTE_ETH_EVENT_MAX] = NULL,
>  };
>
> @@ -394,7 +396,9 @@ uint32_t event_print_mask = (UINT32_C(1) <<
> RTE_ETH_EVENT_UNKNOWN) |
>                           (UINT32_C(1) << RTE_ETH_EVENT_IPSEC) |
>                           (UINT32_C(1) << RTE_ETH_EVENT_MACSEC) |
>                           (UINT32_C(1) << RTE_ETH_EVENT_INTR_RMV) |
> -                         (UINT32_C(1) << RTE_ETH_EVENT_FLOW_AGED);
> +                         (UINT32_C(1) << RTE_ETH_EVENT_FLOW_AGED) |
> +                         (UINT32_C(1) << RTE_ETH_EVENT_RESET) |
> +                         (UINT32_C(1) << RTE_ETH_EVENT_RECOVERED);
>  /*
>   * Decide if all memory are locked for performance.
>   */
> --
> 2.10.1


--
Regards,
Kalesh A P

Re: [dpdk-dev] [RFC PATCH v4 3/3] app/testpmd: handle device recovery event

[Ferruh Yigit]

On 10/7/2020 5:46 AM, Kalesh Anakkur Purayil wrote:
> Hi Ophir,
> 
> Thank you for the comments. I will address them in the next version.
> 
> I will push these changes as Patches next time and not as an RFC. Hope that
> is OK.
> 
> Regards,
> Kalesh
> 
> On Tue, Oct 6, 2020 at 10:55 PM Ophir Munk <ophirmu@nvidia.com> wrote:
> 
>> Hi Kalesh,
>> Please find a few comments.
>> The name you gave to the event (EVENT_RESET) is very close to an already
>> existing one: "EVENT_INTR_RESET".
>> But they are different.
>> EVENT_INTR_RESET originates from a port reset. It requires application
>> reaction. It is widely used. It is documented in *.rst files.
>> EVENT_RESET originates from FW error (or maybe any error). It requires no
>> application reaction (PMD manages by itself). It is not documented.
>> I therefore suggest renaming it (maybe EVENT_ERR_RECOVERING) and please
>> document it in *.rst files.

+1 to renaming and documenting the event.

And agree to proceed as regular patch instead of RFC.


>> More comments below:
>>
>>> -----Original Message-----
>>> From: dev <dev-bounces@dpdk.org> On Behalf Of Kalesh A P
>>> Sent: Wednesday, September 30, 2020 3:33 PM
>>> To: dev@dpdk.org
>>> Subject: [dpdk-dev] [RFC PATCH v4 3/3] app/testpmd: handle device
>> recovery
>>> event
>>>
>>> From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
>>>
>>> Added code to handle device reset and recovery event in testpmd.
>>> This is an indication from the PMD that device has reset and recovered
>> error
>>> condition.
>>>
>>> Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
>>> Reviewed-by: Ajit Kumar Khaparde <ajit.khaparde@broadcom.com>
>>> ---
>>>   app/test-pmd/testpmd.c | 6 +++++-
>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>>> fe6450c..1c8fb46 100644
>>> --- a/app/test-pmd/testpmd.c
>>> +++ b/app/test-pmd/testpmd.c
>>> @@ -380,6 +380,8 @@ static const char * const eth_event_desc[] = {
>>>        [RTE_ETH_EVENT_NEW] = "device probed",
>>>        [RTE_ETH_EVENT_DESTROY] = "device released",
>>>        [RTE_ETH_EVENT_FLOW_AGED] = "flow aged",
>>> +     [RTE_ETH_EVENT_RESET] = "device reset",
>>
>> "device reset" is similar to the existing "reset" string. Can you suggest
>> a different one? Maybe "error under recovery" ?
>>
>>> +     [RTE_ETH_EVENT_RECOVERED] = "device recovery",
>>
>> Wouldn't you prefer "device recovered" ?
>>
>>>        [RTE_ETH_EVENT_MAX] = NULL,
>>>   };
>>>
>>> @@ -394,7 +396,9 @@ uint32_t event_print_mask = (UINT32_C(1) <<
>>> RTE_ETH_EVENT_UNKNOWN) |
>>>                            (UINT32_C(1) << RTE_ETH_EVENT_IPSEC) |
>>>                            (UINT32_C(1) << RTE_ETH_EVENT_MACSEC) |
>>>                            (UINT32_C(1) << RTE_ETH_EVENT_INTR_RMV) |
>>> -                         (UINT32_C(1) << RTE_ETH_EVENT_FLOW_AGED);
>>> +                         (UINT32_C(1) << RTE_ETH_EVENT_FLOW_AGED) |
>>> +                         (UINT32_C(1) << RTE_ETH_EVENT_RESET) |
>>> +                         (UINT32_C(1) << RTE_ETH_EVENT_RECOVERED);
>>>   /*
>>>    * Decide if all memory are locked for performance.
>>>    */
>>> --
>>> 2.10.1
>>
>>
> 

[dpdk-dev] [PATCH v5 0/3] librte_ethdev: error recovery support

[Kalesh A P]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

The error recovery solution is a protocol implemented between firmware
and bnxt PMD to recover from the fatal errors without a system reboot.
There is an alarm thread which constantly monitors the health of the
firmware and initiates a recovery when needed.

There are two scenarios here:

1. Hardware or firmware encountered an error which firmware detected.
   Firmware is in operational status here. In this case, firmware can
   reset the chip and notify the driver about the reset.
2. Hardware or firmware encountered an error but firmware is dead/hung.
   Firmware is not in operational status. In this case, the only possible
   way to recover the adapter is through host driver(bnxt PMD).

In both cases, bnxt PMD reinitializes with the FW again after the reset.
During that recovery process, data path will be halted and any control path
operation would fail. So, the PMD has to notify the application about this
reset/error event to prevent any activities from the application while
the PMD is recovering from the error.

This patch set adds support for the reset and recovery event in
the rte_eth_event framework. FW error and FW reset conditions would be
managed by the PMD. Driver uses RTE_ETH_EVENT_RESET event to notify
the applications about the FW reset or error. In such cases,
PMD use the RTE_ETH_EVENT_RECOVERED event to notify application about
PMD has recovered from FW reset or FW error.

v5: Addressed comments from Ophir.
    1. Renamed the new event name to RTE_ETH_EVENT_ERR_RECOVERING.
    2. Fixed testpmd logs.
    3. Documented the new recovery events.
v4: Added doxygen comments about new events.
V3: Fixed a typo in commit log.
V2: Added a new event RTE_ETH_EVENT_RESET instead of using the
    RTE_ETH_EVENT_INTR_RESET to notify applications about device reset.

Kalesh AP (3):
  ethdev: support device reset and recovery events
  net/bnxt: notify applications about device reset/recovery
  app/testpmd: handle device recovery event

 app/test-pmd/testpmd.c                  |  6 +++++-
 doc/guides/prog_guide/poll_mode_drv.rst | 18 ++++++++++++++++++
 drivers/net/bnxt/bnxt_cpr.c             |  3 +++
 drivers/net/bnxt/bnxt_ethdev.c          |  9 +++++++++
 lib/librte_ethdev/rte_ethdev.h          | 17 +++++++++++++++++
 5 files changed, 52 insertions(+), 1 deletion(-)

-- 
2.10.1

[dpdk-dev] [PATCH v5 1/3] ethdev: support device reset and recovery events

[Kalesh A P]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

Adding support for device reset and recovery events in the
rte_eth_event framework. FW error and FW reset conditions would be
managed internally by PMD without needing application intervention.
In such cases, PMD would need reset/recovery events to notify application
that PMD is undergoing a reset.

Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
---
 doc/guides/prog_guide/poll_mode_drv.rst | 18 ++++++++++++++++++
 lib/librte_ethdev/rte_ethdev.h          | 17 +++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/doc/guides/prog_guide/poll_mode_drv.rst b/doc/guides/prog_guide/poll_mode_drv.rst
index 86e0a14..c03f0ef 100644
--- a/doc/guides/prog_guide/poll_mode_drv.rst
+++ b/doc/guides/prog_guide/poll_mode_drv.rst
@@ -615,3 +615,21 @@ by application.
 The PMD itself should not call rte_eth_dev_reset(). The PMD can trigger
 the application to handle reset event. It is duty of application to
 handle all synchronization before it calls rte_eth_dev_reset().
+
+Error recovery support
+~~~~~~~~~~~~~~~~~~~~~~
+
+When the PMD detects a FW reset or error condition, it will try to recover
+from the error without needing the application intervention. In such cases,
+PMD would need events to notify the application that it is undergoing
+an error recovery.
+
+The PMD will trigger RTE_ETH_EVENT_ERR_RECOVERING event to notify the
+application that PMD detected a FW reset or FW error condition. PMD will
+try to recover from the error by itself. Data path will be halted and
+control path operations would fail during the recovery period.
+
+The PMD will trigger RTE_ETH_EVENT_RECOVERED event to notify the application
+that the it has recovered from the error condition. Control path and data path
+are up now. Since the device undergone a reset, flow rules offloaded prior to
+the reset will be lost and the application has to recreate the rules again.
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index 9759f13..9b4b015 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -3207,6 +3207,23 @@ enum rte_eth_event_type {
 	RTE_ETH_EVENT_DESTROY,  /**< port is released */
 	RTE_ETH_EVENT_IPSEC,    /**< IPsec offload related event */
 	RTE_ETH_EVENT_FLOW_AGED,/**< New aged-out flows is detected */
+	RTE_ETH_EVENT_ERR_RECOVERING,
+			/**< port recovering from an error
+			 *
+			 * PMD detected a FW reset or error condition.
+			 * PMD will try to recover from the error.
+			 * Data path will be halted and Control path operations
+			 * would fail at this time.
+			 */
+	RTE_ETH_EVENT_RECOVERED,
+			/**< port recovered from an error
+			 *
+			 * PMD has recovered from the error condition.
+			 * Control path and Data path are up now.
+			 * Since the device undergone a reset, flow rules
+			 * offloaded prior to the reset will be lost and
+			 * the application has to recreate the rules again.
+			 */
 	RTE_ETH_EVENT_MAX       /**< max value of this enum */
 };
 
-- 
2.10.1

[dpdk-dev] [PATCH v5 2/3] net/bnxt: notify applications about device reset/recovery

[Kalesh A P]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

When the driver receives RESET_NOTIFY async event from FW or detected
an error condition, it should update the application that FW is going
to reset. Once the driver recoveres from the reset, update the reset
recovery status to application as well.

The recovery process is transparent to the application as the driver
itself tries to recover from FW reset or FW error conditions.

Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Signed-off-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
---
 drivers/net/bnxt/bnxt_cpr.c    | 3 +++
 drivers/net/bnxt/bnxt_ethdev.c | 9 +++++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/net/bnxt/bnxt_cpr.c b/drivers/net/bnxt/bnxt_cpr.c
index 8311e26..987c010 100644
--- a/drivers/net/bnxt/bnxt_cpr.c
+++ b/drivers/net/bnxt/bnxt_cpr.c
@@ -129,6 +129,9 @@ void bnxt_handle_async_event(struct bnxt *bp,
 			bp->flags |= BNXT_FLAG_FATAL_ERROR;
 			return;
 		}
+		rte_eth_dev_callback_process(bp->eth_dev,
+					     RTE_ETH_EVENT_ERR_RECOVERING,
+					     NULL);
 
 		event_data = rte_le_to_cpu_32(async_cmp->event_data1);
 		/* timestamp_lo/hi values are in units of 100ms */
diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
index b99c712..e3798de 100644
--- a/drivers/net/bnxt/bnxt_ethdev.c
+++ b/drivers/net/bnxt/bnxt_ethdev.c
@@ -4566,6 +4566,9 @@ static void bnxt_dev_recover(void *arg)
 		goto err_start;
 
 	PMD_DRV_LOG(INFO, "Recovered from FW reset\n");
+	rte_eth_dev_callback_process(bp->eth_dev,
+				     RTE_ETH_EVENT_RECOVERED,
+				     NULL);
 	return;
 err_start:
 	bnxt_dev_stop_op(bp->eth_dev);
@@ -4573,6 +4576,9 @@ static void bnxt_dev_recover(void *arg)
 	bp->flags |= BNXT_FLAG_FATAL_ERROR;
 	bnxt_uninit_resources(bp, false);
 	PMD_DRV_LOG(ERR, "Failed to recover from FW reset\n");
+	rte_eth_dev_callback_process(bp->eth_dev,
+				     RTE_ETH_EVENT_INTR_RMV,
+				     NULL);
 }
 
 void bnxt_dev_reset_and_resume(void *arg)
@@ -4708,6 +4714,9 @@ static void bnxt_check_fw_health(void *arg)
 	bp->flags |= BNXT_FLAG_FW_RESET;
 
 	PMD_DRV_LOG(ERR, "Detected FW dead condition\n");
+	rte_eth_dev_callback_process(bp->eth_dev,
+				     RTE_ETH_EVENT_ERR_RECOVERING,
+				     NULL);
 
 	if (bnxt_is_master_func(bp))
 		wait_msec = info->master_func_wait_period;
-- 
2.10.1

[dpdk-dev] [PATCH v5 3/3] app/testpmd: handle device recovery event

[Kalesh A P]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

Added code to handle error recovery events in testpmd.
This is an indication from the PMD that it is undergoing
an error recovery and recovered from the error condition.

Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
Reviewed-by: Ajit Kumar Khaparde <ajit.khaparde@broadcom.com>
---
 app/test-pmd/testpmd.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index fe6450c..80ae3fa 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -380,6 +380,8 @@ static const char * const eth_event_desc[] = {
 	[RTE_ETH_EVENT_NEW] = "device probed",
 	[RTE_ETH_EVENT_DESTROY] = "device released",
 	[RTE_ETH_EVENT_FLOW_AGED] = "flow aged",
+	[RTE_ETH_EVENT_ERR_RECOVERING] = "device error under recovery",
+	[RTE_ETH_EVENT_RECOVERED] = "device recovered",
 	[RTE_ETH_EVENT_MAX] = NULL,
 };
 
@@ -394,7 +396,9 @@ uint32_t event_print_mask = (UINT32_C(1) << RTE_ETH_EVENT_UNKNOWN) |
 			    (UINT32_C(1) << RTE_ETH_EVENT_IPSEC) |
 			    (UINT32_C(1) << RTE_ETH_EVENT_MACSEC) |
 			    (UINT32_C(1) << RTE_ETH_EVENT_INTR_RMV) |
-			    (UINT32_C(1) << RTE_ETH_EVENT_FLOW_AGED);
+			    (UINT32_C(1) << RTE_ETH_EVENT_FLOW_AGED) |
+			    (UINT32_C(1) << RTE_ETH_EVENT_ERR_RECOVERING) |
+			    (UINT32_C(1) << RTE_ETH_EVENT_RECOVERED);
 /*
  * Decide if all memory are locked for performance.
  */
-- 
2.10.1

Re: [dpdk-dev] [RFC PATCH v4 3/3] app/testpmd: handle device recovery event

[Ajit Khaparde]

On Wed, Oct 7, 2020 at 2:37 AM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> On 10/7/2020 5:46 AM, Kalesh Anakkur Purayil wrote:
> > Hi Ophir,
> >
> > Thank you for the comments. I will address them in the next version.
> >
> > I will push these changes as Patches next time and not as an RFC. Hope that
> > is OK.
> >
> > Regards,
> > Kalesh
> >
> > On Tue, Oct 6, 2020 at 10:55 PM Ophir Munk <ophirmu@nvidia.com> wrote:
> >
> >> Hi Kalesh,
> >> Please find a few comments.
> >> The name you gave to the event (EVENT_RESET) is very close to an already
> >> existing one: "EVENT_INTR_RESET".
> >> But they are different.
> >> EVENT_INTR_RESET originates from a port reset. It requires application
> >> reaction. It is widely used. It is documented in *.rst files.
> >> EVENT_RESET originates from FW error (or maybe any error). It requires no
> >> application reaction (PMD manages by itself). It is not documented.
> >> I therefore suggest renaming it (maybe EVENT_ERR_RECOVERING) and please
> >> document it in *.rst files.
>
> +1 to renaming and documenting the event.
>
> And agree to proceed as regular patch instead of RFC.
Ferruh,
If/when the new version of patch is good,
Can you pick the bnxt PMD patch along with the ethdev and testpmd patch?
Let me know.


>
>
> >> More comments below:
> >>
> >>> -----Original Message-----
> >>> From: dev <dev-bounces@dpdk.org> On Behalf Of Kalesh A P
> >>> Sent: Wednesday, September 30, 2020 3:33 PM
> >>> To: dev@dpdk.org
> >>> Subject: [dpdk-dev] [RFC PATCH v4 3/3] app/testpmd: handle device
> >> recovery
> >>> event
> >>>
> >>> From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> >>>
> >>> Added code to handle device reset and recovery event in testpmd.
> >>> This is an indication from the PMD that device has reset and recovered
> >> error
> >>> condition.
> >>>
> >>> Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> >>> Reviewed-by: Ajit Kumar Khaparde <ajit.khaparde@broadcom.com>
> >>> ---
> >>>   app/test-pmd/testpmd.c | 6 +++++-
> >>>   1 file changed, 5 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> >>> fe6450c..1c8fb46 100644
> >>> --- a/app/test-pmd/testpmd.c
> >>> +++ b/app/test-pmd/testpmd.c
> >>> @@ -380,6 +380,8 @@ static const char * const eth_event_desc[] = {
> >>>        [RTE_ETH_EVENT_NEW] = "device probed",
> >>>        [RTE_ETH_EVENT_DESTROY] = "device released",
> >>>        [RTE_ETH_EVENT_FLOW_AGED] = "flow aged",
> >>> +     [RTE_ETH_EVENT_RESET] = "device reset",
> >>
> >> "device reset" is similar to the existing "reset" string. Can you suggest
> >> a different one? Maybe "error under recovery" ?
> >>
> >>> +     [RTE_ETH_EVENT_RECOVERED] = "device recovery",
> >>
> >> Wouldn't you prefer "device recovered" ?
> >>
> >>>        [RTE_ETH_EVENT_MAX] = NULL,
> >>>   };
> >>>
> >>> @@ -394,7 +396,9 @@ uint32_t event_print_mask = (UINT32_C(1) <<
> >>> RTE_ETH_EVENT_UNKNOWN) |
> >>>                            (UINT32_C(1) << RTE_ETH_EVENT_IPSEC) |
> >>>                            (UINT32_C(1) << RTE_ETH_EVENT_MACSEC) |
> >>>                            (UINT32_C(1) << RTE_ETH_EVENT_INTR_RMV) |
> >>> -                         (UINT32_C(1) << RTE_ETH_EVENT_FLOW_AGED);
> >>> +                         (UINT32_C(1) << RTE_ETH_EVENT_FLOW_AGED) |
> >>> +                         (UINT32_C(1) << RTE_ETH_EVENT_RESET) |
> >>> +                         (UINT32_C(1) << RTE_ETH_EVENT_RECOVERED);
> >>>   /*
> >>>    * Decide if all memory are locked for performance.
> >>>    */
> >>> --
> >>> 2.10.1
> >>
> >>
> >
>

Re: [dpdk-dev] [PATCH v5 1/3] ethdev: support device reset and recovery events

[Asaf Penso]

>-----Original Message-----
>From: dev <dev-bounces@dpdk.org> On Behalf Of Kalesh A P
>Sent: Wednesday, October 7, 2020 7:49 PM
>To: dev@dpdk.org
>Subject: [dpdk-dev] [PATCH v5 1/3] ethdev: support device reset and
>recovery events
>
>From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
>
>Adding support for device reset and recovery events in the rte_eth_event
>framework. FW error and FW reset conditions would be managed internally
>by PMD without needing application intervention.
>In such cases, PMD would need reset/recovery events to notify application
>that PMD is undergoing a reset.
>
>Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
>Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
>Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
>---
> doc/guides/prog_guide/poll_mode_drv.rst | 18 ++++++++++++++++++
> lib/librte_ethdev/rte_ethdev.h          | 17 +++++++++++++++++
> 2 files changed, 35 insertions(+)
>
>diff --git a/doc/guides/prog_guide/poll_mode_drv.rst
>b/doc/guides/prog_guide/poll_mode_drv.rst
>index 86e0a14..c03f0ef 100644
>--- a/doc/guides/prog_guide/poll_mode_drv.rst
>+++ b/doc/guides/prog_guide/poll_mode_drv.rst
>@@ -615,3 +615,21 @@ by application.
> The PMD itself should not call rte_eth_dev_reset(). The PMD can trigger  the
>application to handle reset event. It is duty of application to  handle all
>synchronization before it calls rte_eth_dev_reset().
>+
>+Error recovery support
>+~~~~~~~~~~~~~~~~~~~~~~
>+
>+When the PMD detects a FW reset or error condition, it will try to
>+recover from the error without needing the application intervention. In
>+such cases, PMD would need events to notify the application that it is
>+undergoing an error recovery.
>+
>+The PMD will trigger RTE_ETH_EVENT_ERR_RECOVERING event to notify the
>+application that PMD detected a FW reset or FW error condition. PMD
>+will try to recover from the error by itself. Data path will be halted
>+and control path operations would fail during the recovery period.
>+
>+The PMD will trigger RTE_ETH_EVENT_RECOVERED event to notify the
>+application that the it has recovered from the error condition. Control
>+path and data path are up now. Since the device undergone a reset, flow
>+rules offloaded prior to the reset will be lost and the application has to
>recreate the rules again.
>diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
>index 9759f13..9b4b015 100644
>--- a/lib/librte_ethdev/rte_ethdev.h
>+++ b/lib/librte_ethdev/rte_ethdev.h
>@@ -3207,6 +3207,23 @@ enum rte_eth_event_type {
> 	RTE_ETH_EVENT_DESTROY,  /**< port is released */
> 	RTE_ETH_EVENT_IPSEC,    /**< IPsec offload related event */
> 	RTE_ETH_EVENT_FLOW_AGED,/**< New aged-out flows is detected
>*/
>+	RTE_ETH_EVENT_ERR_RECOVERING,
>+			/**< port recovering from an error
>+			 *
>+			 * PMD detected a FW reset or error condition.
>+			 * PMD will try to recover from the error.
>+			 * Data path will be halted and Control path operations
>+			 * would fail at this time.
>+			 */
>+	RTE_ETH_EVENT_RECOVERED,
>+			/**< port recovered from an error
>+			 *
>+			 * PMD has recovered from the error condition.
>+			 * Control path and Data path are up now.
>+			 * Since the device undergone a reset, flow rules
>+			 * offloaded prior to the reset will be lost and
>+			 * the application has to recreate the rules again.
>+			 */
> 	RTE_ETH_EVENT_MAX       /**< max value of this enum */
> };
>
>--
>2.10.1

LGTM
Reviewed-by: Asaf Penso <asafp@nvidia.com>

Re: [dpdk-dev] [RFC PATCH v3 3/3] app/testpmd: handle device recovery event

[Asaf Penso]

>-----Original Message-----
>From: dev <dev-bounces@dpdk.org> On Behalf Of Kalesh A P
>Sent: Wednesday, September 30, 2020 10:13 AM
>To: dev@dpdk.org
>Cc: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>;
>ferruh.yigit@intel.com; ajit.khaparde@broadcom.com
>Subject: [dpdk-dev] [RFC PATCH v3 3/3] app/testpmd: handle device recovery
>event
>
>From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
>
>Added code to handle device reset and recovery event in testpmd.
>This is an indication from the PMD that device has reset and recovered error
>condition.
>
>Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
>Reviewed-by: Ajit Kumar Khaparde <ajit.khaparde@broadcom.com>
>---
> app/test-pmd/testpmd.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
>diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>fe6450c..1c8fb46 100644
>--- a/app/test-pmd/testpmd.c
>+++ b/app/test-pmd/testpmd.c
>@@ -380,6 +380,8 @@ static const char * const eth_event_desc[] = {
> 	[RTE_ETH_EVENT_NEW] = "device probed",
> 	[RTE_ETH_EVENT_DESTROY] = "device released",
> 	[RTE_ETH_EVENT_FLOW_AGED] = "flow aged",
>+	[RTE_ETH_EVENT_RESET] = "device reset",
>+	[RTE_ETH_EVENT_RECOVERED] = "device recovery",
> 	[RTE_ETH_EVENT_MAX] = NULL,
> };
>
>@@ -394,7 +396,9 @@ uint32_t event_print_mask = (UINT32_C(1) <<
>RTE_ETH_EVENT_UNKNOWN) |
> 			    (UINT32_C(1) << RTE_ETH_EVENT_IPSEC) |
> 			    (UINT32_C(1) << RTE_ETH_EVENT_MACSEC) |
> 			    (UINT32_C(1) << RTE_ETH_EVENT_INTR_RMV) |
>-			    (UINT32_C(1) << RTE_ETH_EVENT_FLOW_AGED);
>+			    (UINT32_C(1) << RTE_ETH_EVENT_FLOW_AGED) |
>+			    (UINT32_C(1) << RTE_ETH_EVENT_RESET) |
>+			    (UINT32_C(1) << RTE_ETH_EVENT_RECOVERED);
> /*
>  * Decide if all memory are locked for performance.
>  */
>--
>2.10.1

I think you would need also to update parse_event_printing_config function in parameters.c file.
Do you think the release_notes_20.11.rst should be updated with these new events?

[dpdk-dev] [PATCH v6 0/3] librte_ethdev: error recovery support

[Kalesh A P]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

The error recovery solution is a protocol implemented between firmware
and bnxt PMD to recover from the fatal errors without a system reboot.
There is an alarm thread which constantly monitors the health of the
firmware and initiates a recovery when needed.

There are two scenarios here:

1. Hardware or firmware encountered an error which firmware detected.
   Firmware is in operational status here. In this case, firmware can
   reset the chip and notify the driver about the reset.
2. Hardware or firmware encountered an error but firmware is dead/hung.
   Firmware is not in operational status. In this case, the only possible
   way to recover the adapter is through host driver(bnxt PMD).

In both cases, bnxt PMD reinitializes with the FW again after the reset.
During that recovery process, data path will be halted and any control path
operation would fail. So, the PMD has to notify the application about this
reset/error event to prevent any activities from the application while
the PMD is recovering from the error.

This patch set adds support for the reset and recovery event in
the rte_eth_event framework. FW error and FW reset conditions would be
managed by the PMD. Driver uses RTE_ETH_EVENT_RESET event to notify
the applications about the FW reset or error. In such cases,
PMD use the RTE_ETH_EVENT_RECOVERED event to notify application about
PMD has recovered from FW reset or FW error.

v6: Addressed comments from Asaf Penso.
    1. Updated 20.11 release notes with the new events added.
    2. updated testpmd parse_event_printing_config function.
v5: Addressed comments from Ophir Munk.
    1. Renamed the new event name to RTE_ETH_EVENT_ERR_RECOVERING.
    2. Fixed testpmd logs.
    3. Documented the new recovery events.
v4: Addressed comments from Thomas Monjalon
    1. Added doxygen comments about new events.
V3: Fixed a typo in commit log.
V2: Added a new event RTE_ETH_EVENT_RESET instead of using the
    RTE_ETH_EVENT_INTR_RESET to notify applications about device reset.

Kalesh AP (3):
  ethdev: support device reset and recovery events
  net/bnxt: notify applications about device reset/recovery
  app/testpmd: handle device recovery event

 app/test-pmd/parameters.c               |  8 ++++++--
 app/test-pmd/testpmd.c                  |  6 +++++-
 doc/guides/prog_guide/poll_mode_drv.rst | 18 ++++++++++++++++++
 doc/guides/rel_notes/release_20_11.rst  | 10 ++++++++++
 drivers/net/bnxt/bnxt_cpr.c             |  3 +++
 drivers/net/bnxt/bnxt_ethdev.c          |  9 +++++++++
 lib/librte_ethdev/rte_ethdev.h          | 17 +++++++++++++++++
 7 files changed, 68 insertions(+), 3 deletions(-)

-- 
2.10.1

[dpdk-dev] [PATCH v6 1/3] ethdev: support device reset and recovery events

[Kalesh A P]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

Adding support for device reset and recovery events in the
rte_eth_event framework. FW error and FW reset conditions would be
managed internally by PMD without needing application intervention.
In such cases, PMD would need reset/recovery events to notify application
that PMD is undergoing a reset.

Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
Reviewed-by: Asaf Penso <asafp@nvidia.com>
---
 doc/guides/prog_guide/poll_mode_drv.rst | 18 ++++++++++++++++++
 lib/librte_ethdev/rte_ethdev.h          | 17 +++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/doc/guides/prog_guide/poll_mode_drv.rst b/doc/guides/prog_guide/poll_mode_drv.rst
index 86e0a14..c03f0ef 100644
--- a/doc/guides/prog_guide/poll_mode_drv.rst
+++ b/doc/guides/prog_guide/poll_mode_drv.rst
@@ -615,3 +615,21 @@ by application.
 The PMD itself should not call rte_eth_dev_reset(). The PMD can trigger
 the application to handle reset event. It is duty of application to
 handle all synchronization before it calls rte_eth_dev_reset().
+
+Error recovery support
+~~~~~~~~~~~~~~~~~~~~~~
+
+When the PMD detects a FW reset or error condition, it will try to recover
+from the error without needing the application intervention. In such cases,
+PMD would need events to notify the application that it is undergoing
+an error recovery.
+
+The PMD will trigger RTE_ETH_EVENT_ERR_RECOVERING event to notify the
+application that PMD detected a FW reset or FW error condition. PMD will
+try to recover from the error by itself. Data path will be halted and
+control path operations would fail during the recovery period.
+
+The PMD will trigger RTE_ETH_EVENT_RECOVERED event to notify the application
+that the it has recovered from the error condition. Control path and data path
+are up now. Since the device undergone a reset, flow rules offloaded prior to
+the reset will be lost and the application has to recreate the rules again.
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index 9759f13..9b4b015 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -3207,6 +3207,23 @@ enum rte_eth_event_type {
 	RTE_ETH_EVENT_DESTROY,  /**< port is released */
 	RTE_ETH_EVENT_IPSEC,    /**< IPsec offload related event */
 	RTE_ETH_EVENT_FLOW_AGED,/**< New aged-out flows is detected */
+	RTE_ETH_EVENT_ERR_RECOVERING,
+			/**< port recovering from an error
+			 *
+			 * PMD detected a FW reset or error condition.
+			 * PMD will try to recover from the error.
+			 * Data path will be halted and Control path operations
+			 * would fail at this time.
+			 */
+	RTE_ETH_EVENT_RECOVERED,
+			/**< port recovered from an error
+			 *
+			 * PMD has recovered from the error condition.
+			 * Control path and Data path are up now.
+			 * Since the device undergone a reset, flow rules
+			 * offloaded prior to the reset will be lost and
+			 * the application has to recreate the rules again.
+			 */
 	RTE_ETH_EVENT_MAX       /**< max value of this enum */
 };
 
-- 
2.10.1

[dpdk-dev] [PATCH v6 2/3] net/bnxt: notify applications about device reset/recovery

[Kalesh A P]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

When the driver receives RESET_NOTIFY async event from FW or detected
an error condition, it should update the application that FW is going
to reset. Once the driver recoveres from the reset, update the reset
recovery status to application as well.

The recovery process is transparent to the application as the driver
itself tries to recover from FW reset or FW error conditions.

Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Signed-off-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
---
 drivers/net/bnxt/bnxt_cpr.c    | 3 +++
 drivers/net/bnxt/bnxt_ethdev.c | 9 +++++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/net/bnxt/bnxt_cpr.c b/drivers/net/bnxt/bnxt_cpr.c
index 8311e26..987c010 100644
--- a/drivers/net/bnxt/bnxt_cpr.c
+++ b/drivers/net/bnxt/bnxt_cpr.c
@@ -129,6 +129,9 @@ void bnxt_handle_async_event(struct bnxt *bp,
 			bp->flags |= BNXT_FLAG_FATAL_ERROR;
 			return;
 		}
+		rte_eth_dev_callback_process(bp->eth_dev,
+					     RTE_ETH_EVENT_ERR_RECOVERING,
+					     NULL);
 
 		event_data = rte_le_to_cpu_32(async_cmp->event_data1);
 		/* timestamp_lo/hi values are in units of 100ms */
diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
index b99c712..e3798de 100644
--- a/drivers/net/bnxt/bnxt_ethdev.c
+++ b/drivers/net/bnxt/bnxt_ethdev.c
@@ -4566,6 +4566,9 @@ static void bnxt_dev_recover(void *arg)
 		goto err_start;
 
 	PMD_DRV_LOG(INFO, "Recovered from FW reset\n");
+	rte_eth_dev_callback_process(bp->eth_dev,
+				     RTE_ETH_EVENT_RECOVERED,
+				     NULL);
 	return;
 err_start:
 	bnxt_dev_stop_op(bp->eth_dev);
@@ -4573,6 +4576,9 @@ static void bnxt_dev_recover(void *arg)
 	bp->flags |= BNXT_FLAG_FATAL_ERROR;
 	bnxt_uninit_resources(bp, false);
 	PMD_DRV_LOG(ERR, "Failed to recover from FW reset\n");
+	rte_eth_dev_callback_process(bp->eth_dev,
+				     RTE_ETH_EVENT_INTR_RMV,
+				     NULL);
 }
 
 void bnxt_dev_reset_and_resume(void *arg)
@@ -4708,6 +4714,9 @@ static void bnxt_check_fw_health(void *arg)
 	bp->flags |= BNXT_FLAG_FW_RESET;
 
 	PMD_DRV_LOG(ERR, "Detected FW dead condition\n");
+	rte_eth_dev_callback_process(bp->eth_dev,
+				     RTE_ETH_EVENT_ERR_RECOVERING,
+				     NULL);
 
 	if (bnxt_is_master_func(bp))
 		wait_msec = info->master_func_wait_period;
-- 
2.10.1

[dpdk-dev] [PATCH v6 3/3] app/testpmd: handle device recovery event

[Kalesh A P]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

Added code to handle error recovery events in testpmd.
This is an indication from the PMD that it is undergoing
an error recovery and recovered from the error condition.

Updated 20.11 release notes as well.

Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
Reviewed-by: Ajit Kumar Khaparde <ajit.khaparde@broadcom.com>
---
 app/test-pmd/parameters.c              |  8 ++++++--
 app/test-pmd/testpmd.c                 |  6 +++++-
 doc/guides/rel_notes/release_20_11.rst | 10 ++++++++++
 3 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index 1ead595..560f9ba 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -192,9 +192,9 @@ usage(char* progname)
 	printf("  --no-rmv-interrupt: disable device removal interrupt.\n");
 	printf("  --bitrate-stats=N: set the logical core N to perform "
 		"bit-rate calculation.\n");
-	printf("  --print-event <unknown|intr_lsc|queue_state|intr_reset|vf_mbox|macsec|intr_rmv|flow_aged|all>: "
+	printf("  --print-event <unknown|intr_lsc|queue_state|intr_reset|vf_mbox|macsec|intr_rmv|flow_aged|err_recovering|recovered|all>: "
 	       "enable print of designated event or all of them.\n");
-	printf("  --mask-event <unknown|intr_lsc|queue_state|intr_reset|vf_mbox|macsec|intr_rmv|flow_aged|all>: "
+	printf("  --mask-event <unknown|intr_lsc|queue_state|intr_reset|vf_mbox|macsec|intr_rmv|flow_aged|err_recovering|recovered|all>: "
 	       "disable print of designated event or all of them.\n");
 	printf("  --flow-isolate-all: "
 	       "requests flow API isolated mode on all ports at initialization time.\n");
@@ -556,6 +556,10 @@ parse_event_printing_config(const char *optarg, int enable)
 		mask = UINT32_C(1) << RTE_ETH_EVENT_DESTROY;
 	else if (!strcmp(optarg, "flow_aged"))
 		mask = UINT32_C(1) << RTE_ETH_EVENT_FLOW_AGED;
+	else if (!strcmp(optarg, "err_recovering"))
+		mask = UINT32_C(1) << RTE_ETH_EVENT_ERR_RECOVERING;
+	else if (!strcmp(optarg, "recovered"))
+		mask = UINT32_C(1) << RTE_ETH_EVENT_RECOVERED;
 	else if (!strcmp(optarg, "all"))
 		mask = ~UINT32_C(0);
 	else {
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index fe6450c..80ae3fa 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -380,6 +380,8 @@ static const char * const eth_event_desc[] = {
 	[RTE_ETH_EVENT_NEW] = "device probed",
 	[RTE_ETH_EVENT_DESTROY] = "device released",
 	[RTE_ETH_EVENT_FLOW_AGED] = "flow aged",
+	[RTE_ETH_EVENT_ERR_RECOVERING] = "device error under recovery",
+	[RTE_ETH_EVENT_RECOVERED] = "device recovered",
 	[RTE_ETH_EVENT_MAX] = NULL,
 };
 
@@ -394,7 +396,9 @@ uint32_t event_print_mask = (UINT32_C(1) << RTE_ETH_EVENT_UNKNOWN) |
 			    (UINT32_C(1) << RTE_ETH_EVENT_IPSEC) |
 			    (UINT32_C(1) << RTE_ETH_EVENT_MACSEC) |
 			    (UINT32_C(1) << RTE_ETH_EVENT_INTR_RMV) |
-			    (UINT32_C(1) << RTE_ETH_EVENT_FLOW_AGED);
+			    (UINT32_C(1) << RTE_ETH_EVENT_FLOW_AGED) |
+			    (UINT32_C(1) << RTE_ETH_EVENT_ERR_RECOVERING) |
+			    (UINT32_C(1) << RTE_ETH_EVENT_RECOVERED);
 /*
  * Decide if all memory are locked for performance.
  */
diff --git a/doc/guides/rel_notes/release_20_11.rst b/doc/guides/rel_notes/release_20_11.rst
index 4bcf220..f732ff6 100644
--- a/doc/guides/rel_notes/release_20_11.rst
+++ b/doc/guides/rel_notes/release_20_11.rst
@@ -78,11 +78,21 @@ New Features
     ``--portmask=N``
     where N represents the hexadecimal bitmask of ports used.
 
+* **Added error recovery support.**
+
+  Added error recovery support to detect and recover from device errors including:
+
+  * Added new event: ``RTE_ETH_EVENT_ERR_RECOVERING`` for the driver to report
+    that the port is recovering from an error.
+  * Added new event: ``RTE_ETH_EVENT_RECOVERED`` for the driver to report
+    that the port has recovered from an error.
+
 * **Updated Broadcom bnxt driver.**
 
   Updated the Broadcom bnxt driver with new features and improvements, including:
 
   * Added support for 200G PAM4 link speed.
+  * Added support to handle device recovery events.
 
 
 Removed Items
-- 
2.10.1

Re: [dpdk-dev] [PATCH v6 1/3] ethdev: support device reset and recovery events

[Thomas Monjalon]

09/10/2020 05:48, Kalesh A P:
> From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> 
> Adding support for device reset and recovery events in the
> rte_eth_event framework. FW error and FW reset conditions would be
> managed internally by PMD without needing application intervention.
> In such cases, PMD would need reset/recovery events to notify application
> that PMD is undergoing a reset.
> 
> Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
> Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> Reviewed-by: Asaf Penso <asafp@nvidia.com>

The ethdev maintainers are not Cc'ed.
Please use the option --cc-cmd devtools/get-maintainer.sh


> +Error recovery support
> +~~~~~~~~~~~~~~~~~~~~~~
> +
> +When the PMD detects a FW reset or error condition, it will try to recover
> +from the error without needing the application intervention. In such cases,
> +PMD would need events to notify the application that it is undergoing
> +an error recovery.
> +
> +The PMD will trigger RTE_ETH_EVENT_ERR_RECOVERING event to notify the
> +application that PMD detected a FW reset or FW error condition. PMD will
> +try to recover from the error by itself. Data path will be halted and
> +control path operations would fail during the recovery period.
> +
> +The PMD will trigger RTE_ETH_EVENT_RECOVERED event to notify the application
> +that the it has recovered from the error condition. Control path and data path
> +are up now. Since the device undergone a reset, flow rules offloaded prior to
> +the reset will be lost and the application has to recreate the rules again.
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index 9759f13..9b4b015 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -3207,6 +3207,23 @@ enum rte_eth_event_type {
>  	RTE_ETH_EVENT_DESTROY,  /**< port is released */
>  	RTE_ETH_EVENT_IPSEC,    /**< IPsec offload related event */
>  	RTE_ETH_EVENT_FLOW_AGED,/**< New aged-out flows is detected */
> +	RTE_ETH_EVENT_ERR_RECOVERING,
> +			/**< port recovering from an error
> +			 *
> +			 * PMD detected a FW reset or error condition.
> +			 * PMD will try to recover from the error.
> +			 * Data path will be halted and Control path operations
> +			 * would fail at this time.
> +			 */

Does it mean the application has nothing to do when receiving this event?
I think the app should stop polling at least.

> +	RTE_ETH_EVENT_RECOVERED,
> +			/**< port recovered from an error
> +			 *
> +			 * PMD has recovered from the error condition.
> +			 * Control path and Data path are up now.
> +			 * Since the device undergone a reset, flow rules
> +			 * offloaded prior to the reset will be lost and
> +			 * the application has to recreate the rules again.
> +			 */

Please be more precise.
Should the app re-configure the port, setup the queues, start the port?

Re: [dpdk-dev] [PATCH v6 1/3] ethdev: support device reset and recovery events

[Andrew Rybchenko]

On 10/12/20 12:29 AM, Thomas Monjalon wrote:
> 09/10/2020 05:48, Kalesh A P:
>> From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
>>
>> Adding support for device reset and recovery events in the
>> rte_eth_event framework. FW error and FW reset conditions would be
>> managed internally by PMD without needing application intervention.
>> In such cases, PMD would need reset/recovery events to notify application
>> that PMD is undergoing a reset.
>>
>> Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
>> Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
>> Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
>> Reviewed-by: Asaf Penso <asafp@nvidia.com>
> 
> The ethdev maintainers are not Cc'ed.
> Please use the option --cc-cmd devtools/get-maintainer.sh
> 
> 
>> +Error recovery support
>> +~~~~~~~~~~~~~~~~~~~~~~
>> +
>> +When the PMD detects a FW reset or error condition, it will try to recover
>> +from the error without needing the application intervention. In such cases,
>> +PMD would need events to notify the application that it is undergoing
>> +an error recovery.
>> +
>> +The PMD will trigger RTE_ETH_EVENT_ERR_RECOVERING event to notify the
>> +application that PMD detected a FW reset or FW error condition. PMD will
>> +try to recover from the error by itself. Data path will be halted and
>> +control path operations would fail during the recovery period.
>> +
>> +The PMD will trigger RTE_ETH_EVENT_RECOVERED event to notify the application
>> +that the it has recovered from the error condition. Control path and data path
>> +are up now. Since the device undergone a reset, flow rules offloaded prior to
>> +the reset will be lost and the application has to recreate the rules again.

What should be done if the state is not recoverable?

>> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
>> index 9759f13..9b4b015 100644
>> --- a/lib/librte_ethdev/rte_ethdev.h
>> +++ b/lib/librte_ethdev/rte_ethdev.h
>> @@ -3207,6 +3207,23 @@ enum rte_eth_event_type {
>>  	RTE_ETH_EVENT_DESTROY,  /**< port is released */
>>  	RTE_ETH_EVENT_IPSEC,    /**< IPsec offload related event */
>>  	RTE_ETH_EVENT_FLOW_AGED,/**< New aged-out flows is detected */
>> +	RTE_ETH_EVENT_ERR_RECOVERING,
>> +			/**< port recovering from an error
>> +			 *
>> +			 * PMD detected a FW reset or error condition.
>> +			 * PMD will try to recover from the error.
>> +			 * Data path will be halted and Control path operations
>> +			 * would fail at this time.
>> +			 */
> 
> Does it mean the application has nothing to do when receiving this event?
> I think the app should stop polling at least.
> 
>> +	RTE_ETH_EVENT_RECOVERED,
>> +			/**< port recovered from an error
>> +			 *
>> +			 * PMD has recovered from the error condition.
>> +			 * Control path and Data path are up now.
>> +			 * Since the device undergone a reset, flow rules
>> +			 * offloaded prior to the reset will be lost and
>> +			 * the application has to recreate the rules again.
>> +			 */
> 
> Please be more precise.
> Should the app re-configure the port, setup the queues, start the port?
> 
> 

Re: [dpdk-dev] [PATCH v6 1/3] ethdev: support device reset and recovery events

[Ferruh Yigit]

On 10/12/2020 9:09 AM, Andrew Rybchenko wrote:
> On 10/12/20 12:29 AM, Thomas Monjalon wrote:
>> 09/10/2020 05:48, Kalesh A P:
>>> From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
>>>
>>> Adding support for device reset and recovery events in the
>>> rte_eth_event framework. FW error and FW reset conditions would be
>>> managed internally by PMD without needing application intervention.
>>> In such cases, PMD would need reset/recovery events to notify application
>>> that PMD is undergoing a reset.
>>>
>>> Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
>>> Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
>>> Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
>>> Reviewed-by: Asaf Penso <asafp@nvidia.com>
>>
>> The ethdev maintainers are not Cc'ed.
>> Please use the option --cc-cmd devtools/get-maintainer.sh
>>
>>
>>> +Error recovery support
>>> +~~~~~~~~~~~~~~~~~~~~~~
>>> +
>>> +When the PMD detects a FW reset or error condition, it will try to recover
>>> +from the error without needing the application intervention. In such cases,
>>> +PMD would need events to notify the application that it is undergoing
>>> +an error recovery.
>>> +
>>> +The PMD will trigger RTE_ETH_EVENT_ERR_RECOVERING event to notify the
>>> +application that PMD detected a FW reset or FW error condition. PMD will
>>> +try to recover from the error by itself. Data path will be halted and
>>> +control path operations would fail during the recovery period.
>>> +
>>> +The PMD will trigger RTE_ETH_EVENT_RECOVERED event to notify the application
>>> +that the it has recovered from the error condition. Control path and data path
>>> +are up now. Since the device undergone a reset, flow rules offloaded prior to
>>> +the reset will be lost and the application has to recreate the rules again.
> 
> What should be done if the state is not recoverable?
> 
>>> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
>>> index 9759f13..9b4b015 100644
>>> --- a/lib/librte_ethdev/rte_ethdev.h
>>> +++ b/lib/librte_ethdev/rte_ethdev.h
>>> @@ -3207,6 +3207,23 @@ enum rte_eth_event_type {
>>>   	RTE_ETH_EVENT_DESTROY,  /**< port is released */
>>>   	RTE_ETH_EVENT_IPSEC,    /**< IPsec offload related event */
>>>   	RTE_ETH_EVENT_FLOW_AGED,/**< New aged-out flows is detected */
>>> +	RTE_ETH_EVENT_ERR_RECOVERING,
>>> +			/**< port recovering from an error
>>> +			 *
>>> +			 * PMD detected a FW reset or error condition.
>>> +			 * PMD will try to recover from the error.
>>> +			 * Data path will be halted and Control path operations
>>> +			 * would fail at this time.
>>> +			 */
>>
>> Does it mean the application has nothing to do when receiving this event?
>> I think the app should stop polling at least.
>>
>>> +	RTE_ETH_EVENT_RECOVERED,
>>> +			/**< port recovered from an error
>>> +			 *
>>> +			 * PMD has recovered from the error condition.
>>> +			 * Control path and Data path are up now.
>>> +			 * Since the device undergone a reset, flow rules
>>> +			 * offloaded prior to the reset will be lost and
>>> +			 * the application has to recreate the rules again.
>>> +			 */
>>
>> Please be more precise.
>> Should the app re-configure the port, setup the queues, start the port?
>>
>>
> 

Hi Kalesh Anakkur,

The mechanics of notifying the application looks good, but the concerns seems 
more about what application should do with this information.

PMD notifies the application on the FW/HW reset and pushes some 
tasks/responsibilities to the application, but for this to be useful, these 
tasks should be clear to application.

Think yourself in a situation that you are developing an application and you 
received these events from a device that you don't know its internals, what will 
you do?

Both Thomas and Andrew put cases that needs more clarification for application. 
Can you please send a new version with those clarifications?


Thanks,
ferruh

[dpdk-dev] [PATCH v7 0/4] ethdev: error recovery support

[Kalesh A P]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

The error recovery solution is a protocol implemented between firmware
and bnxt PMD to recover from the fatal errors without a system reboot.
There is an alarm thread which constantly monitors the health of the
firmware and initiates a recovery when needed.

There are two scenarios here:

1. Hardware or firmware encountered an error which firmware detected.
   Firmware is in operational status here. In this case, firmware can
   reset the chip and notify the driver about the reset.
2. Hardware or firmware encountered an error but firmware is dead/hung.
   Firmware is not in operational status. In this case, the only possible
   way to recover the adapter is through host driver(bnxt PMD).

In both cases, bnxt PMD reinitializes with the FW again after the reset.
During that recovery process, data path will be halted and any control path
operation would fail. So, the PMD has to notify the application about this
reset/error event to prevent any activities from the application while
the PMD is recovering from the error.

While most of the recovery process is transparent to the application since
most of the driver ensures recovery from FW reset or FW error conditions,
the application will have to reprogram any flows which were offloaded to
the underlying hardware.

This patch set adds support for the reset and recovery event in
the rte_eth_event framework. FW error and FW reset conditions would be
managed by the PMD. Driver uses RTE_ETH_EVENT_ERR_RECOVERING event to
notify the applications about the FW reset or error.
PMD uses the RTE_ETH_EVENT_RECOVERED event to notify application about
PMD has recovered from FW reset or FW error.
The application should stop polling till it receives the
RTE_ETH_EVENT_RECOVERED event from the PMD.

v7: Addressed comments from Thomas and Andrew.
v6: Addressed comments from Asaf Penso.
    1. Updated 20.11 release notes with the new events added.
    2. updated testpmd parse_event_printing_config function.
v5: Addressed comments from Ophir Munk.
    1. Renamed the new event name to RTE_ETH_EVENT_ERR_RECOVERING.
    2. Fixed testpmd logs.
    3. Documented the new recovery events.
v4: Addressed comments from Thomas Monjalon
    1. Added doxygen comments about new events.
V3: Fixed a typo in commit log.
V2: Added a new event RTE_ETH_EVENT_RESET instead of using the
    RTE_ETH_EVENT_INTR_RESET to notify applications about device reset.

Kalesh AP (4):
  ethdev: support device reset and recovery events
  app/testpmd: handle device recovery event
  net/bnxt: notify applications about device reset/recovery
  doc: update release notes

 app/test-pmd/parameters.c               |  8 ++++++--
 app/test-pmd/testpmd.c                  | 10 +++++++++-
 doc/guides/prog_guide/poll_mode_drv.rst | 24 ++++++++++++++++++++++++
 doc/guides/rel_notes/release_22_03.rst  | 15 +++++++++++++++
 drivers/net/bnxt/bnxt_cpr.c             |  4 ++++
 drivers/net/bnxt/bnxt_ethdev.c          |  8 +++++++-
 lib/ethdev/rte_ethdev.h                 | 18 ++++++++++++++++++
 7 files changed, 83 insertions(+), 4 deletions(-)

-- 
2.10.1

[dpdk-dev] [PATCH v7 1/4] ethdev: support device reset and recovery events

[Kalesh A P]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

Adding support for the device reset and recovery events in the
rte_eth_event framework. FW error and FW reset conditions would be
managed internally by the PMD without needing application intervention.
In such cases, PMD would need reset/recovery events to notify application
that PMD is undergoing a reset.

While most of the recovery process is transparent to the application since
most of the driver ensures recovery from FW reset or FW error conditions,
the application will have to reprogram any flows which were offloaded to
the underlying hardware.

Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
---
 doc/guides/prog_guide/poll_mode_drv.rst | 24 ++++++++++++++++++++++++
 lib/ethdev/rte_ethdev.h                 | 18 ++++++++++++++++++
 2 files changed, 42 insertions(+)

diff --git a/doc/guides/prog_guide/poll_mode_drv.rst b/doc/guides/prog_guide/poll_mode_drv.rst
index 6831289..9ecc0e4 100644
--- a/doc/guides/prog_guide/poll_mode_drv.rst
+++ b/doc/guides/prog_guide/poll_mode_drv.rst
@@ -623,3 +623,27 @@ by application.
 The PMD itself should not call rte_eth_dev_reset(). The PMD can trigger
 the application to handle reset event. It is duty of application to
 handle all synchronization before it calls rte_eth_dev_reset().
+
+Error recovery support
+~~~~~~~~~~~~~~~~~~~~~~
+
+When the PMD detects a FW reset or error condition, it may try to recover
+from the error without needing the application intervention. In such cases,
+PMD would need events to notify the application that it is undergoing
+an error recovery.
+
+The PMD should trigger RTE_ETH_EVENT_ERR_RECOVERING event to notify the
+application that PMD detected a FW reset or FW error condition. PMD may
+try to recover from the error by itself. Data path may be quiesced and
+control path operations may fail during the recovery period. The application
+should stop polling till it receives RTE_ETH_EVENT_RECOVERED event from the PMD.
+
+The PMD should trigger RTE_ETH_EVENT_RECOVERED event to notify the application
+that the it has recovered from the error condition. PMD re-configures the port
+to the state prior to the error condition. Control path and data path are up now.
+Since the device has undergone a reset, flow rules offloaded prior to reset
+may be lost and the application should recreate the rules again.
+
+The PMD should trigger RTE_ETH_EVENT_INTR_RMV event to notify the application
+that it has failed to recover from the error condition. The device may not be
+usable anymore.
diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index 147cc1c..a46819f 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -3818,6 +3818,24 @@ enum rte_eth_event_type {
 	RTE_ETH_EVENT_DESTROY,  /**< port is released */
 	RTE_ETH_EVENT_IPSEC,    /**< IPsec offload related event */
 	RTE_ETH_EVENT_FLOW_AGED,/**< New aged-out flows is detected */
+	RTE_ETH_EVENT_ERR_RECOVERING,
+			/**< port recovering from an error
+			 *
+			 * PMD detected a FW reset or error condition.
+			 * PMD will try to recover from the error.
+			 * Data path may be quiesced and Control path operations
+			 * may fail at this time.
+			 */
+	RTE_ETH_EVENT_RECOVERED,
+			/**< port recovered from an error
+			 *
+			 * PMD has recovered from the error condition.
+			 * Control path and Data path are up now.
+			 * PMD re-configures the port to the state prior to the error.
+			 * Since the device has undergone a reset, flow rules
+			 * offloaded prior to reset may be lost and
+			 * the application should recreate the rules again.
+			 */
 	RTE_ETH_EVENT_MAX       /**< max value of this enum */
 };
 
-- 
2.10.1

[dpdk-dev] [PATCH v7 2/4] app/testpmd: handle device recovery event

[Kalesh A P]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

Added code to handle error recovery events in testpmd.
This is an indication from the PMD that it is undergoing
an error recovery and recovered from the error condition.

Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
---
 app/test-pmd/parameters.c |  8 ++++++--
 app/test-pmd/testpmd.c    | 10 +++++++++-
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index daf6a31..deea29f 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -175,9 +175,9 @@ usage(char* progname)
 	printf("  --no-rmv-interrupt: disable device removal interrupt.\n");
 	printf("  --bitrate-stats=N: set the logical core N to perform "
 		"bit-rate calculation.\n");
-	printf("  --print-event <unknown|intr_lsc|queue_state|intr_reset|vf_mbox|macsec|intr_rmv|flow_aged|all>: "
+	printf("  --print-event <unknown|intr_lsc|queue_state|intr_reset|vf_mbox|macsec|intr_rmv|flow_aged|err_recovering|recovered|all>: "
 	       "enable print of designated event or all of them.\n");
-	printf("  --mask-event <unknown|intr_lsc|queue_state|intr_reset|vf_mbox|macsec|intr_rmv|flow_aged|all>: "
+	printf("  --mask-event <unknown|intr_lsc|queue_state|intr_reset|vf_mbox|macsec|intr_rmv|flow_aged|err_recovering|recovered|all>: "
 	       "disable print of designated event or all of them.\n");
 	printf("  --flow-isolate-all: "
 	       "requests flow API isolated mode on all ports at initialization time.\n");
@@ -461,6 +461,10 @@ parse_event_printing_config(const char *optarg, int enable)
 		mask = UINT32_C(1) << RTE_ETH_EVENT_DESTROY;
 	else if (!strcmp(optarg, "flow_aged"))
 		mask = UINT32_C(1) << RTE_ETH_EVENT_FLOW_AGED;
+	else if (!strcmp(optarg, "err_recovering"))
+		mask = UINT32_C(1) << RTE_ETH_EVENT_ERR_RECOVERING;
+	else if (!strcmp(optarg, "recovered"))
+		mask = UINT32_C(1) << RTE_ETH_EVENT_RECOVERED;
 	else if (!strcmp(optarg, "all"))
 		mask = ~UINT32_C(0);
 	else {
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index e1da961..7b64751 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -417,6 +417,8 @@ static const char * const eth_event_desc[] = {
 	[RTE_ETH_EVENT_NEW] = "device probed",
 	[RTE_ETH_EVENT_DESTROY] = "device released",
 	[RTE_ETH_EVENT_FLOW_AGED] = "flow aged",
+	[RTE_ETH_EVENT_ERR_RECOVERING] = "device error, recovery in progress",
+	[RTE_ETH_EVENT_RECOVERED] = "device recovered",
 	[RTE_ETH_EVENT_MAX] = NULL,
 };
 
@@ -431,7 +433,9 @@ uint32_t event_print_mask = (UINT32_C(1) << RTE_ETH_EVENT_UNKNOWN) |
 			    (UINT32_C(1) << RTE_ETH_EVENT_IPSEC) |
 			    (UINT32_C(1) << RTE_ETH_EVENT_MACSEC) |
 			    (UINT32_C(1) << RTE_ETH_EVENT_INTR_RMV) |
-			    (UINT32_C(1) << RTE_ETH_EVENT_FLOW_AGED);
+			    (UINT32_C(1) << RTE_ETH_EVENT_FLOW_AGED) |
+			    (UINT32_C(1) << RTE_ETH_EVENT_ERR_RECOVERING) |
+			    (UINT32_C(1) << RTE_ETH_EVENT_RECOVERED);
 /*
  * Decide if all memory are locked for performance.
  */
@@ -3564,6 +3568,10 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
 		ports[port_id].port_status = RTE_PORT_CLOSED;
 		printf("Port %u is closed\n", port_id);
 		break;
+	case RTE_ETH_EVENT_RECOVERED:
+		/* for now, flush flows to avoid displaying stale entries */
+		port_flow_flush(port_id);
+		break;
 	default:
 		break;
 	}
-- 
2.10.1

[dpdk-dev] [PATCH v7 3/4] net/bnxt: notify applications about device reset/recovery

[Kalesh A P]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

When the driver receives RESET_NOTIFY async event from FW or detects
an error condition, it should update the application that FW is going
to reset. Once the driver recovers from the reset, update the reset
recovery status to application as well.

Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
---
 drivers/net/bnxt/bnxt_cpr.c    | 4 ++++
 drivers/net/bnxt/bnxt_ethdev.c | 8 +++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/net/bnxt/bnxt_cpr.c b/drivers/net/bnxt/bnxt_cpr.c
index 9b9285b..738eac7 100644
--- a/drivers/net/bnxt/bnxt_cpr.c
+++ b/drivers/net/bnxt/bnxt_cpr.c
@@ -180,6 +180,10 @@ void bnxt_handle_async_event(struct bnxt *bp,
 			return;
 		}
 
+		rte_eth_dev_callback_process(bp->eth_dev,
+					     RTE_ETH_EVENT_ERR_RECOVERING,
+					     NULL);
+
 		pthread_mutex_lock(&bp->err_recovery_lock);
 		event_data = data1;
 		/* timestamp_lo/hi values are in units of 100ms */
diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
index cc95211..579e571 100644
--- a/drivers/net/bnxt/bnxt_ethdev.c
+++ b/drivers/net/bnxt/bnxt_ethdev.c
@@ -4383,7 +4383,9 @@ static void bnxt_dev_recover(void *arg)
 	PMD_DRV_LOG(INFO, "Port: %u Recovered from FW reset\n",
 		    bp->eth_dev->data->port_id);
 	pthread_mutex_unlock(&bp->err_recovery_lock);
-
+	rte_eth_dev_callback_process(bp->eth_dev,
+				     RTE_ETH_EVENT_RECOVERED,
+				     NULL);
 	return;
 err_start:
 	bnxt_dev_stop(bp->eth_dev);
@@ -4561,6 +4563,10 @@ static void bnxt_check_fw_health(void *arg)
 
 	PMD_DRV_LOG(ERR, "Detected FW dead condition\n");
 
+	rte_eth_dev_callback_process(bp->eth_dev,
+				     RTE_ETH_EVENT_ERR_RECOVERING,
+				     NULL);
+
 	if (bnxt_is_primary_func(bp))
 		wait_msec = info->primary_func_wait_period;
 	else
-- 
2.10.1

[dpdk-dev] [PATCH v7 4/4] doc: update release notes

[Kalesh A P]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

Updated release notes with new error recovery event support.

Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
---
 doc/guides/rel_notes/release_22_03.rst | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/doc/guides/rel_notes/release_22_03.rst b/doc/guides/rel_notes/release_22_03.rst
index 33be324..88755ba 100644
--- a/doc/guides/rel_notes/release_22_03.rst
+++ b/doc/guides/rel_notes/release_22_03.rst
@@ -24,6 +24,21 @@ DPDK Release 22.03
 New Features
 ------------
 
+* **Added error recovery support.**
+
+  Added error recovery support to detect and recover from device errors including:
+
+  * Added new event: ``RTE_ETH_EVENT_ERR_RECOVERING`` for the driver to report
+    that the port is recovering from an error.
+  * Added new event: ``RTE_ETH_EVENT_RECOVERED`` for the driver to report
+    that the port has recovered from an error.
+
+* **Updated Broadcom bnxt driver.**
+
+  Updated the Broadcom bnxt driver with new features and improvements, including:
+
+  * Added support to handle device recovery events.
+
 .. This section should contain new features added in this release.
    Sample format:
 
-- 
2.10.1

Re: [dpdk-dev] [PATCH v7 1/4] ethdev: support device reset and recovery events

[Ferruh Yigit]

On 1/28/2022 12:48 PM, Kalesh A P wrote:
> From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> 
> Adding support for the device reset and recovery events in the
> rte_eth_event framework. FW error and FW reset conditions would be
> managed internally by the PMD without needing application intervention.
> In such cases, PMD would need reset/recovery events to notify application
> that PMD is undergoing a reset.
> 
> While most of the recovery process is transparent to the application since
> most of the driver ensures recovery from FW reset or FW error conditions,
> the application will have to reprogram any flows which were offloaded to
> the underlying hardware.
> 
> Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
> Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>

More developer cc'ed.

> ---
>   doc/guides/prog_guide/poll_mode_drv.rst | 24 ++++++++++++++++++++++++
>   lib/ethdev/rte_ethdev.h                 | 18 ++++++++++++++++++
>   2 files changed, 42 insertions(+)
> 
> diff --git a/doc/guides/prog_guide/poll_mode_drv.rst b/doc/guides/prog_guide/poll_mode_drv.rst
> index 6831289..9ecc0e4 100644
> --- a/doc/guides/prog_guide/poll_mode_drv.rst
> +++ b/doc/guides/prog_guide/poll_mode_drv.rst
> @@ -623,3 +623,27 @@ by application.
>   The PMD itself should not call rte_eth_dev_reset(). The PMD can trigger
>   the application to handle reset event. It is duty of application to
>   handle all synchronization before it calls rte_eth_dev_reset().
> +
> +Error recovery support
> +~~~~~~~~~~~~~~~~~~~~~~
> +
> +When the PMD detects a FW reset or error condition, it may try to recover
> +from the error without needing the application intervention. In such cases,
> +PMD would need events to notify the application that it is undergoing
> +an error recovery.
> +
> +The PMD should trigger RTE_ETH_EVENT_ERR_RECOVERING event to notify the
> +application that PMD detected a FW reset or FW error condition. PMD may
> +try to recover from the error by itself. Data path may be quiesced and
> +control path operations may fail during the recovery period. The application
> +should stop polling till it receives RTE_ETH_EVENT_RECOVERED event from the PMD.
> +

Between the time FW error occurred and the application receive the RECOVERING event,
datapath will continue to poll and application may call control APIs, so the event
really is not solving the issue and driver somehow should be sure this won't crash
the application, in that case not sure about the benefit of this event.

> +The PMD should trigger RTE_ETH_EVENT_RECOVERED event to notify the application
> +that the it has recovered from the error condition. PMD re-configures the port
> +to the state prior to the error condition. Control path and data path are up now.
> +Since the device has undergone a reset, flow rules offloaded prior to reset
> +may be lost and the application should recreate the rules again.
> +

I think the most difficult part here is clarify what application should do
when this event received consistent for all devices, "flow rules may be lost"
looks very vague to me.
Unless it is not clear for application what to do when this event is received,
it is not that useful or it will be specific to some PMDs. And I can see it is
hard to clarify this but perhaps we can define a set of common behavior.
Not sure what others are thinking.

> +The PMD should trigger RTE_ETH_EVENT_INTR_RMV event to notify the application
> +that it has failed to recover from the error condition. The device may not be
> +usable anymore.
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index 147cc1c..a46819f 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -3818,6 +3818,24 @@ enum rte_eth_event_type {
>   	RTE_ETH_EVENT_DESTROY,  /**< port is released */
>   	RTE_ETH_EVENT_IPSEC,    /**< IPsec offload related event */
>   	RTE_ETH_EVENT_FLOW_AGED,/**< New aged-out flows is detected */
> +	RTE_ETH_EVENT_ERR_RECOVERING,
> +			/**< port recovering from an error
> +			 *
> +			 * PMD detected a FW reset or error condition.
> +			 * PMD will try to recover from the error.
> +			 * Data path may be quiesced and Control path operations
> +			 * may fail at this time.
> +			 */

Please put multi line comments before enum, Andrew did a set of cleanups for these.

> +	RTE_ETH_EVENT_RECOVERED,
> +			/**< port recovered from an error
> +			 *
> +			 * PMD has recovered from the error condition.
> +			 * Control path and Data path are up now.
> +			 * PMD re-configures the port to the state prior to the error.
> +			 * Since the device has undergone a reset, flow rules
> +			 * offloaded prior to reset may be lost and
> +			 * the application should recreate the rules again.
> +			 */
>   	RTE_ETH_EVENT_MAX       /**< max value of this enum */
>   };
>   

Re: [dpdk-dev] [PATCH v7 4/4] doc: update release notes

[Ferruh Yigit]

On 1/28/2022 12:48 PM, Kalesh A P wrote:
> From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> 
> Updated release notes with new error recovery event support.
> 

Instead of having a separate patch for release notes, please distribute the release
notes updates to the patches that actually does the documented change.

> Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> ---
>   doc/guides/rel_notes/release_22_03.rst | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/release_22_03.rst b/doc/guides/rel_notes/release_22_03.rst
> index 33be324..88755ba 100644
> --- a/doc/guides/rel_notes/release_22_03.rst
> +++ b/doc/guides/rel_notes/release_22_03.rst
> @@ -24,6 +24,21 @@ DPDK Release 22.03
>   New Features
>   ------------
>   
> +* **Added error recovery support.**
> +
> +  Added error recovery support to detect and recover from device errors including:
> +
> +  * Added new event: ``RTE_ETH_EVENT_ERR_RECOVERING`` for the driver to report
> +    that the port is recovering from an error.
> +  * Added new event: ``RTE_ETH_EVENT_RECOVERED`` for the driver to report
> +    that the port has recovered from an error.
> +
> +* **Updated Broadcom bnxt driver.**
> +
> +  Updated the Broadcom bnxt driver with new features and improvements, including:
> +
> +  * Added support to handle device recovery events.
> +
>   .. This section should contain new features added in this release.
>      Sample format:
>   

Re: [dpdk-dev] [PATCH v7 1/4] ethdev: support device reset and recovery events

[Ferruh Yigit]

On 1/28/2022 12:48 PM, Kalesh A P wrote:
> From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> 
> Adding support for the device reset and recovery events in the
> rte_eth_event framework. FW error and FW reset conditions would be
> managed internally by the PMD without needing application intervention.
> In such cases, PMD would need reset/recovery events to notify application
> that PMD is undergoing a reset.
> 
> While most of the recovery process is transparent to the application since
> most of the driver ensures recovery from FW reset or FW error conditions,
> the application will have to reprogram any flows which were offloaded to
> the underlying hardware.
> 
> Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
> Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> ---
>   doc/guides/prog_guide/poll_mode_drv.rst | 24 ++++++++++++++++++++++++
>   lib/ethdev/rte_ethdev.h                 | 18 ++++++++++++++++++
>   2 files changed, 42 insertions(+)
> 
> diff --git a/doc/guides/prog_guide/poll_mode_drv.rst b/doc/guides/prog_guide/poll_mode_drv.rst
> index 6831289..9ecc0e4 100644
> --- a/doc/guides/prog_guide/poll_mode_drv.rst
> +++ b/doc/guides/prog_guide/poll_mode_drv.rst
> @@ -623,3 +623,27 @@ by application.
>   The PMD itself should not call rte_eth_dev_reset(). The PMD can trigger
>   the application to handle reset event. It is duty of application to
>   handle all synchronization before it calls rte_eth_dev_reset().
> +
> +Error recovery support
> +~~~~~~~~~~~~~~~~~~~~~~
> +
> +When the PMD detects a FW reset or error condition, it may try to recover
> +from the error without needing the application intervention. In such cases,
> +PMD would need events to notify the application that it is undergoing
> +an error recovery.
> +
> +The PMD should trigger RTE_ETH_EVENT_ERR_RECOVERING event to notify the
> +application that PMD detected a FW reset or FW error condition. PMD may
> +try to recover from the error by itself. Data path may be quiesced and
> +control path operations may fail during the recovery period. The application
> +should stop polling till it receives RTE_ETH_EVENT_RECOVERED event from the PMD.
> +
> +The PMD should trigger RTE_ETH_EVENT_RECOVERED event to notify the application
> +that the it has recovered from the error condition. PMD re-configures the port
> +to the state prior to the error condition. Control path and data path are up now.
> +Since the device has undergone a reset, flow rules offloaded prior to reset
> +may be lost and the application should recreate the rules again.
> +
> +The PMD should trigger RTE_ETH_EVENT_INTR_RMV event to notify the application
> +that it has failed to recover from the error condition. The device may not be
> +usable anymore.
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index 147cc1c..a46819f 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -3818,6 +3818,24 @@ enum rte_eth_event_type {
>   	RTE_ETH_EVENT_DESTROY,  /**< port is released */
>   	RTE_ETH_EVENT_IPSEC,    /**< IPsec offload related event */
>   	RTE_ETH_EVENT_FLOW_AGED,/**< New aged-out flows is detected */
> +	RTE_ETH_EVENT_ERR_RECOVERING,
> +			/**< port recovering from an error
> +			 *
> +			 * PMD detected a FW reset or error condition.
> +			 * PMD will try to recover from the error.
> +			 * Data path may be quiesced and Control path operations
> +			 * may fail at this time.
> +			 */
> +	RTE_ETH_EVENT_RECOVERED,
> +			/**< port recovered from an error
> +			 *
> +			 * PMD has recovered from the error condition.
> +			 * Control path and Data path are up now.
> +			 * PMD re-configures the port to the state prior to the error.
> +			 * Since the device has undergone a reset, flow rules
> +			 * offloaded prior to reset may be lost and
> +			 * the application should recreate the rules again.
> +			 */
>   	RTE_ETH_EVENT_MAX       /**< max value of this enum */


Also ABI check complains about 'RTE_ETH_EVENT_MAX' value check, cc'ed more people
to evaluate if it is a false positive:


1 function with some indirect sub-type change:
   [C] 'function int rte_eth_dev_callback_register(uint16_t, rte_eth_event_type, rte_eth_dev_cb_fn, void*)' at rte_ethdev.c:4637:1 has some indirect sub-type changes:
     parameter 3 of type 'typedef rte_eth_dev_cb_fn' has sub-type changes:
       underlying type 'int (typedef uint16_t, enum rte_eth_event_type, void*, void*)*' changed:
         in pointed to type 'function type int (typedef uint16_t, enum rte_eth_event_type, void*, void*)':
           parameter 2 of type 'enum rte_eth_event_type' has sub-type changes:
             type size hasn't changed
             2 enumerator insertions:
               'rte_eth_event_type::RTE_ETH_EVENT_ERR_RECOVERING' value '11'
               'rte_eth_event_type::RTE_ETH_EVENT_RECOVERED' value '12'
             1 enumerator change:
               'rte_eth_event_type::RTE_ETH_EVENT_MAX' from value '11' to '13' at rte_ethdev.h:3807:1

Re: [dpdk-dev] [PATCH v7 1/4] ethdev: support device reset and recovery events

[Kalesh Anakkur Purayil]

[-- Attachment #1: Type: text/plain, Size: 6089 bytes --]

Thank you Ferruh for the review. Please see inline.

On Tue, Feb 1, 2022 at 5:41 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 1/28/2022 12:48 PM, Kalesh A P wrote:
> > From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> >
> > Adding support for the device reset and recovery events in the
> > rte_eth_event framework. FW error and FW reset conditions would be
> > managed internally by the PMD without needing application intervention.
> > In such cases, PMD would need reset/recovery events to notify application
> > that PMD is undergoing a reset.
> >
> > While most of the recovery process is transparent to the application
> since
> > most of the driver ensures recovery from FW reset or FW error conditions,
> > the application will have to reprogram any flows which were offloaded to
> > the underlying hardware.
> >
> > Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> > Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
> > Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
>
> More developer cc'ed.
>
> > ---
> >   doc/guides/prog_guide/poll_mode_drv.rst | 24 ++++++++++++++++++++++++
> >   lib/ethdev/rte_ethdev.h                 | 18 ++++++++++++++++++
> >   2 files changed, 42 insertions(+)
> >
> > diff --git a/doc/guides/prog_guide/poll_mode_drv.rst
> b/doc/guides/prog_guide/poll_mode_drv.rst
> > index 6831289..9ecc0e4 100644
> > --- a/doc/guides/prog_guide/poll_mode_drv.rst
> > +++ b/doc/guides/prog_guide/poll_mode_drv.rst
> > @@ -623,3 +623,27 @@ by application.
> >   The PMD itself should not call rte_eth_dev_reset(). The PMD can trigger
> >   the application to handle reset event. It is duty of application to
> >   handle all synchronization before it calls rte_eth_dev_reset().
> > +
> > +Error recovery support
> > +~~~~~~~~~~~~~~~~~~~~~~
> > +
> > +When the PMD detects a FW reset or error condition, it may try to
> recover
> > +from the error without needing the application intervention. In such
> cases,
> > +PMD would need events to notify the application that it is undergoing
> > +an error recovery.
> > +
> > +The PMD should trigger RTE_ETH_EVENT_ERR_RECOVERING event to notify the
> > +application that PMD detected a FW reset or FW error condition. PMD may
> > +try to recover from the error by itself. Data path may be quiesced and
> > +control path operations may fail during the recovery period. The
> application
> > +should stop polling till it receives RTE_ETH_EVENT_RECOVERED event from
> the PMD.
> > +
>
> Between the time FW error occurred and the application receive the
> RECOVERING event,
> datapath will continue to poll and application may call control APIs, so
> the event
> really is not solving the issue and driver somehow should be sure this
> won't crash
> the application, in that case not sure about the benefit of this event.
>
[Kalesh]: As soon as the driver detects a FW dead or reset condition, it
sets the fastpath pointers to dummy functions. This will prevent the crash.
All control path operations would fail with -EBUSY. This change is already
there in bnxt PMD. This event is a notification to the application that the
PMD is recovering from a FW error condition so that it can stop polling and
issue control path operations.

>
> > +The PMD should trigger RTE_ETH_EVENT_RECOVERED event to notify the
> application
> > +that the it has recovered from the error condition. PMD re-configures
> the port
> > +to the state prior to the error condition. Control path and data path
> are up now.
> > +Since the device has undergone a reset, flow rules offloaded prior to
> reset
> > +may be lost and the application should recreate the rules again.
> > +
>
> I think the most difficult part here is clarify what application should do
> when this event received consistent for all devices, "flow rules may be
> lost"
> looks very vague to me.
> Unless it is not clear for application what to do when this event is
> received,
> it is not that useful or it will be specific to some PMDs. And I can see
> it is
> hard to clarify this but perhaps we can define a set of common behavior.
> Not sure what others are thinking.
>
[Kalesh]: Sure, let's wait for others' opinions as well.

>
> > +The PMD should trigger RTE_ETH_EVENT_INTR_RMV event to notify the
> application
> > +that it has failed to recover from the error condition. The device may
> not be
> > +usable anymore.
> > diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> > index 147cc1c..a46819f 100644
> > --- a/lib/ethdev/rte_ethdev.h
> > +++ b/lib/ethdev/rte_ethdev.h
> > @@ -3818,6 +3818,24 @@ enum rte_eth_event_type {
> >       RTE_ETH_EVENT_DESTROY,  /**< port is released */
> >       RTE_ETH_EVENT_IPSEC,    /**< IPsec offload related event */
> >       RTE_ETH_EVENT_FLOW_AGED,/**< New aged-out flows is detected */
> > +     RTE_ETH_EVENT_ERR_RECOVERING,
> > +                     /**< port recovering from an error
> > +                      *
> > +                      * PMD detected a FW reset or error condition.
> > +                      * PMD will try to recover from the error.
> > +                      * Data path may be quiesced and Control path
> operations
> > +                      * may fail at this time.
> > +                      */
>
> Please put multi line comments before enum, Andrew did a set of cleanups
> for these.
>
[Kalesh]: Sure, will do.

>
> > +     RTE_ETH_EVENT_RECOVERED,
> > +                     /**< port recovered from an error
> > +                      *
> > +                      * PMD has recovered from the error condition.
> > +                      * Control path and Data path are up now.
> > +                      * PMD re-configures the port to the state prior
> to the error.
> > +                      * Since the device has undergone a reset, flow
> rules
> > +                      * offloaded prior to reset may be lost and
> > +                      * the application should recreate the rules again.
> > +                      */
> >       RTE_ETH_EVENT_MAX       /**< max value of this enum */
> >   };
> >
>
>

-- 
Regards,
Kalesh A P

[-- Attachment #2: Type: text/html, Size: 8030 bytes --]

Re: [dpdk-dev] [PATCH v7 1/4] ethdev: support device reset and recovery events

[Ferruh Yigit]

On 2/1/2022 1:09 PM, Kalesh Anakkur Purayil wrote:
> Thank you Ferruh for the review. Please see inline.
> 
> On Tue, Feb 1, 2022 at 5:41 PM Ferruh Yigit <ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>> wrote:
> 
>     On 1/28/2022 12:48 PM, Kalesh A P wrote:
>      > From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com <mailto:kalesh-anakkur.purayil@broadcom.com>>
>      >
>      > Adding support for the device reset and recovery events in the
>      > rte_eth_event framework. FW error and FW reset conditions would be
>      > managed internally by the PMD without needing application intervention.
>      > In such cases, PMD would need reset/recovery events to notify application
>      > that PMD is undergoing a reset.
>      >
>      > While most of the recovery process is transparent to the application since
>      > most of the driver ensures recovery from FW reset or FW error conditions,
>      > the application will have to reprogram any flows which were offloaded to
>      > the underlying hardware.
>      >
>      > Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com <mailto:kalesh-anakkur.purayil@broadcom.com>>
>      > Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com <mailto:somnath.kotur@broadcom.com>>
>      > Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com <mailto:ajit.khaparde@broadcom.com>>
> 
>     More developer cc'ed.
> 
>      > ---
>      >   doc/guides/prog_guide/poll_mode_drv.rst | 24 ++++++++++++++++++++++++
>      >   lib/ethdev/rte_ethdev.h                 | 18 ++++++++++++++++++
>      >   2 files changed, 42 insertions(+)
>      >
>      > diff --git a/doc/guides/prog_guide/poll_mode_drv.rst b/doc/guides/prog_guide/poll_mode_drv.rst
>      > index 6831289..9ecc0e4 100644
>      > --- a/doc/guides/prog_guide/poll_mode_drv.rst
>      > +++ b/doc/guides/prog_guide/poll_mode_drv.rst
>      > @@ -623,3 +623,27 @@ by application.
>      >   The PMD itself should not call rte_eth_dev_reset(). The PMD can trigger
>      >   the application to handle reset event. It is duty of application to
>      >   handle all synchronization before it calls rte_eth_dev_reset().
>      > +
>      > +Error recovery support
>      > +~~~~~~~~~~~~~~~~~~~~~~
>      > +
>      > +When the PMD detects a FW reset or error condition, it may try to recover
>      > +from the error without needing the application intervention. In such cases,
>      > +PMD would need events to notify the application that it is undergoing
>      > +an error recovery.
>      > +
>      > +The PMD should trigger RTE_ETH_EVENT_ERR_RECOVERING event to notify the
>      > +application that PMD detected a FW reset or FW error condition. PMD may
>      > +try to recover from the error by itself. Data path may be quiesced and
>      > +control path operations may fail during the recovery period. The application
>      > +should stop polling till it receives RTE_ETH_EVENT_RECOVERED event from the PMD.
>      > +
> 
>     Between the time FW error occurred and the application receive the RECOVERING event,
>     datapath will continue to poll and application may call control APIs, so the event
>     really is not solving the issue and driver somehow should be sure this won't crash
>     the application, in that case not sure about the benefit of this event.
> 
> [Kalesh]: As soon as the driver detects a FW dead or reset condition, it sets the fastpath pointers to dummy functions. This will prevent the crash. All control path operations would fail with -EBUSY. This change is already there in bnxt PMD. This event is a notification to the application that the PMD is recovering from a FW error condition so that it can stop polling and issue control path operations.
> 

This was my point indeed. PMD can't rely on this event and should take actions (for both
datapath and control path) to prevent possible crash/error. If that is the case event
doesn't have that much benefit since PMD already has to protect itself.

> 
>      > +The PMD should trigger RTE_ETH_EVENT_RECOVERED event to notify the application
>      > +that the it has recovered from the error condition. PMD re-configures the port
>      > +to the state prior to the error condition. Control path and data path are up now.
>      > +Since the device has undergone a reset, flow rules offloaded prior to reset
>      > +may be lost and the application should recreate the rules again.
>      > +
> 
>     I think the most difficult part here is clarify what application should do
>     when this event received consistent for all devices, "flow rules may be lost"
>     looks very vague to me.
>     Unless it is not clear for application what to do when this event is received,
>     it is not that useful or it will be specific to some PMDs. And I can see it is
>     hard to clarify this but perhaps we can define a set of common behavior.
>     Not sure what others are thinking.
> 
> [Kalesh]: Sure, let's wait for others' opinions as well.
> 
> 
>      > +The PMD should trigger RTE_ETH_EVENT_INTR_RMV event to notify the application
>      > +that it has failed to recover from the error condition. The device may not be
>      > +usable anymore.
>      > diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
>      > index 147cc1c..a46819f 100644
>      > --- a/lib/ethdev/rte_ethdev.h
>      > +++ b/lib/ethdev/rte_ethdev.h
>      > @@ -3818,6 +3818,24 @@ enum rte_eth_event_type {
>      >       RTE_ETH_EVENT_DESTROY,  /**< port is released */
>      >       RTE_ETH_EVENT_IPSEC,    /**< IPsec offload related event */
>      >       RTE_ETH_EVENT_FLOW_AGED,/**< New aged-out flows is detected */
>      > +     RTE_ETH_EVENT_ERR_RECOVERING,
>      > +                     /**< port recovering from an error
>      > +                      *
>      > +                      * PMD detected a FW reset or error condition.
>      > +                      * PMD will try to recover from the error.
>      > +                      * Data path may be quiesced and Control path operations
>      > +                      * may fail at this time.
>      > +                      */
> 
>     Please put multi line comments before enum, Andrew did a set of cleanups for these.
> 
> [Kalesh]: Sure, will do.
> 
> 
>      > +     RTE_ETH_EVENT_RECOVERED,
>      > +                     /**< port recovered from an error
>      > +                      *
>      > +                      * PMD has recovered from the error condition.
>      > +                      * Control path and Data path are up now.
>      > +                      * PMD re-configures the port to the state prior to the error.
>      > +                      * Since the device has undergone a reset, flow rules
>      > +                      * offloaded prior to reset may be lost and
>      > +                      * the application should recreate the rules again.
>      > +                      */
>      >       RTE_ETH_EVENT_MAX       /**< max value of this enum */
>      >   };
>      >
> 
> 
> 
> -- 
> Regards,
> Kalesh A P

Re: [dpdk-dev] [PATCH v7 1/4] ethdev: support device reset and recovery events

[Ray Kinsella]

Ferruh Yigit <ferruh.yigit@intel.com> writes:

> On 1/28/2022 12:48 PM, Kalesh A P wrote:
>> From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
>> Adding support for the device reset and recovery events in the
>> rte_eth_event framework. FW error and FW reset conditions would be
>> managed internally by the PMD without needing application intervention.
>> In such cases, PMD would need reset/recovery events to notify application
>> that PMD is undergoing a reset.
>> While most of the recovery process is transparent to the application since
>> most of the driver ensures recovery from FW reset or FW error conditions,
>> the application will have to reprogram any flows which were offloaded to
>> the underlying hardware.
>> Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
>> Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
>> Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
>> ---
>>   doc/guides/prog_guide/poll_mode_drv.rst | 24 ++++++++++++++++++++++++
>>   lib/ethdev/rte_ethdev.h                 | 18 ++++++++++++++++++
>>   2 files changed, 42 insertions(+)
>> diff --git a/doc/guides/prog_guide/poll_mode_drv.rst
>> b/doc/guides/prog_guide/poll_mode_drv.rst
>> index 6831289..9ecc0e4 100644
>> --- a/doc/guides/prog_guide/poll_mode_drv.rst
>> +++ b/doc/guides/prog_guide/poll_mode_drv.rst
>> @@ -623,3 +623,27 @@ by application.
>>   The PMD itself should not call rte_eth_dev_reset(). The PMD can trigger
>>   the application to handle reset event. It is duty of application to
>>   handle all synchronization before it calls rte_eth_dev_reset().
>> +
>> +Error recovery support
>> +~~~~~~~~~~~~~~~~~~~~~~
>> +
>> +When the PMD detects a FW reset or error condition, it may try to recover
>> +from the error without needing the application intervention. In such cases,
>> +PMD would need events to notify the application that it is undergoing
>> +an error recovery.
>> +
>> +The PMD should trigger RTE_ETH_EVENT_ERR_RECOVERING event to notify the
>> +application that PMD detected a FW reset or FW error condition. PMD may
>> +try to recover from the error by itself. Data path may be quiesced and
>> +control path operations may fail during the recovery period. The application
>> +should stop polling till it receives RTE_ETH_EVENT_RECOVERED event from the PMD.
>> +
>> +The PMD should trigger RTE_ETH_EVENT_RECOVERED event to notify the application
>> +that the it has recovered from the error condition. PMD re-configures the port
>> +to the state prior to the error condition. Control path and data path are up now.
>> +Since the device has undergone a reset, flow rules offloaded prior to reset
>> +may be lost and the application should recreate the rules again.
>> +
>> +The PMD should trigger RTE_ETH_EVENT_INTR_RMV event to notify the application
>> +that it has failed to recover from the error condition. The device may not be
>> +usable anymore.
>> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
>> index 147cc1c..a46819f 100644
>> --- a/lib/ethdev/rte_ethdev.h
>> +++ b/lib/ethdev/rte_ethdev.h
>> @@ -3818,6 +3818,24 @@ enum rte_eth_event_type {
>>   	RTE_ETH_EVENT_DESTROY,  /**< port is released */
>>   	RTE_ETH_EVENT_IPSEC,    /**< IPsec offload related event */
>>   	RTE_ETH_EVENT_FLOW_AGED,/**< New aged-out flows is detected */
>> +	RTE_ETH_EVENT_ERR_RECOVERING,
>> +			/**< port recovering from an error
>> +			 *
>> +			 * PMD detected a FW reset or error condition.
>> +			 * PMD will try to recover from the error.
>> +			 * Data path may be quiesced and Control path operations
>> +			 * may fail at this time.
>> +			 */
>> +	RTE_ETH_EVENT_RECOVERED,
>> +			/**< port recovered from an error
>> +			 *
>> +			 * PMD has recovered from the error condition.
>> +			 * Control path and Data path are up now.
>> +			 * PMD re-configures the port to the state prior to the error.
>> +			 * Since the device has undergone a reset, flow rules
>> +			 * offloaded prior to reset may be lost and
>> +			 * the application should recreate the rules again.
>> +			 */
>>   	RTE_ETH_EVENT_MAX       /**< max value of this enum */
>
>
> Also ABI check complains about 'RTE_ETH_EVENT_MAX' value check, cc'ed more people
> to evaluate if it is a false positive:
>
>
> 1 function with some indirect sub-type change:
>   [C] 'function int rte_eth_dev_callback_register(uint16_t, rte_eth_event_type, rte_eth_dev_cb_fn, void*)' at rte_ethdev.c:4637:1 has some indirect sub-type changes:
>     parameter 3 of type 'typedef rte_eth_dev_cb_fn' has sub-type changes:
>       underlying type 'int (typedef uint16_t, enum rte_eth_event_type, void*, void*)*' changed:
>         in pointed to type 'function type int (typedef uint16_t, enum rte_eth_event_type, void*, void*)':
>           parameter 2 of type 'enum rte_eth_event_type' has sub-type changes:
>             type size hasn't changed
>             2 enumerator insertions:
>               'rte_eth_event_type::RTE_ETH_EVENT_ERR_RECOVERING' value '11'
>               'rte_eth_event_type::RTE_ETH_EVENT_RECOVERED' value '12'
>             1 enumerator change:
>               'rte_eth_event_type::RTE_ETH_EVENT_MAX' from value '11' to '13' at rte_ethdev.h:3807:1

I don't immediately see the problem that this would cause.
There are no array sizes etc dependent on the value of MAX for instance.

Looks safe?

-- 
Regards, Ray K

Re: [dpdk-dev] [PATCH v7 1/4] ethdev: support device reset and recovery events

[Ajit Khaparde]

[-- Attachment #1: Type: text/plain, Size: 8887 bytes --]

On Tue, Feb 1, 2022 at 5:20 AM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> On 2/1/2022 1:09 PM, Kalesh Anakkur Purayil wrote:
> > Thank you Ferruh for the review. Please see inline.
> >
> > On Tue, Feb 1, 2022 at 5:41 PM Ferruh Yigit <ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>> wrote:
> >
> >     On 1/28/2022 12:48 PM, Kalesh A P wrote:
> >      > From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com <mailto:kalesh-anakkur.purayil@broadcom.com>>
> >      >
> >      > Adding support for the device reset and recovery events in the
> >      > rte_eth_event framework. FW error and FW reset conditions would be
> >      > managed internally by the PMD without needing application intervention.
> >      > In such cases, PMD would need reset/recovery events to notify application
> >      > that PMD is undergoing a reset.
> >      >
> >      > While most of the recovery process is transparent to the application since
> >      > most of the driver ensures recovery from FW reset or FW error conditions,
> >      > the application will have to reprogram any flows which were offloaded to
> >      > the underlying hardware.
> >      >
> >      > Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com <mailto:kalesh-anakkur.purayil@broadcom.com>>
> >      > Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com <mailto:somnath.kotur@broadcom.com>>
> >      > Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com <mailto:ajit.khaparde@broadcom.com>>
> >
> >     More developer cc'ed.
> >
> >      > ---
> >      >   doc/guides/prog_guide/poll_mode_drv.rst | 24 ++++++++++++++++++++++++
> >      >   lib/ethdev/rte_ethdev.h                 | 18 ++++++++++++++++++
> >      >   2 files changed, 42 insertions(+)
> >      >
> >      > diff --git a/doc/guides/prog_guide/poll_mode_drv.rst b/doc/guides/prog_guide/poll_mode_drv.rst
> >      > index 6831289..9ecc0e4 100644
> >      > --- a/doc/guides/prog_guide/poll_mode_drv.rst
> >      > +++ b/doc/guides/prog_guide/poll_mode_drv.rst
> >      > @@ -623,3 +623,27 @@ by application.
> >      >   The PMD itself should not call rte_eth_dev_reset(). The PMD can trigger
> >      >   the application to handle reset event. It is duty of application to
> >      >   handle all synchronization before it calls rte_eth_dev_reset().
> >      > +
> >      > +Error recovery support
> >      > +~~~~~~~~~~~~~~~~~~~~~~
> >      > +
> >      > +When the PMD detects a FW reset or error condition, it may try to recover
> >      > +from the error without needing the application intervention. In such cases,
> >      > +PMD would need events to notify the application that it is undergoing
> >      > +an error recovery.
> >      > +
> >      > +The PMD should trigger RTE_ETH_EVENT_ERR_RECOVERING event to notify the
> >      > +application that PMD detected a FW reset or FW error condition. PMD may
> >      > +try to recover from the error by itself. Data path may be quiesced and
> >      > +control path operations may fail during the recovery period. The application
> >      > +should stop polling till it receives RTE_ETH_EVENT_RECOVERED event from the PMD.
> >      > +
> >
> >     Between the time FW error occurred and the application receive the RECOVERING event,
> >     datapath will continue to poll and application may call control APIs, so the event
> >     really is not solving the issue and driver somehow should be sure this won't crash
> >     the application, in that case not sure about the benefit of this event.
> >
> > [Kalesh]: As soon as the driver detects a FW dead or reset condition, it sets the fastpath pointers to dummy functions. This will prevent the crash. All control path operations would fail with -EBUSY. This change is already there in bnxt PMD. This event is a notification to the application that the PMD is recovering from a FW error condition so that it can stop polling and issue control path operations.
> >
>
> This was my point indeed. PMD can't rely on this event and should take actions (for both
> datapath and control path) to prevent possible crash/error. If that is the case event
> doesn't have that much benefit since PMD already has to protect itself.

This is an attempt to prevent PMD and applications from encountering
unexpected situations because of an error in hardware or firmware.
The unexpected scenarios can include lockups to segfaults apart from
other situations.

When hardware or firmware encounters and error, propagating it to the
application for it to react can be slow, time consuming and could be
vendor specific.

Instead in the case of Broadcom devices we are trying to contain the
detection and recovery within the hardware, firmware mostly and to a
small extent driver framework.
In the case of kernel drivers, this allows avoiding a system reset as
much as possible while in the case of DPDK PMD, it can prevent
application reloads or ungraceful exits.

The PMD generated notification to the application will be more like a
"For Your Information" for most of the applications.
But applications can decide to act on the notification and take
specific steps - like flushing or deleting all the existing flow
rules, meters etc.. without waiting for success or failure indication.
Applications can also re-offload the flow rules, recreate meters based
on the notification allowing them to sync with the fresh hardware,
firmware state.

We think other PMD can also take advantage of these notifications to
provide better reliability, accessibility and serviceability in
general.

Thanks

Ajit
>
> >
> >      > +The PMD should trigger RTE_ETH_EVENT_RECOVERED event to notify the application
> >      > +that the it has recovered from the error condition. PMD re-configures the port
> >      > +to the state prior to the error condition. Control path and data path are up now.
> >      > +Since the device has undergone a reset, flow rules offloaded prior to reset
> >      > +may be lost and the application should recreate the rules again.
> >      > +
> >
> >     I think the most difficult part here is clarify what application should do
> >     when this event received consistent for all devices, "flow rules may be lost"
> >     looks very vague to me.
> >     Unless it is not clear for application what to do when this event is received,
> >     it is not that useful or it will be specific to some PMDs. And I can see it is
> >     hard to clarify this but perhaps we can define a set of common behavior.
> >     Not sure what others are thinking.
> >
> > [Kalesh]: Sure, let's wait for others' opinions as well.
> >
> >
> >      > +The PMD should trigger RTE_ETH_EVENT_INTR_RMV event to notify the application
> >      > +that it has failed to recover from the error condition. The device may not be
> >      > +usable anymore.
> >      > diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> >      > index 147cc1c..a46819f 100644
> >      > --- a/lib/ethdev/rte_ethdev.h
> >      > +++ b/lib/ethdev/rte_ethdev.h
> >      > @@ -3818,6 +3818,24 @@ enum rte_eth_event_type {
> >      >       RTE_ETH_EVENT_DESTROY,  /**< port is released */
> >      >       RTE_ETH_EVENT_IPSEC,    /**< IPsec offload related event */
> >      >       RTE_ETH_EVENT_FLOW_AGED,/**< New aged-out flows is detected */
> >      > +     RTE_ETH_EVENT_ERR_RECOVERING,
> >      > +                     /**< port recovering from an error
> >      > +                      *
> >      > +                      * PMD detected a FW reset or error condition.
> >      > +                      * PMD will try to recover from the error.
> >      > +                      * Data path may be quiesced and Control path operations
> >      > +                      * may fail at this time.
> >      > +                      */
> >
> >     Please put multi line comments before enum, Andrew did a set of cleanups for these.
> >
> > [Kalesh]: Sure, will do.
> >
> >
> >      > +     RTE_ETH_EVENT_RECOVERED,
> >      > +                     /**< port recovered from an error
> >      > +                      *
> >      > +                      * PMD has recovered from the error condition.
> >      > +                      * Control path and Data path are up now.
> >      > +                      * PMD re-configures the port to the state prior to the error.
> >      > +                      * Since the device has undergone a reset, flow rules
> >      > +                      * offloaded prior to reset may be lost and
> >      > +                      * the application should recreate the rules again.
> >      > +                      */
> >      >       RTE_ETH_EVENT_MAX       /**< max value of this enum */
> >      >   };
> >      >
> >
> >
> >
> > --
> > Regards,
> > Kalesh A P
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4218 bytes --]

Re: [dpdk-dev] [PATCH v7 1/4] ethdev: support device reset and recovery events

[Thomas Monjalon]

02/02/2022 12:44, Ray Kinsella:
> Ferruh Yigit <ferruh.yigit@intel.com> writes:
> > On 1/28/2022 12:48 PM, Kalesh A P wrote:
> >> --- a/lib/ethdev/rte_ethdev.h
> >> +++ b/lib/ethdev/rte_ethdev.h
> >> @@ -3818,6 +3818,24 @@ enum rte_eth_event_type {
> >>   	RTE_ETH_EVENT_DESTROY,  /**< port is released */
> >>   	RTE_ETH_EVENT_IPSEC,    /**< IPsec offload related event */
> >>   	RTE_ETH_EVENT_FLOW_AGED,/**< New aged-out flows is detected */
> >> +	RTE_ETH_EVENT_ERR_RECOVERING,
> >> +			/**< port recovering from an error
> >> +			 *
> >> +			 * PMD detected a FW reset or error condition.
> >> +			 * PMD will try to recover from the error.
> >> +			 * Data path may be quiesced and Control path operations
> >> +			 * may fail at this time.
> >> +			 */
> >> +	RTE_ETH_EVENT_RECOVERED,
> >> +			/**< port recovered from an error
> >> +			 *
> >> +			 * PMD has recovered from the error condition.
> >> +			 * Control path and Data path are up now.
> >> +			 * PMD re-configures the port to the state prior to the error.
> >> +			 * Since the device has undergone a reset, flow rules
> >> +			 * offloaded prior to reset may be lost and
> >> +			 * the application should recreate the rules again.
> >> +			 */
> >>   	RTE_ETH_EVENT_MAX       /**< max value of this enum */
> >
> >
> > Also ABI check complains about 'RTE_ETH_EVENT_MAX' value check, cc'ed more people
> > to evaluate if it is a false positive:
> >
> >
> > 1 function with some indirect sub-type change:
> >   [C] 'function int rte_eth_dev_callback_register(uint16_t, rte_eth_event_type, rte_eth_dev_cb_fn, void*)' at rte_ethdev.c:4637:1 has some indirect sub-type changes:
> >     parameter 3 of type 'typedef rte_eth_dev_cb_fn' has sub-type changes:
> >       underlying type 'int (typedef uint16_t, enum rte_eth_event_type, void*, void*)*' changed:
> >         in pointed to type 'function type int (typedef uint16_t, enum rte_eth_event_type, void*, void*)':
> >           parameter 2 of type 'enum rte_eth_event_type' has sub-type changes:
> >             type size hasn't changed
> >             2 enumerator insertions:
> >               'rte_eth_event_type::RTE_ETH_EVENT_ERR_RECOVERING' value '11'
> >               'rte_eth_event_type::RTE_ETH_EVENT_RECOVERED' value '12'
> >             1 enumerator change:
> >               'rte_eth_event_type::RTE_ETH_EVENT_MAX' from value '11' to '13' at rte_ethdev.h:3807:1
> 
> I don't immediately see the problem that this would cause.
> There are no array sizes etc dependent on the value of MAX for instance.
> 
> Looks safe?

We never know how this enum will be used by the application.
The max value may be used for the size of an event array.
It looks a real ABI issue unfortunately.

PS: I am not Cc'ed in this patchset,
so copying what I said on v6 (more than a year ago):
Please use the option --cc-cmd devtools/get-maintainer.sh

Re: [dpdk-dev] [PATCH v7 1/4] ethdev: support device reset and recovery events

[Thomas Monjalon]

03/02/2022 21:28, Ajit Khaparde:
> On Tue, Feb 1, 2022 at 5:20 AM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> > On 2/1/2022 1:09 PM, Kalesh Anakkur Purayil wrote:
> > > On Tue, Feb 1, 2022 at 5:41 PM Ferruh Yigit <ferruh.yigit@intel.com <mailto:ferruh.yigit@intel.com>> wrote:
> > >     On 1/28/2022 12:48 PM, Kalesh A P wrote:
> > >      > From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com <mailto:kalesh-anakkur.purayil@broadcom.com>>
> > >      > --- a/doc/guides/prog_guide/poll_mode_drv.rst
> > >      > +++ b/doc/guides/prog_guide/poll_mode_drv.rst
> > >      > +Error recovery support
> > >      > +~~~~~~~~~~~~~~~~~~~~~~
> > >      > +
> > >      > +When the PMD detects a FW reset or error condition, it may try to recover
> > >      > +from the error without needing the application intervention. In such cases,

Wrong, the application needs to react on the event.

> > >      > +PMD would need events to notify the application that it is undergoing
> > >      > +an error recovery.
> > >      > +
> > >      > +The PMD should trigger RTE_ETH_EVENT_ERR_RECOVERING event to notify the
> > >      > +application that PMD detected a FW reset or FW error condition. PMD may
> > >      > +try to recover from the error by itself. Data path may be quiesced and

PMD may try to recover by itself but the application must take action.

> > >      > +control path operations may fail during the recovery period. The application
> > >      > +should stop polling till it receives RTE_ETH_EVENT_RECOVERED event from the PMD.

The application should stop polling and doing anything else with the device,
otherwise it will receive an error code during the recovery,
meaning until the RECOVERED event is received.
Said like that, it is less vague, you see.

> > >     Between the time FW error occurred and the application receive the RECOVERING event,
> > >     datapath will continue to poll and application may call control APIs, so the event
> > >     really is not solving the issue and driver somehow should be sure this won't crash
> > >     the application, in that case not sure about the benefit of this event.
> > >
> > > [Kalesh]: As soon as the driver detects a FW dead or reset condition, it sets the fastpath pointers to dummy functions. This will prevent the crash. All control path operations would fail with -EBUSY. This change is already there in bnxt PMD. This event is a notification to the application that the PMD is recovering from a FW error condition so that it can stop polling and issue control path operations.
> >
> > This was my point indeed. PMD can't rely on this event and should take actions (for both
> > datapath and control path) to prevent possible crash/error. If that is the case event
> > doesn't have that much benefit since PMD already has to protect itself.
> 
> This is an attempt to prevent PMD and applications from encountering
> unexpected situations because of an error in hardware or firmware.
> The unexpected scenarios can include lockups to segfaults apart from
> other situations.
> 
> When hardware or firmware encounters and error, propagating it to the
> application for it to react can be slow, time consuming and could be
> vendor specific.
> 
> Instead in the case of Broadcom devices we are trying to contain the
> detection and recovery within the hardware, firmware mostly and to a
> small extent driver framework.
> In the case of kernel drivers, this allows avoiding a system reset as
> much as possible while in the case of DPDK PMD, it can prevent
> application reloads or ungraceful exits.
> 
> The PMD generated notification to the application will be more like a
> "For Your Information" for most of the applications.

The most important for the application is to be tolerant to the errors
returned in the datapath and in control API.

> But applications can decide to act on the notification and take
> specific steps - like flushing or deleting all the existing flow
> rules, meters etc.. without waiting for success or failure indication.
> Applications can also re-offload the flow rules, recreate meters based
> on the notification allowing them to sync with the fresh hardware,
> firmware state.

We need to know what has to be recreated.
I think we should keep the same rule as for a restart.

> We think other PMD can also take advantage of these notifications to
> provide better reliability, accessibility and serviceability in
> general.

Indeed others can benefit of some mechanisms if they are common
*and well explained*.

[...]
> > >      > +The PMD should trigger RTE_ETH_EVENT_RECOVERED event to notify the application

s/should/must/

> > >      > +that the it has recovered from the error condition. PMD re-configures the port
> > >      > +to the state prior to the error condition. Control path and data path are up now.
> > >      > +Since the device has undergone a reset, flow rules offloaded prior to reset
> > >      > +may be lost and the application should recreate the rules again.

Please refer to the restart situation, including these capabilities:

/** Device supports keeping flow rules across restart. */
#define RTE_ETH_DEV_CAPA_FLOW_RULE_KEEP         RTE_BIT64(3)
/** Device supports keeping shared flow objects across restart. */
#define RTE_ETH_DEV_CAPA_FLOW_SHARED_OBJECT_KEEP RTE_BIT64(4)

> > >     I think the most difficult part here is clarify what application should do
> > >     when this event received consistent for all devices, "flow rules may be lost"
> > >     looks very vague to me.

This is the rule for a restart:

 * Please note that some configuration is not stored between calls to
 * rte_eth_dev_stop()/rte_eth_dev_start(). The following configuration will
 * be retained:
 *
 *     - MTU
 *     - flow control settings
 *     - receive mode configuration (promiscuous mode, all-multicast mode,
 *       hardware checksum mode, RSS/VMDq settings etc.)
 *     - VLAN filtering configuration
 *     - default MAC address
 *     - MAC addresses supplied to MAC address array
 *     - flow director filtering mode (but not filtering rules)
 *     - NIC queue statistics mappings
 *
 * The following configuration may be retained or not
 * depending on the device capabilities:
 *
 *     - flow rules
 *     - flow-related shared objects, e.g. indirect actions
 *
 * Any other configuration will not be stored and will need to be re-entered
 * before a call to rte_eth_dev_start().

> > >     Unless it is not clear for application what to do when this event is received,
> > >     it is not that useful or it will be specific to some PMDs. And I can see it is
> > >     hard to clarify this but perhaps we can define a set of common behavior.
> > >     Not sure what others are thinking.

+1

> > > [Kalesh]: Sure, let's wait for others' opinions as well.

Nothing new, we already did such comment in 2020.

> > >      > +The PMD should trigger RTE_ETH_EVENT_INTR_RMV event to notify the application
> > >      > +that it has failed to recover from the error condition. The device may not be
> > >      > +usable anymore.

Yes good idea to use RMV event in case of failure.
You should update the comment of RTE_ETH_EVENT_INTR_RMV
to list cases when it is fired:
	- device unplugged
	- recovery failure
	- reset failure?

> > >      > diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> > >      > index 147cc1c..a46819f 100644
> > >      > --- a/lib/ethdev/rte_ethdev.h
> > >      > +++ b/lib/ethdev/rte_ethdev.h
> > >      > @@ -3818,6 +3818,24 @@ enum rte_eth_event_type {
> > >      >       RTE_ETH_EVENT_DESTROY,  /**< port is released */
> > >      >       RTE_ETH_EVENT_IPSEC,    /**< IPsec offload related event */
> > >      >       RTE_ETH_EVENT_FLOW_AGED,/**< New aged-out flows is detected */
> > >      > +     RTE_ETH_EVENT_ERR_RECOVERING,
> > >      > +                     /**< port recovering from an error
> > >      > +                      *
> > >      > +                      * PMD detected a FW reset or error condition.
> > >      > +                      * PMD will try to recover from the error.
> > >      > +                      * Data path may be quiesced and Control path operations
> > >      > +                      * may fail at this time.
> > >      > +                      */
> > >
> > >     Please put multi line comments before enum, Andrew did a set of cleanups for these.
> > >
> > > [Kalesh]: Sure, will do.
> > >
> > >
> > >      > +     RTE_ETH_EVENT_RECOVERED,
> > >      > +                     /**< port recovered from an error
> > >      > +                      *
> > >      > +                      * PMD has recovered from the error condition.
> > >      > +                      * Control path and Data path are up now.
> > >      > +                      * PMD re-configures the port to the state prior to the error.
> > >      > +                      * Since the device has undergone a reset, flow rules
> > >      > +                      * offloaded prior to reset may be lost and
> > >      > +                      * the application should recreate the rules again.
> > >      > +                      */

Same as for the RST, the comments need to be very clear,
while being concise here.

I think there are a lot of tips in this thread to make this mechanism
work in a generic way. Please work on a precise new version, thanks.

Re: [dpdk-dev] [PATCH v7 1/4] ethdev: support device reset and recovery events

[Ray Kinsella]

Thomas Monjalon <thomas@monjalon.net> writes:

> 02/02/2022 12:44, Ray Kinsella:
>> Ferruh Yigit <ferruh.yigit@intel.com> writes:
>> > On 1/28/2022 12:48 PM, Kalesh A P wrote:
>> >> --- a/lib/ethdev/rte_ethdev.h
>> >> +++ b/lib/ethdev/rte_ethdev.h
>> >> @@ -3818,6 +3818,24 @@ enum rte_eth_event_type {
>> >>   	RTE_ETH_EVENT_DESTROY,  /**< port is released */
>> >>   	RTE_ETH_EVENT_IPSEC,    /**< IPsec offload related event */
>> >>   	RTE_ETH_EVENT_FLOW_AGED,/**< New aged-out flows is detected */
>> >> +	RTE_ETH_EVENT_ERR_RECOVERING,
>> >> +			/**< port recovering from an error
>> >> +			 *
>> >> +			 * PMD detected a FW reset or error condition.
>> >> +			 * PMD will try to recover from the error.
>> >> +			 * Data path may be quiesced and Control path operations
>> >> +			 * may fail at this time.
>> >> +			 */
>> >> +	RTE_ETH_EVENT_RECOVERED,
>> >> +			/**< port recovered from an error
>> >> +			 *
>> >> +			 * PMD has recovered from the error condition.
>> >> +			 * Control path and Data path are up now.
>> >> +			 * PMD re-configures the port to the state prior to the error.
>> >> +			 * Since the device has undergone a reset, flow rules
>> >> +			 * offloaded prior to reset may be lost and
>> >> +			 * the application should recreate the rules again.
>> >> +			 */
>> >>   	RTE_ETH_EVENT_MAX       /**< max value of this enum */
>> >
>> >
>> > Also ABI check complains about 'RTE_ETH_EVENT_MAX' value check, cc'ed more people
>> > to evaluate if it is a false positive:
>> >
>> >
>> > 1 function with some indirect sub-type change:
>> >   [C] 'function int rte_eth_dev_callback_register(uint16_t, rte_eth_event_type, rte_eth_dev_cb_fn, void*)' at rte_ethdev.c:4637:1 has some indirect sub-type changes:
>> >     parameter 3 of type 'typedef rte_eth_dev_cb_fn' has sub-type changes:
>> >       underlying type 'int (typedef uint16_t, enum rte_eth_event_type, void*, void*)*' changed:
>> >         in pointed to type 'function type int (typedef uint16_t, enum rte_eth_event_type, void*, void*)':
>> >           parameter 2 of type 'enum rte_eth_event_type' has sub-type changes:
>> >             type size hasn't changed
>> >             2 enumerator insertions:
>> >               'rte_eth_event_type::RTE_ETH_EVENT_ERR_RECOVERING' value '11'
>> >               'rte_eth_event_type::RTE_ETH_EVENT_RECOVERED' value '12'
>> >             1 enumerator change:
>> >               'rte_eth_event_type::RTE_ETH_EVENT_MAX' from value '11' to '13' at rte_ethdev.h:3807:1
>> 
>> I don't immediately see the problem that this would cause.
>> There are no array sizes etc dependent on the value of MAX for instance.
>> 
>> Looks safe?
>
> We never know how this enum will be used by the application.
> The max value may be used for the size of an event array.
> It looks a real ABI issue unfortunately.

Right - but we only really care about it when an array size based on MAX
is likely to be passed to DPDK, which doesn't apply in this case.

I noted that some Linux folks explicitly mark similar MAX values as not
part of the ABI.

/usr/include/linux/perf_event.h
37:     PERF_TYPE_MAX,                          /* non-ABI */
60:     PERF_COUNT_HW_MAX,                      /* non-ABI */
79:     PERF_COUNT_HW_CACHE_MAX,                /* non-ABI */
87:     PERF_COUNT_HW_CACHE_OP_MAX,             /* non-ABI */
94:     PERF_COUNT_HW_CACHE_RESULT_MAX,         /* non-ABI */
116:    PERF_COUNT_SW_MAX,                      /* non-ABI */
149:    PERF_SAMPLE_MAX = 1U << 24,             /* non-ABI */
151:    __PERF_SAMPLE_CALLCHAIN_EARLY           = 1ULL << 63, /*
non-ABI; internal use */
189:    PERF_SAMPLE_BRANCH_MAX_SHIFT            /* non-ABI */
267:    PERF_TXN_MAX            = (1 << 8), /* non-ABI */
301:    PERF_FORMAT_MAX = 1U << 4,              /* non-ABI */
1067:   PERF_RECORD_MAX,                        /* non-ABI */
1078:   PERF_RECORD_KSYMBOL_TYPE_MAX            /* non-ABI */
1087:   PERF_BPF_EVENT_MAX,             /* non-ABI */

>
> PS: I am not Cc'ed in this patchset,
> so copying what I said on v6 (more than a year ago):
> Please use the option --cc-cmd devtools/get-maintainer.sh


-- 
Regards, Ray K

Re: [dpdk-dev] [PATCH v7 1/4] ethdev: support device reset and recovery events

[Ray Kinsella]

Ray Kinsella <mdr@ashroe.eu> writes:

> Thomas Monjalon <thomas@monjalon.net> writes:
>
>> 02/02/2022 12:44, Ray Kinsella:
>>> Ferruh Yigit <ferruh.yigit@intel.com> writes:
>>> > On 1/28/2022 12:48 PM, Kalesh A P wrote:
>>> >> --- a/lib/ethdev/rte_ethdev.h
>>> >> +++ b/lib/ethdev/rte_ethdev.h
>>> >> @@ -3818,6 +3818,24 @@ enum rte_eth_event_type {
>>> >>   	RTE_ETH_EVENT_DESTROY,  /**< port is released */
>>> >>   	RTE_ETH_EVENT_IPSEC,    /**< IPsec offload related event */
>>> >>   	RTE_ETH_EVENT_FLOW_AGED,/**< New aged-out flows is detected */
>>> >> +	RTE_ETH_EVENT_ERR_RECOVERING,
>>> >> +			/**< port recovering from an error
>>> >> +			 *
>>> >> +			 * PMD detected a FW reset or error condition.
>>> >> +			 * PMD will try to recover from the error.
>>> >> +			 * Data path may be quiesced and Control path operations
>>> >> +			 * may fail at this time.
>>> >> +			 */
>>> >> +	RTE_ETH_EVENT_RECOVERED,
>>> >> +			/**< port recovered from an error
>>> >> +			 *
>>> >> +			 * PMD has recovered from the error condition.
>>> >> +			 * Control path and Data path are up now.
>>> >> +			 * PMD re-configures the port to the state prior to the error.
>>> >> +			 * Since the device has undergone a reset, flow rules
>>> >> +			 * offloaded prior to reset may be lost and
>>> >> +			 * the application should recreate the rules again.
>>> >> +			 */
>>> >>   	RTE_ETH_EVENT_MAX       /**< max value of this enum */
>>> >
>>> >
>>> > Also ABI check complains about 'RTE_ETH_EVENT_MAX' value check, cc'ed more people
>>> > to evaluate if it is a false positive:
>>> >
>>> >
>>> > 1 function with some indirect sub-type change:
>>> >   [C] 'function int rte_eth_dev_callback_register(uint16_t, rte_eth_event_type, rte_eth_dev_cb_fn, void*)' at rte_ethdev.c:4637:1 has some indirect sub-type changes:
>>> >     parameter 3 of type 'typedef rte_eth_dev_cb_fn' has sub-type changes:
>>> >       underlying type 'int (typedef uint16_t, enum rte_eth_event_type, void*, void*)*' changed:
>>> >         in pointed to type 'function type int (typedef uint16_t, enum rte_eth_event_type, void*, void*)':
>>> >           parameter 2 of type 'enum rte_eth_event_type' has sub-type changes:
>>> >             type size hasn't changed
>>> >             2 enumerator insertions:
>>> >               'rte_eth_event_type::RTE_ETH_EVENT_ERR_RECOVERING' value '11'
>>> >               'rte_eth_event_type::RTE_ETH_EVENT_RECOVERED' value '12'
>>> >             1 enumerator change:
>>> >               'rte_eth_event_type::RTE_ETH_EVENT_MAX' from value '11' to '13' at rte_ethdev.h:3807:1
>>> 
>>> I don't immediately see the problem that this would cause.
>>> There are no array sizes etc dependent on the value of MAX for instance.
>>> 
>>> Looks safe?
>>
>> We never know how this enum will be used by the application.
>> The max value may be used for the size of an event array.
>> It looks a real ABI issue unfortunately.
>
> Right - but we only really care about it when an array size based on MAX
> is likely to be passed to DPDK, which doesn't apply in this case.
>
> I noted that some Linux folks explicitly mark similar MAX values as not
> part of the ABI.
>
> /usr/include/linux/perf_event.h
> 37:     PERF_TYPE_MAX,                          /* non-ABI */
> 60:     PERF_COUNT_HW_MAX,                      /* non-ABI */
> 79:     PERF_COUNT_HW_CACHE_MAX,                /* non-ABI */
> 87:     PERF_COUNT_HW_CACHE_OP_MAX,             /* non-ABI */
> 94:     PERF_COUNT_HW_CACHE_RESULT_MAX,         /* non-ABI */
> 116:    PERF_COUNT_SW_MAX,                      /* non-ABI */
> 149:    PERF_SAMPLE_MAX = 1U << 24,             /* non-ABI */
> 151:    __PERF_SAMPLE_CALLCHAIN_EARLY           = 1ULL << 63, /*
> non-ABI; internal use */
> 189:    PERF_SAMPLE_BRANCH_MAX_SHIFT            /* non-ABI */
> 267:    PERF_TXN_MAX            = (1 << 8), /* non-ABI */
> 301:    PERF_FORMAT_MAX = 1U << 4,              /* non-ABI */
> 1067:   PERF_RECORD_MAX,                        /* non-ABI */
> 1078:   PERF_RECORD_KSYMBOL_TYPE_MAX            /* non-ABI */
> 1087:   PERF_BPF_EVENT_MAX,             /* non-ABI */
>
>>
>> PS: I am not Cc'ed in this patchset,
>> so copying what I said on v6 (more than a year ago):
>> Please use the option --cc-cmd devtools/get-maintainer.sh

Any thoughts on similarly annotating all our _MAX enums in the same way?
We could also add a section in the ABI Policy to make it explicit _MAX
enum values are not part of the ABI - what do folks think?

-- 
Regards, Ray K

Re: [dpdk-dev] [PATCH v7 1/4] ethdev: support device reset and recovery events

[Thomas Monjalon]

14/02/2022 11:16, Ray Kinsella:
> Ray Kinsella <mdr@ashroe.eu> writes:
> > Thomas Monjalon <thomas@monjalon.net> writes:
> >> 02/02/2022 12:44, Ray Kinsella:
> >>> Ferruh Yigit <ferruh.yigit@intel.com> writes:
> >>> > On 1/28/2022 12:48 PM, Kalesh A P wrote:
> >>> >> --- a/lib/ethdev/rte_ethdev.h
> >>> >> +++ b/lib/ethdev/rte_ethdev.h
> >>> >> @@ -3818,6 +3818,24 @@ enum rte_eth_event_type {
> >>> >>   	RTE_ETH_EVENT_DESTROY,  /**< port is released */
> >>> >>   	RTE_ETH_EVENT_IPSEC,    /**< IPsec offload related event */
> >>> >>   	RTE_ETH_EVENT_FLOW_AGED,/**< New aged-out flows is detected */
> >>> >> +	RTE_ETH_EVENT_ERR_RECOVERING,
> >>> >> +			/**< port recovering from an error
> >>> >> +			 *
> >>> >> +			 * PMD detected a FW reset or error condition.
> >>> >> +			 * PMD will try to recover from the error.
> >>> >> +			 * Data path may be quiesced and Control path operations
> >>> >> +			 * may fail at this time.
> >>> >> +			 */
> >>> >> +	RTE_ETH_EVENT_RECOVERED,
> >>> >> +			/**< port recovered from an error
> >>> >> +			 *
> >>> >> +			 * PMD has recovered from the error condition.
> >>> >> +			 * Control path and Data path are up now.
> >>> >> +			 * PMD re-configures the port to the state prior to the error.
> >>> >> +			 * Since the device has undergone a reset, flow rules
> >>> >> +			 * offloaded prior to reset may be lost and
> >>> >> +			 * the application should recreate the rules again.
> >>> >> +			 */
> >>> >>   	RTE_ETH_EVENT_MAX       /**< max value of this enum */
> >>> >
> >>> >
> >>> > Also ABI check complains about 'RTE_ETH_EVENT_MAX' value check, cc'ed more people
> >>> > to evaluate if it is a false positive:
> >>> >
> >>> >
> >>> > 1 function with some indirect sub-type change:
> >>> >   [C] 'function int rte_eth_dev_callback_register(uint16_t, rte_eth_event_type, rte_eth_dev_cb_fn, void*)' at rte_ethdev.c:4637:1 has some indirect sub-type changes:
> >>> >     parameter 3 of type 'typedef rte_eth_dev_cb_fn' has sub-type changes:
> >>> >       underlying type 'int (typedef uint16_t, enum rte_eth_event_type, void*, void*)*' changed:
> >>> >         in pointed to type 'function type int (typedef uint16_t, enum rte_eth_event_type, void*, void*)':
> >>> >           parameter 2 of type 'enum rte_eth_event_type' has sub-type changes:
> >>> >             type size hasn't changed
> >>> >             2 enumerator insertions:
> >>> >               'rte_eth_event_type::RTE_ETH_EVENT_ERR_RECOVERING' value '11'
> >>> >               'rte_eth_event_type::RTE_ETH_EVENT_RECOVERED' value '12'
> >>> >             1 enumerator change:
> >>> >               'rte_eth_event_type::RTE_ETH_EVENT_MAX' from value '11' to '13' at rte_ethdev.h:3807:1
> >>> 
> >>> I don't immediately see the problem that this would cause.
> >>> There are no array sizes etc dependent on the value of MAX for instance.
> >>> 
> >>> Looks safe?
> >>
> >> We never know how this enum will be used by the application.
> >> The max value may be used for the size of an event array.
> >> It looks a real ABI issue unfortunately.
> >
> > Right - but we only really care about it when an array size based on MAX
> > is likely to be passed to DPDK, which doesn't apply in this case.

I don't completely agree.
A developer may assume an event will never exceed MAX value.
However, after an upgrade of DPDK without app rebuild,
a higher event value may be received in the app,
breaking the assumption.
Should we consider this case as an ABI breakage?

> > I noted that some Linux folks explicitly mark similar MAX values as not
> > part of the ABI.
> >
> > /usr/include/linux/perf_event.h
> > 37:     PERF_TYPE_MAX,                          /* non-ABI */
> > 60:     PERF_COUNT_HW_MAX,                      /* non-ABI */
> > 79:     PERF_COUNT_HW_CACHE_MAX,                /* non-ABI */
> > 87:     PERF_COUNT_HW_CACHE_OP_MAX,             /* non-ABI */
> > 94:     PERF_COUNT_HW_CACHE_RESULT_MAX,         /* non-ABI */
> > 116:    PERF_COUNT_SW_MAX,                      /* non-ABI */
> > 149:    PERF_SAMPLE_MAX = 1U << 24,             /* non-ABI */
> > 151:    __PERF_SAMPLE_CALLCHAIN_EARLY           = 1ULL << 63, /*
> > non-ABI; internal use */
> > 189:    PERF_SAMPLE_BRANCH_MAX_SHIFT            /* non-ABI */
> > 267:    PERF_TXN_MAX            = (1 << 8), /* non-ABI */
> > 301:    PERF_FORMAT_MAX = 1U << 4,              /* non-ABI */
> > 1067:   PERF_RECORD_MAX,                        /* non-ABI */
> > 1078:   PERF_RECORD_KSYMBOL_TYPE_MAX            /* non-ABI */
> > 1087:   PERF_BPF_EVENT_MAX,             /* non-ABI */
> 
> Any thoughts on similarly annotating all our _MAX enums in the same way?
> We could also add a section in the ABI Policy to make it explicit _MAX
> enum values are not part of the ABI - what do folks think?

Interesting. I am not sure it is always ABI-safe though.

Re: [dpdk-dev] [PATCH v7 1/4] ethdev: support device reset and recovery events

[Ray Kinsella]

Thomas Monjalon <thomas@monjalon.net> writes:

> 14/02/2022 11:16, Ray Kinsella:
>> Ray Kinsella <mdr@ashroe.eu> writes:
>> > Thomas Monjalon <thomas@monjalon.net> writes:
>> >> 02/02/2022 12:44, Ray Kinsella:
>> >>> Ferruh Yigit <ferruh.yigit@intel.com> writes:
>> >>> > On 1/28/2022 12:48 PM, Kalesh A P wrote:
>> >>> >> --- a/lib/ethdev/rte_ethdev.h
>> >>> >> +++ b/lib/ethdev/rte_ethdev.h
>> >>> >> @@ -3818,6 +3818,24 @@ enum rte_eth_event_type {
>> >>> >>   	RTE_ETH_EVENT_DESTROY,  /**< port is released */
>> >>> >>   	RTE_ETH_EVENT_IPSEC,    /**< IPsec offload related event */
>> >>> >>   	RTE_ETH_EVENT_FLOW_AGED,/**< New aged-out flows is detected */
>> >>> >> +	RTE_ETH_EVENT_ERR_RECOVERING,
>> >>> >> +			/**< port recovering from an error
>> >>> >> +			 *
>> >>> >> +			 * PMD detected a FW reset or error condition.
>> >>> >> +			 * PMD will try to recover from the error.
>> >>> >> +			 * Data path may be quiesced and Control path operations
>> >>> >> +			 * may fail at this time.
>> >>> >> +			 */
>> >>> >> +	RTE_ETH_EVENT_RECOVERED,
>> >>> >> +			/**< port recovered from an error
>> >>> >> +			 *
>> >>> >> +			 * PMD has recovered from the error condition.
>> >>> >> +			 * Control path and Data path are up now.
>> >>> >> +			 * PMD re-configures the port to the state prior to the error.
>> >>> >> +			 * Since the device has undergone a reset, flow rules
>> >>> >> +			 * offloaded prior to reset may be lost and
>> >>> >> +			 * the application should recreate the rules again.
>> >>> >> +			 */
>> >>> >>   	RTE_ETH_EVENT_MAX       /**< max value of this enum */
>> >>> >
>> >>> >
>> >>> > Also ABI check complains about 'RTE_ETH_EVENT_MAX' value check, cc'ed more people
>> >>> > to evaluate if it is a false positive:
>> >>> >
>> >>> >
>> >>> > 1 function with some indirect sub-type change:
>> >>> >   [C] 'function int rte_eth_dev_callback_register(uint16_t, rte_eth_event_type, rte_eth_dev_cb_fn, void*)' at rte_ethdev.c:4637:1 has some indirect sub-type changes:
>> >>> >     parameter 3 of type 'typedef rte_eth_dev_cb_fn' has sub-type changes:
>> >>> >       underlying type 'int (typedef uint16_t, enum rte_eth_event_type, void*, void*)*' changed:
>> >>> >         in pointed to type 'function type int (typedef uint16_t, enum rte_eth_event_type, void*, void*)':
>> >>> >           parameter 2 of type 'enum rte_eth_event_type' has sub-type changes:
>> >>> >             type size hasn't changed
>> >>> >             2 enumerator insertions:
>> >>> >               'rte_eth_event_type::RTE_ETH_EVENT_ERR_RECOVERING' value '11'
>> >>> >               'rte_eth_event_type::RTE_ETH_EVENT_RECOVERED' value '12'
>> >>> >             1 enumerator change:
>> >>> >               'rte_eth_event_type::RTE_ETH_EVENT_MAX' from value '11' to '13' at rte_ethdev.h:3807:1
>> >>> 
>> >>> I don't immediately see the problem that this would cause.
>> >>> There are no array sizes etc dependent on the value of MAX for instance.
>> >>> 
>> >>> Looks safe?
>> >>
>> >> We never know how this enum will be used by the application.
>> >> The max value may be used for the size of an event array.
>> >> It looks a real ABI issue unfortunately.
>> >
>> > Right - but we only really care about it when an array size based on MAX
>> > is likely to be passed to DPDK, which doesn't apply in this case.
>
> I don't completely agree.
> A developer may assume an event will never exceed MAX value.
> However, after an upgrade of DPDK without app rebuild,
> a higher event value may be received in the app,
> breaking the assumption.
> Should we consider this case as an ABI breakage?

Nope - I think we should explicitly exclude MAX values from any
ABI guarantee, as being able to change them is key to our be able to
evolve DPDK while maintaining ABI stability. 

Consider what it means applying the ABI policy to a MAX value, you are
in effect saying that that no value can be added to this enumeration
until the next ABI version, for me this is very restrictive without a
solid reason. 

>
>> > I noted that some Linux folks explicitly mark similar MAX values as not
>> > part of the ABI.
>> >
>> > /usr/include/linux/perf_event.h
>> > 37:     PERF_TYPE_MAX,                          /* non-ABI */
>> > 60:     PERF_COUNT_HW_MAX,                      /* non-ABI */
>> > 79:     PERF_COUNT_HW_CACHE_MAX,                /* non-ABI */
>> > 87:     PERF_COUNT_HW_CACHE_OP_MAX,             /* non-ABI */
>> > 94:     PERF_COUNT_HW_CACHE_RESULT_MAX,         /* non-ABI */
>> > 116:    PERF_COUNT_SW_MAX,                      /* non-ABI */
>> > 149:    PERF_SAMPLE_MAX = 1U << 24,             /* non-ABI */
>> > 151:    __PERF_SAMPLE_CALLCHAIN_EARLY           = 1ULL << 63, /*
>> > non-ABI; internal use */
>> > 189:    PERF_SAMPLE_BRANCH_MAX_SHIFT            /* non-ABI */
>> > 267:    PERF_TXN_MAX            = (1 << 8), /* non-ABI */
>> > 301:    PERF_FORMAT_MAX = 1U << 4,              /* non-ABI */
>> > 1067:   PERF_RECORD_MAX,                        /* non-ABI */
>> > 1078:   PERF_RECORD_KSYMBOL_TYPE_MAX            /* non-ABI */
>> > 1087:   PERF_BPF_EVENT_MAX,             /* non-ABI */
>> 
>> Any thoughts on similarly annotating all our _MAX enums in the same way?
>> We could also add a section in the ABI Policy to make it explicit _MAX
>> enum values are not part of the ABI - what do folks think?
>
> Interesting. I am not sure it is always ABI-safe though.


-- 
Regards, Ray K

Re: [dpdk-dev] [PATCH v7 1/4] ethdev: support device reset and recovery events

[Thomas Monjalon]

14/02/2022 17:06, Ray Kinsella:
> Thomas Monjalon <thomas@monjalon.net> writes:
> > 14/02/2022 11:16, Ray Kinsella:
> >> Ray Kinsella <mdr@ashroe.eu> writes:
> >> > Thomas Monjalon <thomas@monjalon.net> writes:
> >> >> 02/02/2022 12:44, Ray Kinsella:
> >> >>> Ferruh Yigit <ferruh.yigit@intel.com> writes:
> >> >>> > On 1/28/2022 12:48 PM, Kalesh A P wrote:
> >> >>> >> --- a/lib/ethdev/rte_ethdev.h
> >> >>> >> +++ b/lib/ethdev/rte_ethdev.h
> >> >>> >> @@ -3818,6 +3818,24 @@ enum rte_eth_event_type {
> >> >>> >>   	RTE_ETH_EVENT_DESTROY,  /**< port is released */
> >> >>> >>   	RTE_ETH_EVENT_IPSEC,    /**< IPsec offload related event */
> >> >>> >>   	RTE_ETH_EVENT_FLOW_AGED,/**< New aged-out flows is detected */
> >> >>> >> +	RTE_ETH_EVENT_ERR_RECOVERING,
> >> >>> >> +			/**< port recovering from an error
> >> >>> >> +			 *
> >> >>> >> +			 * PMD detected a FW reset or error condition.
> >> >>> >> +			 * PMD will try to recover from the error.
> >> >>> >> +			 * Data path may be quiesced and Control path operations
> >> >>> >> +			 * may fail at this time.
> >> >>> >> +			 */
> >> >>> >> +	RTE_ETH_EVENT_RECOVERED,
> >> >>> >> +			/**< port recovered from an error
> >> >>> >> +			 *
> >> >>> >> +			 * PMD has recovered from the error condition.
> >> >>> >> +			 * Control path and Data path are up now.
> >> >>> >> +			 * PMD re-configures the port to the state prior to the error.
> >> >>> >> +			 * Since the device has undergone a reset, flow rules
> >> >>> >> +			 * offloaded prior to reset may be lost and
> >> >>> >> +			 * the application should recreate the rules again.
> >> >>> >> +			 */
> >> >>> >>   	RTE_ETH_EVENT_MAX       /**< max value of this enum */
> >> >>> >
> >> >>> >
> >> >>> > Also ABI check complains about 'RTE_ETH_EVENT_MAX' value check, cc'ed more people
> >> >>> > to evaluate if it is a false positive:
> >> >>> >
> >> >>> >
> >> >>> > 1 function with some indirect sub-type change:
> >> >>> >   [C] 'function int rte_eth_dev_callback_register(uint16_t, rte_eth_event_type, rte_eth_dev_cb_fn, void*)' at rte_ethdev.c:4637:1 has some indirect sub-type changes:
> >> >>> >     parameter 3 of type 'typedef rte_eth_dev_cb_fn' has sub-type changes:
> >> >>> >       underlying type 'int (typedef uint16_t, enum rte_eth_event_type, void*, void*)*' changed:
> >> >>> >         in pointed to type 'function type int (typedef uint16_t, enum rte_eth_event_type, void*, void*)':
> >> >>> >           parameter 2 of type 'enum rte_eth_event_type' has sub-type changes:
> >> >>> >             type size hasn't changed
> >> >>> >             2 enumerator insertions:
> >> >>> >               'rte_eth_event_type::RTE_ETH_EVENT_ERR_RECOVERING' value '11'
> >> >>> >               'rte_eth_event_type::RTE_ETH_EVENT_RECOVERED' value '12'
> >> >>> >             1 enumerator change:
> >> >>> >               'rte_eth_event_type::RTE_ETH_EVENT_MAX' from value '11' to '13' at rte_ethdev.h:3807:1
> >> >>> 
> >> >>> I don't immediately see the problem that this would cause.
> >> >>> There are no array sizes etc dependent on the value of MAX for instance.
> >> >>> 
> >> >>> Looks safe?
> >> >>
> >> >> We never know how this enum will be used by the application.
> >> >> The max value may be used for the size of an event array.
> >> >> It looks a real ABI issue unfortunately.
> >> >
> >> > Right - but we only really care about it when an array size based on MAX
> >> > is likely to be passed to DPDK, which doesn't apply in this case.
> >
> > I don't completely agree.
> > A developer may assume an event will never exceed MAX value.
> > However, after an upgrade of DPDK without app rebuild,
> > a higher event value may be received in the app,
> > breaking the assumption.
> > Should we consider this case as an ABI breakage?
> 
> Nope - I think we should explicitly exclude MAX values from any
> ABI guarantee, as being able to change them is key to our be able to
> evolve DPDK while maintaining ABI stability.

Or we can simply remove the MAX values so there is no confusion.

> Consider what it means applying the ABI policy to a MAX value, you are
> in effect saying that that no value can be added to this enumeration
> until the next ABI version, for me this is very restrictive without a
> solid reason. 

I agree it is too much restrictive, that's why I am advocating
for their removal.

> >> > I noted that some Linux folks explicitly mark similar MAX values as not
> >> > part of the ABI.
> >> >
> >> > /usr/include/linux/perf_event.h
> >> > 37:     PERF_TYPE_MAX,                          /* non-ABI */
> >> > 60:     PERF_COUNT_HW_MAX,                      /* non-ABI */
> >> > 79:     PERF_COUNT_HW_CACHE_MAX,                /* non-ABI */
> >> > 87:     PERF_COUNT_HW_CACHE_OP_MAX,             /* non-ABI */
> >> > 94:     PERF_COUNT_HW_CACHE_RESULT_MAX,         /* non-ABI */
> >> > 116:    PERF_COUNT_SW_MAX,                      /* non-ABI */
> >> > 149:    PERF_SAMPLE_MAX = 1U << 24,             /* non-ABI */
> >> > 151:    __PERF_SAMPLE_CALLCHAIN_EARLY           = 1ULL << 63, /*
> >> > non-ABI; internal use */
> >> > 189:    PERF_SAMPLE_BRANCH_MAX_SHIFT            /* non-ABI */
> >> > 267:    PERF_TXN_MAX            = (1 << 8), /* non-ABI */
> >> > 301:    PERF_FORMAT_MAX = 1U << 4,              /* non-ABI */
> >> > 1067:   PERF_RECORD_MAX,                        /* non-ABI */
> >> > 1078:   PERF_RECORD_KSYMBOL_TYPE_MAX            /* non-ABI */
> >> > 1087:   PERF_BPF_EVENT_MAX,             /* non-ABI */
> >> 
> >> Any thoughts on similarly annotating all our _MAX enums in the same way?
> >> We could also add a section in the ABI Policy to make it explicit _MAX
> >> enum values are not part of the ABI - what do folks think?
> >
> > Interesting. I am not sure it is always ABI-safe though.

Re: [dpdk-dev] [PATCH v7 1/4] ethdev: support device reset and recovery events

[Ray Kinsella]

Thomas Monjalon <thomas@monjalon.net> writes:

> 14/02/2022 17:06, Ray Kinsella:
>> Thomas Monjalon <thomas@monjalon.net> writes:
>> > 14/02/2022 11:16, Ray Kinsella:
>> >> Ray Kinsella <mdr@ashroe.eu> writes:
>> >> > Thomas Monjalon <thomas@monjalon.net> writes:
>> >> >> 02/02/2022 12:44, Ray Kinsella:
>> >> >>> Ferruh Yigit <ferruh.yigit@intel.com> writes:
>> >> >>> > On 1/28/2022 12:48 PM, Kalesh A P wrote:
>> >> >>> >> --- a/lib/ethdev/rte_ethdev.h
>> >> >>> >> +++ b/lib/ethdev/rte_ethdev.h
>> >> >>> >> @@ -3818,6 +3818,24 @@ enum rte_eth_event_type {
>> >> >>> >>   	RTE_ETH_EVENT_DESTROY,  /**< port is released */
>> >> >>> >>   	RTE_ETH_EVENT_IPSEC,    /**< IPsec offload related event */
>> >> >>> >>   	RTE_ETH_EVENT_FLOW_AGED,/**< New aged-out flows is detected */
>> >> >>> >> +	RTE_ETH_EVENT_ERR_RECOVERING,
>> >> >>> >> +			/**< port recovering from an error
>> >> >>> >> +			 *
>> >> >>> >> +			 * PMD detected a FW reset or error condition.
>> >> >>> >> +			 * PMD will try to recover from the error.
>> >> >>> >> +			 * Data path may be quiesced and Control path operations
>> >> >>> >> +			 * may fail at this time.
>> >> >>> >> +			 */
>> >> >>> >> +	RTE_ETH_EVENT_RECOVERED,
>> >> >>> >> +			/**< port recovered from an error
>> >> >>> >> +			 *
>> >> >>> >> +			 * PMD has recovered from the error condition.
>> >> >>> >> +			 * Control path and Data path are up now.
>> >> >>> >> +			 * PMD re-configures the port to the state prior to the error.
>> >> >>> >> +			 * Since the device has undergone a reset, flow rules
>> >> >>> >> +			 * offloaded prior to reset may be lost and
>> >> >>> >> +			 * the application should recreate the rules again.
>> >> >>> >> +			 */
>> >> >>> >>   	RTE_ETH_EVENT_MAX       /**< max value of this enum */
>> >> >>> >
>> >> >>> >
>> >> >>> > Also ABI check complains about 'RTE_ETH_EVENT_MAX' value check, cc'ed more people
>> >> >>> > to evaluate if it is a false positive:
>> >> >>> >
>> >> >>> >
>> >> >>> > 1 function with some indirect sub-type change:
>> >> >>> >   [C] 'function int rte_eth_dev_callback_register(uint16_t, rte_eth_event_type, rte_eth_dev_cb_fn, void*)' at rte_ethdev.c:4637:1 has some indirect sub-type changes:
>> >> >>> >     parameter 3 of type 'typedef rte_eth_dev_cb_fn' has sub-type changes:
>> >> >>> >       underlying type 'int (typedef uint16_t, enum rte_eth_event_type, void*, void*)*' changed:
>> >> >>> >         in pointed to type 'function type int (typedef uint16_t, enum rte_eth_event_type, void*, void*)':
>> >> >>> >           parameter 2 of type 'enum rte_eth_event_type' has sub-type changes:
>> >> >>> >             type size hasn't changed
>> >> >>> >             2 enumerator insertions:
>> >> >>> >               'rte_eth_event_type::RTE_ETH_EVENT_ERR_RECOVERING' value '11'
>> >> >>> >               'rte_eth_event_type::RTE_ETH_EVENT_RECOVERED' value '12'
>> >> >>> >             1 enumerator change:
>> >> >>> >               'rte_eth_event_type::RTE_ETH_EVENT_MAX' from value '11' to '13' at rte_ethdev.h:3807:1
>> >> >>> 
>> >> >>> I don't immediately see the problem that this would cause.
>> >> >>> There are no array sizes etc dependent on the value of MAX for instance.
>> >> >>> 
>> >> >>> Looks safe?
>> >> >>
>> >> >> We never know how this enum will be used by the application.
>> >> >> The max value may be used for the size of an event array.
>> >> >> It looks a real ABI issue unfortunately.
>> >> >
>> >> > Right - but we only really care about it when an array size based on MAX
>> >> > is likely to be passed to DPDK, which doesn't apply in this case.
>> >
>> > I don't completely agree.
>> > A developer may assume an event will never exceed MAX value.
>> > However, after an upgrade of DPDK without app rebuild,
>> > a higher event value may be received in the app,
>> > breaking the assumption.
>> > Should we consider this case as an ABI breakage?
>> 
>> Nope - I think we should explicitly exclude MAX values from any
>> ABI guarantee, as being able to change them is key to our be able to
>> evolve DPDK while maintaining ABI stability.
>
> Or we can simply remove the MAX values so there is no confusion.
>
>> Consider what it means applying the ABI policy to a MAX value, you are
>> in effect saying that that no value can be added to this enumeration
>> until the next ABI version, for me this is very restrictive without a
>> solid reason. 
>
> I agree it is too much restrictive, that's why I am advocating
> for their removal.

I think that would be simplest yes - may require some rework of the
sample code though. I will take a look at it.

>
>> >> > I noted that some Linux folks explicitly mark similar MAX values as not
>> >> > part of the ABI.
>> >> >
>> >> > /usr/include/linux/perf_event.h
>> >> > 37:     PERF_TYPE_MAX,                          /* non-ABI */
>> >> > 60:     PERF_COUNT_HW_MAX,                      /* non-ABI */
>> >> > 79:     PERF_COUNT_HW_CACHE_MAX,                /* non-ABI */
>> >> > 87:     PERF_COUNT_HW_CACHE_OP_MAX,             /* non-ABI */
>> >> > 94:     PERF_COUNT_HW_CACHE_RESULT_MAX,         /* non-ABI */
>> >> > 116:    PERF_COUNT_SW_MAX,                      /* non-ABI */
>> >> > 149:    PERF_SAMPLE_MAX = 1U << 24,             /* non-ABI */
>> >> > 151:    __PERF_SAMPLE_CALLCHAIN_EARLY           = 1ULL << 63, /*
>> >> > non-ABI; internal use */
>> >> > 189:    PERF_SAMPLE_BRANCH_MAX_SHIFT            /* non-ABI */
>> >> > 267:    PERF_TXN_MAX            = (1 << 8), /* non-ABI */
>> >> > 301:    PERF_FORMAT_MAX = 1U << 4,              /* non-ABI */
>> >> > 1067:   PERF_RECORD_MAX,                        /* non-ABI */
>> >> > 1078:   PERF_RECORD_KSYMBOL_TYPE_MAX            /* non-ABI */
>> >> > 1087:   PERF_BPF_EVENT_MAX,             /* non-ABI */
>> >> 
>> >> Any thoughts on similarly annotating all our _MAX enums in the same way?
>> >> We could also add a section in the ABI Policy to make it explicit _MAX
>> >> enum values are not part of the ABI - what do folks think?
>> >
>> > Interesting. I am not sure it is always ABI-safe though.


-- 
Regards, Ray K

Re: [dpdk-dev] [PATCH v7 1/4] ethdev: support device reset and recovery events

[Ray Kinsella]

Ray Kinsella <mdr@ashroe.eu> writes:

> Thomas Monjalon <thomas@monjalon.net> writes:
>
>> 14/02/2022 17:06, Ray Kinsella:
>>> Thomas Monjalon <thomas@monjalon.net> writes:
>>> > 14/02/2022 11:16, Ray Kinsella:
>>> >> Ray Kinsella <mdr@ashroe.eu> writes:
>>> >> > Thomas Monjalon <thomas@monjalon.net> writes:
>>> >> >> 02/02/2022 12:44, Ray Kinsella:
>>> >> >>> Ferruh Yigit <ferruh.yigit@intel.com> writes:
>>> >> >>> > On 1/28/2022 12:48 PM, Kalesh A P wrote:
>>> >> >>> >> --- a/lib/ethdev/rte_ethdev.h
>>> >> >>> >> +++ b/lib/ethdev/rte_ethdev.h
>>> >> >>> >> @@ -3818,6 +3818,24 @@ enum rte_eth_event_type {
>>> >> >>> >>   	RTE_ETH_EVENT_DESTROY,  /**< port is released */
>>> >> >>> >>   	RTE_ETH_EVENT_IPSEC,    /**< IPsec offload related event */
>>> >> >>> >>   	RTE_ETH_EVENT_FLOW_AGED,/**< New aged-out flows is detected */
>>> >> >>> >> +	RTE_ETH_EVENT_ERR_RECOVERING,
>>> >> >>> >> +			/**< port recovering from an error
>>> >> >>> >> +			 *
>>> >> >>> >> +			 * PMD detected a FW reset or error condition.
>>> >> >>> >> +			 * PMD will try to recover from the error.
>>> >> >>> >> +			 * Data path may be quiesced and Control path operations
>>> >> >>> >> +			 * may fail at this time.
>>> >> >>> >> +			 */
>>> >> >>> >> +	RTE_ETH_EVENT_RECOVERED,
>>> >> >>> >> +			/**< port recovered from an error
>>> >> >>> >> +			 *
>>> >> >>> >> +			 * PMD has recovered from the error condition.
>>> >> >>> >> +			 * Control path and Data path are up now.
>>> >> >>> >> +			 * PMD re-configures the port to the state prior to the error.
>>> >> >>> >> +			 * Since the device has undergone a reset, flow rules
>>> >> >>> >> +			 * offloaded prior to reset may be lost and
>>> >> >>> >> +			 * the application should recreate the rules again.
>>> >> >>> >> +			 */
>>> >> >>> >>   	RTE_ETH_EVENT_MAX       /**< max value of this enum */
>>> >> >>> >
>>> >> >>> >
>>> >> >>> > Also ABI check complains about 'RTE_ETH_EVENT_MAX' value check, cc'ed more people
>>> >> >>> > to evaluate if it is a false positive:
>>> >> >>> >
>>> >> >>> >
>>> >> >>> > 1 function with some indirect sub-type change:
>>> >> >>> >   [C] 'function int rte_eth_dev_callback_register(uint16_t, rte_eth_event_type, rte_eth_dev_cb_fn, void*)' at rte_ethdev.c:4637:1 has some indirect sub-type changes:
>>> >> >>> >     parameter 3 of type 'typedef rte_eth_dev_cb_fn' has sub-type changes:
>>> >> >>> >       underlying type 'int (typedef uint16_t, enum rte_eth_event_type, void*, void*)*' changed:
>>> >> >>> >         in pointed to type 'function type int (typedef uint16_t, enum rte_eth_event_type, void*, void*)':
>>> >> >>> >           parameter 2 of type 'enum rte_eth_event_type' has sub-type changes:
>>> >> >>> >             type size hasn't changed
>>> >> >>> >             2 enumerator insertions:
>>> >> >>> >               'rte_eth_event_type::RTE_ETH_EVENT_ERR_RECOVERING' value '11'
>>> >> >>> >               'rte_eth_event_type::RTE_ETH_EVENT_RECOVERED' value '12'
>>> >> >>> >             1 enumerator change:
>>> >> >>> >               'rte_eth_event_type::RTE_ETH_EVENT_MAX' from value '11' to '13' at rte_ethdev.h:3807:1
>>> >> >>> 
>>> >> >>> I don't immediately see the problem that this would cause.
>>> >> >>> There are no array sizes etc dependent on the value of MAX for instance.
>>> >> >>> 
>>> >> >>> Looks safe?
>>> >> >>
>>> >> >> We never know how this enum will be used by the application.
>>> >> >> The max value may be used for the size of an event array.
>>> >> >> It looks a real ABI issue unfortunately.
>>> >> >
>>> >> > Right - but we only really care about it when an array size based on MAX
>>> >> > is likely to be passed to DPDK, which doesn't apply in this case.
>>> >
>>> > I don't completely agree.
>>> > A developer may assume an event will never exceed MAX value.
>>> > However, after an upgrade of DPDK without app rebuild,
>>> > a higher event value may be received in the app,
>>> > breaking the assumption.
>>> > Should we consider this case as an ABI breakage?
>>> 
>>> Nope - I think we should explicitly exclude MAX values from any
>>> ABI guarantee, as being able to change them is key to our be able to
>>> evolve DPDK while maintaining ABI stability.
>>
>> Or we can simply remove the MAX values so there is no confusion.
>>
>>> Consider what it means applying the ABI policy to a MAX value, you are
>>> in effect saying that that no value can be added to this enumeration
>>> until the next ABI version, for me this is very restrictive without a
>>> solid reason. 
>>
>> I agree it is too much restrictive, that's why I am advocating
>> for their removal.
>
> I think that would be simplest yes - may require some rework of the
> sample code though. I will take a look at it.

Thinking about this some more - we can't remove the MAX values between
now the next stable ABI. So we may need a short term plan, and long term
plan.

Long term, I agree we look at every _MAX enumeration value and ask do we
need it.

Short term (until the next ABI), we still need to answer the question do
we allow people to change _MAX values?

>>
>>> >> > I noted that some Linux folks explicitly mark similar MAX values as not
>>> >> > part of the ABI.
>>> >> >
>>> >> > /usr/include/linux/perf_event.h
>>> >> > 37:     PERF_TYPE_MAX,                          /* non-ABI */
>>> >> > 60:     PERF_COUNT_HW_MAX,                      /* non-ABI */
>>> >> > 79:     PERF_COUNT_HW_CACHE_MAX,                /* non-ABI */
>>> >> > 87:     PERF_COUNT_HW_CACHE_OP_MAX,             /* non-ABI */
>>> >> > 94:     PERF_COUNT_HW_CACHE_RESULT_MAX,         /* non-ABI */
>>> >> > 116:    PERF_COUNT_SW_MAX,                      /* non-ABI */
>>> >> > 149:    PERF_SAMPLE_MAX = 1U << 24,             /* non-ABI */
>>> >> > 151:    __PERF_SAMPLE_CALLCHAIN_EARLY           = 1ULL << 63, /*
>>> >> > non-ABI; internal use */
>>> >> > 189:    PERF_SAMPLE_BRANCH_MAX_SHIFT            /* non-ABI */
>>> >> > 267:    PERF_TXN_MAX            = (1 << 8), /* non-ABI */
>>> >> > 301:    PERF_FORMAT_MAX = 1U << 4,              /* non-ABI */
>>> >> > 1067:   PERF_RECORD_MAX,                        /* non-ABI */
>>> >> > 1078:   PERF_RECORD_KSYMBOL_TYPE_MAX            /* non-ABI */
>>> >> > 1087:   PERF_BPF_EVENT_MAX,             /* non-ABI */
>>> >> 
>>> >> Any thoughts on similarly annotating all our _MAX enums in the same way?
>>> >> We could also add a section in the ABI Policy to make it explicit _MAX
>>> >> enum values are not part of the ABI - what do folks think?
>>> >
>>> > Interesting. I am not sure it is always ABI-safe though.


-- 
Regards, Ray K

Re: [dpdk-dev] [PATCH v7 1/4] ethdev: support device reset and recovery events

[Thomas Monjalon]

15/02/2022 14:55, Ray Kinsella:
> Ray Kinsella <mdr@ashroe.eu> writes:
> > Thomas Monjalon <thomas@monjalon.net> writes:
> >> 14/02/2022 17:06, Ray Kinsella:
> >>> Thomas Monjalon <thomas@monjalon.net> writes:
> >>> > 14/02/2022 11:16, Ray Kinsella:
> >>> >> Ray Kinsella <mdr@ashroe.eu> writes:
> >>> >> > Thomas Monjalon <thomas@monjalon.net> writes:
> >>> >> >> We never know how this enum will be used by the application.
> >>> >> >> The max value may be used for the size of an event array.
> >>> >> >> It looks a real ABI issue unfortunately.
> >>> >> >
> >>> >> > Right - but we only really care about it when an array size based on MAX
> >>> >> > is likely to be passed to DPDK, which doesn't apply in this case.
> >>> >
> >>> > I don't completely agree.
> >>> > A developer may assume an event will never exceed MAX value.
> >>> > However, after an upgrade of DPDK without app rebuild,
> >>> > a higher event value may be received in the app,
> >>> > breaking the assumption.
> >>> > Should we consider this case as an ABI breakage?
> >>> 
> >>> Nope - I think we should explicitly exclude MAX values from any
> >>> ABI guarantee, as being able to change them is key to our be able to
> >>> evolve DPDK while maintaining ABI stability.
> >>
> >> Or we can simply remove the MAX values so there is no confusion.
> >>
> >>> Consider what it means applying the ABI policy to a MAX value, you are
> >>> in effect saying that that no value can be added to this enumeration
> >>> until the next ABI version, for me this is very restrictive without a
> >>> solid reason. 
> >>
> >> I agree it is too much restrictive, that's why I am advocating
> >> for their removal.
> >
> > I think that would be simplest yes - may require some rework of the
> > sample code though. I will take a look at it.
> 
> Thinking about this some more - we can't remove the MAX values between
> now the next stable ABI. So we may need a short term plan, and long term
> plan.
> 
> Long term, I agree we look at every _MAX enumeration value and ask do we
> need it.
> 
> Short term (until the next ABI), we still need to answer the question do
> we allow people to change _MAX values?

There's a problem of incentive.
We already said in the past that we should remove the _MAX values,
but it doesn't happen because our time on this planet is limited.
I think it would work if the developers have a special interest in such work.
And guess what? Here there is a special interest to remove this one.
If we are at the negotiation table, we could imagine a short-term exception
if the long-term patch is available and reviewed.

Re: [dpdk-dev] [PATCH v7 1/4] ethdev: support device reset and recovery events

[Ray Kinsella]

Thomas Monjalon <thomas@monjalon.net> writes:

> 15/02/2022 14:55, Ray Kinsella:
>> Ray Kinsella <mdr@ashroe.eu> writes:
>> > Thomas Monjalon <thomas@monjalon.net> writes:
>> >> 14/02/2022 17:06, Ray Kinsella:
>> >>> Thomas Monjalon <thomas@monjalon.net> writes:
>> >>> > 14/02/2022 11:16, Ray Kinsella:
>> >>> >> Ray Kinsella <mdr@ashroe.eu> writes:
>> >>> >> > Thomas Monjalon <thomas@monjalon.net> writes:
>> >>> >> >> We never know how this enum will be used by the application.
>> >>> >> >> The max value may be used for the size of an event array.
>> >>> >> >> It looks a real ABI issue unfortunately.
>> >>> >> >
>> >>> >> > Right - but we only really care about it when an array size based on MAX
>> >>> >> > is likely to be passed to DPDK, which doesn't apply in this case.
>> >>> >
>> >>> > I don't completely agree.
>> >>> > A developer may assume an event will never exceed MAX value.
>> >>> > However, after an upgrade of DPDK without app rebuild,
>> >>> > a higher event value may be received in the app,
>> >>> > breaking the assumption.
>> >>> > Should we consider this case as an ABI breakage?
>> >>> 
>> >>> Nope - I think we should explicitly exclude MAX values from any
>> >>> ABI guarantee, as being able to change them is key to our be able to
>> >>> evolve DPDK while maintaining ABI stability.
>> >>
>> >> Or we can simply remove the MAX values so there is no confusion.
>> >>
>> >>> Consider what it means applying the ABI policy to a MAX value, you are
>> >>> in effect saying that that no value can be added to this enumeration
>> >>> until the next ABI version, for me this is very restrictive without a
>> >>> solid reason. 
>> >>
>> >> I agree it is too much restrictive, that's why I am advocating
>> >> for their removal.
>> >
>> > I think that would be simplest yes - may require some rework of the
>> > sample code though. I will take a look at it.
>> 
>> Thinking about this some more - we can't remove the MAX values between
>> now the next stable ABI. So we may need a short term plan, and long term
>> plan.
>> 
>> Long term, I agree we look at every _MAX enumeration value and ask do we
>> need it.
>> 
>> Short term (until the next ABI), we still need to answer the question do
>> we allow people to change _MAX values?
>
> There's a problem of incentive.
> We already said in the past that we should remove the _MAX values,
> but it doesn't happen because our time on this planet is limited.
> I think it would work if the developers have a special interest in such work.
> And guess what? Here there is a special interest to remove this one.
> If we are at the negotiation table, we could imagine a short-term exception
> if the long-term patch is available and reviewed.

:-) ... I like that plan.

-- 
Regards, Ray K

Re: [dpdk-dev] [PATCH v7 1/4] ethdev: support device reset and recovery events

[fengchengwen]

Hi all,

  This patch lasts for a long time. Are we waiting for 22.11 to deal with it?

  We have the same requirements for the reset or recovery mechanism, but there are differences:

    APP                                    PMD
     |                                      |
     |                                  detect error
     |     <---report error event---        |
     |                                      |
do error stats                              |
and report                                  |
     |      ---start recover-->             |
     |                                  do recover
     |     <---report recover result        |
     |                                      |
if succ just log
else may migrate
service

Can we generalize these processes(means that the implementation is at the framework layer)? or only at PMD API?


On 2022/2/15 0:06, Ray Kinsella wrote:
> 
> Thomas Monjalon <thomas@monjalon.net> writes:
> 
>> 14/02/2022 11:16, Ray Kinsella:
>>> Ray Kinsella <mdr@ashroe.eu> writes:
>>>> Thomas Monjalon <thomas@monjalon.net> writes:
>>>>> 02/02/2022 12:44, Ray Kinsella:
>>>>>> Ferruh Yigit <ferruh.yigit@intel.com> writes:
>>>>>>> On 1/28/2022 12:48 PM, Kalesh A P wrote:
>>>>>>>> --- a/lib/ethdev/rte_ethdev.h
>>>>>>>> +++ b/lib/ethdev/rte_ethdev.h
>>>>>>>> @@ -3818,6 +3818,24 @@ enum rte_eth_event_type {
>>>>>>>>   	RTE_ETH_EVENT_DESTROY,  /**< port is released */
>>>>>>>>   	RTE_ETH_EVENT_IPSEC,    /**< IPsec offload related event */
>>>>>>>>   	RTE_ETH_EVENT_FLOW_AGED,/**< New aged-out flows is detected */
>>>>>>>> +	RTE_ETH_EVENT_ERR_RECOVERING,
>>>>>>>> +			/**< port recovering from an error
>>>>>>>> +			 *
>>>>>>>> +			 * PMD detected a FW reset or error condition.
>>>>>>>> +			 * PMD will try to recover from the error.
>>>>>>>> +			 * Data path may be quiesced and Control path operations
>>>>>>>> +			 * may fail at this time.
>>>>>>>> +			 */
>>>>>>>> +	RTE_ETH_EVENT_RECOVERED,
>>>>>>>> +			/**< port recovered from an error
>>>>>>>> +			 *
>>>>>>>> +			 * PMD has recovered from the error condition.
>>>>>>>> +			 * Control path and Data path are up now.
>>>>>>>> +			 * PMD re-configures the port to the state prior to the error.
>>>>>>>> +			 * Since the device has undergone a reset, flow rules
>>>>>>>> +			 * offloaded prior to reset may be lost and
>>>>>>>> +			 * the application should recreate the rules again.
>>>>>>>> +			 */
>>>>>>>>   	RTE_ETH_EVENT_MAX       /**< max value of this enum */
>>>>>>>
>>>>>>>
>>>>>>> Also ABI check complains about 'RTE_ETH_EVENT_MAX' value check, cc'ed more people
>>>>>>> to evaluate if it is a false positive:
>>>>>>>
>>>>>>>
>>>>>>> 1 function with some indirect sub-type change:
>>>>>>>   [C] 'function int rte_eth_dev_callback_register(uint16_t, rte_eth_event_type, rte_eth_dev_cb_fn, void*)' at rte_ethdev.c:4637:1 has some indirect sub-type changes:
>>>>>>>     parameter 3 of type 'typedef rte_eth_dev_cb_fn' has sub-type changes:
>>>>>>>       underlying type 'int (typedef uint16_t, enum rte_eth_event_type, void*, void*)*' changed:
>>>>>>>         in pointed to type 'function type int (typedef uint16_t, enum rte_eth_event_type, void*, void*)':
>>>>>>>           parameter 2 of type 'enum rte_eth_event_type' has sub-type changes:
>>>>>>>             type size hasn't changed
>>>>>>>             2 enumerator insertions:
>>>>>>>               'rte_eth_event_type::RTE_ETH_EVENT_ERR_RECOVERING' value '11'
>>>>>>>               'rte_eth_event_type::RTE_ETH_EVENT_RECOVERED' value '12'
>>>>>>>             1 enumerator change:
>>>>>>>               'rte_eth_event_type::RTE_ETH_EVENT_MAX' from value '11' to '13' at rte_ethdev.h:3807:1
>>>>>>
>>>>>> I don't immediately see the problem that this would cause.
>>>>>> There are no array sizes etc dependent on the value of MAX for instance.
>>>>>>
>>>>>> Looks safe?
>>>>>
>>>>> We never know how this enum will be used by the application.
>>>>> The max value may be used for the size of an event array.
>>>>> It looks a real ABI issue unfortunately.
>>>>
>>>> Right - but we only really care about it when an array size based on MAX
>>>> is likely to be passed to DPDK, which doesn't apply in this case.
>>
>> I don't completely agree.
>> A developer may assume an event will never exceed MAX value.
>> However, after an upgrade of DPDK without app rebuild,
>> a higher event value may be received in the app,
>> breaking the assumption.
>> Should we consider this case as an ABI breakage?
> 
> Nope - I think we should explicitly exclude MAX values from any
> ABI guarantee, as being able to change them is key to our be able to
> evolve DPDK while maintaining ABI stability. 
> 
> Consider what it means applying the ABI policy to a MAX value, you are
> in effect saying that that no value can be added to this enumeration
> until the next ABI version, for me this is very restrictive without a
> solid reason. 
> 
>>
>>>> I noted that some Linux folks explicitly mark similar MAX values as not
>>>> part of the ABI.
>>>>
>>>> /usr/include/linux/perf_event.h
>>>> 37:     PERF_TYPE_MAX,                          /* non-ABI */
>>>> 60:     PERF_COUNT_HW_MAX,                      /* non-ABI */
>>>> 79:     PERF_COUNT_HW_CACHE_MAX,                /* non-ABI */
>>>> 87:     PERF_COUNT_HW_CACHE_OP_MAX,             /* non-ABI */
>>>> 94:     PERF_COUNT_HW_CACHE_RESULT_MAX,         /* non-ABI */
>>>> 116:    PERF_COUNT_SW_MAX,                      /* non-ABI */
>>>> 149:    PERF_SAMPLE_MAX = 1U << 24,             /* non-ABI */
>>>> 151:    __PERF_SAMPLE_CALLCHAIN_EARLY           = 1ULL << 63, /*
>>>> non-ABI; internal use */
>>>> 189:    PERF_SAMPLE_BRANCH_MAX_SHIFT            /* non-ABI */
>>>> 267:    PERF_TXN_MAX            = (1 << 8), /* non-ABI */
>>>> 301:    PERF_FORMAT_MAX = 1U << 4,              /* non-ABI */
>>>> 1067:   PERF_RECORD_MAX,                        /* non-ABI */
>>>> 1078:   PERF_RECORD_KSYMBOL_TYPE_MAX            /* non-ABI */
>>>> 1087:   PERF_BPF_EVENT_MAX,             /* non-ABI */
>>>
>>> Any thoughts on similarly annotating all our _MAX enums in the same way?
>>> We could also add a section in the ABI Policy to make it explicit _MAX
>>> enum values are not part of the ABI - what do folks think?
>>
>> Interesting. I am not sure it is always ABI-safe though.
> 
> 

Re: [dpdk-dev] [PATCH v7 1/4] ethdev: support device reset and recovery events

[Ray Kinsella]

fengchengwen <fengchengwen@huawei.com> writes:

> Hi all,
>
>   This patch lasts for a long time. Are we waiting for 22.11 to deal with it?

That was my read, as can't reliably change the value of _MAX at this
stage without it having impact elsewhere. 


>   We have the same requirements for the reset or recovery mechanism, but there are differences:
>
>     APP                                    PMD
>      |                                      |
>      |                                  detect error
>      |     <---report error event---        |
>      |                                      |
> do error stats                              |
> and report                                  |
>      |      ---start recover-->             |
>      |                                  do recover
>      |     <---report recover result        |
>      |                                      |
> if succ just log
> else may migrate
> service
>
> Can we generalize these processes(means that the implementation is at the framework layer)? or only at PMD API?
>
>
> On 2022/2/15 0:06, Ray Kinsella wrote:
>> 
>> Thomas Monjalon <thomas@monjalon.net> writes:
>> 
>>> 14/02/2022 11:16, Ray Kinsella:
>>>> Ray Kinsella <mdr@ashroe.eu> writes:
>>>>> Thomas Monjalon <thomas@monjalon.net> writes:
>>>>>> 02/02/2022 12:44, Ray Kinsella:
>>>>>>> Ferruh Yigit <ferruh.yigit@intel.com> writes:
>>>>>>>> On 1/28/2022 12:48 PM, Kalesh A P wrote:
>>>>>>>>> --- a/lib/ethdev/rte_ethdev.h
>>>>>>>>> +++ b/lib/ethdev/rte_ethdev.h
>>>>>>>>> @@ -3818,6 +3818,24 @@ enum rte_eth_event_type {
>>>>>>>>>   	RTE_ETH_EVENT_DESTROY,  /**< port is released */
>>>>>>>>>   	RTE_ETH_EVENT_IPSEC,    /**< IPsec offload related event */
>>>>>>>>>   	RTE_ETH_EVENT_FLOW_AGED,/**< New aged-out flows is detected */
>>>>>>>>> +	RTE_ETH_EVENT_ERR_RECOVERING,
>>>>>>>>> +			/**< port recovering from an error
>>>>>>>>> +			 *
>>>>>>>>> +			 * PMD detected a FW reset or error condition.
>>>>>>>>> +			 * PMD will try to recover from the error.
>>>>>>>>> +			 * Data path may be quiesced and Control path operations
>>>>>>>>> +			 * may fail at this time.
>>>>>>>>> +			 */
>>>>>>>>> +	RTE_ETH_EVENT_RECOVERED,
>>>>>>>>> +			/**< port recovered from an error
>>>>>>>>> +			 *
>>>>>>>>> +			 * PMD has recovered from the error condition.
>>>>>>>>> +			 * Control path and Data path are up now.
>>>>>>>>> +			 * PMD re-configures the port to the state prior to the error.
>>>>>>>>> +			 * Since the device has undergone a reset, flow rules
>>>>>>>>> +			 * offloaded prior to reset may be lost and
>>>>>>>>> +			 * the application should recreate the rules again.
>>>>>>>>> +			 */
>>>>>>>>>   	RTE_ETH_EVENT_MAX       /**< max value of this enum */
>>>>>>>>
>>>>>>>>
>>>>>>>> Also ABI check complains about 'RTE_ETH_EVENT_MAX' value check, cc'ed more people
>>>>>>>> to evaluate if it is a false positive:
>>>>>>>>
>>>>>>>>
>>>>>>>> 1 function with some indirect sub-type change:
>>>>>>>>   [C] 'function int rte_eth_dev_callback_register(uint16_t, rte_eth_event_type, rte_eth_dev_cb_fn, void*)' at rte_ethdev.c:4637:1 has some indirect sub-type changes:
>>>>>>>>     parameter 3 of type 'typedef rte_eth_dev_cb_fn' has sub-type changes:
>>>>>>>>       underlying type 'int (typedef uint16_t, enum rte_eth_event_type, void*, void*)*' changed:
>>>>>>>>         in pointed to type 'function type int (typedef uint16_t, enum rte_eth_event_type, void*, void*)':
>>>>>>>>           parameter 2 of type 'enum rte_eth_event_type' has sub-type changes:
>>>>>>>>             type size hasn't changed
>>>>>>>>             2 enumerator insertions:
>>>>>>>>               'rte_eth_event_type::RTE_ETH_EVENT_ERR_RECOVERING' value '11'
>>>>>>>>               'rte_eth_event_type::RTE_ETH_EVENT_RECOVERED' value '12'
>>>>>>>>             1 enumerator change:
>>>>>>>>               'rte_eth_event_type::RTE_ETH_EVENT_MAX' from value '11' to '13' at rte_ethdev.h:3807:1
>>>>>>>
>>>>>>> I don't immediately see the problem that this would cause.
>>>>>>> There are no array sizes etc dependent on the value of MAX for instance.
>>>>>>>
>>>>>>> Looks safe?
>>>>>>
>>>>>> We never know how this enum will be used by the application.
>>>>>> The max value may be used for the size of an event array.
>>>>>> It looks a real ABI issue unfortunately.
>>>>>
>>>>> Right - but we only really care about it when an array size based on MAX
>>>>> is likely to be passed to DPDK, which doesn't apply in this case.
>>>
>>> I don't completely agree.
>>> A developer may assume an event will never exceed MAX value.
>>> However, after an upgrade of DPDK without app rebuild,
>>> a higher event value may be received in the app,
>>> breaking the assumption.
>>> Should we consider this case as an ABI breakage?
>> 
>> Nope - I think we should explicitly exclude MAX values from any
>> ABI guarantee, as being able to change them is key to our be able to
>> evolve DPDK while maintaining ABI stability. 
>> 
>> Consider what it means applying the ABI policy to a MAX value, you are
>> in effect saying that that no value can be added to this enumeration
>> until the next ABI version, for me this is very restrictive without a
>> solid reason. 
>> 
>>>
>>>>> I noted that some Linux folks explicitly mark similar MAX values as not
>>>>> part of the ABI.
>>>>>
>>>>> /usr/include/linux/perf_event.h
>>>>> 37:     PERF_TYPE_MAX,                          /* non-ABI */
>>>>> 60:     PERF_COUNT_HW_MAX,                      /* non-ABI */
>>>>> 79:     PERF_COUNT_HW_CACHE_MAX,                /* non-ABI */
>>>>> 87:     PERF_COUNT_HW_CACHE_OP_MAX,             /* non-ABI */
>>>>> 94:     PERF_COUNT_HW_CACHE_RESULT_MAX,         /* non-ABI */
>>>>> 116:    PERF_COUNT_SW_MAX,                      /* non-ABI */
>>>>> 149:    PERF_SAMPLE_MAX = 1U << 24,             /* non-ABI */
>>>>> 151:    __PERF_SAMPLE_CALLCHAIN_EARLY           = 1ULL << 63, /*
>>>>> non-ABI; internal use */
>>>>> 189:    PERF_SAMPLE_BRANCH_MAX_SHIFT            /* non-ABI */
>>>>> 267:    PERF_TXN_MAX            = (1 << 8), /* non-ABI */
>>>>> 301:    PERF_FORMAT_MAX = 1U << 4,              /* non-ABI */
>>>>> 1067:   PERF_RECORD_MAX,                        /* non-ABI */
>>>>> 1078:   PERF_RECORD_KSYMBOL_TYPE_MAX            /* non-ABI */
>>>>> 1087:   PERF_BPF_EVENT_MAX,             /* non-ABI */
>>>>
>>>> Any thoughts on similarly annotating all our _MAX enums in the same way?
>>>> We could also add a section in the ABI Policy to make it explicit _MAX
>>>> enum values are not part of the ABI - what do folks think?
>>>
>>> Interesting. I am not sure it is always ABI-safe though.
>> 
>> 


-- 
Regards, Ray K

Re: [dpdk-dev] [PATCH v7 1/4] ethdev: support device reset and recovery events

[fengchengwen]

On 2022/5/24 23:11, Ray Kinsella wrote:
> 
> fengchengwen <fengchengwen@huawei.com> writes:
> 
>> Hi all,
>>
>>   This patch lasts for a long time. Are we waiting for 22.11 to deal with it?
> 
> That was my read, as can't reliably change the value of _MAX at this
> stage without it having impact elsewhere. 
> 
> 
>>   We have the same requirements for the reset or recovery mechanism, but there are differences:
>>
>>     APP                                    PMD
>>      |                                      |
>>      |                                  detect error
>>      |     <---report error event---        |
>>      |                                      |
>> do error stats                              |
>> and report                                  |
>>      |      ---start recover-->             |
>>      |                                  do recover
>>      |     <---report recover result        |
>>      |                                      |
>> if succ just log
>> else may migrate
>> service
>>
>> Can we generalize these processes(means that the implementation is at the framework layer)? or only at PMD API?
>>
>>
>> On 2022/2/15 0:06, Ray Kinsella wrote:
>>>
>>> Thomas Monjalon <thomas@monjalon.net> writes:
>>>
>>>> 14/02/2022 11:16, Ray Kinsella:
>>>>> Ray Kinsella <mdr@ashroe.eu> writes:
>>>>>> Thomas Monjalon <thomas@monjalon.net> writes:
>>>>>>> 02/02/2022 12:44, Ray Kinsella:
>>>>>>>> Ferruh Yigit <ferruh.yigit@intel.com> writes:
>>>>>>>>> On 1/28/2022 12:48 PM, Kalesh A P wrote:
>>>>>>>>>> --- a/lib/ethdev/rte_ethdev.h
>>>>>>>>>> +++ b/lib/ethdev/rte_ethdev.h
>>>>>>>>>> @@ -3818,6 +3818,24 @@ enum rte_eth_event_type {
>>>>>>>>>>   	RTE_ETH_EVENT_DESTROY,  /**< port is released */
>>>>>>>>>>   	RTE_ETH_EVENT_IPSEC,    /**< IPsec offload related event */
>>>>>>>>>>   	RTE_ETH_EVENT_FLOW_AGED,/**< New aged-out flows is detected */
>>>>>>>>>> +	RTE_ETH_EVENT_ERR_RECOVERING,
>>>>>>>>>> +			/**< port recovering from an error
>>>>>>>>>> +			 *
>>>>>>>>>> +			 * PMD detected a FW reset or error condition.
>>>>>>>>>> +			 * PMD will try to recover from the error.
>>>>>>>>>> +			 * Data path may be quiesced and Control path operations
>>>>>>>>>> +			 * may fail at this time.
>>>>>>>>>> +			 */

I think we should standard error reason which could pass to application, so that
application know the really reason. the error reason could as the ret_param of
rte_eth_dev_callback_process().

But I think it could be done later.

>>>>>>>>>> +	RTE_ETH_EVENT_RECOVERED,
>>>>>>>>>> +			/**< port recovered from an error
>>>>>>>>>> +			 *
>>>>>>>>>> +			 * PMD has recovered from the error condition.
>>>>>>>>>> +			 * Control path and Data path are up now.
>>>>>>>>>> +			 * PMD re-configures the port to the state prior to the error.
>>>>>>>>>> +			 * Since the device has undergone a reset, flow rules
>>>>>>>>>> +			 * offloaded prior to reset may be lost and
>>>>>>>>>> +			 * the application should recreate the rules again.
>>>>>>>>>> +			 */

please add RTE_ETH_EVENT_RECOVER_FAIL event, the RTE_ETH_EVENT_INTR_RMV event is
a big event, it has its own usecase. So please add the RECOVER_FAIL event to let
application decide remove or keep it.

>>>>>>>>>>   	RTE_ETH_EVENT_MAX       /**< max value of this enum */
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Also ABI check complains about 'RTE_ETH_EVENT_MAX' value check, cc'ed more people
>>>>>>>>> to evaluate if it is a false positive:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 1 function with some indirect sub-type change:
>>>>>>>>>   [C] 'function int rte_eth_dev_callback_register(uint16_t, rte_eth_event_type, rte_eth_dev_cb_fn, void*)' at rte_ethdev.c:4637:1 has some indirect sub-type changes:
>>>>>>>>>     parameter 3 of type 'typedef rte_eth_dev_cb_fn' has sub-type changes:
>>>>>>>>>       underlying type 'int (typedef uint16_t, enum rte_eth_event_type, void*, void*)*' changed:
>>>>>>>>>         in pointed to type 'function type int (typedef uint16_t, enum rte_eth_event_type, void*, void*)':
>>>>>>>>>           parameter 2 of type 'enum rte_eth_event_type' has sub-type changes:
>>>>>>>>>             type size hasn't changed
>>>>>>>>>             2 enumerator insertions:
>>>>>>>>>               'rte_eth_event_type::RTE_ETH_EVENT_ERR_RECOVERING' value '11'
>>>>>>>>>               'rte_eth_event_type::RTE_ETH_EVENT_RECOVERED' value '12'
>>>>>>>>>             1 enumerator change:
>>>>>>>>>               'rte_eth_event_type::RTE_ETH_EVENT_MAX' from value '11' to '13' at rte_ethdev.h:3807:1
>>>>>>>>
>>>>>>>> I don't immediately see the problem that this would cause.
>>>>>>>> There are no array sizes etc dependent on the value of MAX for instance.
>>>>>>>>
>>>>>>>> Looks safe?
>>>>>>>
>>>>>>> We never know how this enum will be used by the application.
>>>>>>> The max value may be used for the size of an event array.
>>>>>>> It looks a real ABI issue unfortunately.
>>>>>>
>>>>>> Right - but we only really care about it when an array size based on MAX
>>>>>> is likely to be passed to DPDK, which doesn't apply in this case.
>>>>
>>>> I don't completely agree.
>>>> A developer may assume an event will never exceed MAX value.
>>>> However, after an upgrade of DPDK without app rebuild,
>>>> a higher event value may be received in the app,
>>>> breaking the assumption.
>>>> Should we consider this case as an ABI breakage?
>>>
>>> Nope - I think we should explicitly exclude MAX values from any
>>> ABI guarantee, as being able to change them is key to our be able to
>>> evolve DPDK while maintaining ABI stability. 
>>>
>>> Consider what it means applying the ABI policy to a MAX value, you are
>>> in effect saying that that no value can be added to this enumeration
>>> until the next ABI version, for me this is very restrictive without a
>>> solid reason. 
>>>
>>>>
>>>>>> I noted that some Linux folks explicitly mark similar MAX values as not
>>>>>> part of the ABI.
>>>>>>
>>>>>> /usr/include/linux/perf_event.h
>>>>>> 37:     PERF_TYPE_MAX,                          /* non-ABI */
>>>>>> 60:     PERF_COUNT_HW_MAX,                      /* non-ABI */
>>>>>> 79:     PERF_COUNT_HW_CACHE_MAX,                /* non-ABI */
>>>>>> 87:     PERF_COUNT_HW_CACHE_OP_MAX,             /* non-ABI */
>>>>>> 94:     PERF_COUNT_HW_CACHE_RESULT_MAX,         /* non-ABI */
>>>>>> 116:    PERF_COUNT_SW_MAX,                      /* non-ABI */
>>>>>> 149:    PERF_SAMPLE_MAX = 1U << 24,             /* non-ABI */
>>>>>> 151:    __PERF_SAMPLE_CALLCHAIN_EARLY           = 1ULL << 63, /*
>>>>>> non-ABI; internal use */
>>>>>> 189:    PERF_SAMPLE_BRANCH_MAX_SHIFT            /* non-ABI */
>>>>>> 267:    PERF_TXN_MAX            = (1 << 8), /* non-ABI */
>>>>>> 301:    PERF_FORMAT_MAX = 1U << 4,              /* non-ABI */
>>>>>> 1067:   PERF_RECORD_MAX,                        /* non-ABI */
>>>>>> 1078:   PERF_RECORD_KSYMBOL_TYPE_MAX            /* non-ABI */
>>>>>> 1087:   PERF_BPF_EVENT_MAX,             /* non-ABI */
>>>>>
>>>>> Any thoughts on similarly annotating all our _MAX enums in the same way?
>>>>> We could also add a section in the ABI Policy to make it explicit _MAX
>>>>> enum values are not part of the ABI - what do folks think?
>>>>
>>>> Interesting. I am not sure it is always ABI-safe though.
>>>
>>>
> 
> 

[PATCH v8 0/4] ethdev: support error recovery notification

[Chengwen Feng]

This patchset support error recovery notification.

---
v8: Addressed comments from Thomas and Ferruh.
    Also introduced RECOVER_FAIL event.
    Add hns3 driver patch.
v7: Addressed comments from Thomas and Andrew.
v6: Addressed comments from Asaf Penso.
    1. Updated 20.11 release notes with the new events added.
    2. updated testpmd parse_event_printing_config function.
v5: Addressed comments from Ophir Munk.
    1. Renamed the new event name to RTE_ETH_EVENT_ERR_RECOVERING.
    2. Fixed testpmd logs.
    3. Documented the new recovery events.
v4: Addressed comments from Thomas Monjalon
    1. Added doxygen comments about new events.
V3: Fixed a typo in commit log.
V2: Added a new event RTE_ETH_EVENT_RESET instead of using the
    RTE_ETH_EVENT_INTR_RESET to notify applications about device reset.

Chengwen Feng (1):
  net/hns3: support error recovery notification event

Kalesh AP (3):
  ethdev: support device error recovery notification
  app/testpmd: handle error recovery notification event
  net/bnxt: notify applications about device reset/recovery

 app/test-pmd/parameters.c               | 10 ++++++--
 app/test-pmd/testpmd.c                  |  8 ++++++-
 doc/guides/prog_guide/poll_mode_drv.rst | 32 +++++++++++++++++++++++++
 doc/guides/rel_notes/release_22_07.rst  | 11 +++++++++
 drivers/net/bnxt/bnxt_cpr.c             |  4 ++++
 drivers/net/bnxt/bnxt_ethdev.c          | 11 ++++++++-
 drivers/net/hns3/hns3_intr.c            | 24 +++++++++++++++++++
 lib/ethdev/rte_ethdev.h                 |  6 +++++
 8 files changed, 102 insertions(+), 4 deletions(-)

-- 
2.33.0

[PATCH v8 1/4] ethdev: support device error recovery notification

[Chengwen Feng]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

Some PMDs (e.g. hns3) could detect hardware or firmware errors, and try
to recover from the errors. In this process, the PMD sets the data path
pointers to dummy functions (which will prevent the crash), and also
make sure the control path operations failed with retcode -EBUSY.

Also in this process, from the perspective of application, services are
affected. For example, the Rx/Tx bust APIs cannot receive and send
packets, and the control plane API return failure.

In some service scenarios, application needs to be aware of the event
to determine whether to migrate services. So three events were
introduced:

1. RTE_ETH_EVENT_ERR_RECOVERING: the PMD must trigger this event to
notify the application that it detected a hardware or firmware error
and tries to recover.
2. RTE_ETH_EVENT_RECOVER_SUCCESS: the PMD must trigger this event to
notify the application that it has recovered from the error. And PMD
already re-configures the port to the state prior to the error.
3. RTE_ETH_EVENT_RECOVER_FAILED: the PMD must trigger this event to
notify the application that it has failed to recover from the error.
The port may not be usable anymore.

Note: the error recovery of these events is mainly performed by the
PMD. Unlike the RTE_ETH_EVENT_INTR_RESET which the error recovery is
performed by the application. The PMD must ensure that the above two
error handling methods cannot be used at the same time.

Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
---
 doc/guides/prog_guide/poll_mode_drv.rst | 32 +++++++++++++++++++++++++
 doc/guides/rel_notes/release_22_07.rst  | 11 +++++++++
 lib/ethdev/rte_ethdev.h                 |  6 +++++
 3 files changed, 49 insertions(+)

diff --git a/doc/guides/prog_guide/poll_mode_drv.rst b/doc/guides/prog_guide/poll_mode_drv.rst
index 9d081b1cba..6398917485 100644
--- a/doc/guides/prog_guide/poll_mode_drv.rst
+++ b/doc/guides/prog_guide/poll_mode_drv.rst
@@ -627,3 +627,35 @@ by application.
 The PMD itself should not call rte_eth_dev_reset(). The PMD can trigger
 the application to handle reset event. It is duty of application to
 handle all synchronization before it calls rte_eth_dev_reset().
+
+Error Recovery Notification
+~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Some PMDs (e.g. hns3) could detect hardware or firmware errors, and try to
+recover from the errors. In this process, the PMD sets the data path pointers
+to dummy functions (which will prevent the crash), and also make sure the
+control path operations failed with retcode -EBUSY.
+
+Also in this process, from the perspective of application, services are
+affected. For example, the Rx/Tx bust APIs cannot receive and send packets,
+and the control plane API return failure.
+
+In some service scenarios, application needs to be aware of the event to
+determine whether to migrate services. So three events was introduced.
+
+The PMD must trigger RTE_ETH_EVENT_ERR_RECOVERING event to notify the
+application that it detected a hardware or firmware error and tries to recover.
+
+The PMD must trigger RTE_ETH_EVENT_RECOVER_SUCCESS event to notify the
+application that it has recovered from the error. And PMD already re-configures
+the port to the state prior to the error.
+
+The PMD must trigger RTE_ETH_EVENT_RECOVER_FAILED event to notify the
+application that it has failed to recover from the error. The port may not be
+usable anymore.
+
+.. note::
+        The error recovery of these events is mainly performed by the PMD.
+        Unlike the RTE_ETH_EVENT_INTR_RESET which the error recovery is
+        performed by the application. The PMD must ensure that the above two
+        error handling methods cannot be used at the same time.
diff --git a/doc/guides/rel_notes/release_22_07.rst b/doc/guides/rel_notes/release_22_07.rst
index 6fc044edaa..b237bd3303 100644
--- a/doc/guides/rel_notes/release_22_07.rst
+++ b/doc/guides/rel_notes/release_22_07.rst
@@ -108,6 +108,17 @@ New Features
 
   Added an API which can get the device type of vDPA device.
 
+* **Added error recover notification.**
+
+  Added error recover notification to application including:
+
+  * Added new event: ``RTE_ETH_EVENT_ERR_RECOVERING`` for the PMD to report
+    that the port is recovering from an error.
+  * Added new event: ``RTE_ETH_EVENT_RECOVER_SUCCESS`` for the PMD to report
+    that the port recover successful from an error.
+  * Added new event: ``RTE_ETH_EVENT_RECOVER_FAILED`` for the PMD to report
+    that the prot recover failed from an error.
+
 * **Updated Amazon ena driver.**
 
   The new driver version (v2.7.0) includes:
diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
index 045ee64747..6998f6f0be 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -3928,6 +3928,12 @@ enum rte_eth_event_type {
 	 * @see rte_eth_rx_avail_thresh_set()
 	 */
 	RTE_ETH_EVENT_RX_AVAIL_THRESH,
+	/** Port recovering from a hardware or firmware error */
+	RTE_ETH_EVENT_ERR_RECOVERING,
+	/** Port recovers successful from the error */
+	RTE_ETH_EVENT_RECOVER_SUCCESS,
+	/** Port recovers failed from the error */
+	RTE_ETH_EVENT_RECOVER_FAILED,
 	RTE_ETH_EVENT_MAX       /**< max value of this enum */
 };
 
-- 
2.33.0

[PATCH v8 2/4] app/testpmd: handle error recovery notification event

[Chengwen Feng]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

This patch handles error recovery notification event.

Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
---
 app/test-pmd/parameters.c | 10 ++++++++--
 app/test-pmd/testpmd.c    |  8 +++++++-
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index 641c9c767e..779a699371 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -175,9 +175,9 @@ usage(char* progname)
 	printf("  --no-rmv-interrupt: disable device removal interrupt.\n");
 	printf("  --bitrate-stats=N: set the logical core N to perform "
 		"bit-rate calculation.\n");
-	printf("  --print-event <unknown|intr_lsc|queue_state|intr_reset|vf_mbox|macsec|intr_rmv|flow_aged|all>: "
+	printf("  --print-event <unknown|intr_lsc|queue_state|intr_reset|vf_mbox|macsec|intr_rmv|flow_aged|err_recovering|recover_success|recover_failed|all>: "
 	       "enable print of designated event or all of them.\n");
-	printf("  --mask-event <unknown|intr_lsc|queue_state|intr_reset|vf_mbox|macsec|intr_rmv|flow_aged|all>: "
+	printf("  --mask-event <unknown|intr_lsc|queue_state|intr_reset|vf_mbox|macsec|intr_rmv|flow_aged|err_recovering|recover_success|recover_failed||all>: "
 	       "disable print of designated event or all of them.\n");
 	printf("  --flow-isolate-all: "
 	       "requests flow API isolated mode on all ports at initialization time.\n");
@@ -461,6 +461,12 @@ parse_event_printing_config(const char *optarg, int enable)
 		mask = UINT32_C(1) << RTE_ETH_EVENT_DESTROY;
 	else if (!strcmp(optarg, "flow_aged"))
 		mask = UINT32_C(1) << RTE_ETH_EVENT_FLOW_AGED;
+	else if (!strcmp(optarg, "err_recovering"))
+		mask = UINT32_C(1) << RTE_ETH_EVENT_ERR_RECOVERING;
+	else if (!strcmp(optarg, "recover_success"))
+		mask = UINT32_C(1) << RTE_ETH_EVENT_RECOVER_SUCCESS;
+	else if (!strcmp(optarg, "recover_failed"))
+		mask = UINT32_C(1) << RTE_ETH_EVENT_RECOVER_FAILED;
 	else if (!strcmp(optarg, "all"))
 		mask = ~UINT32_C(0);
 	else {
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 04c39adc21..96f5c8a63b 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -421,6 +421,9 @@ static const char * const eth_event_desc[] = {
 	[RTE_ETH_EVENT_DESTROY] = "device released",
 	[RTE_ETH_EVENT_FLOW_AGED] = "flow aged",
 	[RTE_ETH_EVENT_RX_AVAIL_THRESH] = "RxQ available descriptors threshold reached",
+	[RTE_ETH_EVENT_ERR_RECOVERING] = "error recovering",
+	[RTE_ETH_EVENT_RECOVER_SUCCESS] = "error recover successful",
+	[RTE_ETH_EVENT_RECOVER_FAILED] = "error recover failed",
 	[RTE_ETH_EVENT_MAX] = NULL,
 };
 
@@ -435,7 +438,10 @@ uint32_t event_print_mask = (UINT32_C(1) << RTE_ETH_EVENT_UNKNOWN) |
 			    (UINT32_C(1) << RTE_ETH_EVENT_IPSEC) |
 			    (UINT32_C(1) << RTE_ETH_EVENT_MACSEC) |
 			    (UINT32_C(1) << RTE_ETH_EVENT_INTR_RMV) |
-			    (UINT32_C(1) << RTE_ETH_EVENT_FLOW_AGED);
+			    (UINT32_C(1) << RTE_ETH_EVENT_FLOW_AGED) |
+			    (UINT32_C(1) << RTE_ETH_EVENT_ERR_RECOVERING) |
+			    (UINT32_C(1) << RTE_ETH_EVENT_RECOVER_SUCCESS) |
+			    (UINT32_C(1) << RTE_ETH_EVENT_RECOVER_FAILED);
 /*
  * Decide if all memory are locked for performance.
  */
-- 
2.33.0

[PATCH v8 3/4] net/hns3: support error recovery notification event

[Chengwen Feng]

This patch supports error recovery notification event:
1. RTE_ETH_EVENT_ERR_RECOVERING: the PMD report this event to nofify
the application that it detected a hardware or firmware error and
tries to recover.
2. RTE_ETH_EVENT_RECOVER_SUCCESS: the PMD report this event to nofify
the application that it has recovered from the error. And PMD already
re-configures the port to the state prior to the error.
3. RTE_ETH_EVENT_RECOVER_FAILED: the PMD report this event to notify
the application that it has failed to recover from the error. The port
may not be usable anymore.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 drivers/net/hns3/hns3_intr.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/net/hns3/hns3_intr.c b/drivers/net/hns3/hns3_intr.c
index 3ca2e1e338..d6d2209f73 100644
--- a/drivers/net/hns3/hns3_intr.c
+++ b/drivers/net/hns3/hns3_intr.c
@@ -1486,6 +1486,27 @@ static const struct hns3_hw_err_type hns3_hw_error_type[] = {
 	}
 };
 
+static void
+hns3_report_reset_begin(struct hns3_hw *hw)
+{
+	struct rte_eth_dev *dev = &rte_eth_devices[hw->data->port_id];
+	rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_ERR_RECOVERING, NULL);
+}
+
+static void
+hns3_report_reset_success(struct hns3_hw *hw)
+{
+	struct rte_eth_dev *dev = &rte_eth_devices[hw->data->port_id];
+	rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_RECOVER_SUCCESS, NULL);
+}
+
+static void
+hns3_report_reset_failed(struct hns3_hw *hw)
+{
+	struct rte_eth_dev *dev = &rte_eth_devices[hw->data->port_id];
+	rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_RECOVER_FAILED, NULL);
+}
+
 static int
 hns3_config_ncsi_hw_err_int(struct hns3_adapter *hns, bool en)
 {
@@ -2645,6 +2666,7 @@ hns3_reset_pre(struct hns3_adapter *hns)
 	if (hw->reset.stage == RESET_STAGE_NONE) {
 		__atomic_store_n(&hns->hw.reset.resetting, 1, __ATOMIC_RELAXED);
 		hw->reset.stage = RESET_STAGE_DOWN;
+		hns3_report_reset_begin(hw);
 		ret = hw->reset.ops->stop_service(hns);
 		hns3_clock_gettime(&tv);
 		if (ret) {
@@ -2754,6 +2776,7 @@ hns3_reset_post(struct hns3_adapter *hns)
 			  hns3_clock_calctime_ms(&tv_delta),
 			  tv.tv_sec, tv.tv_usec);
 		hw->reset.level = HNS3_NONE_RESET;
+		hns3_report_reset_success(hw);
 	}
 	return 0;
 
@@ -2799,6 +2822,7 @@ hns3_reset_fail_handle(struct hns3_adapter *hns)
 		  hns3_clock_calctime_ms(&tv_delta),
 		  tv.tv_sec, tv.tv_usec);
 	hw->reset.level = HNS3_NONE_RESET;
+	hns3_report_reset_failed(hw);
 }
 
 /*
-- 
2.33.0

[PATCH v8 4/4] net/bnxt: notify applications about device reset/recovery

[Chengwen Feng]

From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>

When the driver receives RESET_NOTIFY async event from FW or detects
an error condition, it should update the application that FW is going
to reset. Once the driver recovers from the reset, update the reset
recovery status to application as well.

Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
Reviewed-by: Chengwen Feng <fengchengwen@huawei.com>
---
 drivers/net/bnxt/bnxt_cpr.c    |  4 ++++
 drivers/net/bnxt/bnxt_ethdev.c | 11 ++++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/net/bnxt/bnxt_cpr.c b/drivers/net/bnxt/bnxt_cpr.c
index 99af0f9e87..5bb376d4d5 100644
--- a/drivers/net/bnxt/bnxt_cpr.c
+++ b/drivers/net/bnxt/bnxt_cpr.c
@@ -180,6 +180,10 @@ void bnxt_handle_async_event(struct bnxt *bp,
 			return;
 		}
 
+		rte_eth_dev_callback_process(bp->eth_dev,
+					     RTE_ETH_EVENT_ERR_RECOVERING,
+					     NULL);
+
 		pthread_mutex_lock(&bp->err_recovery_lock);
 		event_data = data1;
 		/* timestamp_lo/hi values are in units of 100ms */
diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
index 0f0f40b95b..cc02951d95 100644
--- a/drivers/net/bnxt/bnxt_ethdev.c
+++ b/drivers/net/bnxt/bnxt_ethdev.c
@@ -4397,13 +4397,18 @@ static void bnxt_dev_recover(void *arg)
 	PMD_DRV_LOG(INFO, "Port: %u Recovered from FW reset\n",
 		    bp->eth_dev->data->port_id);
 	pthread_mutex_unlock(&bp->err_recovery_lock);
-
+	rte_eth_dev_callback_process(bp->eth_dev,
+				     RTE_ETH_EVENT_RECOVER_SUCCESS,
+				     NULL);
 	return;
 err_start:
 	bnxt_dev_stop(bp->eth_dev);
 err:
 	bp->flags |= BNXT_FLAG_FATAL_ERROR;
 	bnxt_uninit_resources(bp, false);
+	rte_eth_dev_callback_process(bp->eth_dev,
+				     RTE_ETH_EVENT_RECOVER_FAILED,
+				     NULL);
 	if (bp->eth_dev->data->dev_conf.intr_conf.rmv)
 		rte_eth_dev_callback_process(bp->eth_dev,
 					     RTE_ETH_EVENT_INTR_RMV,
@@ -4575,6 +4580,10 @@ static void bnxt_check_fw_health(void *arg)
 
 	PMD_DRV_LOG(ERR, "Detected FW dead condition\n");
 
+	rte_eth_dev_callback_process(bp->eth_dev,
+				     RTE_ETH_EVENT_ERR_RECOVERING,
+				     NULL);
+
 	if (bnxt_is_primary_func(bp))
 		wait_msec = info->primary_func_wait_period;
 	else
-- 
2.33.0

Re: [PATCH v8 1/4] ethdev: support device error recovery notification

[Thomas Monjalon]

16/06/2022 11:41, Chengwen Feng:
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -3928,6 +3928,12 @@ enum rte_eth_event_type {
>  	 * @see rte_eth_rx_avail_thresh_set()
>  	 */
>  	RTE_ETH_EVENT_RX_AVAIL_THRESH,
> +	/** Port recovering from a hardware or firmware error */
> +	RTE_ETH_EVENT_ERR_RECOVERING,
> +	/** Port recovers successful from the error */
> +	RTE_ETH_EVENT_RECOVER_SUCCESS,
> +	/** Port recovers failed from the error */
> +	RTE_ETH_EVENT_RECOVER_FAILED,
>  	RTE_ETH_EVENT_MAX       /**< max value of this enum */
>  };

The descriptions here are not enough.
We cannot understand what has changed on the port,
and which action must be taken.

Note: I believe that's the same problem from the beginning:
we need a generic handling which is described in details.

Re: [PATCH v8 1/4] ethdev: support device error recovery notification

[fengchengwen]

Hi Thomas,

On 2022/6/21 1:42, Thomas Monjalon wrote:
> 16/06/2022 11:41, Chengwen Feng:
>> --- a/lib/ethdev/rte_ethdev.h
>> +++ b/lib/ethdev/rte_ethdev.h
>> @@ -3928,6 +3928,12 @@ enum rte_eth_event_type {
>>  	 * @see rte_eth_rx_avail_thresh_set()
>>  	 */
>>  	RTE_ETH_EVENT_RX_AVAIL_THRESH,
>> +	/** Port recovering from a hardware or firmware error */
>> +	RTE_ETH_EVENT_ERR_RECOVERING,
>> +	/** Port recovers successful from the error */
>> +	RTE_ETH_EVENT_RECOVER_SUCCESS,
>> +	/** Port recovers failed from the error */
>> +	RTE_ETH_EVENT_RECOVER_FAILED,
>>  	RTE_ETH_EVENT_MAX       /**< max value of this enum */
>>  };
> 
> The descriptions here are not enough.
> We cannot understand what has changed on the port,
> and which action must be taken.

There are detail descriptions in /doc/guides/prog_guide/poll_mode_drv.rst,
I will add your review in poll_mode_drv.rst.

Another question: do we need to add a detail description here as well? I think the poll_mode_drv.rst is enough.

> 
> Note: I believe that's the same problem from the beginning:
> we need a generic handling which is described in details.
> 
> 
> 
> .
> 

Re: [PATCH v8 1/4] ethdev: support device error recovery notification

[Thomas Monjalon]

21/06/2022 03:38, fengchengwen:
> Hi Thomas,
> 
> On 2022/6/21 1:42, Thomas Monjalon wrote:
> > 16/06/2022 11:41, Chengwen Feng:
> >> --- a/lib/ethdev/rte_ethdev.h
> >> +++ b/lib/ethdev/rte_ethdev.h
> >> @@ -3928,6 +3928,12 @@ enum rte_eth_event_type {
> >>  	 * @see rte_eth_rx_avail_thresh_set()
> >>  	 */
> >>  	RTE_ETH_EVENT_RX_AVAIL_THRESH,
> >> +	/** Port recovering from a hardware or firmware error */
> >> +	RTE_ETH_EVENT_ERR_RECOVERING,
> >> +	/** Port recovers successful from the error */
> >> +	RTE_ETH_EVENT_RECOVER_SUCCESS,
> >> +	/** Port recovers failed from the error */
> >> +	RTE_ETH_EVENT_RECOVER_FAILED,
> >>  	RTE_ETH_EVENT_MAX       /**< max value of this enum */
> >>  };
> > 
> > The descriptions here are not enough.
> > We cannot understand what has changed on the port,
> > and which action must be taken.
> 
> There are detail descriptions in /doc/guides/prog_guide/poll_mode_drv.rst,
> I will add your review in poll_mode_drv.rst.
> 
> Another question: do we need to add a detail description here as well? I think the poll_mode_drv.rst is enough.

It is the opposite: the RST guide is to give the overview,
while the doxygen comments are the precise API documentation.
You need to explain what is the state of the device.
Is it the same as after a call to rte_eth_dev_stop() ?
The application needs to know what must be reconfigured.

Re: [PATCH v8 1/4] ethdev: support device error recovery notification

[Ray Kinsella]

Chengwen Feng <fengchengwen@huawei.com> writes:

> From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
>
> Some PMDs (e.g. hns3) could detect hardware or firmware errors, and try
> to recover from the errors. In this process, the PMD sets the data path
> pointers to dummy functions (which will prevent the crash), and also
> make sure the control path operations failed with retcode -EBUSY.
>
> Also in this process, from the perspective of application, services are
> affected. For example, the Rx/Tx bust APIs cannot receive and send
> packets, and the control plane API return failure.
>
> In some service scenarios, application needs to be aware of the event
> to determine whether to migrate services. So three events were
> introduced:
>
> 1. RTE_ETH_EVENT_ERR_RECOVERING: the PMD must trigger this event to
> notify the application that it detected a hardware or firmware error
> and tries to recover.
> 2. RTE_ETH_EVENT_RECOVER_SUCCESS: the PMD must trigger this event to
> notify the application that it has recovered from the error. And PMD
> already re-configures the port to the state prior to the error.
> 3. RTE_ETH_EVENT_RECOVER_FAILED: the PMD must trigger this event to
> notify the application that it has failed to recover from the error.
> The port may not be usable anymore.
>
> Note: the error recovery of these events is mainly performed by the
> PMD. Unlike the RTE_ETH_EVENT_INTR_RESET which the error recovery is
> performed by the application. The PMD must ensure that the above two
> error handling methods cannot be used at the same time.
>
> Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> ---
>  doc/guides/prog_guide/poll_mode_drv.rst | 32 +++++++++++++++++++++++++
>  doc/guides/rel_notes/release_22_07.rst  | 11 +++++++++
>  lib/ethdev/rte_ethdev.h                 |  6 +++++
>  3 files changed, 49 insertions(+)
>
> diff --git a/doc/guides/prog_guide/poll_mode_drv.rst b/doc/guides/prog_guide/poll_mode_drv.rst
> index 9d081b1cba..6398917485 100644
> --- a/doc/guides/prog_guide/poll_mode_drv.rst
> +++ b/doc/guides/prog_guide/poll_mode_drv.rst
> @@ -627,3 +627,35 @@ by application.
>  The PMD itself should not call rte_eth_dev_reset(). The PMD can trigger
>  the application to handle reset event. It is duty of application to
>  handle all synchronization before it calls rte_eth_dev_reset().
> +
> +Error Recovery Notification
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +Some PMDs (e.g. hns3) could detect hardware or firmware errors, and try to
> +recover from the errors. In this process, the PMD sets the data path pointers
> +to dummy functions (which will prevent the crash), and also make sure the
> +control path operations failed with retcode -EBUSY.
> +
> +Also in this process, from the perspective of application, services are
> +affected. For example, the Rx/Tx bust APIs cannot receive and send packets,
> +and the control plane API return failure.
> +
> +In some service scenarios, application needs to be aware of the event to
> +determine whether to migrate services. So three events was introduced.
> +
> +The PMD must trigger RTE_ETH_EVENT_ERR_RECOVERING event to notify the
> +application that it detected a hardware or firmware error and tries to recover.
> +
> +The PMD must trigger RTE_ETH_EVENT_RECOVER_SUCCESS event to notify the
> +application that it has recovered from the error. And PMD already re-configures
> +the port to the state prior to the error.
> +
> +The PMD must trigger RTE_ETH_EVENT_RECOVER_FAILED event to notify the
> +application that it has failed to recover from the error. The port may not be
> +usable anymore.
> +
> +.. note::
> +        The error recovery of these events is mainly performed by the PMD.
> +        Unlike the RTE_ETH_EVENT_INTR_RESET which the error recovery is
> +        performed by the application. The PMD must ensure that the above two
> +        error handling methods cannot be used at the same time.
> diff --git a/doc/guides/rel_notes/release_22_07.rst b/doc/guides/rel_notes/release_22_07.rst
> index 6fc044edaa..b237bd3303 100644
> --- a/doc/guides/rel_notes/release_22_07.rst
> +++ b/doc/guides/rel_notes/release_22_07.rst
> @@ -108,6 +108,17 @@ New Features
>  
>    Added an API which can get the device type of vDPA device.
>  
> +* **Added error recover notification.**
> +
> +  Added error recover notification to application including:
> +
> +  * Added new event: ``RTE_ETH_EVENT_ERR_RECOVERING`` for the PMD to report
> +    that the port is recovering from an error.
> +  * Added new event: ``RTE_ETH_EVENT_RECOVER_SUCCESS`` for the PMD to report
> +    that the port recover successful from an error.

RTE_ETH_EVENT_RECOVERY_SUCCESS

> +  * Added new event: ``RTE_ETH_EVENT_RECOVER_FAILED`` for the PMD to
> report

RTE_ETH_EVENT_RECOVERY_FAILED

> +    that the prot recover failed from an error

to report that port recovery failed

> +
>  * **Updated Amazon ena driver.**
>  
>    The new driver version (v2.7.0) includes:
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index 045ee64747..6998f6f0be 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -3928,6 +3928,12 @@ enum rte_eth_event_type {
>  	 * @see rte_eth_rx_avail_thresh_set()
>  	 */
>  	RTE_ETH_EVENT_RX_AVAIL_THRESH,
> +	/** Port recovering from a hardware or firmware error */
> +	RTE_ETH_EVENT_ERR_RECOVERING,
> +	/** Port recovers successful from the error */
> +	RTE_ETH_EVENT_RECOVER_SUCCESS,

RTE_ETH_EVENT_RECOVERY_SUCCESS

> +	/** Port recovers failed from the error */
> +	RTE_ETH_EVENT_RECOVER_FAILED,
RTE_ETH_EVENT_RECOVERY_FAILED

>  	RTE_ETH_EVENT_MAX       /**< max value of this enum */
>  };



-- 
Regards, Ray K

Re: [PATCH v8 1/4] ethdev: support device error recovery notification

[fengchengwen]

Hi Thomas,

On 2022/6/21 15:04, Thomas Monjalon wrote:
> 21/06/2022 03:38, fengchengwen:
>> Hi Thomas,
>>
>> On 2022/6/21 1:42, Thomas Monjalon wrote:
>>> 16/06/2022 11:41, Chengwen Feng:
>>>> --- a/lib/ethdev/rte_ethdev.h
>>>> +++ b/lib/ethdev/rte_ethdev.h
>>>> @@ -3928,6 +3928,12 @@ enum rte_eth_event_type {
>>>>  	 * @see rte_eth_rx_avail_thresh_set()
>>>>  	 */
>>>>  	RTE_ETH_EVENT_RX_AVAIL_THRESH,
>>>> +	/** Port recovering from a hardware or firmware error */
>>>> +	RTE_ETH_EVENT_ERR_RECOVERING,
>>>> +	/** Port recovers successful from the error */
>>>> +	RTE_ETH_EVENT_RECOVER_SUCCESS,
>>>> +	/** Port recovers failed from the error */
>>>> +	RTE_ETH_EVENT_RECOVER_FAILED,
>>>>  	RTE_ETH_EVENT_MAX       /**< max value of this enum */
>>>>  };
>>>
>>> The descriptions here are not enough.
>>> We cannot understand what has changed on the port,
>>> and which action must be taken.
>>
>> There are detail descriptions in /doc/guides/prog_guide/poll_mode_drv.rst,
>> I will add your review in poll_mode_drv.rst.
>>
>> Another question: do we need to add a detail description here as well? I think the poll_mode_drv.rst is enough.
> 
> It is the opposite: the RST guide is to give the overview,
> while the doxygen comments are the precise API documentation.
> You need to explain what is the state of the device.
> Is it the same as after a call to rte_eth_dev_stop() ?
> The application needs to know what must be reconfigured.

V9 already sent to address these, please take time to review, thanks.

> 
> 
> 
> 
> .
>