Messages in this thread | | | Date | Thu, 8 Dec 2022 12:28:19 +0200 | From | Leon Romanovsky <> | Subject | Re: [PATCH v5 0/7] Add Auxiliary driver support |
| |
On Wed, Dec 07, 2022 at 09:53:03AM -0800, 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. > > 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. > And now these 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 option in future. > > 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.
I still see wrong usage of auxiliary driver model, especially for RDMA device. That model mimics general driver model, where you should separate between device creation and configuration.
I would expect that your bnxt_en create pre-configured devices with right amount of MSI-X, limits, capabilities e.t.c and RDMA driver will simply bind to it. It means that calls like bnxt_re_request_msix() should go too. All PCI-related logic needs to be in netdev.
In addition, I saw IS_ERR_OR_NULL(..) and "if(dev)" checks in various uninit functions and it can be one of two: wrong unwind flow or wrong use of driver model. In right implementation, your driver will be called only on valid device and uninit won't be called for not-initialized device.
Also I spotted .ulp_async_notifier, which is not used and bnxt_re_sriov_config() is prune to races due to separation between driver bind and device creation. You should configure SR-IOV in device creation stage.
Thanks
> > v2->v3: > - Addressed review comments including cleanup of some unnecessary wrappers > - Fixed warnings seen during cross compilation > > v3->v4: > - Cleaned up bnxt_ulp.c and bnxt_ulp.h further > - Removed some more dead code > - Sending the patchset as a standalone series > > v4->v5: > - Removed the SRIOV config callback which bnxt_en driver was calling into > bnxt_re driver. > - Removed excessive checks for rdev and other pointers. > > Please apply. Thanks. > > Ajit Khaparde (6): > bnxt_en: Add auxiliary driver support > RDMA/bnxt_re: Use auxiliary driver interface > bnxt_en: Remove usage of ulp_id > bnxt_en: Use direct API instead of indirection > bnxt_en: Use auxiliary bus calls over proprietary calls > RDMA/bnxt_re: Remove the sriov config callback > > Hongguang Gao (1): > bnxt_en: Remove struct bnxt access from RoCE driver > > drivers/infiniband/hw/bnxt_re/bnxt_re.h | 9 +- > drivers/infiniband/hw/bnxt_re/main.c | 591 +++++++----------- > drivers/net/ethernet/broadcom/bnxt/bnxt.c | 10 +- > drivers/net/ethernet/broadcom/bnxt/bnxt.h | 8 + > .../net/ethernet/broadcom/bnxt/bnxt_sriov.c | 7 +- > drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c | 413 ++++++------ > drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h | 53 +- > 7 files changed, 490 insertions(+), 601 deletions(-) > > -- > 2.37.1 (Apple Git-137.1) >
| |