netdev - Re: [PATCH rdma-next v2 RESEND 0/4] RDMA/bnxt_re: Support for FW async event handling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAH-L+nPYMQAHwb+k=LDaGwUUFs+j4jPc2tC079GhtyS8fQDrNQ@mail.gmail.com>
Date: Wed, 15 Jan 2025 09:31:42 +0530
From: Kalesh Anakkur Purayil <kalesh-anakkur.purayil@...adcom.com>
To: Leon Romanovsky <leon@...nel.org>
Cc: jgg@...pe.ca, linux-rdma@...r.kernel.org, davem@...emloft.net, 
	edumazet@...gle.com, kuba@...nel.org, netdev@...r.kernel.org, 
	pabeni@...hat.com, andrew+netdev@...n.ch, andrew.gospodarek@...adcom.com, 
	selvin.xavier@...adcom.com, michael.chan@...adcom.com, 
	pavan.chebbi@...adcom.com
Subject: Re: [PATCH rdma-next v2 RESEND 0/4] RDMA/bnxt_re: Support for FW
 async event handling

On Tue, Jan 14, 2025 at 2:35 PM Leon Romanovsky <leon@...nel.org> wrote:
>
> On Tue, Jan 07, 2025 at 08:15:48AM +0530, Kalesh AP wrote:
> > This patch series adds support for FW async event handling
> > in the bnxt_re driver.
> >
> > V1->V2:
> > 1. Rebased on top of the latest "for-next" tree.
> > 2. Split Patch#1 into 2 - one for Ethernet driver changes and
> >    another one for RDMA driver changes.
> > 3. Addressed Leon's comments on Patch#1 and Patch #3.
> > V1: https://lore.kernel.org/linux-rdma/1725363051-19268-1-git-send-email-selvin.xavier@broadcom.com/T/#t
> >
> > Patch #1:
> > 1. Removed BNXT_EN_FLAG_ULP_STOPPED state check from bnxt_ulp_async_events().
> >    The ulp_ops are protected by RCU. This means that during bnxt_unregister_dev(),
> >    Ethernet driver set the ulp_ops pointer to NULL and do RCU sync before return
> >    to the RDMA driver.
> >    So ulp_ops and the pointers in ulp_ops are always valid or NULL when the
> >    Ethernet driver references ulp_ops. ULP_STOPPED is a state and should be
> >    unrelated to async events. It should not affect whether async events should
> >    or should not be passed to the RDMA driver.
> > 2. Changed Author of Ethernet driver changes to Michael Chan.
> > 3. Removed unnecessary export of function bnxt_ulp_async_events.
> >
> > Patch #3:
> > 1. Removed unnecessary flush_workqueue() before destroy_workqueue()
> > 2. Removed unnecessary NULL assignment after free.
> > 3. Changed to use "ibdev_xxx" and reduce level of couple of logs to debug.
> >
> > Please review and apply.
> >
> > Regards,
> > Kalesh
> >
> >
> > Kalesh AP (3):
> >   RDMA/bnxt_re: Add Async event handling support
> >   RDMA/bnxt_re: Query firmware defaults of CC params during probe
> >   RDMA/bnxt_re: Add support to handle DCB_CONFIG_CHANGE event
> >
> > Michael Chan (1):
> >   bnxt_en: Add ULP call to notify async events
> >
> >  drivers/infiniband/hw/bnxt_re/bnxt_re.h       |   3 +
> >  drivers/infiniband/hw/bnxt_re/main.c          | 156 ++++++++++++++++++
> >  drivers/infiniband/hw/bnxt_re/qplib_fp.h      |   1 +
> >  drivers/infiniband/hw/bnxt_re/qplib_sp.c      | 113 +++++++++++++
> >  drivers/infiniband/hw/bnxt_re/qplib_sp.h      |   3 +
> >  drivers/net/ethernet/broadcom/bnxt/bnxt.c     |   1 +
> >  drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c |  28 ++++
> >  drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h |   2 +
> >  8 files changed, 307 insertions(+)
>
> Applied with the following fix
>
> diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
> index 1dc305689d7bb..54dee0f5dd3f5 100644
> --- a/drivers/infiniband/hw/bnxt_re/main.c
> +++ b/drivers/infiniband/hw/bnxt_re/main.c
> @@ -1802,30 +1802,22 @@ static int bnxt_re_setup_qos(struct bnxt_re_dev *rdev)
>
>  static void bnxt_re_net_unregister_async_event(struct bnxt_re_dev *rdev)
>  {
> -       int rc;
> -
>         if (rdev->is_virtfn)
>                 return;
>
>         memset(&rdev->event_bitmap, 0, sizeof(rdev->event_bitmap));
> -       rc = bnxt_register_async_events(rdev->en_dev, &rdev->event_bitmap,
> -                                       ASYNC_EVENT_CMPL_EVENT_ID_DCB_CONFIG_CHANGE);
> -       if (rc)
> -               ibdev_err(&rdev->ibdev, "Failed to unregister async event");
> +       bnxt_register_async_events(rdev->en_dev, &rdev->event_bitmap,
> +                                  ASYNC_EVENT_CMPL_EVENT_ID_DCB_CONFIG_CHANGE);
>  }
>
>  static void bnxt_re_net_register_async_event(struct bnxt_re_dev *rdev)
>  {
> -       int rc;
> -
>         if (rdev->is_virtfn)
>                 return;
>
>         rdev->event_bitmap |= (1 << ASYNC_EVENT_CMPL_EVENT_ID_DCB_CONFIG_CHANGE);
> -       rc = bnxt_register_async_events(rdev->en_dev, &rdev->event_bitmap,
> -                                       ASYNC_EVENT_CMPL_EVENT_ID_DCB_CONFIG_CHANGE);
> -       if (rc)
> -               ibdev_err(&rdev->ibdev, "Failed to unregister async event");
> +       bnxt_register_async_events(rdev->en_dev, &rdev->event_bitmap,
> +                                  ASYNC_EVENT_CMPL_EVENT_ID_DCB_CONFIG_CHANGE);
>  }
>
>  static void bnxt_re_query_hwrm_intf_version(struct bnxt_re_dev *rdev)
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
> index 59c280634bc5f..3e17db0a453e0 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c
> @@ -373,9 +373,8 @@ void bnxt_ulp_async_events(struct bnxt *bp, struct hwrm_async_event_cmpl *cmpl)
>         rcu_read_unlock();
>  }
>
> -int bnxt_register_async_events(struct bnxt_en_dev *edev,
> -                              unsigned long *events_bmap,
> -                              u16 max_id)
> +void bnxt_register_async_events(struct bnxt_en_dev *edev,
> +                               unsigned long *events_bmap, u16 max_id)
>  {
>         struct net_device *dev = edev->net;
>         struct bnxt *bp = netdev_priv(dev);
> @@ -387,7 +386,6 @@ int bnxt_register_async_events(struct bnxt_en_dev *edev,
>         smp_wmb();
>         ulp->max_async_event_id = max_id;
>         bnxt_hwrm_func_drv_rgtr(bp, events_bmap, max_id + 1, true);
> -       return 0;
>  }
>  EXPORT_SYMBOL(bnxt_register_async_events);
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h
> index a21294cf197b8..ee6a5b8562c3e 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h
> @@ -126,6 +126,6 @@ int bnxt_register_dev(struct bnxt_en_dev *edev, struct bnxt_ulp_ops *ulp_ops,
>                       void *handle);
>  void bnxt_unregister_dev(struct bnxt_en_dev *edev);
>  int bnxt_send_msg(struct bnxt_en_dev *edev, struct bnxt_fw_msg *fw_msg);
> -int bnxt_register_async_events(struct bnxt_en_dev *edev,
> -                              unsigned long *events_bmap, u16 max_id);
> +void bnxt_register_async_events(struct bnxt_en_dev *edev,
> +                               unsigned long *events_bmap, u16 max_id);
>  #endif

Makes sense and LGTM, thank you Leon!
>
> >
> > --
> > 2.43.5
> >




--
Regards,
Kalesh AP

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4239 bytes)