From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from bali.collaboradmins.com (bali.collaboradmins.com [148.251.105.195]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id DD093233928 for ; Mon, 3 Aug 2026 15:59:51 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=148.251.105.195 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785772793; cv=none; b=rjZv0pYgjY2vJd/2g2SokCEwbCWh7+l6FL2fa7EOvLPXu4hc4x9NSWdi9z0EsoZiCt3W4d2RB09SMuwKS0Qqiv5ecx21zSBG0VZUTKWs8XcMYodCIAHX8cQJtr7tNoLW96tVSq7eBWpQHEG/7Jtfr1K+gNbXAZdWkgNm7NPLh7M= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785772793; c=relaxed/simple; bh=fZYG0O/tPcBi7Y0vzF0OznXX2JFW1TwNXlaYh02yVqw=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=F3teg9MDHTXA7bPlogwyZnWzNBvCg5kAUCY7zqngBruBcOJ45BfLI1jChQyZI1SDr2B+CLk8yekHq+RfdtMMySjWPu9Mpgmyy1XKaaflLPfGmC5YzOrhc+yjVP6gTn44sadyR2K+oZtxVhgwUU3xXW0URKsfizv4Er1R/kUMnSs= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=collabora.com; spf=pass smtp.mailfrom=collabora.com; dkim=pass (2048-bit key) header.d=collabora.com header.i=@collabora.com header.b=I7DzS0lZ; arc=none smtp.client-ip=148.251.105.195 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=collabora.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=collabora.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=collabora.com header.i=@collabora.com header.b="I7DzS0lZ" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1785772789; bh=fZYG0O/tPcBi7Y0vzF0OznXX2JFW1TwNXlaYh02yVqw=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=I7DzS0lZt9DBsHaiHS/k3A0R2tBi8v/xxrT/7vul+bnKQuUztP49SGxUyMePMROmx o1FJRcXyGM6jJ6U+2T909jEEpXgGF5Z9A/nY9UH3caCH2w15tk9J8pbCkVPERKbYt9 sWeBDzZK/rEoD8UsYSpHSkQq0mB+XJCFCeSd6zgpt1Y3VIT/NJGvoUQ0R6m4KrJcet r0Zxx0gDESNIQxuvyIqgrBM/SVDJiTiwYsNTUt3lQRsa+RqhJv2d/LYgxtLVYH4G5f zOwUXEyEFE2Pt52UGS4E2qmZIkFszaUQ0WXBQkRxE1PPQ1H/ZBv0Tl25/4/Pxv6QpK lTKmuERywJMlw== Received: from fedora1.home (unknown [100.64.0.11]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange secp256r1 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: bbrezillon) by bali.collaboradmins.com (Postfix) with ESMTPSA id CF2F817E025E; Mon, 03 Aug 2026 17:59:48 +0200 (CEST) Date: Mon, 3 Aug 2026 17:59:45 +0200 From: Boris Brezillon To: Nicolas Frattaroli Cc: Ingo Molnar , Peter Zijlstra , Juri Lelli , Vincent Guittot , Dietmar Eggemann , Steven Rostedt , Ben Segall , Mel Gorman , Valentin Schneider , K Prateek Nayak , Steven Price , Liviu Dudau , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie , Simona Vetter , Grant Likely , Heiko Stuebner , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, kernel@collabora.com Subject: Re: [PATCH v2 2/3] drm/panthor: Revisit reqs_lock handling in flush/reset paths Message-ID: <20260803175945.10c3c175@fedora1.home> In-Reply-To: References: <20260730-panthor-cache-flush-fix-v2-0-28790478bfff@collabora.com> <20260730-panthor-cache-flush-fix-v2-2-28790478bfff@collabora.com> <20260803105324.478ed594@fedora1.home> Organization: Collabora X-Mailer: Claws Mail 4.4.0 (GTK 3.24.52; x86_64-redhat-linux-gnu) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit On Mon, 03 Aug 2026 15:13:25 +0200 Nicolas Frattaroli wrote: > On Monday, 3 August 2026 10:53:24 Central European Summer Time Boris Brezillon wrote: > > Hello Nicolas, > > > > On Thu, 30 Jul 2026 13:45:15 +0200 > > Nicolas Frattaroli wrote: > > > > > panthor_gpu_flush_caches() and panthor_gpu_soft_reset() would read (and > > > even reset) the contents of the pending_reqs register outside of holding > > > the reqs_lock. > > > > Can you elaborate a bit on the race being fixed here? If pending_reqs bits > > are truly cleared before the wake_up_all() call (which would require a > > WRITE_ONCE() to be enforced, admittedly), there's no risk for the > > wait_event() call to do a test before the bits have been updated, > > and this holds even if the test is done without the lock held. > > > > The other race I could think of is two threads calling > > panthor_gpu_flush_caches() concurrently, and the second one stealing > > the FLUSH_COMPLETED event the first thread waits on and re-issuing a > > second flush on top, thus delaying the completion for the first thread. > > But that should be covered by the cache_flush_lock. > > panthor_gpu_flush_caches() is not the only thing that sets/gets > pending_reqs. Notably, the threaded interrupt handler does, as > well as any other functionality using the same member for reqs > tracking (e.g. the soft reset). > > Consider the following serialisation of events: > 1. T1 asks to flush caches by writing GPU_CMD and setting pending_reqs > 2. T1 drops reqs_lock. > 3. T2 enters IRQ handler for flush complete, spins lock waiting for > reqs_lock > 4. T1 sleeps at wait_event_timeout > 5. T2 updates pending_reqs and wakes up the waiter in any order, since > the effects of those two can't consistently be observed as sequential > logic without the outer reqs_lock being held by the observer > 6. T1 wakes up, checks pending_reqs, but since pending_reqs is checked > without holding any lock, so we implictly depend on the synchronisation > point that is the waitqueue's lock rather than the reqs_lock spinlock, > which says nothing about whether the pending_reqs change materialised > on T1's side yet as far as I can tell? > 7. T1 sees that pending_reqs & GPU_IRQ_CLEAN_CACHES_COMPLETED is still != 0, > so goes back to sleep for some future wake-up of reqs_acked or a timeout. > > I'm not 100% sure, but I think 6. means that the memory model would permit > T1 to re-use the pending_reqs it previously set, rather than the updated > one set by T2, since there's nothing stopping us from being woken up before > the pending_reqs change has made itself known to observers not serialising > with the reqs_lock being released by T2 in panthor_gpu_irq_handler after. If I read the "SLEEP AND WAKE-UP FUNCTIONS" section in [1] correctly, I think we're covered by the "general memory barrier" here. > > If you check lock_stat before the change, you see that the reqs_lock is > actually never contended. This isn't a good sign because it means > whatever situation it exists to protect against never occurs, so either > the lock is pointless or the lock is non-functional. Or we're just in a situation where there's almost never two threads updating the pending_reqs concurrently, which is not the same as saying this can't happen at all. > > > > Additionally, when it did hold the lock, it did so with > > > the irqsave/irqrestore variants, even though the spinlock was never > > > acquired in an atomic context, just the threaded handler. > > > > > > Use the new wait_event_lock_timeout() macro to check pending_reqs under > > > the lock, and only do so without disabling interrupts. > > > > > > Fixes: 5cd894e258c4 ("drm/panthor: Add the GPU logical block") > > > Signed-off-by: Nicolas Frattaroli > > > --- > > > drivers/gpu/drm/panthor/panthor_gpu.c | 25 +++++++++++-------------- > > > 1 file changed, 11 insertions(+), 14 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c b/drivers/gpu/drm/panthor/panthor_gpu.c > > > index c013d6bf9a59..f015bde80abf 100644 > > > --- a/drivers/gpu/drm/panthor/panthor_gpu.c > > > +++ b/drivers/gpu/drm/panthor/panthor_gpu.c > > > @@ -330,35 +330,34 @@ int panthor_gpu_flush_caches(struct panthor_device *ptdev, > > > u32 l2, u32 lsc, u32 other) > > > { > > > struct panthor_gpu *gpu = ptdev->gpu; > > > - unsigned long flags; > > > int ret = 0; > > > > > > /* Serialize cache flush operations. */ > > > guard(mutex)(&ptdev->gpu->cache_flush_lock); > > > > > > - spin_lock_irqsave(&ptdev->gpu->reqs_lock, flags); > > > + spin_lock(&ptdev->gpu->reqs_lock); > > > > Can we make the _irq{save,restore}-drop its own patch? > > I'm not sure it's fine to drop the IRQ disabling without fixing the read of > pending_reqs outside its lock. Can be done after fixing the race, if it's really making the race more likely. Although, as I said above, I'm not yet convinced the race we're chasing is the one we've been discussing here. > > > > > > if (!(ptdev->gpu->pending_reqs & GPU_IRQ_CLEAN_CACHES_COMPLETED)) { > > > ptdev->gpu->pending_reqs |= GPU_IRQ_CLEAN_CACHES_COMPLETED; > > > gpu_write(gpu->iomem, GPU_CMD, GPU_FLUSH_CACHES(l2, lsc, other)); > > > } else { > > > ret = -EIO; > > > } > > > - spin_unlock_irqrestore(&ptdev->gpu->reqs_lock, flags); > > > > > > - if (ret) > > > + if (ret) { > > > + spin_unlock(&ptdev->gpu->reqs_lock); > > > return ret; > > > + } > > > > > > - if (!wait_event_timeout(ptdev->gpu->reqs_acked, > > > + if (!wait_event_lock_timeout(ptdev->gpu->reqs_acked, > > > !(ptdev->gpu->pending_reqs & GPU_IRQ_CLEAN_CACHES_COMPLETED), > > > > Assuming we really need to do the test with the lock held, could we add > > a patch at the beginning of the series that fixes the race without depending > > on the new wait macro, so that we have a version that can easily be backported? > > It's either backporting the prerequisite new macro or still doing this all > with IRQs disabled using the pre-existing wait_event_lock_irq_timeout macro, > and the IRQ disabled thing is what caused problems. Can't we have the following helper function? static bool cache_flush_done(struct panthor_device *ptdev) { guard(spinlock_irqsave)(&ptdev->gpu->reqs_lock); return ptdev->gpu->pending_reqs & GPU_IRQ_CLEAN_CACHES_COMPLETED; } and then: if (!wait_event_timeout(ptdev->gpu->reqs_acked, cache_flush_done(ptdev), msecs_to_jiffies(100))) { .... [1]https://docs.kernel.org/core-api/wrappers/memory-barriers.html#