| 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
| ||
|
Message-ID: <cc73ce51-39c1-4f2d-8bcb-99fd220fd34c@de.bosch.com>
Date: Fri, 16 Jan 2026 14:00:54 +0100
From: Dirk Behme <dirk.behme@...bosch.com>
To: Daniel Almeida <daniel.almeida@...labora.com>, Dirk Behme
<dirk.behme@...il.com>
CC: Danilo Krummrich <dakr@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard
<mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, David Airlie
<airlied@...il.com>, Simona Vetter <simona@...ll.ch>, 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>, Trevor Gross <tmgross@...ch.edu>, "Steven
Price" <steven.price@....com>, <dri-devel@...ts.freedesktop.org>,
<linux-kernel@...r.kernel.org>, <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH] rust/drm: tyr: Convert to the register!() macro
Hi Daniel,
On 16/01/2026 13:23, Daniel Almeida wrote:
> Hi Dirk, thanks for the review!
>
>> On 15 Jan 2026, at 14:05, Dirk Behme <dirk.behme@...il.com> wrote:
>>
>> Hi Daniel,
>>
>> On 14.01.26 23:53, Daniel Almeida wrote:
>>> Replace regs::Register with kernel::register. This allow us to more
>>> succinctly express the register set by introducing the ability to describe
>>> fields and their documentation and to auto-generate the accessors. In
>>> particular, this is very helpful as it does away with a lot of manual masks
>>> and shifts.
>>
>>
>> As mentioned somewhere else already I really like switching to
>> register!(). Thanks!
>>
>> Some coments below:
>>
>>
>>> A future commit will eliminate HI/LO pairs once there is support for 64bit
>>> reads and writes in kernel::register.
>>>
>>> Signed-off-by: Daniel Almeida <daniel.almeida@...labora.com>
>>> ---
>>> Note that this patch depends on a rebased version of Joel's patch at [0].
>>>
>>> That version is stale, so I ended up rebasing it locally myself for the
>>> purpose of developing this patch and gathering some reviews on the list. In
>>> other words, the current patch does not apply for the time being, but will
>>> once a v7 for Joel's series is out.
>>>
>>> [0]: https://lore.kernel.org/rust-for-linux/20251003154748.1687160-1-joelagnelf@nvidia.com/
>>> ---
>>> drivers/gpu/drm/tyr/driver.rs | 15 ++-
>>> drivers/gpu/drm/tyr/gpu.rs | 55 ++++----
>>> drivers/gpu/drm/tyr/regs.rs | 302 ++++++++++++++++++++++++++++++++----------
>>> 3 files changed, 267 insertions(+), 105 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
>>> index 0389c558c036..8e06db5320bf 100644
>>> --- a/drivers/gpu/drm/tyr/driver.rs
>>> +++ b/drivers/gpu/drm/tyr/driver.rs
>>> @@ -66,19 +66,20 @@ unsafe impl Send for TyrData {}
>>> unsafe impl Sync for TyrData {}
>>>
>>> fn issue_soft_reset(dev: &Device<Bound>, iomem: &Devres<IoMem>) -> Result {
>>> - regs::GPU_CMD.write(dev, iomem, regs::GPU_CMD_SOFT_RESET)?;
>>> + let io = iomem.access(dev)?;
>>> +
>>> + regs::GpuCommand::default()
>>> + .set_command(regs::GPU_CMD_SOFT_RESET)
>>> + .write(io);
>>>
>>> // TODO: We cannot poll, as there is no support in Rust currently, so we
>>> // sleep. Change this when read_poll_timeout() is implemented in Rust.
>>> kernel::time::delay::fsleep(time::Delta::from_millis(100));
>>>
>>> - if regs::GPU_IRQ_RAWSTAT.read(dev, iomem)? & regs::GPU_IRQ_RAWSTAT_RESET_COMPLETED == 0 {
>>> + let rawstat = regs::GpuIrqRawstat::read(io);
>>> + if !rawstat.reset_completed() {
>>> dev_err!(dev, "GPU reset failed with errno\n");
>>> - dev_err!(
>>> - dev,
>>> - "GPU_INT_RAWSTAT is {}\n",
>>> - regs::GPU_IRQ_RAWSTAT.read(dev, iomem)?
>>> - );
>>> + dev_err!(dev, "GPU_INT_RAWSTAT is {}\n", u32::from(rawstat));
>>
>>
>> This is pre-existing, but printing `... INT ...` for `...IRQ...`
>> register looks confusing (wrong?).
>
> Yeah, this needs to change indeed.
>
>>
>>
>>> return Err(EIO);
>>> }
>>> diff --git a/drivers/gpu/drm/tyr/gpu.rs b/drivers/gpu/drm/tyr/gpu.rs
>>> index 6c582910dd5d..7c698fb1e36a 100644
>>> --- a/drivers/gpu/drm/tyr/gpu.rs
>>> +++ b/drivers/gpu/drm/tyr/gpu.rs
>>> @@ -44,34 +44,36 @@ pub(crate) struct GpuInfo {
>>>
>>> impl GpuInfo {
>>> pub(crate) fn new(dev: &Device<Bound>, iomem: &Devres<IoMem>) -> Result<Self> {
>>> - let gpu_id = regs::GPU_ID.read(dev, iomem)?;
>>> - let csf_id = regs::GPU_CSF_ID.read(dev, iomem)?;
>>> - let gpu_rev = regs::GPU_REVID.read(dev, iomem)?;
>>> - let core_features = regs::GPU_CORE_FEATURES.read(dev, iomem)?;
>>> - let l2_features = regs::GPU_L2_FEATURES.read(dev, iomem)?;
>>> - let tiler_features = regs::GPU_TILER_FEATURES.read(dev, iomem)?;
>>> - let mem_features = regs::GPU_MEM_FEATURES.read(dev, iomem)?;
>>> - let mmu_features = regs::GPU_MMU_FEATURES.read(dev, iomem)?;
>>> - let thread_features = regs::GPU_THREAD_FEATURES.read(dev, iomem)?;
>>> - let max_threads = regs::GPU_THREAD_MAX_THREADS.read(dev, iomem)?;
>>> - let thread_max_workgroup_size = regs::GPU_THREAD_MAX_WORKGROUP_SIZE.read(dev, iomem)?;
>>> - let thread_max_barrier_size = regs::GPU_THREAD_MAX_BARRIER_SIZE.read(dev, iomem)?;
>>> - let coherency_features = regs::GPU_COHERENCY_FEATURES.read(dev, iomem)?;
>>> -
>>> - let texture_features = regs::GPU_TEXTURE_FEATURES0.read(dev, iomem)?;
>>> -
>>> - let as_present = regs::GPU_AS_PRESENT.read(dev, iomem)?;
>>> -
>>> - let shader_present = u64::from(regs::GPU_SHADER_PRESENT_LO.read(dev, iomem)?);
>>> + let io = (*iomem).access(dev)?;
>>> +
>>> + let gpu_id = regs::GpuId::read(io).into();
>>> + let csf_id = regs::CsfId::read(io).into();
>>> + let gpu_rev = regs::RevIdr::read(io).into();
>>> + let core_features = regs::CoreFeatures::read(io).into();
>>> + let l2_features = regs::L2Features::read(io).into();
>>> + let tiler_features = regs::TilerFeatures::read(io).into();
>>> + let mem_features = regs::MemFeatures::read(io).into();
>>> + let mmu_features = regs::MmuFeatures::read(io).into();
>>> + let thread_features = regs::ThreadFeatures::read(io).into();
>>> + let max_threads = regs::ThreadMaxThreads::read(io).into();
>>> + let thread_max_workgroup_size = regs::ThreadMaxWorkgroupSize::read(io).into();
>>> + let thread_max_barrier_size = regs::ThreadMaxBarrierSize::read(io).into();
>>> + let coherency_features = regs::CoherencyFeatures::read(io).into();
>>
>>
>> Is there any reason why you replace the UPPERCASE register names with
>> CamelCase ones?
>>
>> I was under the impression that we want to use UPPERCASE for register
>> names. Like in nova
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/nova-core/regs.rs
>
> Not really. UPPERCASE for non-const items will trigger the linter. The Nova
> people chose to #[allow] this to align with OpenRM and, IIRC from the LPC
> discussions, their registers are automatically generated from some internal
> docs.
>
> We have only a few, we can simply convert them to CamelCase.
I'm under the impression that we define the "future RFL register!()
style standard" here.
So we want to make the CamelCase the default? And nova is the exception?
I'm fine with that. Just want to make sure we talked about it :)
....
>>> pub(crate) const MCU_CONTROL_ENABLE: u32 = 1;
>>> pub(crate) const MCU_CONTROL_AUTO: u32 = 2;
>>> pub(crate) const MCU_CONTROL_DISABLE: u32 = 0;
>>>
>>> -pub(crate) const MCU_STATUS: Register<0x704> = Register;
>>> +register!(McuControl @ 0x700, "Controls the execution state of the MCU subsystem" {
>>> + 1:0 req as u32, "Request state change";
>>> +});
>>
>>
>> Any reason why req is a u32 and not a u8? Same for some other places.
>
>
> I tend to default to u32/i32 in general, as that’s usually the native machine integer type.
>
> All we get from smaller types is a spam of `into()`, `from()` and their `try_`
> equivalents. When stored in a struct, they usually do not save space due to
> padding that is usually inserted to fix the alignment for the type. IMHO not
> worth it unless it really matters. Correct me if I'm wrong, but it doesn't seem
> to be the case here.
Wouldn't using u8 prevent any accidental access to 31:8 ?
Best regards
Dirk
Powered by blists - more mailing lists