Date: Wed, 29 Sep 2021 16:42:24 +0100 From: Andrew Turner <andrew@fubar.geek.nz> To: Marcin Wojtas <mw@semihalf.com> Cc: Andrew Turner <andrew@freebsd.org>, src-committers <src-committers@freebsd.org>, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: f3aa0098a82e - main - Use mtx_lock_spin in the gic driver Message-ID: <66731C91-BAE9-4D5D-B7DE-F1D7AA94395D@fubar.geek.nz> In-Reply-To: <CAPv3WKc_8%2B14=Vccv542=BYgpeL1a8xb_6dseiH=bHH8Zz52bA@mail.gmail.com> References: <202109281243.18SChldh001988@gitrepo.freebsd.org> <CAPv3WKc_8%2B14=Vccv542=BYgpeL1a8xb_6dseiH=bHH8Zz52bA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
> On 28 Sep 2021, at 16:36, Marcin Wojtas <mw@semihalf.com> wrote: > > Hi Andrew, > > wt., 28 wrz 2021 o 14:43 Andrew Turner <andrew@freebsd.org> napisał(a): >> >> The branch main has been updated by andrew: >> >> URL: https://cgit.FreeBSD.org/src/commit/?id=f3aa0098a82ebf7712aa13716d794aa7e4ac59cd >> >> commit f3aa0098a82ebf7712aa13716d794aa7e4ac59cd >> Author: Andrew Turner <andrew@FreeBSD.org> >> AuthorDate: 2021-09-28 11:36:42 +0000 >> Commit: Andrew Turner <andrew@FreeBSD.org> >> CommitDate: 2021-09-28 11:42:06 +0000 >> >> Use mtx_lock_spin in the gic driver >> >> The mutex was changed to a spin lock when the MSI/MSI-X handling was >> moved from the gicv2m to the gic driver. Update the calls to lock >> and unlock the mutex to the spin variant. >> >> Submitted by: jrtc27 ("Change all the mtx_(un)lock(&sc->mutex) to be the _spin versions.") >> Reported by: mw, antranigv@freebsd.am >> Sponsored by: The FreeBSD Foundation >> --- >> sys/arm/arm/gic.c | 20 ++++++++++---------- >> 1 file changed, 10 insertions(+), 10 deletions(-) >> >> diff --git a/sys/arm/arm/gic.c b/sys/arm/arm/gic.c >> index d7edd7885404..89db4e324600 100644 >> --- a/sys/arm/arm/gic.c >> +++ b/sys/arm/arm/gic.c >> @@ -1056,7 +1056,7 @@ arm_gic_alloc_msi(device_t dev, device_t child, int count, int maxcount, >> >> sc = device_get_softc(dev); >> >> - mtx_lock(&sc->mutex); >> + mtx_lock_spin(&sc->mutex); >> >> found = false; >> for (irq = sc->sc_spi_start; irq < sc->sc_spi_end; irq++) { >> @@ -1091,7 +1091,7 @@ arm_gic_alloc_msi(device_t dev, device_t child, int count, int maxcount, >> >> /* Not enough interrupts were found */ >> if (!found || irq == sc->sc_spi_end) { >> - mtx_unlock(&sc->mutex); >> + mtx_unlock_spin(&sc->mutex); >> return (ENXIO); >> } >> >> @@ -1099,7 +1099,7 @@ arm_gic_alloc_msi(device_t dev, device_t child, int count, int maxcount, >> /* Mark the interrupt as used */ >> sc->gic_irqs[irq + i].gi_flags |= GI_FLAG_MSI_USED; >> } >> - mtx_unlock(&sc->mutex); >> + mtx_unlock_spin(&sc->mutex); >> >> for (i = 0; i < count; i++) >> srcs[i] = (struct intr_irqsrc *)&sc->gic_irqs[irq + i]; >> @@ -1118,7 +1118,7 @@ arm_gic_release_msi(device_t dev, device_t child, int count, >> >> sc = device_get_softc(dev); >> >> - mtx_lock(&sc->mutex); >> + mtx_lock_spin(&sc->mutex); >> for (i = 0; i < count; i++) { >> gi = (struct gic_irqsrc *)isrc[i]; >> >> @@ -1128,7 +1128,7 @@ arm_gic_release_msi(device_t dev, device_t child, int count, >> >> gi->gi_flags &= ~GI_FLAG_MSI_USED; >> } >> - mtx_unlock(&sc->mutex); >> + mtx_unlock_spin(&sc->mutex); >> >> return (0); >> } >> @@ -1142,7 +1142,7 @@ arm_gic_alloc_msix(device_t dev, device_t child, device_t *pic, >> >> sc = device_get_softc(dev); >> >> - mtx_lock(&sc->mutex); >> + mtx_lock_spin(&sc->mutex); >> /* Find an unused interrupt */ >> for (irq = sc->sc_spi_start; irq < sc->sc_spi_end; irq++) { >> KASSERT((sc->gic_irqs[irq].gi_flags & GI_FLAG_MSI) != 0, >> @@ -1152,13 +1152,13 @@ arm_gic_alloc_msix(device_t dev, device_t child, device_t *pic, >> } >> /* No free interrupt was found */ >> if (irq == sc->sc_spi_end) { >> - mtx_unlock(&sc->mutex); >> + mtx_unlock_spin(&sc->mutex); >> return (ENXIO); >> } >> >> /* Mark the interrupt as used */ >> sc->gic_irqs[irq].gi_flags |= GI_FLAG_MSI_USED; >> - mtx_unlock(&sc->mutex); >> + mtx_unlock_spin(&sc->mutex); >> >> *isrcp = (struct intr_irqsrc *)&sc->gic_irqs[irq]; >> *pic = dev; >> @@ -1178,9 +1178,9 @@ arm_gic_release_msix(device_t dev, device_t child, struct intr_irqsrc *isrc) >> KASSERT((gi->gi_flags & GI_FLAG_MSI_USED) == GI_FLAG_MSI_USED, >> ("%s: Trying to release an unused MSI-X interrupt", __func__)); >> >> - mtx_lock(&sc->mutex); >> + mtx_lock_spin(&sc->mutex); >> gi->gi_flags &= ~GI_FLAG_MSI_USED; >> - mtx_unlock(&sc->mutex); >> + mtx_unlock_spin(&sc->mutex); >> >> return (0); >> } > > Thank you for the patch, it fixes the MSI-X in my setup, but in order > to operate properly on Marvell SoCs (equipped with a standard GICv2m), > I have to remove the asserts, which were added in c6f3076d4405 (Move > the GICv2m msi handling to the parent): > > diff --git a/sys/arm/arm/gic.c b/sys/arm/arm/gic.c > index 89db4e324600..b5d72a2a6c49 100644 > --- a/sys/arm/arm/gic.c > +++ b/sys/arm/arm/gic.c > @@ -528,8 +528,6 @@ arm_gic_write_ivar(device_t dev, device_t child, > int which, uintptr_t value) > * GIC_IVAR_MBI_START must be set once and first. This allows > * us to reserve the registers when GIC_IVAR_MBI_COUNT is set. > */ > - MPASS(sc->sc_spi_start == 0); > - MPASS(sc->sc_spi_count == 0); > MPASS(value >= GIC_FIRST_SPI); > MPASS(value < sc->nirqs); > > @@ -537,7 +535,6 @@ arm_gic_write_ivar(device_t dev, device_t child, > int which, uintptr_t value) > return (0); > case GIC_IVAR_MBI_COUNT: > MPASS(sc->sc_spi_start != 0); > - MPASS(sc->sc_spi_count == 0); > > sc->sc_spi_count = value; > sc->sc_spi_end = sc->sc_spi_start + sc->sc_spi_count; > > Any thoughts? Can you try the patch in https://reviews.freebsd.org/D32224. The above change is to check there is only a single mbi range, however it appears your hardware has multiple gicv2m devices so will try to reserve multiple mbi ranges. Andrew
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?66731C91-BAE9-4D5D-B7DE-F1D7AA94395D>