[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 at 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 at 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 at dpdk.org> On Behalf Of Kalesh A P
> >>> Sent: Wednesday, September 30, 2020 3:33 PM
> >>> To: dev at dpdk.org
> >>> Subject: [dpdk-dev] [RFC PATCH v4 3/3] app/testpmd: handle device
> >> recovery
> >>> event
> >>>
> >>> From: Kalesh AP <kalesh-anakkur.purayil at 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 at broadcom.com>
> >>> Reviewed-by: Ajit Kumar Khaparde <ajit.khaparde at 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
> >>
> >>
> >
>