Message-ID: <CAGSQo01-yjNBpo0jCO0sK+0XHLKUyZY8vyA0X2M=fDtcmv6Vkw@mail.gmail.com>
Date: Mon, 15 Dec 2025 16:55:13 -0800
From: Matthew Maurer <mmaurer@...gle.com>
To: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
Cc: 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>, 
	Danilo Krummrich <dakr@...nel.org>, 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

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

>
> >
> > >
> > > > +    Ok(unsafe { core::slice::from_raw_parts(ptr as *const u8, size) })
> > > > +}
> > > > +

Powered by blists - more mailing lists