linux-kernel - Re: [PATCH v2] rust: kernel: device: Add devm_of_platform

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2025042945-aviator-subzero-0263@gregkh>
Date: Tue, 29 Apr 2025 16:57:29 +0200
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Danilo Krummrich <dakr@...nel.org>
Cc: Ayush Singh <ayush@...gleboard.org>,
	Jason Kridner <jkridner@...gleboard.org>,
	Deepak Khatri <lorforlinux@...gleboard.org>,
	Robert Nelson <robertcnelson@...gleboard.org>,
	Dhruva Gole <d-gole@...com>, Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Benno Lossin <benno.lossin@...ton.me>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] rust: kernel: device: Add
 devm_of_platform_populate/depopulate

On Tue, Apr 29, 2025 at 04:44:54PM +0200, Danilo Krummrich wrote:
> On Tue, Apr 29, 2025 at 04:37:49PM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Apr 29, 2025 at 04:31:52PM +0200, Danilo Krummrich wrote:
> > > On Tue, Apr 29, 2025 at 05:09:26PM +0530, Ayush Singh wrote:
> > > > +    /// Remove devices populated from device tree
> > > > +    pub fn devm_of_platform_depopulate(&self) {
> > > > +        // SAFETY: self is valid bound Device reference
> > > > +        unsafe { bindings::devm_of_platform_depopulate(self.as_raw()) }
> > > > +    }
> > > > +}
> > > 
> > > One additional question regarding devm_of_platform_depopulate(). This function
> > > is only used once throughout the whole kernel (in [1]), and at a first glance
> > > the usage there seems unnecessary.
> > > 
> > > In your upcoming driver you call devm_of_platform_depopulate() from a fallible
> > > path [2].
> > > 
> > > So, I think we should change devm_of_platform_depopulate() to return an error
> > > instead of WARN(ret).
> > > 
> > > If [1] needs it for some subtle reason I don't see, then I think we can still
> > > call it from there as
> > > 
> > > 	WARN(devm_of_platform_depopulate())
> > > 
> > > [1] https://elixir.bootlin.com/linux/v6.15-rc4/source/drivers/soc/ti/pruss.c#L558
> > > [2] https://github.com/Ayush1325/linux/commit/cdb1322b7166532445c54b601ad0a252866e574d#diff-7b9e3179e36732d5f3a681034d70c2fda4ff57745c79ad4a656f328c91e54b77R71
> > 
> > Ugh, no, we should just delete this function entirely if only one driver
> > is using it.  That implies it's not really needed at all.
> 
> Ayush's driver calls {de}populate() from a sysfs store path [2]; not sure what
> it's doing semantically or if this is a valid use-case though.

That's going to be rough, and full of tricky corner-cases and probably
shouldn't be doing that at all :)

So let's hold off on this entirely until we see a real user that can
actually pass review.  Trying to do system configuration like this in
sysfs is a much larger discussion than just adding rust bindings.

(hint, configfs is for system configuration, not sysfs...)

Anyway, worst case, you just "open code" the single function call that
this one binding was trying to "wrap".  which is what I think the
in-kernel user should be doing now.

thanks,

greg k-h