On Mon, 2026-09-07 at 20:14 +0200, Danilo Krummrich wrote:
> On Sat Sep 5, 2026 at 10:53 AM CEST, Philipp Stanner wrote:
> > +impl<'a, T: Send + Sync + FenceContextOps> Drop for DriverFence<'a, T> {
> > + fn drop(&mut self) {
> > + let guard = self.as_fence().lock();
> > +
> > + // Use dma_fence_test_signaled_flag() instead of
> > + // dma_fence_is_signaled_locked() because the C backend wants to
> > get rid
> > + // of the latter.
> > +
> > + // SAFETY: `guard` is valid until the `call_rcu()` below.
> > + let signaled: bool = unsafe {
> > bindings::dma_fence_test_signaled_flag(guard.as_raw()) };
> > + if !signaled {
> > + pr_err!("DriverFence drops unsignaled. Danger of memory
> > corruption!\n");
>
> I'm not sure we want to keep this as pr_err!().
>
> If we really want to keep warning about this I'd either make this a WARN_ON()
> or
> dev_warn() (we can easily store a device reference in the fence context), such
> that it is at least clear who's the offender.
>
> My preference would be dev_warn(), as I don't think it is that bad of an error
> condition to begin with. It would be pretty odd to have a driver where a
> DriverFence
> drops while the corresponding GPU job is not dropped. And further it'd be
> pretty
> odd if dropping the GPU job would not imply that the GPU actually stopped
> processing the work associated with the job.
Yeah, it would be odd. I would not expect it to happen. But many things
have happened which I did not expect, so … :)
>
> For the same reason I also think it is a bit misleading to say "Danger of
> memory
> corruption!". It's not the signaling of the fence that does prevent memory
> corruption; it's the driver implementing a proper teardown sequence. And if
> this
> sequence is structurally detached from the lifetime of the DriverFence (and
> Job)
> structure, something is structurally wrong with the driver anyway.
Hypothetically, it could mean that there is a forgotten job still
running on the GPU, which might then access freed resources through
DMA. Sure, that would mean that the driver is fundamentally broken, but
that's what warnings like that are about.
Just to ellaborate on my thinking. I wouldn't insist on any wording.
>From my POV we can just say "… drops unsignaled.". That should be
enough.
>
> Furthermore, it would be very natural to just require the generic Job type to
> own a DriverFence. In this case it becomes natural to either signal the fence
> on
> Job completion, or just drop the Jobqueue, which does the ring teardown and
> subsequently drops all the Jobs, which would also imply signaling the
> DriverFence with ECANCELED. I.e. I think the fact that the DriverFence is
> signaled with ECANCELED if it is still unsignaled should just be an API
> contract
> and not an error condition.
Yup, with the JobQueue design I have in mind this error is largely
impossible, just as the forgetting of DriverFences is. Which is why I
think it's a good design idea to have the JQ own the jobs, isolating
the driver from direct fence interaction.
P.
