Re: [PATCH 4/5] net/bnxt: use fp ops setup function

[Ajit Khaparde <ajit.khaparde@broadcom.com>]

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

On Sun, Mar 5, 2023 at 7:58 AM Konstantin Ananyev
<konstantin.v.ananyev@yandex.ru> wrote:
>
> 03/03/2023 01:38, fengchengwen пишет:
> > On 2023/3/2 20:30, Konstantin Ananyev wrote:
> >>
> >>> Use rte_eth_fp_ops_setup() instead of directly manipulating
> >>> rte_eth_fp_ops variable.
> >>>
> >>> Cc: stable@dpdk.org
> >>>
> >>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> >>> ---
> >>>   drivers/net/bnxt/bnxt_cpr.c    | 5 +----
> >>>   drivers/net/bnxt/bnxt_ethdev.c | 5 +----
> >>>   2 files changed, 2 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/drivers/net/bnxt/bnxt_cpr.c b/drivers/net/bnxt/bnxt_cpr.c
> >>> index 3950840600..a3f33c24c3 100644
> >>> --- a/drivers/net/bnxt/bnxt_cpr.c
> >>> +++ b/drivers/net/bnxt/bnxt_cpr.c
> >>> @@ -416,10 +416,7 @@ void bnxt_stop_rxtx(struct rte_eth_dev *eth_dev)
> >>>     eth_dev->rx_pkt_burst = rte_eth_pkt_burst_dummy;
> >>>     eth_dev->tx_pkt_burst = rte_eth_pkt_burst_dummy;
> >>
> >> I am not that familiar with bnxt driver, but shouldn't we set here
> >> other optional fp_ops (descripto_status, etc.) to some dummy values OR to null values?
> >
> > I checked the bnxt PMD code, the other fp_ops (rx_queue_count/rx_descriptor_status/tx_descriptor_status)
> > both add following logic at the beginning of function:
> >
> >       rc = is_bnxt_in_error(bp);
> >       if (rc)
> >               return rc;
> >
> > So I think it okey to keep it.
>
> I still think it is much safer/cleaner to update all fp_ops in one go
> (use fp_ops_reset()) here.
> But as you believe it would work either way, I'll leave it to bnxt
> maintainers to decide.

We have been operating without the application being aware of the
underlying functionality for some time now.
Each step here is an improvement.
I think it is okay to keep it simple and update them separately.

Thanks
Ajit

>
>
> >
> >>
> >>>
> >>> -   rte_eth_fp_ops[eth_dev->data->port_id].rx_pkt_burst =
> >>> -           eth_dev->rx_pkt_burst;
> >>> -   rte_eth_fp_ops[eth_dev->data->port_id].tx_pkt_burst =
> >>> -           eth_dev->tx_pkt_burst;
> >>> +   rte_eth_fp_ops_setup(eth_dev);
> >>>     rte_mb();
> >>>
> >>>     /* Allow time for threads to exit the real burst functions. */
> >>> diff --git a/drivers/net/bnxt/bnxt_ethdev.c b/drivers/net/bnxt/bnxt_ethdev.c
> >>> index 4083a69d02..d6064ceea4 100644
> >>> --- a/drivers/net/bnxt/bnxt_ethdev.c
> >>> +++ b/drivers/net/bnxt/bnxt_ethdev.c
> >>> @@ -4374,10 +4374,7 @@ static void bnxt_dev_recover(void *arg)
> >>>     if (rc)
> >>>             goto err_start;
> >>>
> >>> -   rte_eth_fp_ops[bp->eth_dev->data->port_id].rx_pkt_burst =
> >>> -           bp->eth_dev->rx_pkt_burst;
> >>> -   rte_eth_fp_ops[bp->eth_dev->data->port_id].tx_pkt_burst =
> >>> -           bp->eth_dev->tx_pkt_burst;
> >>> +   rte_eth_fp_ops_setup(bp->eth_dev);
> >>>     rte_mb();
> >>>
> >>>     PMD_DRV_LOG(INFO, "Port: %u Recovered from FW reset\n",
> >>> --
> >>> 2.17.1
> >>
> >> .
> >>
>

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