From owner-dev-commits-src-all@freebsd.org Wed Sep 29 23:26:30 2021 Return-Path: Delivered-To: dev-commits-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id D770F67DF42 for ; Wed, 29 Sep 2021 23:26:30 +0000 (UTC) (envelope-from mw@semihalf.com) Received: from mail-yb1-xb2f.google.com (mail-yb1-xb2f.google.com [IPv6:2607:f8b0:4864:20::b2f]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4HKXZG590pz3Hls for ; Wed, 29 Sep 2021 23:26:30 +0000 (UTC) (envelope-from mw@semihalf.com) Received: by mail-yb1-xb2f.google.com with SMTP id s64so6014928yba.11 for ; Wed, 29 Sep 2021 16:26:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf-com.20210112.gappssmtp.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=nMnD6dEREN8xivG89DdbcMwkrwTf9d5qac7IR2fILao=; b=5urjGPcH3LFXOv1k8YDh+/2RwDf+r4G/raJwfI4snin8bQlWOLn5FFsXvk8M2Kh3PH M5gl0M2SLbaDLVrFGrrECTtCK/qPXZOMcmmSewMC68IwFyBYW3WyBqOKJSSPDAEK7+Tz J0iREnHwE244zAAye0SEEkLarf2P1QWpsN32GB9m9+XQGO9YDNjzhapf8XrXzPA5WZBT ONAEV03aJ1M9EDZguXkgwZqk/xjRMoWeUnCWVdZPJF21XVr6+PhPraF7cL9FnyiVa+HE hnbMamzqqYhqaPLqPVzCwfHWqLR7NS8yRyLStIxhzMjjKQ8jqlJEPtRIAqZGO6jOka+z m2Aw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=nMnD6dEREN8xivG89DdbcMwkrwTf9d5qac7IR2fILao=; b=i93kBSV4mWIkAoCpNsSW/ZjF1Tu6O5y7D2GR0JvotViTTn3biLeHKTH0h0vEMavWei YLffarOAt8r438yyDdnPUTA3XWxDfVsp600+C5unTaP8pNPJkY3Y78j/sW1qHBIFYoCD v4mI5vEwjWBaGzqKn46HQWKx1jNpD/xo1iFnwFsrw5yvJHc6hcfB9wGqYFonxQF++t15 FRjo4zhw06z8hn5bbvXdGjRZc4AIxBWWwJTnt186YS/ttZJpIrtz/4P8MQuKcsPEZIS4 vYcUsPhpuDSGAvtEVgqVcG1uw2VVgdNVFdSjkI32GxEfk0hKvg7iGQgxMQSRwFP/jsrr ESHw== X-Gm-Message-State: AOAM530K7S+vawsGesVYcOcD2bPuLOzgqXQzre7g86y/P/DwRG1PnIu+ zxolIWXtdSSZMdcjYxxjtIbTxs/lRAlKOstTVzsvJOo/gd0= X-Google-Smtp-Source: ABdhPJxDdAuFHndukuTAua/J+zgdhgX3j2gSybNsRTEG5qiGAJnAomtwCf48MYcmxrsR7VPvIuJFl99j2wrW6empsHo= X-Received: by 2002:a05:6902:1021:: with SMTP id x1mr3310889ybt.544.1632957989991; Wed, 29 Sep 2021 16:26:29 -0700 (PDT) MIME-Version: 1.0 References: <202109281243.18SChldh001988@gitrepo.freebsd.org> <66731C91-BAE9-4D5D-B7DE-F1D7AA94395D@fubar.geek.nz> In-Reply-To: <66731C91-BAE9-4D5D-B7DE-F1D7AA94395D@fubar.geek.nz> From: Marcin Wojtas Date: Thu, 30 Sep 2021 01:26:20 +0200 Message-ID: Subject: Re: git: f3aa0098a82e - main - Use mtx_lock_spin in the gic driver To: Andrew Turner Cc: Andrew Turner , src-committers , dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 4HKXZG590pz3Hls X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] X-BeenThere: dev-commits-src-all@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for all branches of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 29 Sep 2021 23:26:30 -0000 =C5=9Br., 29 wrz 2021 o 17:42 Andrew Turner napisa= =C5=82(a): > > > > > On 28 Sep 2021, at 16:36, Marcin Wojtas wrote: > > > > Hi Andrew, > > > > wt., 28 wrz 2021 o 14:43 Andrew Turner napisa=C5= =82(a): > >> > >> The branch main has been updated by andrew: > >> > >> URL: https://cgit.FreeBSD.org/src/commit/?id=3Df3aa0098a82ebf7712aa137= 16d794aa7e4ac59cd > >> > >> commit f3aa0098a82ebf7712aa13716d794aa7e4ac59cd > >> Author: Andrew Turner > >> AuthorDate: 2021-09-28 11:36:42 +0000 > >> Commit: Andrew Turner > >> 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 wa= s > >> 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 =3D device_get_softc(dev); > >> > >> - mtx_lock(&sc->mutex); > >> + mtx_lock_spin(&sc->mutex); > >> > >> found =3D false; > >> for (irq =3D 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 =3D=3D 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 |=3D GI_FLAG_MSI_USED; > >> } > >> - mtx_unlock(&sc->mutex); > >> + mtx_unlock_spin(&sc->mutex); > >> > >> for (i =3D 0; i < count; i++) > >> srcs[i] =3D (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 =3D device_get_softc(dev); > >> > >> - mtx_lock(&sc->mutex); > >> + mtx_lock_spin(&sc->mutex); > >> for (i =3D 0; i < count; i++) { > >> gi =3D (struct gic_irqsrc *)isrc[i]; > >> > >> @@ -1128,7 +1128,7 @@ arm_gic_release_msi(device_t dev, device_t child= , int count, > >> > >> gi->gi_flags &=3D ~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 =3D device_get_softc(dev); > >> > >> - mtx_lock(&sc->mutex); > >> + mtx_lock_spin(&sc->mutex); > >> /* Find an unused interrupt */ > >> for (irq =3D sc->sc_spi_start; irq < sc->sc_spi_end; irq++) { > >> KASSERT((sc->gic_irqs[irq].gi_flags & GI_FLAG_MSI) !=3D= 0, > >> @@ -1152,13 +1152,13 @@ arm_gic_alloc_msix(device_t dev, device_t chil= d, device_t *pic, > >> } > >> /* No free interrupt was found */ > >> if (irq =3D=3D 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 |=3D GI_FLAG_MSI_USED; > >> - mtx_unlock(&sc->mutex); > >> + mtx_unlock_spin(&sc->mutex); > >> > >> *isrcp =3D (struct intr_irqsrc *)&sc->gic_irqs[irq]; > >> *pic =3D dev; > >> @@ -1178,9 +1178,9 @@ arm_gic_release_msix(device_t dev, device_t chil= d, struct intr_irqsrc *isrc) > >> KASSERT((gi->gi_flags & GI_FLAG_MSI_USED) =3D=3D GI_FLAG_MSI_US= ED, > >> ("%s: Trying to release an unused MSI-X interrupt", __func_= _)); > >> > >> - mtx_lock(&sc->mutex); > >> + mtx_lock_spin(&sc->mutex); > >> gi->gi_flags &=3D ~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 a= llows > > * us to reserve the registers when GIC_IVAR_MBI_COUNT i= s set. > > */ > > - MPASS(sc->sc_spi_start =3D=3D 0); > > - MPASS(sc->sc_spi_count =3D=3D 0); > > MPASS(value >=3D 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 !=3D 0); > > - MPASS(sc->sc_spi_count =3D=3D 0); > > > > sc->sc_spi_count =3D value; > > sc->sc_spi_end =3D sc->sc_spi_start + sc->sc_spi_count; > > > > Any thoughts? > > Can you try the patch in https://reviews.freebsd.org/D32224. The above ch= ange 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 ra= nges. > After applying this patch the MSI-X work fine and I can boot the OS without issue. Thanks, Marcin