Re: [rdma-next 5/5] RDMA/bnxt_re: Add new function to setup NQs

[Leon Romanovsky <leon@kernel.org>]

On Tue, Nov 12, 2024 at 02:55:12PM +0530, Selvin Xavier wrote:
> +Michael Chan
> 
> On Tue, Nov 12, 2024 at 1:47 PM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Fri, Nov 08, 2024 at 12:42:39AM -0800, Selvin Xavier wrote:
> > > From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> > >
> > > Move the logic to setup and enable NQs to a new function.
> > > Similarly moved the NQ cleanup logic to a common function.
> > > Introdued a flag to keep track of NQ allocation status
> > > and added sanity checks inside bnxt_re_stop_irq() and
> > > bnxt_re_start_irq() to avoid possible race conditions.
> > >
> > > Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> > > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
> > > ---
> > >  drivers/infiniband/hw/bnxt_re/bnxt_re.h |   2 +
> > >  drivers/infiniband/hw/bnxt_re/main.c    | 204 +++++++++++++++++++-------------
> > >  2 files changed, 123 insertions(+), 83 deletions(-)
> >
> > <...>
> >
> > >
> > > +     rtnl_lock();
> > > +     if (test_and_clear_bit(BNXT_RE_FLAG_SETUP_NQ, &rdev->flags))
> > > +             bnxt_re_clean_nqs(rdev);
> > > +     rtnl_unlock();
> >
> > <...>
> >
> > > +             rtnl_lock();
> > >               bnxt_qplib_free_ctx(&rdev->qplib_res, &rdev->qplib_ctx);
> > >               bnxt_qplib_disable_rcfw_channel(&rdev->rcfw);
> > >               type = bnxt_qplib_get_ring_type(rdev->chip_ctx);
> > >               bnxt_re_net_ring_free(rdev, rdev->rcfw.creq.ring_id, type);
> > > +             rtnl_unlock();
> >
> > Please don't add rtnl_lock() to drivers in RDMA subsystem. BNXT driver
> > is managed through netdev and it is there all proper locking should be
> > done.
> The main reason for bnxt_re to take the rtnl is because of the MSIx
> resource configuration.
> This is because the NIC driver is dynamically modifying the MSIx table
> when the number
> of ring change  or ndo->open/close is invoked. So we stop and restart
> the interrupts of RoCE also with rtnl held.

rtnl_lock is a big kernel lock, which blocks almost everything in the netdev.
In your case, you are changing one device configuration and should use
your per-device locks. Even in the system with more than one BNXT device,
the MSI-X on one device will influence other "innocent" devices.

> >
> > Please work to remove existing rtnl_lock() from bnxt_re_update_en_info_rdev() too.
> > IMHO that lock is not needed after your driver conversion to auxbus.
> This check is also to synchronize between the irq_stop and restart
> implementation between
> bnxt_en and bnxt_re driver and roce driver unload.
> 
>  We will review this locking and see if we can handle it. But it is a
> major design change in both L2
> and roce drivers.

You are adding new rtnl_lock and not moving it from one place to
another, so this redesign should be done together with this new
feature.

Thanks

> >
> > Thanks