On Tue, Oct 25, 2022 at 10:31:06AM -0700, Ajit Khaparde wrote:
> Use auxiliary driver interface for driver load, unload ROCE driver.
> The driver does not need to register the interface using the netdev
> notifier anymore. Removed the bnxt_re_dev_list which is not needed.
> Currently probe, remove and shutdown ops have been implemented for
> the auxiliary device.
>
> Signed-off-by: Ajit Khaparde <[email protected]>
> Reviewed-by: Andy Gospodarek <[email protected]>
> ---
> drivers/infiniband/hw/bnxt_re/bnxt_re.h | 9 +-
> drivers/infiniband/hw/bnxt_re/main.c | 387 +++++++-----------
> drivers/net/ethernet/broadcom/bnxt/bnxt.c | 64 ---
> drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c | 67 ++-
> drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h | 3 +
> 5 files changed, 214 insertions(+), 316 deletions(-)
<...>
> -static struct bnxt_en_dev *bnxt_re_dev_probe(struct net_device *netdev)
> +static struct bnxt_en_dev *bnxt_re_dev_probe(struct auxiliary_device *adev)
> {
> - struct bnxt_en_dev *en_dev;
> + struct bnxt_aux_dev *aux_dev =
> + container_of(adev, struct bnxt_aux_dev, aux_dev);
> + struct bnxt_en_dev *en_dev = NULL;
> struct pci_dev *pdev;
>
> - en_dev = ((struct bnxt*)netdev_priv(netdev))->edev;
> - if (IS_ERR(en_dev))
> - return en_dev;
> + if (aux_dev)
> + en_dev = aux_dev->edev;
> +
> + if (!en_dev)
> + return NULL;
Thank you for working to convert this driver to auxiliary bus. I'm
confident that it will be ready soon.
In order to effectively review this series, you need to structure
patches in such way that you don't remove in patch X+1 code that you
added in patch X.
Also you should remove maze of redundant functions that do nothing, but
just call to another function with useless checks.
Auxiliary devices shouldn't be created if en_dev == NULL.
Thanks
On Wed, Oct 26, 2022 at 2:24 AM Leon Romanovsky <[email protected]> wrote:
>
> On Tue, Oct 25, 2022 at 10:31:06AM -0700, Ajit Khaparde wrote:
> > Use auxiliary driver interface for driver load, unload ROCE driver.
> > The driver does not need to register the interface using the netdev
> > notifier anymore. Removed the bnxt_re_dev_list which is not needed.
> > Currently probe, remove and shutdown ops have been implemented for
> > the auxiliary device.
> >
> > Signed-off-by: Ajit Khaparde <[email protected]>
> > Reviewed-by: Andy Gospodarek <[email protected]>
> > ---
> > drivers/infiniband/hw/bnxt_re/bnxt_re.h | 9 +-
> > drivers/infiniband/hw/bnxt_re/main.c | 387 +++++++-----------
> > drivers/net/ethernet/broadcom/bnxt/bnxt.c | 64 ---
> > drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c | 67 ++-
> > drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h | 3 +
> > 5 files changed, 214 insertions(+), 316 deletions(-)
>
> <...>
>
> > -static struct bnxt_en_dev *bnxt_re_dev_probe(struct net_device *netdev)
> > +static struct bnxt_en_dev *bnxt_re_dev_probe(struct auxiliary_device *adev)
> > {
> > - struct bnxt_en_dev *en_dev;
> > + struct bnxt_aux_dev *aux_dev =
> > + container_of(adev, struct bnxt_aux_dev, aux_dev);
> > + struct bnxt_en_dev *en_dev = NULL;
> > struct pci_dev *pdev;
> >
> > - en_dev = ((struct bnxt*)netdev_priv(netdev))->edev;
> > - if (IS_ERR(en_dev))
> > - return en_dev;
> > + if (aux_dev)
> > + en_dev = aux_dev->edev;
> > +
> > + if (!en_dev)
> > + return NULL;
>
> Thank you for working to convert this driver to auxiliary bus. I'm
> confident that it will be ready soon.
Thanks
>
> In order to effectively review this series, you need to structure
> patches in such way that you don't remove in patch X+1 code that you
> added in patch X.
Sure. Moving from v1 to v2, I surely ran into the situation and cleaned them.
I will clean up the rest in case I missed something.
>
> Also you should remove maze of redundant functions that do nothing, but
> just call to another function with useless checks.
ACK
>
> Auxiliary devices shouldn't be created if en_dev == NULL.
ACK
& Thanks
>
> Thanks
On Fri, Nov 04, 2022 at 09:27:32AM -0700, Ajit Khaparde wrote:
> Wherever possible use the function ops provided by auxiliary bus
> instead of using proprietary ops.
>
> Defined bnxt_re_suspend and bnxt_re_resume calls which can be
> invoked by the bnxt_en driver instead of the ULP stop/start calls.
>
> Signed-off-by: Ajit Khaparde <[email protected]>
> Reviewed-by: Andy Gospodarek <[email protected]>
> ---
> drivers/infiniband/hw/bnxt_re/main.c | 102 +++++++++++-------
> drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c | 40 ++++---
> drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h | 2 -
> 3 files changed, 87 insertions(+), 57 deletions(-)
<...>
> void bnxt_ulp_sriov_cfg(struct bnxt *bp, int num_vfs)
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h
> index 26b7c627342b..e96f93d38a30 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h
> @@ -29,8 +29,6 @@ struct bnxt_msix_entry {
> struct bnxt_ulp_ops {
Once you convert to use AUX bus, this struct should go too.
> /* async_notifier() cannot sleep (in BH context) */
> void (*ulp_async_notifier)(void *, struct hwrm_async_event_cmpl *);
> - void (*ulp_stop)(void *);
> - void (*ulp_start)(void *);
> void (*ulp_sriov_config)(void *, int);
> void (*ulp_shutdown)(void *);
> void (*ulp_irq_stop)(void *);
> --
> 2.37.1 (Apple Git-137.1)
>
On Fri, Nov 04, 2022 at 09:27:27AM -0700, Ajit Khaparde wrote:
> Add auxiliary device driver for Broadcom devices.
> The bnxt_en driver will register and initialize an aux device
> if RDMA is enabled in the underlying device.
> The bnxt_re driver will then probe and initialize the
> RoCE interfaces with the infiniband stack.
>
> v1->v2:
> - Incorporated review comments including usage of ulp_id &
> complex function indirections.
> - Used function calls provided by the auxiliary bus interface
> instead of proprietary calls.
> - Refactor code to remove ROCE driver's access to bnxt structure.
>
> v2->v3:
> - Addressed review comments including cleanup of some unnecessary wrappers
> - Fixed warnings seen during cross compilation
>
> Please apply. Thanks.
Please send this series as standalone one and not as a reply
to previous discussion. Reply-to messes review flow, especially
for series.
Jakub, I'll review it once Ajit will send it properly.
Thanks
On Mon, 7 Nov 2022 08:37:13 +0200 Leon Romanovsky wrote:
> Please send this series as standalone one and not as a reply
> to previous discussion. Reply-to messes review flow, especially
> for series.
>
> Jakub, I'll review it once Ajit will send it properly.
IIUC we wait for you or Jason to review any of the RoCE bifurcation
patches before considering them for inclusion.
On Mon, Nov 07, 2022 at 08:06:05AM -0800, Jakub Kicinski wrote:
> On Mon, 7 Nov 2022 08:37:13 +0200 Leon Romanovsky wrote:
> > Please send this series as standalone one and not as a reply
> > to previous discussion. Reply-to messes review flow, especially
> > for series.
> >
> > Jakub, I'll review it once Ajit will send it properly.
>
> IIUC we wait for you or Jason to review any of the RoCE bifurcation
> patches before considering them for inclusion.
Thanks
On Sun, Nov 6, 2022 at 10:37 PM Leon Romanovsky <[email protected]> wrote:
>
> On Fri, Nov 04, 2022 at 09:27:27AM -0700, Ajit Khaparde wrote:
> > Add auxiliary device driver for Broadcom devices.
> > The bnxt_en driver will register and initialize an aux device
> > if RDMA is enabled in the underlying device.
> > The bnxt_re driver will then probe and initialize the
> > RoCE interfaces with the infiniband stack.
> >
> > v1->v2:
> > - Incorporated review comments including usage of ulp_id &
> > complex function indirections.
> > - Used function calls provided by the auxiliary bus interface
> > instead of proprietary calls.
> > - Refactor code to remove ROCE driver's access to bnxt structure.
> >
> > v2->v3:
> > - Addressed review comments including cleanup of some unnecessary wrappers
> > - Fixed warnings seen during cross compilation
> >
> > Please apply. Thanks.
>
> Please send this series as standalone one and not as a reply
> to previous discussion. Reply-to messes review flow, especially
> for series.
ACK.
>
> Jakub, I'll review it once Ajit will send it properly.
>
> Thanks
On Sun, Nov 6, 2022 at 10:41 PM Leon Romanovsky <[email protected]> wrote:
>
> On Fri, Nov 04, 2022 at 09:27:32AM -0700, Ajit Khaparde wrote:
> > Wherever possible use the function ops provided by auxiliary bus
> > instead of using proprietary ops.
> >
> > Defined bnxt_re_suspend and bnxt_re_resume calls which can be
> > invoked by the bnxt_en driver instead of the ULP stop/start calls.
> >
> > Signed-off-by: Ajit Khaparde <[email protected]>
> > Reviewed-by: Andy Gospodarek <[email protected]>
> > ---
> > drivers/infiniband/hw/bnxt_re/main.c | 102 +++++++++++-------
> > drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c | 40 ++++---
> > drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h | 2 -
> > 3 files changed, 87 insertions(+), 57 deletions(-)
>
> <...>
>
> > void bnxt_ulp_sriov_cfg(struct bnxt *bp, int num_vfs)
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h
> > index 26b7c627342b..e96f93d38a30 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h
> > @@ -29,8 +29,6 @@ struct bnxt_msix_entry {
> > struct bnxt_ulp_ops {
>
> Once you convert to use AUX bus, this struct should go too.
We got rid of the bnxt_en_ops which the bnxt_re driver used to
communicate with bnxt_en.
Similarly We have tried to clean up most of the bnxt_ulp_ops.
In most of the cases we used the functions and entry points provided
by the auxiliary bus driver framework.
As you can see in the v4, there are the minimal functions needed to
support the functionality.
We will try to work on getting rid of the remaining if we find any
other viable alternative for those in future.
>
> > /* async_notifier() cannot sleep (in BH context) */
> > void (*ulp_async_notifier)(void *, struct hwrm_async_event_cmpl *);
> > - void (*ulp_stop)(void *);
> > - void (*ulp_start)(void *);
> > void (*ulp_sriov_config)(void *, int);
> > void (*ulp_shutdown)(void *);
> > void (*ulp_irq_stop)(void *);
> > --
> > 2.37.1 (Apple Git-137.1)
> >
>
>