Precedence: bulk
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date: Thu, 18 Dec 2025 10:40:05 +0100
Message-Id: <DF18RFX3IHVP.3GYNJIYAFFJU6@kernel.org>
Subject: Re: [PATCH v3 2/9] rust: debugfs: add lookup contructor
Cc: "Gary Guo" <gary@garyguo.net>, <rust-for-linux@vger.kernel.org>, "John
 Hubbard" <jhubbard@nvidia.com>, "Joel Fernandes" <joelagnelf@nvidia.com>,
 "Alexandre Courbot" <acourbot@nvidia.com>, "Lyude Paul" <lyude@redhat.com>,
 <nouveau@lists.freedesktop.org>, "Alice Ryhl" <aliceryhl@google.com>, "Greg
 Kroah-Hartman" <gregkh@linuxfoundation.org>, "Matthew Maurer"
 <mmaurer@google.com>
To: "Timur Tabi" <ttabi@nvidia.com>
From: "Danilo Krummrich" <dakr@kernel.org>
References: <20251218013910.459045-1-ttabi@nvidia.com>
 <20251218013910.459045-3-ttabi@nvidia.com>
In-Reply-To: <20251218013910.459045-3-ttabi@nvidia.com>

On Thu Dec 18, 2025 at 2:39 AM CET, Timur Tabi wrote:
> Add a new constructor for Dir that uses the debugfs_lookup() API to
> obtain a reference to an existing debugfs directory entry.
>
> The key difference from Dir::new() and Dir::subdir() is the cleanup
> semantics: when a Dir obtained via lookup() is dropped, it calls
> dput() to release the reference rather than debugfs_remove() which
> would delete the directory.
>
> To implement this cleanup distinction, the Entry class now includes
> an is_lookup boolean that specifies how the entry was created and
> therefore how it should be dropped.

I have two main comments about this.

(1) I think it would be better to wrap "lookup entries" into a new type in =
order
to account for the fact that they do not behave like regular entries. I.e. =
when
a "lookup entry" is dropped, it does not cause the entry to disappear from =
the
file system. Analogously, when the regular entry is dropped it disappears f=
rom
the file system regardless of whether a "lookup entry" still has a referenc=
e
count.

(2) The commit message lacks the motiviation for adding this functionality,
which is only for the purpose of a workaround of an unrelated limitation wi=
th
accessing the fields of a module structure (which Gary already works on a
solution for).

I understand it might take a while until the solution Gary works on is read=
y and
I'm perfectly fine with a temporary workaround. But, this workaround has to=
 be
on nova-core only and not introduce a feature in core code that in the near
future becomes dead code.

If we have a use-case for lookup() elsewhere, I'd like to see it first. But=
 if
we don't I prefer not to add this feature as a pure workaround for somethin=
g
else.

In fact, I think that debugfs_lookup() rarely is the correct solution and m=
ore
often indicates some kind of (design) issue, like in this case.

- Danilo