On Mon Jun 1, 2026 at 10:46 AM CEST, Philipp Stanner wrote:
> On Sat, 2026-05-30 at 17:16 +0200, Danilo Krummrich wrote:
>> (Not a full review, but a few drive-by comments.)
>>
>> On Sat May 30, 2026 at 4:35 PM CEST, Philipp Stanner wrote:
>> > +#[allow(unused_unsafe)]
>>
>> What is this needed for?
>
> You know that :-P
I don't, it's a serious question.
>> > + // SAFETY: Once a `DriverFence` is initialized, the inner `fence`
>> > is
>> > + // valid and initialized. It is valid until the refcount drops
>> > + // to 0, which can earliest happen once the `DriverFence` has
>> > been dropped.
>> > + unsafe {
>> > + bindings::dma_fence_lock_irqsave(fence, flag_ptr);
>> > + if !bindings::dma_fence_is_signaled_locked(fence) {
>> > + if let Err(err) = res {
>> > + bindings::dma_fence_set_error(fence, err.to_errno());
>> > + }
>> > + bindings::dma_fence_signal_locked(fence);
>> > + }
>> > + bindings::dma_fence_unlock_irqrestore(fence, flag_ptr);
>> > + }
>>
>> Please use a single unsafe block per unsafe function call, here and in a few
>> other places.
>
> Is that an official rule? If so, the linters should inform about it.
>
> At first glance, I don't see any advantage to it and the disadvantage
> of greatly reducing readability.
The advantage is that it separates the safety justifications per unsafe call,
which increases the chances of catching a bug, makes it easier for the reader to
match requirements against justifications and potentially allows tooling to
perform checks of the justification against the requirement.
For the specific case above, there's no documented requirements in the sense of
Rust safety requirements of course, as they are all FFI calls, but
dma_fence_signal_locked() for instance has the obvious requirement that it must
only be called with the fence lock held and dma_fence_set_error() must only be
called when the fence is actually signaled.
Besides that, since this pattern seems to occur at least twice, you could also
consider adding a lock guard.
