Re: git: 1c45a62a2f66 - main - qlnxe: Fix multiple locking issues

On Tue, Jun 4, 2024 at 4:02 PM John Baldwin <jhb@freebsd.org> wrote:
>
> On 5/28/24 2:43 AM, Kevin Bowling wrote:
> > The branch main has been updated by kbowling:
> >
> > URL: https://cgit.FreeBSD.org/src/commit/?id=1c45a62a2f667b45ec10a92ad58ff5a34e68b569
> >
> > commit 1c45a62a2f667b45ec10a92ad58ff5a34e68b569
> > Author:     Keith Reynolds <keith.reynolds@hpe.com>
> > AuthorDate: 2024-05-28 06:41:05 +0000
> > Commit:     Kevin Bowling <kbowling@FreeBSD.org>
> > CommitDate: 2024-05-28 06:41:05 +0000
> >
> >      qlnxe: Fix multiple locking issues
> >
> >      Multiple issues are reported with WITNESS and code inspection of the
> >      locking and lock initialization.
> >
> >      PR:             278084
> >      MFC after:      1 week
> > ---
> >   sys/dev/qlnx/qlnxe/bcm_osal.h  |  8 +++----
> >   sys/dev/qlnx/qlnxe/ecore.h     |  1 +
> >   sys/dev/qlnx/qlnxe/ecore_mcp.c | 48 +++++++++++++++++++++---------------------
> >   sys/dev/qlnx/qlnxe/ecore_mcp.h |  6 +++---
> >   sys/dev/qlnx/qlnxe/qlnx_def.h  |  2 +-
> >   sys/dev/qlnx/qlnxe/qlnx_os.c   |  9 ++++----
> >   sys/dev/qlnx/qlnxe/qlnx_os.h   |  4 ++--
> >   7 files changed, 40 insertions(+), 38 deletions(-)
> >
> > diff --git a/sys/dev/qlnx/qlnxe/bcm_osal.h b/sys/dev/qlnx/qlnxe/bcm_osal.h
> > index 5d940d3272d6..c820532c9e0a 100644
> > --- a/sys/dev/qlnx/qlnxe/bcm_osal.h
> > +++ b/sys/dev/qlnx/qlnxe/bcm_osal.h
> > @@ -72,7 +72,7 @@ extern void qlnx_dma_free_coherent(void *ecore_dev, void *v_addr,
> >                           bus_addr_t phys, uint32_t size);
> >
> >   extern void qlnx_link_update(void *p_hwfn);
> > -extern void qlnx_barrier(void *p_hwfn);
> > +extern void qlnx_barrier(void *p_dev);
> >
> >   extern void *qlnx_zalloc(uint32_t size);
> >
> > @@ -213,14 +213,14 @@ typedef struct osal_list_t
> >   #define OSAL_SPIN_LOCK_ALLOC(p_hwfn, mutex)
> >   #define OSAL_SPIN_LOCK_DEALLOC(mutex) mtx_destroy(mutex)
> >   #define OSAL_SPIN_LOCK_INIT(lock) {\
> > -             mtx_init(lock, __func__, MTX_NETWORK_LOCK, MTX_SPIN); \
> > +             mtx_init(lock, __func__, "OSAL spin lock", MTX_SPIN); \
> >       }
> >
>
> Do you really need MTX_SPIN here?  Device drivers rarely need spin locks.
> The equivalent to a Linux spin lock in drivers is generally a MTX_DEF
> mutex.

Can you include the author for follow ups?  This driver was a bit of a
disaster, the patch in question doesn't rectify that overall but does
fix the most serious PRs which is worthwhile.  The spin in question
seems to match what the vendor initially did in their OS API, Keith
may be able to test your suggestion.

> --
> John Baldwin
>