Re: [PATCH v2 1/2] rust: add abstraction for struct device

[Danilo Krummrich]

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.

I see two options:

(1) We globally (for struct device) define from which context release() is
    allowed to be called.

(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".

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. :)

- Danilo

> 
> thanks,
> 
> greg k-h
>