Re: [PATCH v2 2/9] rust: debugfs: add SeqShow trait and seq_file file operations

[Alvin Sun <alvin.sun@linux.dev>]

On 7/31/26 21:08, Alice Ryhl wrote:
> On Fri, Jul 31, 2026 at 01:05:40AM +0800, Alvin Sun wrote:
>> +/// Show callback for `SeqShow` types.
>> +///
>> +/// # Safety
>> +///
>> +/// The seq_file core guarantees that `seq` points to a live `seq_file` and that
>> +/// `seq->private` is a valid pointer to a `T` with no outstanding mutable
>> +/// references.
>> +unsafe extern "C" fn seq_file_show<S: SeqShow<T>, T: Sync>(
>> +    seq: *mut bindings::seq_file,
>> +    _: *mut crate::ffi::c_void,
>> +) -> crate::ffi::c_int {
>> +    // SAFETY: `seq->private` is a valid `T` pointer with no outstanding
>> +    // mutable references.
>> +    let data = unsafe { &*((*seq).private.cast::<T>()) };
>> +    // SAFETY: `seq` points to a live `seq_file`.
>> +    let m = unsafe { SeqFile::from_raw(seq) };
>> +    from_result(|| S::show(data, m).map(|()| 0))
>> +}
>> +
>> +/// Renders `data` into a seq_file.
>> +///
>> +/// `data` is the value stashed as the debugfs file's `i_private` at creation
>> +/// time. `show` is invoked on each read to produce the file's contents.
>> +///
>> +/// `open` and `release` are optional lifecycle hooks called during file open
>> +/// and release. They can be used to manage the lifetime of `data` (e.g.,
>> +/// reference counting). Default implementations are no-ops.
>> +pub trait SeqShow<T> {
>> +    /// Writes debugfs output for the file.
>> +    fn show(data: &T, m: &SeqFile) -> Result;
>> +
>> +    /// Called during file open, before `single_open`.
>> +    fn open(_data: &T) -> Result {
>> +        Ok(())
>> +    }
>> +
>> +    /// Called during file release, before `single_release`.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// `data` must point to valid memory, kept alive by actions taken in
>> +    /// [`Self::open`] (e.g., incrementing a reference count).
>> +    unsafe fn release(_data: NonNull<T>) {}
>> +}
> Hmm. So in a later patch you implement SeqShow in the following manner:
>
>      fn open(dev: &drm::Device<T, drm::Normal>) -> Result {
>          // Hold a device reference so the device is not freed while the file is open.
>          dev.inc_ref();
>          Ok(())
>      }
>
>      unsafe fn release(dev: NonNull<drm::Device<T, drm::Normal>>) {
>          // Drop the reference taken in `open`.
>          // SAFETY: `dev` is valid per this function's safety contract.
>          unsafe { drm::Device::<T>::dec_ref(dev) };
>      }
>
> I don't understand why this increment/decrement pair is necessary.
> Just based on the code, it *looks* like you have them because otherwise
> `show()` might get called with a dangling pointer to the drm device, but
> the inc_ref() in open() ensures it stays alive.
>
> However, earlier in this patch you said:
>
>> The seq_file core guarantees that `seq` points to a live `seq_file` and
>> that `seq->private` is a valid pointer to a `T` with no outstanding
>> mutable references.
> So based on this, you are saying that open()/release() do not need to do
> anything special for the value to stay alive.
>
> These seem contradictory. Could you clarify the situation?

You're right. The `SeqShow` trait documentation was inaccurate -- the
framework does not manage the lifetime of `data`, so `inc_ref`/`dec_ref`
in `DrmSeqShow` is necessary to prevent `show()` from accessing a freed
device.

Will fix the doc in the next revision.

Best regards,
Alvin

>
> Alice