* Re: printk's threaded legacy console + fbcon => schedule where it should not
From: Sebastian Andrzej Siewior @ 2026-01-21 13:57 UTC (permalink / raw)
To: Petr Mladek
Cc: Steven Rostedt, linux-kernel, linux-serial, linux-fbdev,
John Ogness, Sergey Senozhatsky, Greg Kroah-Hartman, Jiri Slaby,
Simona Vetter, Helge Deller
In-Reply-To: <aXDYEVlkFgxdSVSG@pathway.suse.cz>
On 2026-01-21 14:43:45 [+0100], Petr Mladek wrote:
> I know that there was a plan to get rid of cond_resched().
> But what is the status now, please?
It is slowly moving => https://lore.kernel.org/all/20251219101502.GB1132199@noisy.programming.kicks-ass.net/
> I still see more that 1k cond_resched() calls in the code:
>
> $> git grep cond_resched\(\) | grep "\.c:" | wc -l
> 1263
>
> And config PREEMPT_VOLUNTARY still talks about the explicit
> preemption points.
>
> > Should we just remove it and see what breaks?
>
> Honestly, I do not feel comfortable with removing it. It is true that
> it has no effect in the printk() code path. But the vt code is used
> also when working on the terminal.
>
> The vt code still uses console_lock() because it was intertwined
> with printk console code since very old days. console_lock is a kind
> of big kernel lock there.
Do you a have path which loops and would mandate it? I found a few which
do not matter and have their own cond_resched() around. So I don't see a
reason to keep it. And I found one which breaks things so a removal
makes sense.
> Alternative solution is to get rid of the spin_trylock(). The only
> purpose is to prevent race in console_flush_on_panic(). It used
> to be a simple bit operation. The spin_lock() was added just to
> get barriers right. But we have a great atomic_t API these days.
>
> IMHO, it is a win-win solution because a preemptive context is
> always better.
So why do we keep warts again? Just because it *might* be required?
Keeping things preemptible makes sense but this is locking with no
annotation what so ever.
Again. printk has its cond_resched, the tty has it, too.
I'm with Steven on the removal side.
Sebastian
^ permalink raw reply
* Re: printk's threaded legacy console + fbcon => schedule where it should not
From: Petr Mladek @ 2026-01-21 16:08 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Steven Rostedt, linux-kernel, linux-serial, linux-fbdev,
John Ogness, Sergey Senozhatsky, Greg Kroah-Hartman, Jiri Slaby,
Simona Vetter, Helge Deller
In-Reply-To: <20260121135737.K7b-4M5r@linutronix.de>
On Wed 2026-01-21 14:57:37, Sebastian Andrzej Siewior wrote:
> On 2026-01-21 14:43:45 [+0100], Petr Mladek wrote:
> > I know that there was a plan to get rid of cond_resched().
> > But what is the status now, please?
>
> It is slowly moving => https://lore.kernel.org/all/20251219101502.GB1132199@noisy.programming.kicks-ass.net/
Good to know.
> > I still see more that 1k cond_resched() calls in the code:
> >
> > $> git grep cond_resched\(\) | grep "\.c:" | wc -l
> > 1263
> >
> > And config PREEMPT_VOLUNTARY still talks about the explicit
> > preemption points.
> >
> > > Should we just remove it and see what breaks?
> >
> > Honestly, I do not feel comfortable with removing it. It is true that
> > it has no effect in the printk() code path. But the vt code is used
> > also when working on the terminal.
> >
> > The vt code still uses console_lock() because it was intertwined
> > with printk console code since very old days. console_lock is a kind
> > of big kernel lock there.
>
> Do you a have path which loops and would mandate it? I found a few which
> do not matter and have their own cond_resched() around. So I don't see a
> reason to keep it. And I found one which breaks things so a removal
> makes sense.
Could anyone from VT guys comment on it, please?
> > Alternative solution is to get rid of the spin_trylock(). The only
> > purpose is to prevent race in console_flush_on_panic(). It used
> > to be a simple bit operation. The spin_lock() was added just to
> > get barriers right. But we have a great atomic_t API these days.
> >
> > IMHO, it is a win-win solution because a preemptive context is
> > always better.
>
> So why do we keep warts again? Just because it *might* be required?
> Keeping things preemptible makes sense but this is locking with no
> annotation what so ever.
Well, the current locking is documented but it creates false
positives. The "printing_lock" is taken on a single place
using spin_trylock(). Nobody would ever spin on it. So
sleeping is perfectly fine.
> Again. printk has its cond_resched, the tty has it, too.
> I'm with Steven on the removal side.
As I said, the cond_resched() does not have any effect from
the printk() code path. But the other VT paths might rely on it.
If VT-guys are willing to take the risk and remove it
then I am fine with it.
Best Regards,
Petr
^ permalink raw reply
* Re: [PATCH RFC v6 05/26] nova-core: mm: Add support to use PRAMIN windows to write to VRAM
From: Joel Fernandes @ 2026-01-21 17:52 UTC (permalink / raw)
To: Zhi Wang
Cc: linux-kernel, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
David Airlie, Simona Vetter, Jonathan Corbet, Alex Deucher,
Christian König, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
Tvrtko Ursulin, Huang Rui, Matthew Auld, Matthew Brost,
Lucas De Marchi, Thomas Hellström, Helge Deller,
Danilo Krummrich, Alice Ryhl, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Trevor Gross, John Hubbard, Alistair Popple,
Timur Tabi, Edwin Peer, Alexandre Courbot, Andrea Righi,
Andy Ritger, Alexey Ivanov, Balbir Singh, Philipp Stanner,
Elle Rhumsaa, Daniel Almeida, joel, nouveau, dri-devel,
rust-for-linux, linux-doc, amd-gfx, intel-gfx, intel-xe,
linux-fbdev
In-Reply-To: <20260121100745.2b5a58e5.zhiw@nvidia.com>
Hello, Zhi,
On 1/21/2026 3:07 AM, Zhi Wang wrote:
> On Tue, 20 Jan 2026 15:42:42 -0500
> Joel Fernandes <joelagnelf@nvidia.com> wrote:
>
>> PRAMIN apertures are a crucial mechanism to direct read/write to VRAM.
>> Add support for the same.
>>
>
> I went through the code, this seems not designed for multiple users. As
> this is used for writting PTEs for page tables, can you shed some light
> about the plan of how we should handle the concurrency of writting multiple
> page table PTEs, e.g. when two GPU memory mapping in two different GPU
> page tables are procceding concurrently, this could happen when people
> creating vGPUs concurrently.
Good question. Currently, BarUser::map() requires a mutable reference to both
the BarUser and the GpuMm.
pub(crate) fn map<'a>(
&'a mut self,
mm: &'a mut GpuMm,
GpuMm is owned by the struct Gpu, so from a Rust standpoint, it is already
handled since it is not possible to manipulate the Page table hierarchy (Page
directories and last level Page table).
But yes, we have to look into concurrency once we have channels, and users other
than bar where have multiple users of the same address space doing
mapping/unmapping.
I think we can incrementally build on this series to add support for the same,
it is not something this series directly addresses since I have spend majority
of my time last several months making translation *work* which is itself no east
task. This series is just preliminary based on work from last several months and
to make BAR1 work. For instance, I kept PRAMIN simple based on feedback that we
don't want to over complicate without fully understanding all the requirements.
There is also additional requirements for locking design that have implications
with DMA fencing etc, for instance.
Anyway thinking out loud, I am thinking for handling concurrency at the page
table entry level (if we ever need it), we could use per-PT spinlocks similar to
the Linux kernel. But lets plan on how to do this properly and based on actual
requirements.
--
Joel Fernandes
^ permalink raw reply
* Re: [PATCH RFC v6 01/26] rust: clist: Add support to interface with C linked lists
From: Joel Fernandes @ 2026-01-21 18:12 UTC (permalink / raw)
To: Zhi Wang, Joel Fernandes
Cc: linux-kernel, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
David Airlie, Simona Vetter, Jonathan Corbet, Alex Deucher,
Christian König, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
Tvrtko Ursulin, Huang Rui, Matthew Auld, Matthew Brost,
Lucas De Marchi, Thomas Hellström, Helge Deller,
Danilo Krummrich, Alice Ryhl, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Trevor Gross, John Hubbard, Alistair Popple,
Timur Tabi, Edwin Peer, Alexandre Courbot, Andrea Righi,
Andy Ritger, Alexey Ivanov, Balbir Singh, Philipp Stanner,
Elle Rhumsaa, Daniel Almeida, nouveau, dri-devel, rust-for-linux,
linux-doc, amd-gfx, intel-gfx, intel-xe, linux-fbdev, Boqun Feng,
Paul E. McKenney
In-Reply-To: <20260121092730.3628d290.zhiw@nvidia.com>
On 1/21/2026 2:27 AM, Zhi Wang wrote:
>
>> +/// Initialize a `list_head` object to point to itself.
>> +///
>> +/// # Safety
>> +///
>> +/// `list` must be a valid pointer to a `list_head` object.
>> +#[inline]
>> +pub unsafe fn init_list_head(list: *mut bindings::list_head) {
>> + // SAFETY: Caller guarantees `list` is a valid pointer to a
>> `list_head`.
>> + unsafe {
>> + (*list).next = list;
>> + (*list).prev = list;
>> + }
>> +}
>> +
>
> Might be better to have a C helper? since INIT_LIST_HEAD() has WRITE_ONCE()
> for memory ordering. This one seems not equal to it.
WRITE_ONCE() is not really about CPU memory ordering though, it is about
compiler optimizations. On the C side, I think it is needed in case of
list_for_each_entry_rcu(), to avoid the case of invented stores or store fusing,
but here we are not doing RCU-based iteration.
Anyway, if we want to future proof that, I am Ok with adding the helper back
(which I actually initially had but feedback from past review was to just inline
it into rust).
But I am not sure if we have this issue for the Rust compiler, like we do for C.
Rust does not allow raw pointers to be concurrently read/written using plain
accesses, so should be already protected due to the borrow checker and compiler
itself right?
Adding some interested folks as well to CC for the topic of _ONCE, +Boqun +Paul.
--
Joel Fernandes
^ permalink raw reply
* Re: [PATCH RFC v6 13/26] nova-core: mm: Add unified page table entry wrapper enums
From: Joel Fernandes @ 2026-01-21 18:35 UTC (permalink / raw)
To: Zhi Wang
Cc: linux-kernel, Maarten Lankhorst, Maxime Ripard, Simona Vetter,
Jonathan Corbet, Alex Deucher, Christian König, Jani Nikula,
Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin, Huang Rui,
Matthew Auld, Matthew Brost, Lucas De Marchi,
Thomas Hellström, Helge Deller, Danilo Krummrich, Alice Ryhl,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Alistair Popple, Alexandre Courbot, Andrea Righi,
Alexey Ivanov, Philipp Stanner, Elle Rhumsaa, Daniel Almeida,
joel, nouveau, dri-devel, rust-for-linux, linux-doc, amd-gfx,
intel-gfx, intel-xe, linux-fbdev
In-Reply-To: <20260121115436.45e174d3.zhiw@nvidia.com>
On 1/21/2026 4:54 AM, Zhi Wang wrote:
> On Tue, 20 Jan 2026 15:42:50 -0500
> Joel Fernandes <joelagnelf@nvidia.com> wrote:
>> Add unified Pte, Pde, and DualPde wrapper enums that abstract over
>> MMU v2 and v3 page table entry formats. These enums allow the page
>> table walker and VMM to work with both MMU versions.
>>
>
> snip
>
>> +impl DualPde {
>> + /// Create a [`DualPde`] from raw 128-bit value (two `u64`s) for
>> the given MMU version.
>> + pub(crate) fn new(version: MmuVersion, big: u64, small: u64) ->
>> Self {
>> + match version {
>> + MmuVersion::V2 => Self::V2(ver2::DualPde::new(big, small)),
>> + MmuVersion::V3 => Self::V3(ver3::DualPde::new(big, small)),
>> + }
>> + }
>> +
>> + /// Create a [`DualPde`] with only the small page table pointer set.
>> + pub(crate) fn new_small(version: MmuVersion, table_pfn: Pfn) ->
>> Self {
>> + match version {
>> + MmuVersion::V2 =>
>> Self::V2(ver2::DualPde::new_small(table_pfn)),
>> + MmuVersion::V3 =>
>> Self::V3(ver3::DualPde::new_small(table_pfn)),
>> + }
>> + }
>> +
>> + /// Check if the small page table pointer is valid.
>> + pub(crate) fn has_small(&self) -> bool {
>> + match self {
>> + Self::V2(d) => d.has_small(),
>> + Self::V3(d) => d.has_small(),
>> + }
>> + }
>> +
>
> Should we also have a has_big here as well?
Good catch, I will add that in, thanks.
--
Joel Fernandes
^ permalink raw reply
* Re: [PATCH RFC v6 14/26] nova-core: mm: Add TLB flush support
From: Joel Fernandes @ 2026-01-21 18:45 UTC (permalink / raw)
To: Zhi Wang
Cc: linux-kernel, Maarten Lankhorst, Maxime Ripard, Simona Vetter,
Jonathan Corbet, Alex Deucher, Christian König, Jani Nikula,
Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin, Huang Rui,
Matthew Auld, Matthew Brost, Lucas De Marchi,
Thomas Hellström, Helge Deller, Danilo Krummrich, Alice Ryhl,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Alistair Popple, Alexandre Courbot, Andrea Righi,
Alexey Ivanov, Philipp Stanner, Elle Rhumsaa, Daniel Almeida,
joel, nouveau, dri-devel, rust-for-linux, linux-doc, amd-gfx,
intel-gfx, intel-xe, linux-fbdev
In-Reply-To: <20260121115917.73cfcc7f.zhiw@nvidia.com>
Hello, Zhi,
On 1/21/2026 4:59 AM, Zhi Wang wrote:
> On Tue, 20 Jan 2026 15:42:51 -0500
> Joel Fernandes <joelagnelf@nvidia.com> wrote:
>
>> Add TLB (Translation Lookaside Buffer) flush support for GPU MMU.
>>
> The same concern as in PATCH 5, guess we need to think of concurrency for
> TLB flush.
Will change:
pub(crate) fn flush(&self, pdb_addr: VramAddress)
to:
pub(crate) fn flush(&mut self, pdb_addr: VramAddress)
and also changing in mm/mod.rs:
pub(crate) fn tlb(&self) -> &Tlb {
to:
pub(crate) fn tlb(&mut self) -> &mut Tlb.
Since TLB operations modify registers, that does make sense to me.
For the buddy allocator, however, I am locking internally so I left it as is:
/// Access the [`GpuBuddy`] allocator.
pub(crate) fn buddy(&self) -> &GpuBuddy {
&self.buddy
}
--
Joel Fernandes
^ permalink raw reply
* Re: [PATCH RFC v6 01/26] rust: clist: Add support to interface with C linked lists
From: Joel Fernandes @ 2026-01-21 19:50 UTC (permalink / raw)
To: Gary Guo, linux-kernel
Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Jonathan Corbet, Alex Deucher,
Christian König, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
Tvrtko Ursulin, Huang Rui, Matthew Auld, Matthew Brost,
Lucas De Marchi, Thomas Hellström, Helge Deller,
Danilo Krummrich, Alice Ryhl, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, John Hubbard, Alistair Popple, Timur Tabi,
Edwin Peer, Alexandre Courbot, Andrea Righi, Andy Ritger,
Zhi Wang, Alexey Ivanov, Balbir Singh, Philipp Stanner,
Elle Rhumsaa, Daniel Almeida, joel, nouveau, dri-devel,
rust-for-linux, linux-doc, amd-gfx, intel-gfx, intel-xe,
linux-fbdev
In-Reply-To: <DFTTGUYGY72V.3VLVSCB2OOXIB@garyguo.net>
Hello, Gary,
On 1/20/2026 6:48 PM, Gary Guo wrote:
> On Tue Jan 20, 2026 at 8:42 PM GMT, Joel Fernandes wrote:
>> Add a new module `clist` for working with C's doubly circular linked
>> lists. Provide low-level iteration over list nodes.
>>
>> Typed iteration over actual items is provided with a `clist_create`
>> macro to assist in creation of the `Clist` type.
>
> This should read "CList".
Sure, will fix.
>
> I was quite dubious about the patch just from the title (everybody knows how
> easy a linked list is in Rust), but it turns out it is not as concerning as I
> expected, mostly due to the read-only nature of the particular implementation
> (a lot of the safety comments would be much more difficult to justify, say, if
> it's mutable). That said, still a lot of feedbacks below.
Sure, the reason for requiring this is interfacing with lists coming from C
code. I'd see a future where we may want it mutable too (example, Rust code
adding elements to the existing). At which point, the invariants/safety
reasoning may change.
> I think something like is okay in the short term. However, there's an growing
> interest in getting our Rust list API improved, so it could be ideal if
> eventually the Rust list can be capable of handling FFI lists, too.
Yeah we looked into that, if you see old threads, the conclusion was it is not a
good fit for existing rust list abstractions. TLDR; it does not fit into their
ownership/borrowing model.
[...]
>> +
>> +/// Initialize a `list_head` object to point to itself.
>> +///
>> +/// # Safety
>> +///
>> +/// `list` must be a valid pointer to a `list_head` object.
>> +#[inline]
>> +pub unsafe fn init_list_head(list: *mut bindings::list_head) {
>> + // SAFETY: Caller guarantees `list` is a valid pointer to a `list_head`.
>> + unsafe {
>> + (*list).next = list;
>> + (*list).prev = list;
>
> This needs to be an atomic write or it'll depart from the C implementation.
I am curious what you mean by atomic write, can you define it? Does rust
compiler have load/store fusing, invented stores, etc, like C does? Sorry I am
only familiar with these concepts on C. Could you provide example of a race
condition in Rust that can happen?
Also I did this addition based on feedback from past review:
https://lore.kernel.org/all/DEI89VUEYXAJ.1IQQPC3QRLITP@nvidia.com/
There was some concerns around pointless function call overhead when the rust
implementation is already quite intertwined with internals of the C linked list
implementation. I do agree with that point of view too.
Also see my other reply to Zhi on this helper topic, lets discuss there too, if
that's Ok.
>> + }
>> +}
>
> I don't think we want to publicly expose this! I've not found a user in the
> subsequent patch, too.
There are 2 users:
pub fn try_init<E>(
and the self-tests:
//! # let head = head.as_mut_ptr();
//! # // SAFETY: head and all the items are test objects allocated in [..]
//! # unsafe { init_list_head(head) };
//! #
>
>> +
>> +/// Wraps a `list_head` object for use in intrusive linked lists.
>> +///
>> +/// # Invariants
>> +///
>> +/// - [`CListHead`] represents an allocated and valid `list_head` structure.
>> +/// - Once a [`CListHead`] is created in Rust, it will not be modified by non-Rust code.
>> +/// - All `list_head` for individual items are not modified for the lifetime of [`CListHead`].
>> +#[repr(transparent)]
>> +pub struct CListHead(Opaque<bindings::list_head>);
>> +
>> +impl CListHead {
>> + /// Create a `&CListHead` reference from a raw `list_head` pointer.
>> + ///
>> + /// # Safety
>> + ///
>> + /// - `ptr` must be a valid pointer to an allocated and initialized `list_head` structure.
>> + /// - `ptr` must remain valid and unmodified for the lifetime `'a`.
>> + #[inline]
>> + pub unsafe fn from_raw<'a>(ptr: *mut bindings::list_head) -> &'a Self {
>> + // SAFETY:
>> + // - [`CListHead`] has same layout as `list_head`.
>> + // - `ptr` is valid and unmodified for 'a.
>> + unsafe { &*ptr.cast() }
>> + }
>> +
>> + /// Get the raw `list_head` pointer.
>> + #[inline]
>> + pub fn as_raw(&self) -> *mut bindings::list_head {
>> + self.0.get()
>> + }
>> +
>> + /// Get the next [`CListHead`] in the list.
>> + #[inline]
>> + pub fn next(&self) -> &Self {
>> + let raw = self.as_raw();
>> + // SAFETY:
>> + // - `self.as_raw()` is valid per type invariants.
>> + // - The `next` pointer is guaranteed to be non-NULL.
>> + unsafe { Self::from_raw((*raw).next) }
>> + }
>> +
>> + /// Get the previous [`CListHead`] in the list.
>> + #[inline]
>> + pub fn prev(&self) -> &Self {
>> + let raw = self.as_raw();
>> + // SAFETY:
>> + // - self.as_raw() is valid per type invariants.
>> + // - The `prev` pointer is guaranteed to be non-NULL.
>> + unsafe { Self::from_raw((*raw).prev) }
>> + }
>> +
>> + /// Check if this node is linked in a list (not isolated).
>> + #[inline]
>> + pub fn is_linked(&self) -> bool {
>> + let raw = self.as_raw();
>> + // SAFETY: self.as_raw() is valid per type invariants.
>> + unsafe { (*raw).next != raw && (*raw).prev != raw }
>
> While is this checking both prev and next? `list_empty` is just
> `READ_ONCE(head->next) == head`.
Sure, I can optimize to just check ->next, that makes sense. Will do.
>
>> + }
>> +
>> + /// Fallible pin-initializer that initializes and then calls user closure.
>> + ///
>> + /// Initializes the list head first, then passes `&CListHead` to the closure.
>> + /// This hides the raw FFI pointer from the user.
>> + pub fn try_init<E>(
>> + init_func: impl FnOnce(&CListHead) -> Result<(), E>,
>> + ) -> impl PinInit<Self, E> {
>> + // SAFETY: init_list_head initializes the list_head to point to itself.
>> + // After initialization, we create a reference to pass to the closure.
>> + unsafe {
>> + pin_init::pin_init_from_closure(move |slot: *mut Self| {
>> + init_list_head(slot.cast());
>> + // SAFETY: slot is now initialized, safe to create reference.
>> + init_func(&*slot)
>
> Why is this callback necessary? The user can just create the list head and
> then reference it later? I don't see what this specifically gains over just
> doing
>
> fn new() -> impl PinInit<Self>;
>
> and have user-side
>
> list <- CListHead::new(),
> _: {
> do_want_ever(&list)
> }
The list initialization can fail, see the GPU buddy patch:
// Create pin-initializer that initializes list and allocates blocks.
let init = try_pin_init!(AllocatedBlocks {
list <- CListHead::try_init(|list| {
// Lock while allocating to serialize with concurrent frees.
let guard = buddy_arc.lock();
// SAFETY: guard provides exclusive access, list is initialized.
to_result(unsafe {
bindings::gpu_buddy_alloc_blocks(
guard.as_raw(),
params.start_range_address,
params.end_range_address,
params.size_bytes,
params.min_block_size_bytes,
list.as_raw(),
params.buddy_flags.as_raw(),
)
})
}),
buddy: Arc::clone(&buddy_arc),
flags: params.buddy_flags,
});
>
>
>> + })
>> + }
>> + }
>> +}
>> +
>> +// SAFETY: [`CListHead`] can be sent to any thread.
>> +unsafe impl Send for CListHead {}
>> +
>> +// SAFETY: [`CListHead`] can be shared among threads as it is not modified
>> +// by non-Rust code per type invariants.
>> +unsafe impl Sync for CListHead {}
>> +
>> +impl PartialEq for CListHead {
>> + fn eq(&self, other: &Self) -> bool {
>> + self.as_raw() == other.as_raw()
>
> Or just `core::ptr::eq(self, other)`
Sure, will fix.
>
>> + }
>> +}
>> +
>> +impl Eq for CListHead {}
>> +
>> +/// Low-level iterator over `list_head` nodes.
>> +///
>> +/// An iterator used to iterate over a C intrusive linked list (`list_head`). Caller has to
>> +/// perform conversion of returned [`CListHead`] to an item (using `container_of` macro or similar).
>> +///
>> +/// # Invariants
>> +///
>> +/// [`CListHeadIter`] is iterating over an allocated, initialized and valid list.
>> +struct CListHeadIter<'a> {
>> + current_head: &'a CListHead,
>> + list_head: &'a CListHead,
>> +}
>> +
>> +impl<'a> Iterator for CListHeadIter<'a> {
>> + type Item = &'a CListHead;
>> +
>> + #[inline]
>> + fn next(&mut self) -> Option<Self::Item> {
>> + // Advance to next node.
>> + let next = self.current_head.next();
>> +
>> + // Check if we've circled back to the sentinel head.
>> + if next == self.list_head {
>> + None
>> + } else {
>> + self.current_head = next;
>> + Some(self.current_head)
>> + }
>
> I think this could match the C iterator behaviour. When the iterator is created,
> a `next` is done first, and then subsequently you only need to check if
> `current_head` is `list_head`.
>
> This is slightly better because the condition check does not need to dereference
> a pointer.
Sure, I can change it to that.
>> +impl<'a> FusedIterator for CListHeadIter<'a> {}
>> +
>> +/// A typed C linked list with a sentinel head.
>> +///
>> +/// A sentinel head represents the entire linked list and can be used for
>> +/// iteration over items of type `T`, it is not associated with a specific item.
>> +///
>> +/// The const generic `OFFSET` specifies the byte offset of the `list_head` field within
>> +/// the struct that `T` wraps.
>> +///
>> +/// # Invariants
>> +///
>> +/// - `head` is an allocated and valid C `list_head` structure that is the list's sentinel.
>> +/// - `OFFSET` is the byte offset of the `list_head` field within the struct that `T` wraps.
>> +/// - All the list's `list_head` nodes are allocated and have valid next/prev pointers.
>> +/// - The underlying `list_head` (and entire list) is not modified for the lifetime `'a`.
>> +pub struct CList<'a, T, const OFFSET: usize> {
>> + head: &'a CListHead,
>> + _phantom: PhantomData<&'a T>,
>> +}
>
> Is there a reason that this is not
>
> #[repr(transparent)]
> struct CList(CListHead)
>
> ? We typically want to avoid putting reference inside the struct if it can be on
> the outside. This allows `&self` to be a single level of reference, not too.
>
> It also means that you can just write `&CList<_>` in many cases, and doesn't need
> `CList<'_, T>` (plus all the benefits of a reference).
Sure! Will change to this. I am guessing you mean the following, but please let
me know if you meant something else:
pub struct CList<T, const OFFSET: usize>(
CListHead,
PhantomData<T>,
);
I don't see any issues with my code using that, at the moment. Will let you know
how it goes.
>> +impl<'a, T, const OFFSET: usize> CList<'a, T, OFFSET> {
>> + /// Create a typed [`CList`] from a raw sentinel `list_head` pointer.
>> + ///
>> + /// # Safety
>> + ///
>> + /// - `ptr` must be a valid pointer to an allocated and initialized `list_head` structure
>> + /// representing a list sentinel.
>> + /// - `ptr` must remain valid and unmodified for the lifetime `'a`.
>> + /// - The list must contain items where the `list_head` field is at byte offset `OFFSET`.
>> + /// - `T` must be `#[repr(transparent)]` over the C struct.
>> + #[inline]
>> + pub unsafe fn from_raw(ptr: *mut bindings::list_head) -> Self {
>> + Self {
>> + // SAFETY: Caller guarantees `ptr` is a valid, sentinel `list_head` object.
>> + head: unsafe { CListHead::from_raw(ptr) },
>> + _phantom: PhantomData,
>> + }
>> + }
>> +
>> + /// Get the raw sentinel `list_head` pointer.
>> + #[inline]
>> + pub fn as_raw(&self) -> *mut bindings::list_head {
>> + self.head.as_raw()
>> + }
>> +
>> + /// Check if the list is empty.
>> + #[inline]
>> + pub fn is_empty(&self) -> bool {
>> + let raw = self.as_raw();
>> + // SAFETY: self.as_raw() is valid per type invariants.
>> + unsafe { (*raw).next == raw }
>
> `self.head.is_linked()`?
I'd considered `is_linked()` to be something that makes sense to call only on
`ClistHead` objects that belong to a particular "item" node, not a sentinel
node, so that was deliberate.
Though, I am Ok with doing it the way you are suggesting too
(`self.head.is_linked()`), since it is functionally equivalent.
>> + }
>> +
>> + /// Create an iterator over typed items.
>> + #[inline]
>> + pub fn iter(&self) -> CListIter<'a, T, OFFSET> {
>> + CListIter {
>> + head_iter: CListHeadIter {
>> + current_head: self.head,
>> + list_head: self.head,
>> + },
>> + _phantom: PhantomData,
>> + }
>> + }
>> +}
>> +
>> +/// High-level iterator over typed list items.
>> +pub struct CListIter<'a, T, const OFFSET: usize> {
>> + head_iter: CListHeadIter<'a>,
>> + _phantom: PhantomData<&'a T>,
>> +}
>> +
>> +impl<'a, T, const OFFSET: usize> Iterator for CListIter<'a, T, OFFSET> {
>> + type Item = &'a T;
>> +
>> + fn next(&mut self) -> Option<Self::Item> {
>> + let head = self.head_iter.next()?;
>> +
>> + // Convert to item using OFFSET.
>> + // SAFETY: `item_ptr` calculation from `OFFSET` (calculated using offset_of!)
>> + // is valid per invariants.
>> + Some(unsafe { &*head.as_raw().byte_sub(OFFSET).cast::<T>() })
>> + }
>> +}
>> +
>> +impl<'a, T, const OFFSET: usize> FusedIterator for CListIter<'a, T, OFFSET> {}
>> +
>> +/// Create a C doubly-circular linked list interface [`CList`] from a raw `list_head` pointer.
>> +///
>> +/// This macro creates a [`CList<T, OFFSET>`] that can iterate over items of type `$rust_type`
>> +/// linked via the `$field` field in the underlying C struct `$c_type`.
>> +///
>> +/// # Arguments
>> +///
>> +/// - `$head`: Raw pointer to the sentinel `list_head` object (`*mut bindings::list_head`).
>> +/// - `$rust_type`: Each item's rust wrapper type.
>> +/// - `$c_type`: Each item's C struct type that contains the embedded `list_head`.
>> +/// - `$field`: The name of the `list_head` field within the C struct.
>> +///
>> +/// # Safety
>> +///
>> +/// The caller must ensure:
>> +/// - `$head` is a valid, initialized sentinel `list_head` pointing to a list that remains
>> +/// unmodified for the lifetime of the rust [`CList`].
>> +/// - The list contains items of type `$c_type` linked via an embedded `$field`.
>> +/// - `$rust_type` is `#[repr(transparent)]` over `$c_type` or has compatible layout.
>> +/// - The macro is called from an unsafe block.
>
> This is not a safe requirement, probably lift it up and say "This is an unsafe
> macro.".
Sure, so like this then:
/// This is an unsafe macro. The caller must ensure:
/// - `$head` is a valid, initialized sentinel `list_head`...
>> +///
>> +/// # Examples
>> +///
>> +/// Refer to the examples in the [`crate::clist`] module documentation.
>> +#[macro_export]
>> +macro_rules! clist_create {
>> + ($head:expr, $rust_type:ty, $c_type:ty, $($field:tt).+) => {{
>> + // Compile-time check that field path is a list_head.
>> + let _: fn(*const $c_type) -> *const $crate::bindings::list_head =
>> + |p| ::core::ptr::addr_of!((*p).$($field).+);
>
> `&raw const` is preferred now.
Sure, will fix.
>
>> +
>> + // Calculate offset and create `CList`.
>> + const OFFSET: usize = ::core::mem::offset_of!($c_type, $($field).+);
>> + $crate::clist::CList::<$rust_type, OFFSET>::from_raw($head)
>> + }};
>> +}
>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>> index f812cf120042..cd7e6a1055b0 100644
>> --- a/rust/kernel/lib.rs
>> +++ b/rust/kernel/lib.rs
>> @@ -75,6 +75,7 @@
>> pub mod bug;
>> #[doc(hidden)]
>> pub mod build_assert;
>> +pub mod clist;
>
> Can we keep this pub(crate)?
Yes, will do.
--
Joel Fernandes
^ permalink raw reply
* Re: [PATCH RFC v6 01/26] rust: clist: Add support to interface with C linked lists
From: Gary Guo @ 2026-01-21 20:36 UTC (permalink / raw)
To: Joel Fernandes, Gary Guo, linux-kernel
Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Jonathan Corbet, Alex Deucher,
Christian König, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
Tvrtko Ursulin, Huang Rui, Matthew Auld, Matthew Brost,
Lucas De Marchi, Thomas Hellström, Helge Deller,
Danilo Krummrich, Alice Ryhl, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, John Hubbard, Alistair Popple, Timur Tabi,
Edwin Peer, Alexandre Courbot, Andrea Righi, Andy Ritger,
Zhi Wang, Alexey Ivanov, Balbir Singh, Philipp Stanner,
Elle Rhumsaa, Daniel Almeida, joel, nouveau, dri-devel,
rust-for-linux, linux-doc, amd-gfx, intel-gfx, intel-xe,
linux-fbdev
In-Reply-To: <01a981f1-64c7-4504-b309-45a024258fe9@nvidia.com>
On Wed Jan 21, 2026 at 7:50 PM GMT, Joel Fernandes wrote:
> Hello, Gary,
>
> On 1/20/2026 6:48 PM, Gary Guo wrote:
>> On Tue Jan 20, 2026 at 8:42 PM GMT, Joel Fernandes wrote:
>>> Add a new module `clist` for working with C's doubly circular linked
>>> lists. Provide low-level iteration over list nodes.
>>>
>>> Typed iteration over actual items is provided with a `clist_create`
>>> macro to assist in creation of the `Clist` type.
>>
>> This should read "CList".
>
> Sure, will fix.
>
>>
>> I was quite dubious about the patch just from the title (everybody knows how
>> easy a linked list is in Rust), but it turns out it is not as concerning as I
>> expected, mostly due to the read-only nature of the particular implementation
>> (a lot of the safety comments would be much more difficult to justify, say, if
>> it's mutable). That said, still a lot of feedbacks below.
>
> Sure, the reason for requiring this is interfacing with lists coming from C
> code. I'd see a future where we may want it mutable too (example, Rust code
> adding elements to the existing). At which point, the invariants/safety
> reasoning may change.
>
>> I think something like is okay in the short term. However, there's an growing
>> interest in getting our Rust list API improved, so it could be ideal if
>> eventually the Rust list can be capable of handling FFI lists, too.
>
> Yeah we looked into that, if you see old threads, the conclusion was it is not a
> good fit for existing rust list abstractions. TLDR; it does not fit into their
> ownership/borrowing model.
Definitely not with the existing one that we have, as it handles only `Arc`.
But the existing abstraction is also not good enough if you want to insert
`Box`...
>
> [...]
>>> +
>>> +/// Initialize a `list_head` object to point to itself.
>>> +///
>>> +/// # Safety
>>> +///
>>> +/// `list` must be a valid pointer to a `list_head` object.
>>> +#[inline]
>>> +pub unsafe fn init_list_head(list: *mut bindings::list_head) {
>>> + // SAFETY: Caller guarantees `list` is a valid pointer to a `list_head`.
>>> + unsafe {
>>> + (*list).next = list;
>>> + (*list).prev = list;
>>
>> This needs to be an atomic write or it'll depart from the C implementation.
>
> I am curious what you mean by atomic write, can you define it? Does rust
> compiler have load/store fusing, invented stores, etc, like C does? Sorry I am
> only familiar with these concepts on C. Could you provide example of a race
> condition in Rust that can happen?
Oh yes, this would definitely happen. It's down to LLVM to compile anyway. If
you create a reference, there'll be even more freedom to do these.
>
> Also I did this addition based on feedback from past review:
> https://lore.kernel.org/all/DEI89VUEYXAJ.1IQQPC3QRLITP@nvidia.com/
>
> There was some concerns around pointless function call overhead when the rust
> implementation is already quite intertwined with internals of the C linked list
> implementation. I do agree with that point of view too.
Overall our practice is to not duplicate code. Even `ERR_PTR` is calling into
helpers.
For performance, it's a valid concern. However Alice and I have series out there
that enable you to inline the helpers. I'd say unless there's an absolute need,
we should do the helpers. Especially with caveats like WRITE_ONCE in this case.
>
> Also see my other reply to Zhi on this helper topic, lets discuss there too, if
> that's Ok.
>
>>> + }
>>> +}
>>
>> I don't think we want to publicly expose this! I've not found a user in the
>> subsequent patch, too.
>
> There are 2 users:
>
> pub fn try_init<E>(
>
> and the self-tests:
This is not really a public user. It's hidden in the doc test too, you could
initialize using try_init too.
>
> //! # let head = head.as_mut_ptr();
> //! # // SAFETY: head and all the items are test objects allocated in [..]
> //! # unsafe { init_list_head(head) };
> //! #
>
>>
>>> +
>>> +/// Wraps a `list_head` object for use in intrusive linked lists.
>>> +///
>>> +/// # Invariants
>>> +///
>>> +/// - [`CListHead`] represents an allocated and valid `list_head` structure.
>>> +/// - Once a [`CListHead`] is created in Rust, it will not be modified by non-Rust code.
>>> +/// - All `list_head` for individual items are not modified for the lifetime of [`CListHead`].
>>> +#[repr(transparent)]
>>> +pub struct CListHead(Opaque<bindings::list_head>);
>>> +
>>> +impl CListHead {
>>> + /// Create a `&CListHead` reference from a raw `list_head` pointer.
>>> + ///
>>> + /// # Safety
>>> + ///
>>> + /// - `ptr` must be a valid pointer to an allocated and initialized `list_head` structure.
>>> + /// - `ptr` must remain valid and unmodified for the lifetime `'a`.
>>> + #[inline]
>>> + pub unsafe fn from_raw<'a>(ptr: *mut bindings::list_head) -> &'a Self {
>>> + // SAFETY:
>>> + // - [`CListHead`] has same layout as `list_head`.
>>> + // - `ptr` is valid and unmodified for 'a.
>>> + unsafe { &*ptr.cast() }
>>> + }
>>> +
>>> + /// Get the raw `list_head` pointer.
>>> + #[inline]
>>> + pub fn as_raw(&self) -> *mut bindings::list_head {
>>> + self.0.get()
>>> + }
>>> +
>>> + /// Get the next [`CListHead`] in the list.
>>> + #[inline]
>>> + pub fn next(&self) -> &Self {
>>> + let raw = self.as_raw();
>>> + // SAFETY:
>>> + // - `self.as_raw()` is valid per type invariants.
>>> + // - The `next` pointer is guaranteed to be non-NULL.
>>> + unsafe { Self::from_raw((*raw).next) }
>>> + }
>>> +
>>> + /// Get the previous [`CListHead`] in the list.
>>> + #[inline]
>>> + pub fn prev(&self) -> &Self {
>>> + let raw = self.as_raw();
>>> + // SAFETY:
>>> + // - self.as_raw() is valid per type invariants.
>>> + // - The `prev` pointer is guaranteed to be non-NULL.
>>> + unsafe { Self::from_raw((*raw).prev) }
>>> + }
>>> +
>>> + /// Check if this node is linked in a list (not isolated).
>>> + #[inline]
>>> + pub fn is_linked(&self) -> bool {
>>> + let raw = self.as_raw();
>>> + // SAFETY: self.as_raw() is valid per type invariants.
>>> + unsafe { (*raw).next != raw && (*raw).prev != raw }
>>
>> While is this checking both prev and next? `list_empty` is just
>> `READ_ONCE(head->next) == head`.
>
> Sure, I can optimize to just check ->next, that makes sense. Will do.
>
The important part is to make sure we don't deviate from C implementation. A
copy is already not good, and difference is worse.
>>
>>> + }
>>> +
>>> + /// Fallible pin-initializer that initializes and then calls user closure.
>>> + ///
>>> + /// Initializes the list head first, then passes `&CListHead` to the closure.
>>> + /// This hides the raw FFI pointer from the user.
>>> + pub fn try_init<E>(
>>> + init_func: impl FnOnce(&CListHead) -> Result<(), E>,
>>> + ) -> impl PinInit<Self, E> {
>>> + // SAFETY: init_list_head initializes the list_head to point to itself.
>>> + // After initialization, we create a reference to pass to the closure.
>>> + unsafe {
>>> + pin_init::pin_init_from_closure(move |slot: *mut Self| {
>>> + init_list_head(slot.cast());
>>> + // SAFETY: slot is now initialized, safe to create reference.
>>> + init_func(&*slot)
>>
>> Why is this callback necessary? The user can just create the list head and
>> then reference it later? I don't see what this specifically gains over just
>> doing
>>
>> fn new() -> impl PinInit<Self>;
>>
>> and have user-side
>>
>> list <- CListHead::new(),
>> _: {
>> do_want_ever(&list)
>> }
>
> The list initialization can fail, see the GPU buddy patch:
>
> // Create pin-initializer that initializes list and allocates blocks.
> let init = try_pin_init!(AllocatedBlocks {
> list <- CListHead::try_init(|list| {
> // Lock while allocating to serialize with concurrent frees.
> let guard = buddy_arc.lock();
>
> // SAFETY: guard provides exclusive access, list is initialized.
> to_result(unsafe {
> bindings::gpu_buddy_alloc_blocks(
> guard.as_raw(),
> params.start_range_address,
> params.end_range_address,
> params.size_bytes,
> params.min_block_size_bytes,
> list.as_raw(),
> params.buddy_flags.as_raw(),
> )
> })
> }),
> buddy: Arc::clone(&buddy_arc),
> flags: params.buddy_flags,
> });
The list initialization doesn't fail? It's the subsequent action you did that
failed.
You can put failing things in the `_: { ... }` block too.
>
>>
>>
>>> + })
>>> + }
>>> + }
>>> +}
>>> +
>>> +// SAFETY: [`CListHead`] can be sent to any thread.
>>> +unsafe impl Send for CListHead {}
>>> +
>>> +// SAFETY: [`CListHead`] can be shared among threads as it is not modified
>>> +// by non-Rust code per type invariants.
>>> +unsafe impl Sync for CListHead {}
>>> +
>>> +impl PartialEq for CListHead {
>>> + fn eq(&self, other: &Self) -> bool {
>>> + self.as_raw() == other.as_raw()
>>
>> Or just `core::ptr::eq(self, other)`
>
> Sure, will fix.
>
>>
>>> + }
>>> +}
>>> +
>>> +impl Eq for CListHead {}
>>> +
>>> +/// Low-level iterator over `list_head` nodes.
>>> +///
>>> +/// An iterator used to iterate over a C intrusive linked list (`list_head`). Caller has to
>>> +/// perform conversion of returned [`CListHead`] to an item (using `container_of` macro or similar).
>>> +///
>>> +/// # Invariants
>>> +///
>>> +/// [`CListHeadIter`] is iterating over an allocated, initialized and valid list.
>>> +struct CListHeadIter<'a> {
>>> + current_head: &'a CListHead,
>>> + list_head: &'a CListHead,
>>> +}
>>> +
>>> +impl<'a> Iterator for CListHeadIter<'a> {
>>> + type Item = &'a CListHead;
>>> +
>>> + #[inline]
>>> + fn next(&mut self) -> Option<Self::Item> {
>>> + // Advance to next node.
>>> + let next = self.current_head.next();
>>> +
>>> + // Check if we've circled back to the sentinel head.
>>> + if next == self.list_head {
>>> + None
>>> + } else {
>>> + self.current_head = next;
>>> + Some(self.current_head)
>>> + }
>>
>> I think this could match the C iterator behaviour. When the iterator is created,
>> a `next` is done first, and then subsequently you only need to check if
>> `current_head` is `list_head`.
>>
>> This is slightly better because the condition check does not need to dereference
>> a pointer.
>
> Sure, I can change it to that.
>>> +impl<'a> FusedIterator for CListHeadIter<'a> {}
>>> +
>>> +/// A typed C linked list with a sentinel head.
>>> +///
>>> +/// A sentinel head represents the entire linked list and can be used for
>>> +/// iteration over items of type `T`, it is not associated with a specific item.
>>> +///
>>> +/// The const generic `OFFSET` specifies the byte offset of the `list_head` field within
>>> +/// the struct that `T` wraps.
>>> +///
>>> +/// # Invariants
>>> +///
>>> +/// - `head` is an allocated and valid C `list_head` structure that is the list's sentinel.
>>> +/// - `OFFSET` is the byte offset of the `list_head` field within the struct that `T` wraps.
>>> +/// - All the list's `list_head` nodes are allocated and have valid next/prev pointers.
>>> +/// - The underlying `list_head` (and entire list) is not modified for the lifetime `'a`.
>>> +pub struct CList<'a, T, const OFFSET: usize> {
>>> + head: &'a CListHead,
>>> + _phantom: PhantomData<&'a T>,
>>> +}
>>
>> Is there a reason that this is not
>>
>> #[repr(transparent)]
>> struct CList(CListHead)
>>
>> ? We typically want to avoid putting reference inside the struct if it can be on
>> the outside. This allows `&self` to be a single level of reference, not too.
>>
>> It also means that you can just write `&CList<_>` in many cases, and doesn't need
>> `CList<'_, T>` (plus all the benefits of a reference).
>
> Sure! Will change to this. I am guessing you mean the following, but please let
> me know if you meant something else:
>
> pub struct CList<T, const OFFSET: usize>(
> CListHead,
> PhantomData<T>,
> );
>
> I don't see any issues with my code using that, at the moment. Will let you know
> how it goes.
Yes, with `#[repr(transparent)]`.
>>> +impl<'a, T, const OFFSET: usize> CList<'a, T, OFFSET> {
>>> + /// Create a typed [`CList`] from a raw sentinel `list_head` pointer.
>>> + ///
>>> + /// # Safety
>>> + ///
>>> + /// - `ptr` must be a valid pointer to an allocated and initialized `list_head` structure
>>> + /// representing a list sentinel.
>>> + /// - `ptr` must remain valid and unmodified for the lifetime `'a`.
>>> + /// - The list must contain items where the `list_head` field is at byte offset `OFFSET`.
>>> + /// - `T` must be `#[repr(transparent)]` over the C struct.
>>> + #[inline]
>>> + pub unsafe fn from_raw(ptr: *mut bindings::list_head) -> Self {
>>> + Self {
>>> + // SAFETY: Caller guarantees `ptr` is a valid, sentinel `list_head` object.
>>> + head: unsafe { CListHead::from_raw(ptr) },
>>> + _phantom: PhantomData,
>>> + }
>>> + }
>>> +
>>> + /// Get the raw sentinel `list_head` pointer.
>>> + #[inline]
>>> + pub fn as_raw(&self) -> *mut bindings::list_head {
>>> + self.head.as_raw()
>>> + }
>>> +
>>> + /// Check if the list is empty.
>>> + #[inline]
>>> + pub fn is_empty(&self) -> bool {
>>> + let raw = self.as_raw();
>>> + // SAFETY: self.as_raw() is valid per type invariants.
>>> + unsafe { (*raw).next == raw }
>>
>> `self.head.is_linked()`?
>
> I'd considered `is_linked()` to be something that makes sense to call only on
> `ClistHead` objects that belong to a particular "item" node, not a sentinel
> node, so that was deliberate.
>
> Though, I am Ok with doing it the way you are suggesting too
> (`self.head.is_linked()`), since it is functionally equivalent.
>
>>> + }
>>> +
>>> + /// Create an iterator over typed items.
>>> + #[inline]
>>> + pub fn iter(&self) -> CListIter<'a, T, OFFSET> {
>>> + CListIter {
>>> + head_iter: CListHeadIter {
>>> + current_head: self.head,
>>> + list_head: self.head,
>>> + },
>>> + _phantom: PhantomData,
>>> + }
>>> + }
>>> +}
>>> +
>>> +/// High-level iterator over typed list items.
>>> +pub struct CListIter<'a, T, const OFFSET: usize> {
>>> + head_iter: CListHeadIter<'a>,
>>> + _phantom: PhantomData<&'a T>,
>>> +}
>>> +
>>> +impl<'a, T, const OFFSET: usize> Iterator for CListIter<'a, T, OFFSET> {
>>> + type Item = &'a T;
>>> +
>>> + fn next(&mut self) -> Option<Self::Item> {
>>> + let head = self.head_iter.next()?;
>>> +
>>> + // Convert to item using OFFSET.
>>> + // SAFETY: `item_ptr` calculation from `OFFSET` (calculated using offset_of!)
>>> + // is valid per invariants.
>>> + Some(unsafe { &*head.as_raw().byte_sub(OFFSET).cast::<T>() })
>>> + }
>>> +}
>>> +
>>> +impl<'a, T, const OFFSET: usize> FusedIterator for CListIter<'a, T, OFFSET> {}
>>> +
>>> +/// Create a C doubly-circular linked list interface [`CList`] from a raw `list_head` pointer.
>>> +///
>>> +/// This macro creates a [`CList<T, OFFSET>`] that can iterate over items of type `$rust_type`
>>> +/// linked via the `$field` field in the underlying C struct `$c_type`.
>>> +///
>>> +/// # Arguments
>>> +///
>>> +/// - `$head`: Raw pointer to the sentinel `list_head` object (`*mut bindings::list_head`).
>>> +/// - `$rust_type`: Each item's rust wrapper type.
>>> +/// - `$c_type`: Each item's C struct type that contains the embedded `list_head`.
>>> +/// - `$field`: The name of the `list_head` field within the C struct.
>>> +///
>>> +/// # Safety
>>> +///
>>> +/// The caller must ensure:
>>> +/// - `$head` is a valid, initialized sentinel `list_head` pointing to a list that remains
>>> +/// unmodified for the lifetime of the rust [`CList`].
>>> +/// - The list contains items of type `$c_type` linked via an embedded `$field`.
>>> +/// - `$rust_type` is `#[repr(transparent)]` over `$c_type` or has compatible layout.
>>> +/// - The macro is called from an unsafe block.
>>
>> This is not a safe requirement, probably lift it up and say "This is an unsafe
>> macro.".
>
> Sure, so like this then:
> /// This is an unsafe macro. The caller must ensure:
> /// - `$head` is a valid, initialized sentinel `list_head`...
Yes.
Best,
Gary
>
>>> +///
>>> +/// # Examples
>>> +///
>>> +/// Refer to the examples in the [`crate::clist`] module documentation.
>>> +#[macro_export]
>>> +macro_rules! clist_create {
>>> + ($head:expr, $rust_type:ty, $c_type:ty, $($field:tt).+) => {{
>>> + // Compile-time check that field path is a list_head.
>>> + let _: fn(*const $c_type) -> *const $crate::bindings::list_head =
>>> + |p| ::core::ptr::addr_of!((*p).$($field).+);
>>
>> `&raw const` is preferred now.
>
> Sure, will fix.
>
>>
>>> +
>>> + // Calculate offset and create `CList`.
>>> + const OFFSET: usize = ::core::mem::offset_of!($c_type, $($field).+);
>>> + $crate::clist::CList::<$rust_type, OFFSET>::from_raw($head)
>>> + }};
>>> +}
>>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>>> index f812cf120042..cd7e6a1055b0 100644
>>> --- a/rust/kernel/lib.rs
>>> +++ b/rust/kernel/lib.rs
>>> @@ -75,6 +75,7 @@
>>> pub mod bug;
>>> #[doc(hidden)]
>>> pub mod build_assert;
>>> +pub mod clist;
>>
>> Can we keep this pub(crate)?
>
> Yes, will do.
^ permalink raw reply
* Re: [PATCH RFC v6 01/26] rust: clist: Add support to interface with C linked lists
From: Joel Fernandes @ 2026-01-21 20:41 UTC (permalink / raw)
To: Gary Guo, linux-kernel
Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Jonathan Corbet, Alex Deucher,
Christian König, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
Tvrtko Ursulin, Huang Rui, Matthew Auld, Matthew Brost,
Lucas De Marchi, Thomas Hellström, Helge Deller,
Danilo Krummrich, Alice Ryhl, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, John Hubbard, Alistair Popple, Timur Tabi,
Edwin Peer, Alexandre Courbot, Andrea Righi, Andy Ritger,
Zhi Wang, Alexey Ivanov, Balbir Singh, Philipp Stanner,
Elle Rhumsaa, Daniel Almeida, joel, nouveau, dri-devel,
rust-for-linux, linux-doc, amd-gfx, intel-gfx, intel-xe,
linux-fbdev
In-Reply-To: <DFUK089V1IEU.U83YQT72BO3@garyguo.net>
On 1/21/2026 3:36 PM, Gary Guo wrote:
>> There are 2 users:
>>
>> pub fn try_init<E>(
>>
>> and the self-tests:
> This is not really a public user. It's hidden in the doc test too, you could
> initialize using try_init too.
>
>> //! # let head = head.as_mut_ptr();
>> //! # // SAFETY: head and all the items are test objects allocated in [..]
>> //! # unsafe { init_list_head(head) };
>> //! #
True, but if we initialize purely within try_init() without using a helper, does
that not defeat the argument of adding a separate INIT_LIST_HEAD helper such
that we don't deviate from the C side?
Regarding your other comment about the try_init block itself, I will take a look
at your suggestion and see if I can simplify.
--
Joel Fernandes
^ permalink raw reply
* Re: [PATCH RFC v6 01/26] rust: clist: Add support to interface with C linked lists
From: Joel Fernandes @ 2026-01-21 20:46 UTC (permalink / raw)
To: Gary Guo, linux-kernel
Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Jonathan Corbet, Alex Deucher,
Christian König, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
Tvrtko Ursulin, Huang Rui, Matthew Auld, Matthew Brost,
Lucas De Marchi, Thomas Hellström, Helge Deller,
Danilo Krummrich, Alice Ryhl, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, John Hubbard, Alistair Popple, Timur Tabi,
Edwin Peer, Alexandre Courbot, Andrea Righi, Andy Ritger,
Zhi Wang, Alexey Ivanov, Balbir Singh, Philipp Stanner,
Elle Rhumsaa, Daniel Almeida, joel, nouveau, dri-devel,
rust-for-linux, linux-doc, amd-gfx, intel-gfx, intel-xe,
linux-fbdev
In-Reply-To: <DFUK089V1IEU.U83YQT72BO3@garyguo.net>
On 1/21/2026 3:36 PM, Gary Guo wrote:
>> [...]
>>>> +
>>>> +/// Initialize a `list_head` object to point to itself.
>>>> +///
>>>> +/// # Safety
>>>> +///
>>>> +/// `list` must be a valid pointer to a `list_head` object.
>>>> +#[inline]
>>>> +pub unsafe fn init_list_head(list: *mut bindings::list_head) {
>>>> + // SAFETY: Caller guarantees `list` is a valid pointer to a `list_head`.
>>>> + unsafe {
>>>> + (*list).next = list;
>>>> + (*list).prev = list;
>>>
>>> This needs to be an atomic write or it'll depart from the C implementation.
>> I am curious what you mean by atomic write, can you define it? Does rust
>> compiler have load/store fusing, invented stores, etc, like C does? Sorry I am
>> only familiar with these concepts on C. Could you provide example of a race
>> condition in Rust that can happen?
>
> Oh yes, this would definitely happen. It's down to LLVM to compile anyway. If
> you create a reference, there'll be even more freedom to do these.
>
Ok.
>> Also I did this addition based on feedback from past review:
>> https://lore.kernel.org/all/DEI89VUEYXAJ.1IQQPC3QRLITP@nvidia.com/
>>
>> There was some concerns around pointless function call overhead when the rust
>> implementation is already quite intertwined with internals of the C linked list
>> implementation. I do agree with that point of view too.
>
> Overall our practice is to not duplicate code. Even `ERR_PTR` is calling into
> helpers.
>
> For performance, it's a valid concern. However Alice and I have series out there
> that enable you to inline the helpers. I'd say unless there's an absolute need,
> we should do the helpers. Especially with caveats like WRITE_ONCE in this case.
Sounds good, so I will then go back to adding a INIT_LIST_HEAD C helper for the
next spin. I agree with the suggestion and now that we are inlining helpers,
there seems little point in adding a separate rust function to do the same.
--
Joel Fernandes
^ permalink raw reply
* [PATCH v8 1/2] staging: fbtft: Fix build failure when CONFIG_FB_DEVICE=n
From: Chintan Patel @ 2026-01-22 3:16 UTC (permalink / raw)
To: linux-fbdev, linux-staging, linux-omap
Cc: linux-kernel, dri-devel, tzimmermann, andy, deller, gregkh,
Chintan Patel, kernel test robot, Andy Shevchenko
When CONFIG_FB_DEVICE is disabled, struct fb_info does
not provide a valid dev pointer. Direct dereferences of
fb_info->dev therefore result in build failures.
Fix this by avoiding direct accesses to fb_info->dev and
switching the affected debug logging to framebuffer helpers
that do not rely on a device pointer.
This fixes the following build failure reported by the
kernel test robot.
Fixes: a06d03f9f238 ("staging: fbtft: Make FB_DEVICE dependency optional")
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202601110740.Y9XK5HtN-lkp@intel.com
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Chintan Patel <chintanlike@gmail.com>
---
Changes in v8:
- Add Reviewed-by tag from Andy Shevchenko.
Changes in v7:
- Split logging cleanups into a separate patch
- Limit this patch to the CONFIG_FB_DEVICE=n build fix only
Changes in v6:
- Switch debug/info logging to fb_dbg() and fb_info()(suggested by Thomas Zimmermann)
- Drop dev_of_fbinfo() usage in favor of framebuffer helpers that implicitly
handle the debug/info context.
- Drop __func__ usage per review feedback(suggested by greg k-h)
- Add Fixes tag for a06d03f9f238 ("staging: fbtft: Make FB_DEVICE dependency optional")
(suggested by Andy Shevchenko)
Changes in v5:
- Initial attempt to replace info->dev accesses using
dev_of_fbinfo() helper
drivers/staging/fbtft/fbtft-core.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
index 8a5ccc8ae0a1..1b3b62950205 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c
@@ -365,9 +365,9 @@ static int fbtft_fb_setcolreg(unsigned int regno, unsigned int red,
unsigned int val;
int ret = 1;
- dev_dbg(info->dev,
- "%s(regno=%u, red=0x%X, green=0x%X, blue=0x%X, trans=0x%X)\n",
- __func__, regno, red, green, blue, transp);
+ fb_dbg(info,
+ "regno=%u, red=0x%X, green=0x%X, blue=0x%X, trans=0x%X\n",
+ regno, red, green, blue, transp);
switch (info->fix.visual) {
case FB_VISUAL_TRUECOLOR:
@@ -391,8 +391,7 @@ static int fbtft_fb_blank(int blank, struct fb_info *info)
struct fbtft_par *par = info->par;
int ret = -EINVAL;
- dev_dbg(info->dev, "%s(blank=%d)\n",
- __func__, blank);
+ fb_dbg(info, "blank=%d\n", blank);
if (!par->fbtftops.blank)
return ret;
@@ -793,11 +792,11 @@ int fbtft_register_framebuffer(struct fb_info *fb_info)
if (spi)
sprintf(text2, ", spi%d.%d at %d MHz", spi->controller->bus_num,
spi_get_chipselect(spi, 0), spi->max_speed_hz / 1000000);
- dev_info(fb_info->dev,
- "%s frame buffer, %dx%d, %d KiB video memory%s, fps=%lu%s\n",
- fb_info->fix.id, fb_info->var.xres, fb_info->var.yres,
- fb_info->fix.smem_len >> 10, text1,
- HZ / fb_info->fbdefio->delay, text2);
+ fb_info(fb_info,
+ "%s frame buffer, %dx%d, %d KiB video memory%s, fps=%lu%s\n",
+ fb_info->fix.id, fb_info->var.xres, fb_info->var.yres,
+ fb_info->fix.smem_len >> 10, text1,
+ HZ / fb_info->fbdefio->delay, text2);
/* Turn on backlight if available */
if (fb_info->bl_dev) {
--
2.43.0
^ permalink raw reply related
* [PATCH v8 2/2] staging: fbtft: Make framebuffer registration message debug-only
From: Chintan Patel @ 2026-01-22 3:16 UTC (permalink / raw)
To: linux-fbdev, linux-staging, linux-omap
Cc: linux-kernel, dri-devel, tzimmermann, andy, deller, gregkh,
Chintan Patel, Andy Shevchenko
In-Reply-To: <20260122031635.11414-1-chintanlike@gmail.com>
The framebuffer registration message is informational only and not
useful during normal operation. Convert it to debug-level logging to
keep the driver quiet when working correctly.
Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Chintan Patel <chintanlike@gmail.com>
---
Changes in v8:
- Add Reviewed-by tag from Andy Shevchenko
- Add Suggested-by tag from Greg Kroah-Hartman
drivers/staging/fbtft/fbtft-core.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
index 1b3b62950205..f427c0914907 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c
@@ -792,11 +792,11 @@ int fbtft_register_framebuffer(struct fb_info *fb_info)
if (spi)
sprintf(text2, ", spi%d.%d at %d MHz", spi->controller->bus_num,
spi_get_chipselect(spi, 0), spi->max_speed_hz / 1000000);
- fb_info(fb_info,
- "%s frame buffer, %dx%d, %d KiB video memory%s, fps=%lu%s\n",
- fb_info->fix.id, fb_info->var.xres, fb_info->var.yres,
- fb_info->fix.smem_len >> 10, text1,
- HZ / fb_info->fbdefio->delay, text2);
+ fb_dbg(fb_info,
+ "%s frame buffer, %dx%d, %d KiB video memory%s, fps=%lu%s\n",
+ fb_info->fix.id, fb_info->var.xres, fb_info->var.yres,
+ fb_info->fix.smem_len >> 10, text1,
+ HZ / fb_info->fbdefio->delay, text2);
/* Turn on backlight if available */
if (fb_info->bl_dev) {
--
2.43.0
^ permalink raw reply related
* [PATCH 0/4] fbdev: defio: Protect against device/module removal
From: Thomas Zimmermann @ 2026-01-22 13:08 UTC (permalink / raw)
To: deller, simona, jayalk; +Cc: linux-fbdev, dri-devel, Thomas Zimmermann
There's a long-standing bug in defio where the framebuffer device or
module gets removed while mmap'ed areas of the framebuffer memory
persists in userspace. Page faults in the area then operate on defined
state.
Patches 1 and 2 fix these problems. Patches 3 and 4 build upon the fix
and put defio state into the correct places.
Thomas Zimmermann (4):
fbdev: defio: Disconnect deferred I/O from the lifetime of struct
fb_info
fbdev: defio: Keep module reference from VMAs
fbdev: defio: Move variable state into struct fb_deferred_io_state
fbdev: defio: Move pageref array to struct fb_deferred_io_state
drivers/video/fbdev/core/fb_defio.c | 266 ++++++++++++++++++++--------
include/linux/fb.h | 9 +-
2 files changed, 195 insertions(+), 80 deletions(-)
base-commit: a3ecd278f9a05323fab7471760a7ea10081251d6
--
2.52.0
^ permalink raw reply
* [PATCH 1/4] fbdev: defio: Disconnect deferred I/O from the lifetime of struct fb_info
From: Thomas Zimmermann @ 2026-01-22 13:08 UTC (permalink / raw)
To: deller, simona, jayalk; +Cc: linux-fbdev, dri-devel, Thomas Zimmermann, stable
In-Reply-To: <20260122131213.992810-1-tzimmermann@suse.de>
Hold state of deferred I/O in struct fb_deferred_io_state. Allocate an
instance as part of initializing deferred I/O and remove it only after
the final mapping has been closed. If the fb_info and the contained
deferred I/O meanwhile goes away, clear struct fb_deferred_io_state.info
to invalidate the mapping. Any access will then result in a SIGBUS
signal.
Fixes a long-standing problem, where a device hot-unplug happens while
user space still has an active mapping of the graphics memory. The hot-
unplug frees the instance of struct fb_info. Accessing the memory will
operate on undefined state.
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: 60b59beafba8 ("fbdev: mm: Deferred IO support")
Cc: Helge Deller <deller@gmx.de>
Cc: linux-fbdev@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v2.6.22+
---
drivers/video/fbdev/core/fb_defio.c | 178 ++++++++++++++++++++++------
include/linux/fb.h | 4 +-
2 files changed, 145 insertions(+), 37 deletions(-)
diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
index 8df2e51e3390..0b099a89a823 100644
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c
@@ -24,6 +24,75 @@
#include <linux/rmap.h>
#include <linux/pagemap.h>
+/*
+ * struct fb_deferred_io_state
+ */
+
+struct fb_deferred_io_state {
+ struct kref ref;
+
+ struct mutex lock; /* mutex that protects the pageref list */
+ /* fields protected by lock */
+ struct fb_info *info;
+};
+
+static struct fb_deferred_io_state *fb_deferred_io_state_alloc(void)
+{
+ struct fb_deferred_io_state *fbdefio_state;
+
+ fbdefio_state = kzalloc(sizeof(*fbdefio_state), GFP_KERNEL);
+ if (!fbdefio_state)
+ return NULL;
+
+ kref_init(&fbdefio_state->ref);
+ mutex_init(&fbdefio_state->lock);
+
+ return fbdefio_state;
+}
+
+static void fb_deferred_io_state_release(struct fb_deferred_io_state *fbdefio_state)
+{
+ mutex_destroy(&fbdefio_state->lock);
+
+ kfree(fbdefio_state);
+}
+
+static void fb_deferred_io_state_get(struct fb_deferred_io_state *fbdefio_state)
+{
+ kref_get(&fbdefio_state->ref);
+}
+
+static void __fb_deferred_io_state_release(struct kref *ref)
+{
+ struct fb_deferred_io_state *fbdefio_state =
+ container_of(ref, struct fb_deferred_io_state, ref);
+
+ fb_deferred_io_state_release(fbdefio_state);
+}
+
+static void fb_deferred_io_state_put(struct fb_deferred_io_state *fbdefio_state)
+{
+ kref_put(&fbdefio_state->ref, __fb_deferred_io_state_release);
+}
+
+/*
+ * struct vm_operations_struct
+ */
+
+static void fb_deferred_io_vm_open(struct vm_area_struct *vma)
+{
+ struct fb_deferred_io_state *fbdefio_state = vma->vm_private_data;
+
+ fb_deferred_io_state_get(fbdefio_state);
+}
+
+static void fb_deferred_io_vm_close(struct vm_area_struct *vma)
+{
+ struct fb_deferred_io_state *fbdefio_state = vma->vm_private_data;
+
+ fb_deferred_io_state_put(fbdefio_state);
+}
+
static struct page *fb_deferred_io_get_page(struct fb_info *info, unsigned long offs)
{
struct fb_deferred_io *fbdefio = info->fbdefio;
@@ -121,25 +190,46 @@ static void fb_deferred_io_pageref_put(struct fb_deferred_io_pageref *pageref,
/* this is to find and return the vmalloc-ed fb pages */
static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
{
+ struct fb_info *info;
unsigned long offset;
struct page *page;
- struct fb_info *info = vmf->vma->vm_private_data;
+ vm_fault_t ret;
+ struct fb_deferred_io_state *fbdefio_state = vmf->vma->vm_private_data;
+
+ mutex_lock(&fbdefio_state->lock);
+
+ info = fbdefio_state->info;
+ if (!info) {
+ ret = VM_FAULT_SIGBUS; /* our device is gone */
+ goto err_mutex_unlock;
+ }
offset = vmf->pgoff << PAGE_SHIFT;
- if (offset >= info->fix.smem_len)
- return VM_FAULT_SIGBUS;
+ if (offset >= info->fix.smem_len) {
+ ret = VM_FAULT_SIGBUS;
+ goto err_mutex_unlock;
+ }
page = fb_deferred_io_get_page(info, offset);
- if (!page)
- return VM_FAULT_SIGBUS;
+ if (!page) {
+ ret = VM_FAULT_SIGBUS;
+ goto err_mutex_unlock;
+ }
if (!vmf->vma->vm_file)
fb_err(info, "no mapping available\n");
BUG_ON(!info->fbdefio->mapping);
+ mutex_unlock(&fbdefio_state->lock);
+
vmf->page = page;
+
return 0;
+
+err_mutex_unlock:
+ mutex_unlock(&fbdefio_state->lock);
+ return ret;
}
int fb_deferred_io_fsync(struct file *file, loff_t start, loff_t end, int datasync)
@@ -166,15 +256,24 @@ EXPORT_SYMBOL_GPL(fb_deferred_io_fsync);
* Adds a page to the dirty list. Call this from struct
* vm_operations_struct.page_mkwrite.
*/
-static vm_fault_t fb_deferred_io_track_page(struct fb_info *info, unsigned long offset,
- struct page *page)
+static vm_fault_t fb_deferred_io_track_page(struct fb_deferred_io_state *fbdefio_state,
+ unsigned long offset, struct page *page)
{
- struct fb_deferred_io *fbdefio = info->fbdefio;
+ struct fb_info *info;
+ struct fb_deferred_io *fbdefio;
struct fb_deferred_io_pageref *pageref;
vm_fault_t ret;
/* protect against the workqueue changing the page list */
- mutex_lock(&fbdefio->lock);
+ mutex_lock(&fbdefio_state->lock);
+
+ info = fbdefio_state->info;
+ if (!info) {
+ ret = VM_FAULT_SIGBUS; /* our device is gone */
+ goto err_mutex_unlock;
+ }
+
+ fbdefio = info->fbdefio;
pageref = fb_deferred_io_pageref_get(info, offset, page);
if (WARN_ON_ONCE(!pageref)) {
@@ -192,50 +291,38 @@ static vm_fault_t fb_deferred_io_track_page(struct fb_info *info, unsigned long
*/
lock_page(pageref->page);
- mutex_unlock(&fbdefio->lock);
+ mutex_unlock(&fbdefio_state->lock);
/* come back after delay to process the deferred IO */
schedule_delayed_work(&info->deferred_work, fbdefio->delay);
return VM_FAULT_LOCKED;
err_mutex_unlock:
- mutex_unlock(&fbdefio->lock);
+ mutex_unlock(&fbdefio_state->lock);
return ret;
}
-/*
- * fb_deferred_io_page_mkwrite - Mark a page as written for deferred I/O
- * @fb_info: The fbdev info structure
- * @vmf: The VM fault
- *
- * This is a callback we get when userspace first tries to
- * write to the page. We schedule a workqueue. That workqueue
- * will eventually mkclean the touched pages and execute the
- * deferred framebuffer IO. Then if userspace touches a page
- * again, we repeat the same scheme.
- *
- * Returns:
- * VM_FAULT_LOCKED on success, or a VM_FAULT error otherwise.
- */
-static vm_fault_t fb_deferred_io_page_mkwrite(struct fb_info *info, struct vm_fault *vmf)
+static vm_fault_t fb_deferred_io_page_mkwrite(struct fb_deferred_io_state *fbdefio_state,
+ struct vm_fault *vmf)
{
unsigned long offset = vmf->pgoff << PAGE_SHIFT;
struct page *page = vmf->page;
file_update_time(vmf->vma->vm_file);
- return fb_deferred_io_track_page(info, offset, page);
+ return fb_deferred_io_track_page(fbdefio_state, offset, page);
}
-/* vm_ops->page_mkwrite handler */
static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
{
- struct fb_info *info = vmf->vma->vm_private_data;
+ struct fb_deferred_io_state *fbdefio_state = vmf->vma->vm_private_data;
- return fb_deferred_io_page_mkwrite(info, vmf);
+ return fb_deferred_io_page_mkwrite(fbdefio_state, vmf);
}
static const struct vm_operations_struct fb_deferred_io_vm_ops = {
+ .open = fb_deferred_io_vm_open,
+ .close = fb_deferred_io_vm_close,
.fault = fb_deferred_io_fault,
.page_mkwrite = fb_deferred_io_mkwrite,
};
@@ -252,7 +339,10 @@ int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
vm_flags_set(vma, VM_DONTEXPAND | VM_DONTDUMP);
if (!(info->flags & FBINFO_VIRTFB))
vm_flags_set(vma, VM_IO);
- vma->vm_private_data = info;
+ vma->vm_private_data = info->fbdefio_state;
+
+ fb_deferred_io_state_get(info->fbdefio_state); /* released in vma->vm_ops->close() */
+
return 0;
}
EXPORT_SYMBOL_GPL(fb_deferred_io_mmap);
@@ -263,9 +353,10 @@ static void fb_deferred_io_work(struct work_struct *work)
struct fb_info *info = container_of(work, struct fb_info, deferred_work.work);
struct fb_deferred_io_pageref *pageref, *next;
struct fb_deferred_io *fbdefio = info->fbdefio;
+ struct fb_deferred_io_state *fbdefio_state = info->fbdefio_state;
/* here we wrprotect the page's mappings, then do all deferred IO. */
- mutex_lock(&fbdefio->lock);
+ mutex_lock(&fbdefio_state->lock);
#ifdef CONFIG_MMU
list_for_each_entry(pageref, &fbdefio->pagereflist, list) {
struct page *page = pageref->page;
@@ -283,12 +374,13 @@ static void fb_deferred_io_work(struct work_struct *work)
list_for_each_entry_safe(pageref, next, &fbdefio->pagereflist, list)
fb_deferred_io_pageref_put(pageref, info);
- mutex_unlock(&fbdefio->lock);
+ mutex_unlock(&fbdefio_state->lock);
}
int fb_deferred_io_init(struct fb_info *info)
{
struct fb_deferred_io *fbdefio = info->fbdefio;
+ struct fb_deferred_io_state *fbdefio_state;
struct fb_deferred_io_pageref *pagerefs;
unsigned long npagerefs;
int ret;
@@ -298,7 +390,11 @@ int fb_deferred_io_init(struct fb_info *info)
if (WARN_ON(!info->fix.smem_len))
return -EINVAL;
- mutex_init(&fbdefio->lock);
+ fbdefio_state = fb_deferred_io_state_alloc();
+ if (!fbdefio_state)
+ return -ENOMEM;
+ fbdefio_state->info = info;
+
INIT_DELAYED_WORK(&info->deferred_work, fb_deferred_io_work);
INIT_LIST_HEAD(&fbdefio->pagereflist);
if (fbdefio->delay == 0) /* set a default of 1 s */
@@ -315,10 +411,12 @@ int fb_deferred_io_init(struct fb_info *info)
info->npagerefs = npagerefs;
info->pagerefs = pagerefs;
+ info->fbdefio_state = fbdefio_state;
+
return 0;
err:
- mutex_destroy(&fbdefio->lock);
+ fb_deferred_io_state_release(fbdefio_state);
return ret;
}
EXPORT_SYMBOL_GPL(fb_deferred_io_init);
@@ -352,11 +450,19 @@ EXPORT_SYMBOL_GPL(fb_deferred_io_release);
void fb_deferred_io_cleanup(struct fb_info *info)
{
struct fb_deferred_io *fbdefio = info->fbdefio;
+ struct fb_deferred_io_state *fbdefio_state = info->fbdefio_state;
fb_deferred_io_lastclose(info);
+ info->fbdefio_state = NULL;
+
+ mutex_lock(&fbdefio_state->lock);
+ fbdefio_state->info = NULL;
+ mutex_unlock(&fbdefio_state->lock);
+
+ fb_deferred_io_state_put(fbdefio_state);
+
kvfree(info->pagerefs);
- mutex_destroy(&fbdefio->lock);
fbdefio->mapping = NULL;
}
EXPORT_SYMBOL_GPL(fb_deferred_io_cleanup);
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 65fb70382675..0bf3f1a5cf1e 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -217,13 +217,14 @@ struct fb_deferred_io {
unsigned long delay;
bool sort_pagereflist; /* sort pagelist by offset */
int open_count; /* number of opened files; protected by fb_info lock */
- struct mutex lock; /* mutex that protects the pageref list */
struct list_head pagereflist; /* list of pagerefs for touched pages */
struct address_space *mapping; /* page cache object for fb device */
/* callback */
struct page *(*get_page)(struct fb_info *info, unsigned long offset);
void (*deferred_io)(struct fb_info *info, struct list_head *pagelist);
};
+
+struct fb_deferred_io_state;
#endif
/*
@@ -486,6 +487,7 @@ struct fb_info {
unsigned long npagerefs;
struct fb_deferred_io_pageref *pagerefs;
struct fb_deferred_io *fbdefio;
+ struct fb_deferred_io_state *fbdefio_state;
#endif
const struct fb_ops *fbops;
--
2.52.0
^ permalink raw reply related
* [PATCH 3/4] fbdev: defio: Move variable state into struct fb_deferred_io_state
From: Thomas Zimmermann @ 2026-01-22 13:08 UTC (permalink / raw)
To: deller, simona, jayalk; +Cc: linux-fbdev, dri-devel, Thomas Zimmermann
In-Reply-To: <20260122131213.992810-1-tzimmermann@suse.de>
Move variable fields from struct fb_deferred_io into struct
fb_deferred_io_state. These fields are internal to the defio code
and should not be exposed to drivers. At some later point, struct
fb_defered_io might become const in all defio code.
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
drivers/video/fbdev/core/fb_defio.c | 37 +++++++++++++++++------------
include/linux/fb.h | 3 ---
2 files changed, 22 insertions(+), 18 deletions(-)
diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
index 331b1a9fe485..c6945b4422cc 100644
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c
@@ -25,6 +25,8 @@
#include <linux/rmap.h>
#include <linux/pagemap.h>
+struct address_space;
+
/*
* struct fb_deferred_io_state
*/
@@ -32,9 +34,13 @@
struct fb_deferred_io_state {
struct kref ref;
+ int open_count; /* number of opened files; protected by fb_info lock */
+ struct address_space *mapping; /* page cache object for fb device */
+
struct mutex lock; /* mutex that protects the pageref list */
/* fields protected by lock */
struct fb_info *info;
+ struct list_head pagereflist; /* list of pagerefs for touched pages */
};
static struct fb_deferred_io_state *fb_deferred_io_state_alloc(void)
@@ -48,11 +54,14 @@ static struct fb_deferred_io_state *fb_deferred_io_state_alloc(void)
kref_init(&fbdefio_state->ref);
mutex_init(&fbdefio_state->lock);
+ INIT_LIST_HEAD(&fbdefio_state->pagereflist);
+
return fbdefio_state;
}
static void fb_deferred_io_state_release(struct fb_deferred_io_state *fbdefio_state)
{
+ WARN_ON(!list_empty(&fbdefio_state->pagereflist));
mutex_destroy(&fbdefio_state->lock);
kfree(fbdefio_state);
@@ -147,7 +156,8 @@ static struct fb_deferred_io_pageref *fb_deferred_io_pageref_get(struct fb_info
struct page *page)
{
struct fb_deferred_io *fbdefio = info->fbdefio;
- struct list_head *pos = &fbdefio->pagereflist;
+ struct fb_deferred_io_state *fbdefio_state = info->fbdefio_state;
+ struct list_head *pos = &fbdefio_state->pagereflist;
struct fb_deferred_io_pageref *pageref, *cur;
pageref = fb_deferred_io_pageref_lookup(info, offset, page);
@@ -171,7 +181,7 @@ static struct fb_deferred_io_pageref *fb_deferred_io_pageref_get(struct fb_info
* pages. If possible, drivers should try to work with
* unsorted page lists instead.
*/
- list_for_each_entry(cur, &fbdefio->pagereflist, list) {
+ list_for_each_entry(cur, &fbdefio_state->pagereflist, list) {
if (cur->offset > pageref->offset)
break;
}
@@ -222,7 +232,7 @@ static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
if (!vmf->vma->vm_file)
fb_err(info, "no mapping available\n");
- BUG_ON(!info->fbdefio->mapping);
+ fb_WARN_ON_ONCE(info, !fbdefio_state->mapping);
mutex_unlock(&fbdefio_state->lock);
@@ -364,20 +374,20 @@ static void fb_deferred_io_work(struct work_struct *work)
/* here we wrprotect the page's mappings, then do all deferred IO. */
mutex_lock(&fbdefio_state->lock);
#ifdef CONFIG_MMU
- list_for_each_entry(pageref, &fbdefio->pagereflist, list) {
+ list_for_each_entry(pageref, &fbdefio_state->pagereflist, list) {
struct page *page = pageref->page;
pgoff_t pgoff = pageref->offset >> PAGE_SHIFT;
- mapping_wrprotect_range(fbdefio->mapping, pgoff,
+ mapping_wrprotect_range(fbdefio_state->mapping, pgoff,
page_to_pfn(page), 1);
}
#endif
/* driver's callback with pagereflist */
- fbdefio->deferred_io(info, &fbdefio->pagereflist);
+ fbdefio->deferred_io(info, &fbdefio_state->pagereflist);
/* clear the list */
- list_for_each_entry_safe(pageref, next, &fbdefio->pagereflist, list)
+ list_for_each_entry_safe(pageref, next, &fbdefio_state->pagereflist, list)
fb_deferred_io_pageref_put(pageref, info);
mutex_unlock(&fbdefio_state->lock);
@@ -402,7 +412,6 @@ int fb_deferred_io_init(struct fb_info *info)
fbdefio_state->info = info;
INIT_DELAYED_WORK(&info->deferred_work, fb_deferred_io_work);
- INIT_LIST_HEAD(&fbdefio->pagereflist);
if (fbdefio->delay == 0) /* set a default of 1 s */
fbdefio->delay = HZ;
@@ -431,11 +440,11 @@ void fb_deferred_io_open(struct fb_info *info,
struct inode *inode,
struct file *file)
{
- struct fb_deferred_io *fbdefio = info->fbdefio;
+ struct fb_deferred_io_state *fbdefio_state = info->fbdefio_state;
- fbdefio->mapping = file->f_mapping;
+ fbdefio_state->mapping = file->f_mapping;
file->f_mapping->a_ops = &fb_deferred_io_aops;
- fbdefio->open_count++;
+ fbdefio_state->open_count++;
}
EXPORT_SYMBOL_GPL(fb_deferred_io_open);
@@ -446,16 +455,15 @@ static void fb_deferred_io_lastclose(struct fb_info *info)
void fb_deferred_io_release(struct fb_info *info)
{
- struct fb_deferred_io *fbdefio = info->fbdefio;
+ struct fb_deferred_io_state *fbdefio_state = info->fbdefio_state;
- if (!--fbdefio->open_count)
+ if (!--fbdefio_state->open_count)
fb_deferred_io_lastclose(info);
}
EXPORT_SYMBOL_GPL(fb_deferred_io_release);
void fb_deferred_io_cleanup(struct fb_info *info)
{
- struct fb_deferred_io *fbdefio = info->fbdefio;
struct fb_deferred_io_state *fbdefio_state = info->fbdefio_state;
fb_deferred_io_lastclose(info);
@@ -469,6 +477,5 @@ void fb_deferred_io_cleanup(struct fb_info *info)
fb_deferred_io_state_put(fbdefio_state);
kvfree(info->pagerefs);
- fbdefio->mapping = NULL;
}
EXPORT_SYMBOL_GPL(fb_deferred_io_cleanup);
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 0bf3f1a5cf1e..2a9d05e51ff4 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -216,9 +216,6 @@ struct fb_deferred_io {
/* delay between mkwrite and deferred handler */
unsigned long delay;
bool sort_pagereflist; /* sort pagelist by offset */
- int open_count; /* number of opened files; protected by fb_info lock */
- struct list_head pagereflist; /* list of pagerefs for touched pages */
- struct address_space *mapping; /* page cache object for fb device */
/* callback */
struct page *(*get_page)(struct fb_info *info, unsigned long offset);
void (*deferred_io)(struct fb_info *info, struct list_head *pagelist);
--
2.52.0
^ permalink raw reply related
* [PATCH 2/4] fbdev: defio: Keep module reference from VMAs
From: Thomas Zimmermann @ 2026-01-22 13:08 UTC (permalink / raw)
To: deller, simona, jayalk; +Cc: linux-fbdev, dri-devel, Thomas Zimmermann
In-Reply-To: <20260122131213.992810-1-tzimmermann@suse.de>
Acquire a module reference on each mmap and VMA open; hold it until
the kernel closes the VMA. Protects against unloading the module
while user space still has a mapping of the graphics memory. The
VMA page-fault handling would then call into undefined code.
This situation can happen if the underlying device has been unplugged
and the driver has been unloaded. It would then be possible to trigger
the bug by unloading the fbdev core module.
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
drivers/video/fbdev/core/fb_defio.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
index 0b099a89a823..331b1a9fe485 100644
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c
@@ -14,6 +14,7 @@
#include <linux/export.h>
#include <linux/string.h>
#include <linux/mm.h>
+#include <linux/module.h>
#include <linux/vmalloc.h>
#include <linux/delay.h>
#include <linux/interrupt.h>
@@ -83,6 +84,7 @@ static void fb_deferred_io_vm_open(struct vm_area_struct *vma)
{
struct fb_deferred_io_state *fbdefio_state = vma->vm_private_data;
+ WARN_ON_ONCE(!try_module_get(THIS_MODULE));
fb_deferred_io_state_get(fbdefio_state);
}
@@ -91,6 +93,7 @@ static void fb_deferred_io_vm_close(struct vm_area_struct *vma)
struct fb_deferred_io_state *fbdefio_state = vma->vm_private_data;
fb_deferred_io_state_put(fbdefio_state);
+ module_put(THIS_MODULE);
}
static struct page *fb_deferred_io_get_page(struct fb_info *info, unsigned long offs)
@@ -335,6 +338,9 @@ int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
{
vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
+ if (!try_module_get(THIS_MODULE))
+ return -EINVAL;
+
vma->vm_ops = &fb_deferred_io_vm_ops;
vm_flags_set(vma, VM_DONTEXPAND | VM_DONTDUMP);
if (!(info->flags & FBINFO_VIRTFB))
--
2.52.0
^ permalink raw reply related
* [PATCH 4/4] fbdev: defio: Move pageref array to struct fb_deferred_io_state
From: Thomas Zimmermann @ 2026-01-22 13:08 UTC (permalink / raw)
To: deller, simona, jayalk; +Cc: linux-fbdev, dri-devel, Thomas Zimmermann
In-Reply-To: <20260122131213.992810-1-tzimmermann@suse.de>
The pageref array stores all pageref structures for a device's defio
helpers. Move it into struct fb_deferred_io_state to not expose it to
drivers.
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
drivers/video/fbdev/core/fb_defio.c | 55 ++++++++++++++---------------
include/linux/fb.h | 2 --
2 files changed, 27 insertions(+), 30 deletions(-)
diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
index c6945b4422cc..c4be85f80d7d 100644
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c
@@ -41,28 +41,46 @@ struct fb_deferred_io_state {
/* fields protected by lock */
struct fb_info *info;
struct list_head pagereflist; /* list of pagerefs for touched pages */
+ unsigned long npagerefs;
+ struct fb_deferred_io_pageref *pagerefs;
};
-static struct fb_deferred_io_state *fb_deferred_io_state_alloc(void)
+static struct fb_deferred_io_state *fb_deferred_io_state_alloc(unsigned long len)
{
struct fb_deferred_io_state *fbdefio_state;
+ struct fb_deferred_io_pageref *pagerefs;
+ unsigned long npagerefs;
fbdefio_state = kzalloc(sizeof(*fbdefio_state), GFP_KERNEL);
if (!fbdefio_state)
return NULL;
+ npagerefs = DIV_ROUND_UP(len, PAGE_SIZE);
+
+ /* alloc a page ref for each page of the display memory */
+ pagerefs = kvcalloc(npagerefs, sizeof(*pagerefs), GFP_KERNEL);
+ if (!pagerefs)
+ goto err_kfree;
+ fbdefio_state->npagerefs = npagerefs;
+ fbdefio_state->pagerefs = pagerefs;
+
kref_init(&fbdefio_state->ref);
mutex_init(&fbdefio_state->lock);
INIT_LIST_HEAD(&fbdefio_state->pagereflist);
return fbdefio_state;
+
+err_kfree:
+ kfree(fbdefio_state);
+ return NULL;
}
static void fb_deferred_io_state_release(struct fb_deferred_io_state *fbdefio_state)
{
WARN_ON(!list_empty(&fbdefio_state->pagereflist));
mutex_destroy(&fbdefio_state->lock);
+ kvfree(fbdefio_state->pagerefs);
kfree(fbdefio_state);
}
@@ -125,18 +143,19 @@ static struct page *fb_deferred_io_get_page(struct fb_info *info, unsigned long
return page;
}
-static struct fb_deferred_io_pageref *fb_deferred_io_pageref_lookup(struct fb_info *info,
- unsigned long offset,
- struct page *page)
+static struct fb_deferred_io_pageref *
+fb_deferred_io_pageref_lookup(struct fb_deferred_io_state *fbdefio_state, unsigned long offset,
+ struct page *page)
{
+ struct fb_info *info = fbdefio_state->info;
unsigned long pgoff = offset >> PAGE_SHIFT;
struct fb_deferred_io_pageref *pageref;
- if (fb_WARN_ON_ONCE(info, pgoff >= info->npagerefs))
+ if (fb_WARN_ON_ONCE(info, pgoff >= fbdefio_state->npagerefs))
return NULL; /* incorrect allocation size */
/* 1:1 mapping between pageref and page offset */
- pageref = &info->pagerefs[pgoff];
+ pageref = &fbdefio_state->pagerefs[pgoff];
if (pageref->page)
goto out;
@@ -160,7 +179,7 @@ static struct fb_deferred_io_pageref *fb_deferred_io_pageref_get(struct fb_info
struct list_head *pos = &fbdefio_state->pagereflist;
struct fb_deferred_io_pageref *pageref, *cur;
- pageref = fb_deferred_io_pageref_lookup(info, offset, page);
+ pageref = fb_deferred_io_pageref_lookup(fbdefio_state, offset, page);
if (!pageref)
return NULL;
@@ -397,16 +416,13 @@ int fb_deferred_io_init(struct fb_info *info)
{
struct fb_deferred_io *fbdefio = info->fbdefio;
struct fb_deferred_io_state *fbdefio_state;
- struct fb_deferred_io_pageref *pagerefs;
- unsigned long npagerefs;
- int ret;
BUG_ON(!fbdefio);
if (WARN_ON(!info->fix.smem_len))
return -EINVAL;
- fbdefio_state = fb_deferred_io_state_alloc();
+ fbdefio_state = fb_deferred_io_state_alloc(info->fix.smem_len);
if (!fbdefio_state)
return -ENOMEM;
fbdefio_state->info = info;
@@ -415,24 +431,9 @@ int fb_deferred_io_init(struct fb_info *info)
if (fbdefio->delay == 0) /* set a default of 1 s */
fbdefio->delay = HZ;
- npagerefs = DIV_ROUND_UP(info->fix.smem_len, PAGE_SIZE);
-
- /* alloc a page ref for each page of the display memory */
- pagerefs = kvcalloc(npagerefs, sizeof(*pagerefs), GFP_KERNEL);
- if (!pagerefs) {
- ret = -ENOMEM;
- goto err;
- }
- info->npagerefs = npagerefs;
- info->pagerefs = pagerefs;
-
info->fbdefio_state = fbdefio_state;
return 0;
-
-err:
- fb_deferred_io_state_release(fbdefio_state);
- return ret;
}
EXPORT_SYMBOL_GPL(fb_deferred_io_init);
@@ -475,7 +476,5 @@ void fb_deferred_io_cleanup(struct fb_info *info)
mutex_unlock(&fbdefio_state->lock);
fb_deferred_io_state_put(fbdefio_state);
-
- kvfree(info->pagerefs);
}
EXPORT_SYMBOL_GPL(fb_deferred_io_cleanup);
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 2a9d05e51ff4..71e2759f3cfd 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -481,8 +481,6 @@ struct fb_info {
#ifdef CONFIG_FB_DEFERRED_IO
struct delayed_work deferred_work;
- unsigned long npagerefs;
- struct fb_deferred_io_pageref *pagerefs;
struct fb_deferred_io *fbdefio;
struct fb_deferred_io_state *fbdefio_state;
#endif
--
2.52.0
^ permalink raw reply related
* Re: [PATCH RFC v6 05/26] nova-core: mm: Add support to use PRAMIN windows to write to VRAM
From: Joel Fernandes @ 2026-01-22 23:16 UTC (permalink / raw)
To: Zhi Wang
Cc: linux-kernel, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann,
David Airlie, Simona Vetter, Jonathan Corbet, Alex Deucher,
Christian Koenig, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
Tvrtko Ursulin, Huang Rui, Matthew Auld, Matthew Brost,
Lucas De Marchi, Thomas Hellstrom, Helge Deller, Danilo Krummrich,
Alice Ryhl, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Bjorn Roy Baron, Benno Lossin, Andreas Hindborg, Trevor Gross,
John Hubbard, Alistair Popple, Timur Tabi, Edwin Peer,
Alexandre Courbot, Andrea Righi, Andy Ritger, Alexey Ivanov,
Balbir Singh, Philipp Stanner, Elle Rhumsaa, Daniel Almeida,
nouveau, dri-devel, rust-for-linux, linux-doc, amd-gfx, intel-gfx,
intel-xe, linux-fbdev
In-Reply-To: <e186973c-ce31-405a-8bfa-dc647737a666@nvidia.com>
On Wed, 21 Jan 2026 12:52:10 -0500, Joel Fernandes wrote:
> I think we can incrementally build on this series to add support for the same,
> it is not something this series directly addresses since I have spend majority
> of my time last several months making translation *work* which is itself no east
> task. This series is just preliminary based on work from last several months and
> to make BAR1 work. For instance, I kept PRAMIN simple based on feedback that we
> don't want to over complicate without fully understanding all the requirements.
> There is also additional requirements for locking design that have implications
> with DMA fencing etc, for instance.
>
> Anyway thinking out loud, I am thinking for handling concurrency at the page
> table entry level (if we ever need it), we could use per-PT spinlocks similar to
> the Linux kernel. But lets plan on how to do this properly and based on actual
> requirements.
Thanks for the discussion on concurrency, Zhi.
My plan is to make TLB and PRAMIN use immutable references in their function
calls and then implement internal locking. I've already done this for the GPU
buddy functions, so it should be doable, and we'll keep it consistent. As a
result, we will have finer-grain locking on the memory management objects
instead of requiring to globally lock a common GpuMm object. I'll plan on
doing this for v7.
Also, the PTE allocation race you mentioned is already handled by PRAMIN
serialization. Since threads must hold the PRAMIN lock to write page table
entries, concurrent writers are not possible:
Thread A: acquire PRAMIN lock
Thread A: read PDE (via PRAMIN) -> NULL
Thread A: alloc PT page, write PDE
Thread A: release PRAMIN lock
Thread B: acquire PRAMIN lock
Thread B: read PDE (via PRAMIN) -> sees A's pointer
Thread B: uses existing PT page, no allocation needed
No atomic compare-and-swap on VRAM is needed because the PRAMIN lock serializes
access. Please let me know if you had a different scenario in mind, but I think
this covers it.
Zhi, feel free to use v6 though for any testing you are doing while I
rework the locking.
--
Joel Fernandes
^ permalink raw reply
* Re: [PATCH RFC v6 05/26] nova-core: mm: Add support to use PRAMIN windows to write to VRAM
From: Zhi Wang @ 2026-01-23 10:13 UTC (permalink / raw)
To: Joel Fernandes
Cc: linux-kernel, Maarten Lankhorst, Maxime Ripard, Simona Vetter,
Jonathan Corbet, Alex Deucher, Christian Koenig, Jani Nikula,
Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin, Huang Rui,
Matthew Auld, Matthew Brost, Lucas De Marchi, Thomas Hellstrom,
Helge Deller, Danilo Krummrich, Alice Ryhl, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Bjorn Roy Baron, Benno Lossin,
Andreas Hindborg, Trevor Gross, Alistair Popple,
Alexandre Courbot, Andrea Righi, Alexey Ivanov, Philipp Stanner,
Elle Rhumsaa, Daniel Almeida, nouveau, dri-devel, rust-for-linux,
linux-doc, amd-gfx, intel-gfx, intel-xe, linux-fbdev
In-Reply-To: <DS0PR12MB6486717785F6DD14EE1F1C46A397A@DS0PR12MB6486.namprd12.prod.outlook.com>
On Thu, 22 Jan 2026 18:16:00 -0500
Joel Fernandes <joelagnelf@nvidia.com> wrote:
> On Wed, 21 Jan 2026 12:52:10 -0500, Joel Fernandes wrote:
> > I think we can incrementally build on this series to add support for
> > the same, it is not something this series directly addresses since I
> > have spend majority of my time last several months making translation
> > *work* which is itself no east task. This series is just preliminary
> > based on work from last several months and to make BAR1 work. For
> > instance, I kept PRAMIN simple based on feedback that we don't want to
> > over complicate without fully understanding all the requirements.
> > There is also additional requirements for locking design that have
> > implications with DMA fencing etc, for instance.
> >
> > Anyway thinking out loud, I am thinking for handling concurrency at
> > the page table entry level (if we ever need it), we could use per-PT
> > spinlocks similar to the Linux kernel. But lets plan on how to do this
> > properly and based on actual requirements.
>
> Thanks for the discussion on concurrency, Zhi.
>
> My plan is to make TLB and PRAMIN use immutable references in their
> function calls and then implement internal locking. I've already done
> this for the GPU buddy functions, so it should be doable, and we'll keep
> it consistent. As a result, we will have finer-grain locking on the
> memory management objects instead of requiring to globally lock a common
> GpuMm object. I'll plan on doing this for v7.
>
> Also, the PTE allocation race you mentioned is already handled by PRAMIN
> serialization. Since threads must hold the PRAMIN lock to write page
> table entries, concurrent writers are not possible:
>
> Thread A: acquire PRAMIN lock
> Thread A: read PDE (via PRAMIN) -> NULL
> Thread A: alloc PT page, write PDE
> Thread A: release PRAMIN lock
>
> Thread B: acquire PRAMIN lock
> Thread B: read PDE (via PRAMIN) -> sees A's pointer
> Thread B: uses existing PT page, no allocation needed
>
> No atomic compare-and-swap on VRAM is needed because the PRAMIN lock
> serializes access. Please let me know if you had a different scenario in
> mind, but I think this covers it.
>
> Zhi, feel free to use v6 though for any testing you are doing while I
> rework the locking.
>
Hi Joel:
Thanks so much for the work and the discussion. It is super important
efforts for me to move on for the vGPU work. :)
As we discussed, the concurrency matters most when booting multiple vGPUs.
At that time, the concurrency happens at:
1) Allocating GPU memory chunks
2) Reserving GPU channels
3) Mapping GPU memory to BAR1 page table
We basically need kinda protection there. E.g. Guard/Access on immutable
references, which is backed by the mutex. I believe there shouldn't be a
non-sleepible path reaching those. This should be fine.
I can see you are thinking of fine-granularity locking scheme, which I
think is the right direction to go. I agreed with the above two locks.
For 1), I can recall that you mentioned there is some lock protection
already there.
For 2), We can think of it when reaching there.
However for 3), We need to have one there as well beside the above two
locks. Have you already had one in the GPU VA allocator?
If yes, the above two locks should be good enough so far. IMO.
Z.
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: backlight: gpio-backlight: allow multiple GPIOs
From: tessolveupstream @ 2026-01-23 11:11 UTC (permalink / raw)
To: Krzysztof Kozlowski, lee, danielt, jingoohan1
Cc: deller, pavel, robh, krzk+dt, conor+dt, dri-devel, linux-fbdev,
linux-leds, devicetree, linux-kernel
In-Reply-To: <3f3c47ea-1660-4bd4-ab89-3bdf58217995@kernel.org>
On 20-01-2026 20:01, Krzysztof Kozlowski wrote:
> On 20/01/2026 13:50, Sudarshan Shetty wrote:
>> Update the gpio-backlight binding to support configurations that require
>> more than one GPIO for enabling/disabling the backlight.
>
>
> Why? Which devices need it? How a backlight would have three enable
> GPIOs? I really do not believe, so you need to write proper hardware
> justification.
>
To clarify our hardware setup:
the panel requires one GPIO for the backlight enable signal, and it
also has a PWM input. Since the QCS615 does not provide a PWM controller
for this use case, the PWM input is connected to a GPIO that is driven
high to provide a constant 100% duty cycle, as explained in the link
below.
https://lore.kernel.org/all/20251028061636.724667-1-tessolveupstream@gmail.com/T/#m93ca4e5c7bf055715ed13316d91f0cd544244cf5
>>
>> Signed-off-by: Sudarshan Shetty <tessolveupstream@gmail.com>
>> ---
>> .../leds/backlight/gpio-backlight.yaml | 24 +++++++++++++++++--
>> 1 file changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/leds/backlight/gpio-backlight.yaml b/Documentation/devicetree/bindings/leds/backlight/gpio-backlight.yaml
>> index 584030b6b0b9..4e4a856cbcd7 100644
>> --- a/Documentation/devicetree/bindings/leds/backlight/gpio-backlight.yaml
>> +++ b/Documentation/devicetree/bindings/leds/backlight/gpio-backlight.yaml
>> @@ -16,8 +16,18 @@ properties:
>> const: gpio-backlight
>>
>> gpios:
>> - description: The gpio that is used for enabling/disabling the backlight.
>> - maxItems: 1
>> + description: |
>> + The gpio that is used for enabling/disabling the backlight.
>> + Multiple GPIOs can be specified for panels that require several
>> + enable signals. All GPIOs are controlled together.
>> + type: array
>
> There is no such syntax in the bindings, from where did you get it? Type
> is already defined.
>
> items:
> minItems: 1
> maxItems: 3
>
>
>> + minItems: 1
>> + items:
>> + type: array
>> + minItems: 3
>> + maxItems: 3
>> + items:
>> + type: integer
>
> All this is some odd stuff - just to be clear, don't send us LLM output.
> I don't want to waste my time to review microslop.
>
> Was it done with help of Microslop?
>
I understand now that the schema changes I proposed were not correct,
and I will address this in the next patch series. My intention was to
check whether the gpio-backlight binding could support more than one
enable-type GPIO.
Could you please advise what would be an appropriate maximum number of
GPIOs for gpio-backlight in such a scenario? For example, would allowing
2 GPIOs be acceptable, or should this case be handled in a different way?
> Best regards,
> Krzysztof
^ permalink raw reply
* [PATCH v7 2/4] backlight: add max25014atg backlight
From: Maud Spierings via B4 Relay @ 2026-01-23 11:31 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Helge Deller, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Liam Girdwood, Mark Brown
Cc: dri-devel, linux-leds, devicetree, linux-kernel, linux-fbdev, imx,
linux-arm-kernel, Maud Spierings
In-Reply-To: <20260123-max25014-v7-0-15e504b9acc7@gocontroll.com>
From: Maud Spierings <maudspierings@gocontroll.com>
The Maxim MAX25014 is a 4-channel automotive grade backlight driver IC
with integrated boost controller.
Signed-off-by: Maud Spierings <maudspierings@gocontroll.com>
---
MAINTAINERS | 1 +
drivers/video/backlight/Kconfig | 7 +
drivers/video/backlight/Makefile | 1 +
drivers/video/backlight/max25014.c | 377 +++++++++++++++++++++++++++++++++++++
4 files changed, 386 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index eb248f4634ac..346a8ede3f71 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15519,6 +15519,7 @@ MAX25014 BACKLIGHT DRIVER
M: Maud Spierings <maudspierings@gocontroll.com>
S: Maintained
F: Documentation/devicetree/bindings/leds/backlight/maxim,max25014.yaml
+F: drivers/video/backlight/max25014.c
MAX31335 RTC DRIVER
M: Antoniu Miclaus <antoniu.miclaus@analog.com>
diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index a7a3fbaf7c29..6cd3a1673511 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -282,6 +282,13 @@ config BACKLIGHT_DA9052
help
Enable the Backlight Driver for DA9052-BC and DA9053-AA/Bx PMICs.
+config BACKLIGHT_MAX25014
+ tristate "Backlight driver for the Maxim MAX25014 chip"
+ depends on I2C
+ select REGMAP_I2C
+ help
+ If you are using a MAX25014 chip as a backlight driver say Y to enable it.
+
config BACKLIGHT_MAX8925
tristate "Backlight driver for MAX8925"
depends on MFD_MAX8925
diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
index 794820a98ed4..21c8313cfb12 100644
--- a/drivers/video/backlight/Makefile
+++ b/drivers/video/backlight/Makefile
@@ -47,6 +47,7 @@ obj-$(CONFIG_BACKLIGHT_LOCOMO) += locomolcd.o
obj-$(CONFIG_BACKLIGHT_LP855X) += lp855x_bl.o
obj-$(CONFIG_BACKLIGHT_LP8788) += lp8788_bl.o
obj-$(CONFIG_BACKLIGHT_LV5207LP) += lv5207lp.o
+obj-$(CONFIG_BACKLIGHT_MAX25014) += max25014.o
obj-$(CONFIG_BACKLIGHT_MAX8925) += max8925_bl.o
obj-$(CONFIG_BACKLIGHT_MP3309C) += mp3309c.o
obj-$(CONFIG_BACKLIGHT_MT6370) += mt6370-backlight.o
diff --git a/drivers/video/backlight/max25014.c b/drivers/video/backlight/max25014.c
new file mode 100644
index 000000000000..3ee45617261f
--- /dev/null
+++ b/drivers/video/backlight/max25014.c
@@ -0,0 +1,377 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Backlight driver for Maxim MAX25014
+ *
+ * Copyright (C) 2025 GOcontroll B.V.
+ * Author: Maud Spierings <maudspierings@gocontroll.com>
+ */
+
+#include <linux/backlight.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#define MAX25014_ISET_DEFAULT_100 11
+#define MAX_BRIGHTNESS 100
+#define MIN_BRIGHTNESS 0
+#define TON_MAX 130720 /* @153Hz */
+#define TON_STEP 1307 /* @153Hz */
+#define TON_MIN 0
+
+#define MAX25014_DEV_ID 0x00
+#define MAX25014_REV_ID 0x01
+#define MAX25014_ISET 0x02
+#define MAX25014_IMODE 0x03
+#define MAX25014_TON1H 0x04
+#define MAX25014_TON1L 0x05
+#define MAX25014_TON2H 0x06
+#define MAX25014_TON2L 0x07
+#define MAX25014_TON3H 0x08
+#define MAX25014_TON3L 0x09
+#define MAX25014_TON4H 0x0A
+#define MAX25014_TON4L 0x0B
+#define MAX25014_TON_1_4_LSB 0x0C
+#define MAX25014_SETTING 0x12
+#define MAX25014_DISABLE 0x13
+#define MAX25014_BSTMON 0x14
+#define MAX25014_IOUT1 0x15
+#define MAX25014_IOUT2 0x16
+#define MAX25014_IOUT3 0x17
+#define MAX25014_IOUT4 0x18
+#define MAX25014_OPEN 0x1B
+#define MAX25014_SHORTGND 0x1C
+#define MAX25014_SHORTED_LED 0x1D
+#define MAX25014_MASK 0x1E
+#define MAX25014_DIAG 0x1F
+
+#define MAX25014_ISET_ENA BIT(5)
+#define MAX25014_ISET_PSEN BIT(4)
+#define MAX25014_IMODE_HDIM BIT(2)
+#define MAX25014_SETTING_FPWM GENMASK(6, 4)
+#define MAX25014_DISABLE_DIS_MASK GENMASK(3, 0)
+#define MAX25014_DIAG_OT BIT(0)
+#define MAX25014_DIAG_OTW BIT(1)
+#define MAX25014_DIAG_HW_RST BIT(2)
+#define MAX25014_DIAG_BSTOV BIT(3)
+#define MAX25014_DIAG_BSTUV BIT(4)
+#define MAX25014_DIAG_IREFOOR BIT(5)
+
+struct max25014 {
+ struct i2c_client *client;
+ struct backlight_device *bl;
+ struct regmap *regmap;
+ struct gpio_desc *enable;
+ uint32_t iset;
+ uint8_t strings_mask;
+};
+
+static const struct regmap_config max25014_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = MAX25014_DIAG,
+};
+
+static int max25014_initial_power_state(struct max25014 *maxim)
+{
+ uint32_t val;
+ int ret;
+
+ ret = regmap_read(maxim->regmap, MAX25014_ISET, &val);
+ if (ret)
+ return ret;
+
+ return val & MAX25014_ISET_ENA ? BACKLIGHT_POWER_ON : BACKLIGHT_POWER_OFF;
+}
+
+static int max25014_check_errors(struct max25014 *maxim)
+{
+ uint32_t val;
+ uint8_t i;
+ int ret;
+
+ ret = regmap_read(maxim->regmap, MAX25014_OPEN, &val);
+ if (ret)
+ return ret;
+ if (val) {
+ dev_err(&maxim->client->dev, "Open led strings detected on:\n");
+ for (i = 0; i < 4; i++) {
+ if (val & 1 << i)
+ dev_err(&maxim->client->dev, "string %d\n", i + 1);
+ }
+ return -EIO;
+ }
+
+ ret = regmap_read(maxim->regmap, MAX25014_SHORTGND, &val);
+ if (ret)
+ return ret;
+ if (val) {
+ dev_err(&maxim->client->dev, "Short to ground detected on:\n");
+ for (i = 0; i < 4; i++) {
+ if (val & 1 << i)
+ dev_err(&maxim->client->dev, "string %d\n", i + 1);
+ }
+ return -EIO;
+ }
+
+ ret = regmap_read(maxim->regmap, MAX25014_SHORTED_LED, &val);
+ if (ret)
+ return ret;
+ if (val) {
+ dev_err(&maxim->client->dev, "Shorted led detected on:\n");
+ for (i = 0; i < 4; i++) {
+ if (val & 1 << i)
+ dev_err(&maxim->client->dev, "string %d\n", i + 1);
+ }
+ return -EIO;
+ }
+
+ ret = regmap_read(maxim->regmap, MAX25014_DIAG, &val);
+ if (ret)
+ return ret;
+ /*
+ * The HW_RST bit always starts at 1 after power up.
+ * It is reset on first read, does not indicate an error.
+ */
+ if (val && val != MAX25014_DIAG_HW_RST) {
+ if (val & MAX25014_DIAG_OT)
+ dev_err(&maxim->client->dev,
+ "Overtemperature shutdown\n");
+ if (val & MAX25014_DIAG_OTW)
+ dev_err(&maxim->client->dev,
+ "Chip is getting too hot (>125C)\n");
+ if (val & MAX25014_DIAG_BSTOV)
+ dev_err(&maxim->client->dev,
+ "Boost converter overvoltage\n");
+ if (val & MAX25014_DIAG_BSTUV)
+ dev_err(&maxim->client->dev,
+ "Boost converter undervoltage\n");
+ if (val & MAX25014_DIAG_IREFOOR)
+ dev_err(&maxim->client->dev, "IREF out of range\n");
+ return -EIO;
+ }
+ return 0;
+}
+
+/*
+ * 1. disable unused strings
+ * 2. set dim mode
+ * 3. set setting register
+ * 4. enable the backlight
+ */
+static int max25014_configure(struct max25014 *maxim, int initial_state)
+{
+ uint32_t val;
+ int ret;
+
+ ret = regmap_read(maxim->regmap, MAX25014_DISABLE, &val);
+ if (ret)
+ return ret;
+
+ /*
+ * Strings can only be disabled when MAX25014_ISET_ENA == 0, check if
+ * it needs to be changed at all to prevent the backlight flashing when
+ * it is configured correctly by the bootloader
+ */
+ if (!((val & MAX25014_DISABLE_DIS_MASK) == maxim->strings_mask)) {
+ if (initial_state == BACKLIGHT_POWER_ON) {
+ ret = regmap_write(maxim->regmap, MAX25014_ISET, 0);
+ if (ret)
+ return ret;
+ }
+ ret = regmap_write(maxim->regmap, MAX25014_DISABLE, maxim->strings_mask);
+ if (ret)
+ return ret;
+ }
+
+ ret = regmap_write(maxim->regmap, MAX25014_IMODE, MAX25014_IMODE_HDIM);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(maxim->regmap, MAX25014_SETTING, &val);
+ if (ret)
+ return ret;
+
+ ret = regmap_write(maxim->regmap, MAX25014_SETTING,
+ val & ~MAX25014_SETTING_FPWM);
+ if (ret)
+ return ret;
+
+ return regmap_write(maxim->regmap, MAX25014_ISET,
+ maxim->iset | MAX25014_ISET_ENA |
+ MAX25014_ISET_PSEN);
+}
+
+static int max25014_update_status(struct backlight_device *bl_dev)
+{
+ struct max25014 *maxim = bl_get_data(bl_dev);
+ uint32_t reg;
+ int ret;
+
+ reg = TON_STEP * backlight_get_brightness(bl_dev);
+
+ /*
+ * 18 bit number lowest, 2 bits in first register,
+ * next lowest 8 in the L register, next 8 in the H register
+ * Seemingly setting the strength of only one string controls all of
+ * them, individual settings don't affect the outcome.
+ */
+ ret = regmap_write(maxim->regmap, MAX25014_TON_1_4_LSB, reg & 0b00000011);
+ if (ret != 0)
+ return ret;
+ ret = regmap_write(maxim->regmap, MAX25014_TON1L, (reg >> 2) & 0b11111111);
+ if (ret != 0)
+ return ret;
+ return regmap_write(maxim->regmap, MAX25014_TON1H, (reg >> 10) & 0b11111111);
+}
+
+static const struct backlight_ops max25014_bl_ops = {
+ .options = BL_CORE_SUSPENDRESUME,
+ .update_status = max25014_update_status,
+};
+
+static int max25014_parse_dt(struct max25014 *maxim,
+ uint32_t *initial_brightness)
+{
+ struct device *dev = &maxim->client->dev;
+ struct device_node *node = dev->of_node;
+ uint32_t strings[4];
+ int res, i;
+
+ res = of_property_count_u32_elems(node, "maxim,strings");
+ if (res == 4) {
+ of_property_read_u32_array(node, "maxim,strings", strings, 4);
+ for (i = 0; i < 4; i++) {
+ if (strings[i] == 0)
+ maxim->strings_mask |= 1 << i;
+ }
+ } else {
+ maxim->strings_mask = 0;
+ }
+
+ *initial_brightness = 50U;
+ of_property_read_u32(node, "default-brightness", initial_brightness);
+
+ maxim->iset = MAX25014_ISET_DEFAULT_100;
+ of_property_read_u32(node, "maxim,iset", &maxim->iset);
+
+ if (maxim->iset > 15)
+ return dev_err_probe(dev, -EINVAL,
+ "Invalid iset, should be a value from 0-15, entered was %d\n",
+ maxim->iset);
+
+ if (*initial_brightness > 100)
+ return dev_err_probe(dev, -EINVAL,
+ "Invalid initial brightness, should be a value from 0-100, entered was %d\n",
+ *initial_brightness);
+
+ return 0;
+}
+
+static int max25014_probe(struct i2c_client *cl)
+{
+ const struct i2c_device_id *id = i2c_client_get_device_id(cl);
+ struct backlight_properties props;
+ uint32_t initial_brightness = 50;
+ struct backlight_device *bl;
+ struct max25014 *maxim;
+ int ret;
+
+ maxim = devm_kzalloc(&cl->dev, sizeof(struct max25014), GFP_KERNEL);
+ if (!maxim)
+ return -ENOMEM;
+
+ maxim->client = cl;
+
+ ret = max25014_parse_dt(maxim, &initial_brightness);
+ if (ret)
+ return ret;
+
+ ret = devm_regulator_get_enable(&maxim->client->dev, "power");
+ if (ret)
+ return dev_err_probe(&maxim->client->dev, ret,
+ "failed to get power-supply");
+
+ maxim->enable = devm_gpiod_get_optional(&maxim->client->dev, "enable",
+ GPIOD_OUT_HIGH);
+ if (IS_ERR(maxim->enable))
+ return dev_err_probe(&maxim->client->dev, PTR_ERR(maxim->enable),
+ "failed to get enable gpio\n");
+
+ /* Datasheet Electrical Characteristics tSTARTUP 2ms */
+ fsleep(2000);
+
+ maxim->regmap = devm_regmap_init_i2c(cl, &max25014_regmap_config);
+ if (IS_ERR(maxim->regmap))
+ return dev_err_probe(&maxim->client->dev, PTR_ERR(maxim->regmap),
+ "failed to initialize the i2c regmap\n");
+
+ i2c_set_clientdata(cl, maxim);
+
+ ret = max25014_check_errors(maxim);
+ if (ret) /* error is already reported in the above function */
+ return ret;
+
+ ret = max25014_initial_power_state(maxim);
+ if (ret < 0)
+ return dev_err_probe(&maxim->client->dev, ret, "Could not get enabled state\n");
+
+ memset(&props, 0, sizeof(struct backlight_properties));
+ props.type = BACKLIGHT_PLATFORM;
+ props.max_brightness = MAX_BRIGHTNESS;
+ props.brightness = initial_brightness;
+ props.scale = BACKLIGHT_SCALE_LINEAR;
+ props.power = ret;
+
+ ret = max25014_configure(maxim, ret);
+ if (ret)
+ return dev_err_probe(&maxim->client->dev, ret, "device config error");
+
+ bl = devm_backlight_device_register(&maxim->client->dev, id->name,
+ &maxim->client->dev, maxim,
+ &max25014_bl_ops, &props);
+ if (IS_ERR(bl))
+ return dev_err_probe(&maxim->client->dev, PTR_ERR(bl),
+ "failed to register backlight\n");
+
+ maxim->bl = bl;
+
+ backlight_update_status(maxim->bl);
+
+ return 0;
+}
+
+static void max25014_remove(struct i2c_client *cl)
+{
+ struct max25014 *maxim = i2c_get_clientdata(cl);
+
+ backlight_device_set_brightness(maxim->bl, 0);
+ gpiod_set_value_cansleep(maxim->enable, 0);
+}
+
+static const struct of_device_id max25014_dt_ids[] = {
+ { .compatible = "maxim,max25014", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, max25014_dt_ids);
+
+static const struct i2c_device_id max25014_ids[] = {
+ { "max25014" },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, max25014_ids);
+
+static struct i2c_driver max25014_driver = {
+ .driver = {
+ .name = KBUILD_MODNAME,
+ .of_match_table = of_match_ptr(max25014_dt_ids),
+ },
+ .probe = max25014_probe,
+ .remove = max25014_remove,
+ .id_table = max25014_ids,
+};
+module_i2c_driver(max25014_driver);
+
+MODULE_DESCRIPTION("Maxim MAX25014 backlight driver");
+MODULE_AUTHOR("Maud Spierings <maudspierings@gocontroll.com>");
+MODULE_LICENSE("GPL");
--
2.52.0
^ permalink raw reply related
* [PATCH v7 0/4] backlight: add new max25014 backlight driver
From: Maud Spierings via B4 Relay @ 2026-01-23 11:31 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Helge Deller, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Liam Girdwood, Mark Brown
Cc: dri-devel, linux-leds, devicetree, linux-kernel, linux-fbdev, imx,
linux-arm-kernel, Maud Spierings
The Maxim MAX25014 is an automotive grade backlight driver IC. Its
datasheet can be found at [1].
With its integrated boost controller, it can power 4 channels (led
strings) and has a number of different modes using pwm and or i2c.
Currently implemented is only i2c control.
link: https://www.analog.com/media/en/technical-documentation/data-sheets/MAX25014.pdf [1]
Signed-off-by: Maud Spierings <maudspierings@gocontroll.com>
---
Changes in v7:
- remove the led subnodes
- always enable the regulator by using devm_regulator_get_enable()
- remove the no longer required gotos and simplify early returns
- fix the name of the SHORTED_LED error field
- fix the name of the SHORTGND error field
- use the proper backlight helper functions for setting/getting
brightness
- Link to v6: https://lore.kernel.org/r/20251201-max25014-v6-0-88e3ac8112ff@gocontroll.com
Changes in v6:
- fixup changes in v4 where default brightness handling was changed but
not noted
- remove leftover comment about initializing brightness
- use BIT definitions for fields in the DIAG register
- apply reverse christmas tree initialization of local variables
- remove !=0 from checks, just check if (ret)
- remove > 0 from checks, just check if (val)
- use dev_err_probe() more
- set enable gpio high in the get() instead of seperately calling set()
- change usleep_range() to fsleep()
- remove null checks when setting gpio value
- get regular regulator, not optional to avoid further NULL checks in
case none is provided
- introduce max25014_initial_power_state() to check if the bootloader
has already initialized the backlight and to correctly set props.power
- squash max25014_register_control() into max25014_update_status()
- in max25014_configure() perform extra checking on the DISABLE register
now that the state from the bootloader is taken into account
- Link to v5: https://lore.kernel.org/r/20251107-max25014-v5-0-9a6aa57306bf@gocontroll.com
Changes in v5:
- moved comment about current functions of the driver to the actual
comment section of the commit
- fixed the led@0 property, regex patternProperty is not needed as of
now
- added extra clarification about the ISET field/register
- moved #address-cells and #size-cells to the correct location
- remove leftover default-brightness in backlight nodes
- Link to v4: https://lore.kernel.org/r/20251009-max25014-v4-0-6adb2a0aa35f@gocontroll.com
Changes in v4:
- remove setting default brightness, let backlight core take care of it
- use a led node to describe the backlight
- use led-sources to enable specific channels
- also wait 2ms when there is a supply but no enable
- change dev_warn() to dev_err() in error path in max25014_check_errors()
- set backlight_properties.scale to BACKLIGHT_SCALE_LINEAR
- rebase latest next
- add address-cells and size-cells to i2c4 in av101hdt-a10.dtso
- Link to v3: https://lore.kernel.org/r/20250911-max25014-v3-0-d03f4eba375e@gocontroll.com
Changes in v3:
- fixed commit message type intgrated -> integrated
- added maximum and description to maxim,iset-property
- dropped unused labels and pinctrl in bindings example
- put the compatible first in the bindings example and dts
- removed brackets around defines
- removed the leftover pdata struct field
- removed the initial_brightness struct field
- Link to v2: https://lore.kernel.org/r/20250819-max25014-v2-0-5fd7aeb141ea@gocontroll.com
Changes in v2:
- Remove leftover unused property from the bindings example
- Complete the bindings example with all properties
- Remove some double info from the maxim,iset property
- Remove platform_data header, fold its data into the max25014 struct
- Don't force defines to be unsigned
- Remove stray struct max25014 declaration
- Remove chipname and device from the max25014 struct
- Inline the max25014_backlight_register() and strings_mask() functions
- Remove CONFIG_OF ifdef
- Link to v1: https://lore.kernel.org/r/20250725-max25014-v1-0-0e8cce92078e@gocontroll.com
---
Maud Spierings (4):
dt-bindings: backlight: Add max25014 support
backlight: add max25014atg backlight
arm64: dts: freescale: moduline-display-av101hdt-a10: add backlight
arm64: dts: freescale: moduline-display-av123z7m-n17: add backlight
.../bindings/leds/backlight/maxim,max25014.yaml | 91 +++++
MAINTAINERS | 6 +
...x8p-ml81-moduline-display-106-av101hdt-a10.dtso | 26 ++
...x8p-ml81-moduline-display-106-av123z7m-n17.dtso | 21 +-
drivers/video/backlight/Kconfig | 7 +
drivers/video/backlight/Makefile | 1 +
drivers/video/backlight/max25014.c | 377 +++++++++++++++++++++
7 files changed, 528 insertions(+), 1 deletion(-)
---
base-commit: a0c666c25aeefd16f4b088c6549a6fb6b65a8a1d
change-id: 20250626-max25014-4207591e1af5
Best regards,
--
Maud Spierings <maudspierings@gocontroll.com>
^ permalink raw reply
* [PATCH v7 1/4] dt-bindings: backlight: Add max25014 support
From: Maud Spierings via B4 Relay @ 2026-01-23 11:31 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Helge Deller, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Liam Girdwood, Mark Brown
Cc: dri-devel, linux-leds, devicetree, linux-kernel, linux-fbdev, imx,
linux-arm-kernel, Maud Spierings
In-Reply-To: <20260123-max25014-v7-0-15e504b9acc7@gocontroll.com>
From: Maud Spierings <maudspierings@gocontroll.com>
The Maxim MAX25014 is a 4-channel automotive grade backlight driver IC
with integrated boost controller.
Signed-off-by: Maud Spierings <maudspierings@gocontroll.com>
---
In the current implementation the control registers for channel 1,
control all channels. So only one led subnode with led-sources is
supported right now. If at some point the driver functionality is
expanded the bindings can be easily extended with it.
---
.../bindings/leds/backlight/maxim,max25014.yaml | 91 ++++++++++++++++++++++
MAINTAINERS | 5 ++
2 files changed, 96 insertions(+)
diff --git a/Documentation/devicetree/bindings/leds/backlight/maxim,max25014.yaml b/Documentation/devicetree/bindings/leds/backlight/maxim,max25014.yaml
new file mode 100644
index 000000000000..c499e6224a8f
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/backlight/maxim,max25014.yaml
@@ -0,0 +1,91 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/backlight/maxim,max25014.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Maxim max25014 backlight controller
+
+maintainers:
+ - Maud Spierings <maudspierings@gocontroll.com>
+
+properties:
+ compatible:
+ enum:
+ - maxim,max25014
+
+ reg:
+ maxItems: 1
+
+ "#address-cells":
+ const: 1
+
+ "#size-cells":
+ const: 0
+
+ default-brightness:
+ minimum: 0
+ maximum: 100
+ default: 50
+
+ enable-gpios:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ power-supply:
+ description: Regulator which controls the boost converter input rail.
+
+ pwms:
+ maxItems: 1
+
+ maxim,iset:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ maximum: 15
+ default: 11
+ description:
+ Value of the ISET field in the ISET register. This controls the current
+ scale of the outputs, a higher number means more current.
+
+ maxim,strings:
+ $ref: /schemas/types.yaml#/definitions/uint32-array
+ description:
+ A 4-bit bitfield that describes which led strings to turn on.
+ minItems: 4
+ maxItems: 4
+ items:
+ maximum: 1
+ default:
+ [1 1 1 1]
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/interrupt-controller/irq.h>
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ backlight@6f {
+ compatible = "maxim,max25014";
+ reg = <0x6f>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ default-brightness = <50>;
+ enable-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>;
+ interrupt-parent = <&gpio1>;
+ interrupts = <2 IRQ_TYPE_EDGE_FALLING>;
+ power-supply = <®_backlight>;
+ pwms = <&pwm1>;
+ maxim,iset = <7>;
+ maxim,strings = <1 1 1 0>;
+ };
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index 9b1b87d08fac..eb248f4634ac 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15515,6 +15515,11 @@ F: Documentation/userspace-api/media/drivers/max2175.rst
F: drivers/media/i2c/max2175*
F: include/uapi/linux/max2175.h
+MAX25014 BACKLIGHT DRIVER
+M: Maud Spierings <maudspierings@gocontroll.com>
+S: Maintained
+F: Documentation/devicetree/bindings/leds/backlight/maxim,max25014.yaml
+
MAX31335 RTC DRIVER
M: Antoniu Miclaus <antoniu.miclaus@analog.com>
L: linux-rtc@vger.kernel.org
--
2.52.0
^ permalink raw reply related
* [PATCH v7 3/4] arm64: dts: freescale: moduline-display-av101hdt-a10: add backlight
From: Maud Spierings via B4 Relay @ 2026-01-23 11:31 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Helge Deller, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Liam Girdwood, Mark Brown
Cc: dri-devel, linux-leds, devicetree, linux-kernel, linux-fbdev, imx,
linux-arm-kernel, Maud Spierings
In-Reply-To: <20260123-max25014-v7-0-15e504b9acc7@gocontroll.com>
From: Maud Spierings <maudspierings@gocontroll.com>
Add the missing backlight driver.
Signed-off-by: Maud Spierings <maudspierings@gocontroll.com>
---
...x8p-ml81-moduline-display-106-av101hdt-a10.dtso | 26 ++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/arch/arm64/boot/dts/freescale/imx8mp-tx8p-ml81-moduline-display-106-av101hdt-a10.dtso b/arch/arm64/boot/dts/freescale/imx8mp-tx8p-ml81-moduline-display-106-av101hdt-a10.dtso
index e3965caca6be..e121c58b730b 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp-tx8p-ml81-moduline-display-106-av101hdt-a10.dtso
+++ b/arch/arm64/boot/dts/freescale/imx8mp-tx8p-ml81-moduline-display-106-av101hdt-a10.dtso
@@ -17,6 +17,7 @@
panel {
compatible = "boe,av101hdt-a10";
+ backlight = <&backlight>;
enable-gpios = <&gpio1 7 GPIO_ACTIVE_HIGH>;
pinctrl-0 = <&pinctrl_panel>;
pinctrl-names = "default";
@@ -40,7 +41,32 @@ reg_vbus: regulator-vbus {
};
};
+&i2c4 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ backlight: backlight@6f {
+ compatible = "maxim,max25014";
+ reg = <0x6f>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ default-brightness = <50>;
+ enable-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_backlight>;
+ maxim,iset = <7>;
+ maxim,strings = <1 1 1 0>;
+ };
+};
+
&iomuxc {
+ pinctrl_backlight: backlightgrp {
+ fsl,pins = <
+ MX8MP_IOMUXC_GPIO1_IO04__GPIO1_IO04
+ (MX8MP_PULL_UP | MX8MP_PULL_ENABLE)
+ >;
+ };
+
pinctrl_panel: panelgrp {
fsl,pins = <
MX8MP_IOMUXC_GPIO1_IO07__GPIO1_IO07
--
2.52.0
^ permalink raw reply related
* [PATCH v7 4/4] arm64: dts: freescale: moduline-display-av123z7m-n17: add backlight
From: Maud Spierings via B4 Relay @ 2026-01-23 11:31 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han, Pavel Machek, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Helge Deller, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
Liam Girdwood, Mark Brown
Cc: dri-devel, linux-leds, devicetree, linux-kernel, linux-fbdev, imx,
linux-arm-kernel, Maud Spierings
In-Reply-To: <20260123-max25014-v7-0-15e504b9acc7@gocontroll.com>
From: Maud Spierings <maudspierings@gocontroll.com>
Add the missing backlight.
Signed-off-by: Maud Spierings <maudspierings@gocontroll.com>
---
...tx8p-ml81-moduline-display-106-av123z7m-n17.dtso | 21 ++++++++++++++++++++-
1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/freescale/imx8mp-tx8p-ml81-moduline-display-106-av123z7m-n17.dtso b/arch/arm64/boot/dts/freescale/imx8mp-tx8p-ml81-moduline-display-106-av123z7m-n17.dtso
index 3eb665ce9d5d..66d98a18d898 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp-tx8p-ml81-moduline-display-106-av123z7m-n17.dtso
+++ b/arch/arm64/boot/dts/freescale/imx8mp-tx8p-ml81-moduline-display-106-av123z7m-n17.dtso
@@ -16,6 +16,7 @@
panel {
compatible = "boe,av123z7m-n17";
+ backlight = <&backlight>;
enable-gpios = <&gpio1 7 GPIO_ACTIVE_HIGH>;
pinctrl-0 = <&pinctrl_panel>;
pinctrl-names = "default";
@@ -91,10 +92,28 @@ lvds1_out: endpoint {
};
};
- /* max25014 @ 0x6f */
+ backlight: backlight@6f {
+ compatible = "maxim,max25014";
+ reg = <0x6f>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ default-brightness = <50>;
+ enable-gpios = <&gpio1 4 GPIO_ACTIVE_HIGH>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_backlight>;
+ maxim,iset = <7>;
+ maxim,strings = <1 1 1 1>;
+ };
};
&iomuxc {
+ pinctrl_backlight: backlightgrp {
+ fsl,pins = <
+ MX8MP_IOMUXC_GPIO1_IO04__GPIO1_IO04
+ (MX8MP_PULL_UP | MX8MP_PULL_ENABLE)
+ >;
+ };
+
pinctrl_lvds_bridge: lvdsbridgegrp {
fsl,pins = <
MX8MP_IOMUXC_SAI1_TXD2__GPIO4_IO14
--
2.52.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox