Re: [PATCH v5 0/7] Add Auxiliary driver support

From: Leon Romanovsky <leon@kernel.org>
To: Ajit Khaparde <ajit.khaparde@broadcom.com>
Cc: andrew.gospodarek@broadcom.com, davem@davemloft.net,
	edumazet@google.com, jgg@ziepe.ca, kuba@kernel.org,
	linux-kernel@vger.kernel.org, linux-rdma@vger.kernel.org,
	michael.chan@broadcom.com, netdev@vger.kernel.org,
	pabeni@redhat.com, selvin.xavier@broadcom.com
Subject: Re: [PATCH v5 0/7] Add Auxiliary driver support
Date: Thu, 8 Dec 2022 12:28:19 +0200	[thread overview]
Message-ID: <Y5G8Qzly9F3fP0Em@unreal> (raw)
In-Reply-To: 20221207175310.23656-1-ajit.khaparde@broadcom.com

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)
> 