Received-SPF: pass (google.com: domain of linux-kernel+bounces-211820-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Wed, 12 Jun 2024 17:50:42 +0200
From: Greg KH <gregkh@linuxfoundation.org>
To: Danilo Krummrich <dakr@redhat.com>
Cc: Boqun Feng <boqun.feng@gmail.com>, rafael@kernel.org, mcgrof@kernel.org,
	russell.h.weight@intel.com, ojeda@kernel.org, alex.gaynor@gmail.com,
	wedsonaf@gmail.com, gary@garyguo.net, bjorn3_gh@protonmail.com,
	benno.lossin@proton.me, a.hindborg@samsung.com,
	aliceryhl@google.com, airlied@gmail.com, fujita.tomonori@gmail.com,
	pstanner@redhat.com, ajanulgu@redhat.com, lyude@redhat.com,
	rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/2] rust: add abstraction for struct device
Message-ID: <2024061214-dusk-channel-e124@gregkh>
References: <20240610180318.72152-1-dakr@redhat.com>
 <20240610180318.72152-2-dakr@redhat.com>
 <ZmdID8AlXtoxUfC1@boqun-archlinux>
 <ZmhPW9yq7y6jbmIg@pollux>
 <2024061136-unbridle-confirm-c653@gregkh>
 <Zmh3oN9sWamaYHOD@Boquns-Mac-mini.home>
 <d74edb73-1dba-43f4-a50c-36354c39d758@redhat.com>
 <2024061245-kangaroo-clothes-76e1@gregkh>
 <ZmnAOfCUFkZqhDji@pollux>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZmnAOfCUFkZqhDji@pollux>

On Wed, Jun 12, 2024 at 05:35:21PM +0200, Danilo Krummrich wrote:
> On Wed, Jun 12, 2024 at 05:02:52PM +0200, Greg KH wrote:
> > On Wed, Jun 12, 2024 at 04:51:42PM +0200, Danilo Krummrich wrote:
> > > On 6/11/24 18:13, Boqun Feng wrote:
> > > > On Tue, Jun 11, 2024 at 03:29:22PM +0200, Greg KH wrote:
> > > > > On Tue, Jun 11, 2024 at 03:21:31PM +0200, Danilo Krummrich wrote:
> > > > > > ...hence, I agree we should indeed add to the #Invariants and #Safety section
> > > > > > that `->release` must be callable  from any thread.
> > > > > > 
> > > > > > However, this is just theory, do we actually have cases where `device::release`
> > > > 
> > > > @Danilo, right, it's only theorical, but it's good to call it out since
> > > > it's the requirement for a safe Rust abstraction.
> > > 
> > > Similar to my previous reply, if we want to call this out as safety requirement
> > > in `Device::from_raw`, we probably want to add it to the documentation of the C
> > > `struct device`, such that we can argue that this is an invariant of C's
> > > `struct device`.
> > > 
> > > Otherwise we'd have to write something like:
> > > 
> > > "It must also be ensured that the `->release` function of a `struct device` can
> > > be called from any non-atomic context. While not being officially documented this
> > > is guaranteed by the invariant of `struct device`."
> > 
> > In the 20+ years of the driver model being part of the kernel, I don't
> > think this has come up yet, so maybe you can call the release function
> > in irq context.  I don't know, I was just guessing :)
> 
> Ah, I see. I thought you know and it's defined, but just not documented.
> 
> This means it's simply undefined what we expect to happen when the last
> reference of a device is dropped from atomic context.
> 
> Now, I understand (and would even expect) that practically this has never been
> an issue. You'd need two circumstances, release() actually does something that
> is not allowed in atomic context plus the last device reference is dropped from
> atomic context - rather unlikely.
> 
> > 
> > So let's not go adding constraints that we just do not have please.
> > Same goes for the C code, so the rust code is no different here.
> 
> I agree we shouldn't add random constraints, but for writing safe code we also
> have to rely on defined behavior.

As the rust code is relying on C code that could change at any point in
time, how can that ever be "safe"?  :)

Sorry, this type of definition annoys me.

> I see two options:
> 
> (1) We globally (for struct device) define from which context release() is
>     allowed to be called.

If you want, feel free to do that work please.  And then find out how to
enforce it in the driver core.

> (2) We define it for the Rust abstraction only and just constrain it to
>     non-atomic context to be able to give a safety guarantee. We can't say
>     "might be safe from any context, but we don't know".

Why can't you say that?  Your "saftey" barrier ends/begins at the
interaction with the rust/c layer.  You can't "guarantee" anything on
the C side...

> But again, this is really just a formality, the C code does it all the way and
> practically there never was an issue, which means we actually do follow some
> rules, it's just about writing them down. :)

Again, this has NEVER come up in 20+ years, so maybe it's just not an
issue?  Not to say it isn't, but maybe do some tests and see what
happens...

thanks,

greg k-h