Re: [PATCH rdma-next v2 0/5] Get rid of custom made module dependency

[Leon Romanovsky <leon@kernel.org>]

On Sun, Apr 18, 2021 at 12:09:16AM +0530, Devesh Sharma wrote:
> On Sat, Apr 17, 2021 at 1:44 PM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Wed, Apr 14, 2021 at 07:15:37PM +0530, Devesh Sharma wrote:
> > > On Mon, Apr 12, 2021 at 1:10 PM Leon Romanovsky <leon@kernel.org> wrote:
> > > >
> > > > On Thu, Apr 08, 2021 at 08:42:57PM +0530, Devesh Sharma wrote:
> > > > > On Thu, Apr 8, 2021 at 5:14 PM Leon Romanovsky <leon@kernel.org> wrote:
> > > > > >
> > > > > > On Thu, Apr 08, 2021 at 05:06:24PM +0530, Devesh Sharma wrote:
> > > > > > > On Sat, Apr 3, 2021 at 5:12 PM Leon Romanovsky <leon@kernel.org> wrote:
> > > > > > > >
> > > > > > > > On Sat, Apr 03, 2021 at 03:52:13PM +0530, Devesh Sharma wrote:
> > > > > > > > > On Thu, Apr 1, 2021 at 12:27 PM Leon Romanovsky <leon@kernel.org> wrote:
> > > > > > > > > >
> > > > > > > > > > From: Leon Romanovsky <leonro@nvidia.com>
> > > > > > > > > >
> > > > > > > > > > Changelog:
> > > > > > > > > > v2:
> > > > > > > > > >  * kbuild spotted that I didn't delete all code in patch #5, so deleted
> > > > > > > > > >    even more ulp_ops derefences.
> > > > > > > > > > v1: https://lore.kernel.org/linux-rdma/20210329085212.257771-1-leon@kernel.org
> > > > > > > > > >  * Go much deeper and removed useless ULP indirection
> > > > > > > > > > v0: https://lore.kernel.org/linux-rdma/20210324142524.1135319-1-leon@kernel.org
> > > > > > > > > > -----------------------------------------------------------------------
> > > > > > > > > >
> > > > > > > > > > The following series fixes issue spotted in [1], where bnxt_re driver
> > > > > > > > > > messed with module reference counting in order to implement symbol
> > > > > > > > > > dependency of bnxt_re and bnxt modules. All of this is done, when in
> > > > > > > > > > upstream we have only one ULP user of that bnxt module. The simple
> > > > > > > > > > declaration of exported symbol would do the trick.
> > > > > > > > > >
> > > > > > > > > > This series removes that custom module_get/_put, which is not supposed
> > > > > > > > > > to be in the driver from the beginning and get rid of nasty indirection
> > > > > > > > > > logic that isn't relevant for the upstream code.
> > > > > > > > > >
> > > > > > > > > > Such small changes allow us to simplify the bnxt code and my hope that
> > > > > > > > > > Devesh will continue where I stopped and remove struct bnxt_ulp_ops too.
> > > > > > > > > >
> > > > > > > > > > Thanks
> > > > > > > > > >
> > > > > > > > > > [1] https://lore.kernel.org/linux-rdma/20210324142524.1135319-1-leon@kernel.org
> > > > > > > > > >
> > > > > > > > > > Leon Romanovsky (5):
> > > > > > > > > >   RDMA/bnxt_re: Depend on bnxt ethernet driver and not blindly select it
> > > > > > > > > >   RDMA/bnxt_re: Create direct symbolic link between bnxt modules
> > > > > > > > > >   RDMA/bnxt_re: Get rid of custom module reference counting
> > > > > > > > > >   net/bnxt: Remove useless check of non-existent ULP id
> > > > > > > > > >   net/bnxt: Use direct API instead of useless indirection
> > > > > > > > > >
> > > > > > > > > >  drivers/infiniband/hw/bnxt_re/Kconfig         |   4 +-
> > > > > > > > > >  drivers/infiniband/hw/bnxt_re/main.c          |  93 ++-----
> > > > > > > > > >  drivers/net/ethernet/broadcom/bnxt/bnxt.c     |   4 +-
> > > > > > > > > >  drivers/net/ethernet/broadcom/bnxt/bnxt.h     |   1 -
> > > > > > > > > >  drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.c | 245 +++++++-----------
> > > > > > > > > >  drivers/net/ethernet/broadcom/bnxt/bnxt_ulp.h |  32 +--
> > > > > > > > > >  6 files changed, 119 insertions(+), 260 deletions(-)
> > > > > > > > >
> > > > > > > > > Hi Leon,
> > > > > > > > >
> > > > > > > > > After a couple of internal discussions we reached a conclusion to
> > > > > > > > > implement the Auxbus driver interface and fix the problem once and for
> > > > > > > > > all.
> > > > > > > >
> > > > > > > > Thanks Devesh,
> > > > > > > >
> > > > > > > > Jason, it looks like we can proceed with this patchset, because in
> > > > > > > > auxbus mode this module refcount and ULP indirection logics will be
> > > > > > > > removed anyway.
> > > > > > > >
> > > > > > > > Thanks
> > > > > > > Hi Leon,
> > > > > > >
> > > > > > > In my internal testing, I am seeing a crash using the 3rd patch. I am
> > > > > > > spending a few cycles on debugging it. expect my input in a day or so.
> > > > > >
> > > > > > Can you please post the kernel crash report here?
> > > > > > I don't see how function rename in patch #3 can cause to the crash.
> > > > > Hey, unfortunately my kdump service config is giving me tough time on
> > > > > my host. I will share if I get it.
> > > >
> > > > Any news here?
> > > Expect something by this Friday. yesterday was a holiday in India.
> >
> > Any update?
> > This series is close to three weeks already and I would like to progress with it.
> Hi Leon,
> 
> The host crash I indicated earlier is actually caused by patch 4 and
> not by patch 3 from this series. I spent time to root cause the
> problem and realized that patch-4 is touching quite many areas which
> would require much intrusive testing and validation.
> As I indicated earlier, we are implementing the PCI Aux driver
> interface at a faster pace. While PCI Aux changes are in progress we
> are willing to retain the existing bnxt_re and bnxt_en interface
> untouched.
> The problem of module referencing would be rectified with PCI aux
> change by inheritance.

Sorry no, the first three patches are not controversial and better to be
applied now. They do the right thing and they are correct.

There is a little trust in your promises above after you didn't show us kernel
panic despite our numerous requests. I also very sceptical in Broadcom ability
to provide auxbus implementation in timely manner.

It is worth to mention that auxbus won't eliminate the patches #4 and #5, but
will embed them into your auxbus conversion.

Jason, please take first three patches so internal HW IB driver won't do the crazy
module management that is totally out of scope for drivers/infiniband and not needed.

Thanks

> >
> > Thanks
> 
> 
> 
> 
> --
> -Regards
> Devesh