Re: [PATCH v2 2/6] RDMA/bnxt_re: Use auxiliary driver interface

Ajit Khaparde
2022-10-26T15:58:24Z

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

On Wed, Oct 26, 2022 at 2:24 AM Leon Romanovsky <leon@kernel.org> 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 <ajit.khaparde@broadcom.com>
> > Reviewed-by: Andy Gospodarek <andrew.gospodarek@broadcom.com>
> > ---
> > 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