Message-Id: <DEZJMPRAWXHW.1CYQA0PONEP97@kernel.org>
Date: Tue, 16 Dec 2025 10:45:47 +0100
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Matthew Maurer" <mmaurer@...gle.com>
Cc: "Dmitry Baryshkov" <dmitry.baryshkov@....qualcomm.com>, "Bjorn
 Andersson" <andersson@...nel.org>, "Konrad Dybcio"
 <konradybcio@...nel.org>, "Satya Durga Srinivasu Prabhala"
 <satyap@...cinc.com>, "Miguel Ojeda" <ojeda@...nel.org>, "Boqun Feng"
 <boqun.feng@...il.com>, "Gary Guo" <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>, "Benno Lossin"
 <lossin@...nel.org>, "Andreas Hindborg" <a.hindborg@...nel.org>, "Alice
 Ryhl" <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>, "Trilok
 Soni" <tsoni@...cinc.com>, <linux-kernel@...r.kernel.org>,
 <linux-arm-msm@...r.kernel.org>, <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH RFC] soc: qcom: socinfo: Re-implement in Rust

On Tue Dec 16, 2025 at 1:55 AM CET, Matthew Maurer wrote:
> I'll get to your other comments in a v2 soon, but wanted to focus on
> the lifetime of `qcom_smem_get` buffers for a moment.
>
>> > > > ---
>> > >
>> > > > +
>> > > > +pub(crate) fn qcom_smem_get(host: i32, item: u32) -> Result<&'static [u8]> {
>> > > > +    let mut size = 0;
>> > > > +    // SAFETY: qcom_smem_get only requires that the size pointer be a writable size_t,
>> > > > +    // host and item are error checked in the qcom_smem module.
>> > > > +    let err_ptr = unsafe { kernel::bindings::qcom_smem_get(host as u32, item, &mut size) };
>> > > > +    let ptr = from_err_ptr(err_ptr)?;
>> > > > +    // SAFETY: If qcom_smem_get does not return an error, the returned pointer points to a readable
>> > > > +    // byte buffer with its size written into size. Because these buffers are derived from the
>> > > > +    // static ranges in the DT, this buffer remains accessible even if the qcom_smem module is
>> > > > +    // unloaded, so 'static is appropriate. The underlying buffer cannot mutate, so upgrading it
>> > > > +    // to a reference is allowed.
>> > >
>> > > This is definitely not true. The smem data is ioremap'ped during
>> > > qcom_smem_probe() and will be unmapped if qcom_smem is unloaded.
>> > > Accessing such a pointer after unloading smem would result in the
>> > > exception. Current socinfo ignores that and keeps on accessing the data
>> > > at runtime (which also allows it to read DSP versions which are updated
>> > > at runtime), but the comment needs to be corrected.
>> >
>> > Thanks for the info. In v2 I'll look into seeing if I can make those
>> > slices hold a reference on the smem module while they're alive.
>>
>> It should be a reference to the _bound_ device. I don't think we have
>> a way to do it. I suggest just fixing the comment instead.
>
> So, unfortunately fixing the comment doesn't really address this issue
> - a `&'static [u8]` is a promise that this buffer is immutable and
> will remain valid to read indefinitely, and it sounds like neither of
> these are actually true (the former due to the DSP live updates, and
> the latter due to the potential for the buffer to be unmapped).
>
> I can deal with "this buffer may mutate" by changing to another
> abstraction, but it'd help a lot if I understood the intended way
> (even in C, we can ignore Rust for a moment) to know that the buffer
> you got from `qcom_smem_get` is still valid. My best guess right now
> is that because the smem driver is a *parent* driver to at least this
> one, we know based on where we are instantiated that it will clean us
> up before we go away. Is that the intended mechanism?
>
> If that is the case, then question for the Rust maintainers, since
> `probe` is not an `unsafe` function, could allow the equivalent of
> safety requirements on probe methods? Since the match tables are
> empty, it will only be probed if explicitly requested, so there's an
> argument to be made for "this driver may only be probed by a device
> descended from a qcom smem device".

This is a driver lifecycle problem that arises from the fact that in this case
you deal with two (technically independent) platform drivers: the QCOM SMEM one
and this one, the SoC info one.

At the same time though there are dependencies between the two, i.e. the SoC
info driver tries to access device resources of the SMEM driver, which requires
a lifetime relationship that mandates that the SoC info driver is unbound once
the SMEM driver is unbound.

So, rather than having two independent platform drivers the correct way to
handle this lifetime relationship through the driver-core would be to let the
SMEM driver create an auxiliary device and register the SoC info driver as
auxiliary driver rather than a platform driver.

With this you get the guarantee that throughout the whole lifetime of the SoC
info driver also the parent driver (QCOM SMEM) is bound and the corresponding
device resources are valid to access.

Or in other words, for "this driver may only be probed by a device descended
from a qcom smem device" the auxiliary bus models exactly this relationship and
is the way to go.

In order to make the precense of the SoC info driver still controllable by an OF
node, the QCOM SMEM driver can parse the OF node and register the auxiliary
device conditionally.

With this, your qcom_smem_get() method should take an additional &Device<Bound>
argument, which is the parent device of the auxiliary::Device<Bound> you get in
probe() in the SoC info driver. I.e. the parent() method of an
auxiliary::Device<Bound> provides you a &Device<Bound> of the parent as the
parent is guaranteed to be bound when the auxiliary device is bound, see also
[1].

Additionally, you should wrap the resulting &'static [u8] into a Devres [2]
container, which will revoke the slice when the parent device, i.e. the owner of
the resource behind the slice is unbound.

Given that we have all the required mechanisms in place (in Rust) to model such
lifetime relationships safely, I'd love to see them being used instead of a
comment mandating to do the right thing. :)

I hope this helps!

- Danilo

[1] https://rust.docs.kernel.org/kernel/auxiliary/struct.Device.html#method.parent
[2] https://rust.docs.kernel.org/kernel/devres/struct.Devres.html

Powered by blists - more mailing lists