Add an abstraction over device' fwnode

[vvarma]

I was conflicted about naming the module fwnode when the functions it implements are from linux/property.h, yet i feel fwnode provides more clarity about what the module provides. I have (currently) implemented only the endpoints method which provides a rust interface to traversing the tree. There were no changes required in binding_helper.rs since linux/property.h is already included in some of the other headers included.

[bjorn3]

I think this should store UnsafeCell<bindings::fwnode_handle> and be #[repr(transparent)] directly and implement AlwaysRefCounted. Then you can return an ARef<Node> from NodeIterator. See for example

linux/rust/kernel/file.rs

Lines 113 to 179 in 729c030

	#[repr(transparent)]
	pub struct File(pub(crate) UnsafeCell<bindings::file>);
	// TODO: Accessing fields of `struct file` through the pointer is UB because other threads may be
	// writing to them. However, this is how the C code currently operates: naked reads and writes to
	// fields. Even if we used relaxed atomics on the Rust side, we can't force this on the C side.
	impl File {
	/// Constructs a new [`struct file`] wrapper from a file descriptor.
	///
	/// The file descriptor belongs to the current process.
	pub fn from_fd(fd: u32) -> Result<ARef<Self>> {
	// SAFETY: FFI call, there are no requirements on `fd`.
	let ptr = ptr::NonNull::new(unsafe { bindings::fget(fd) }).ok_or(EBADF)?;
	// SAFETY: `fget` increments the refcount before returning.
	Ok(unsafe { ARef::from_raw(ptr.cast()) })
	}
	/// Creates a reference to a [`File`] from a valid pointer.
	///
	/// # Safety
	///
	/// The caller must ensure that `ptr` is valid and remains valid for the lifetime of the
	/// returned [`File`] instance.
	pub(crate) unsafe fn from_ptr<'a>(ptr: *const bindings::file) -> &'a File {
	// SAFETY: The safety requirements guarantee the validity of the dereference, while the
	// `File` type being transparent makes the cast ok.
	unsafe { &*ptr.cast() }
	}
	/// Returns the current seek/cursor/pointer position (`struct file::f_pos`).
	pub fn pos(&self) -> u64 {
	// SAFETY: The file is valid because the shared reference guarantees a nonzero refcount.
	unsafe { core::ptr::addr_of!((*self.0.get()).f_pos).read() as _ }
	}
	/// Returns the credentials of the task that originally opened the file.
	pub fn cred(&self) -> &Credential {
	// SAFETY: The file is valid because the shared reference guarantees a nonzero refcount.
	let ptr = unsafe { core::ptr::addr_of!((*self.0.get()).f_cred).read() };
	// SAFETY: The lifetimes of `self` and `Credential` are tied, so it is guaranteed that
	// the credential pointer remains valid (because the file is still alive, and it doesn't
	// change over the lifetime of a file).
	unsafe { Credential::from_ptr(ptr) }
	}
	/// Returns the flags associated with the file.
	///
	/// The flags are a combination of the constants in [`flags`].
	pub fn flags(&self) -> u32 {
	// SAFETY: The file is valid because the shared reference guarantees a nonzero refcount.
	unsafe { core::ptr::addr_of!((*self.0.get()).f_flags).read() }
	}
	}
	// SAFETY: The type invariants guarantee that `File` is always ref-counted.
	unsafe impl AlwaysRefCounted for File {
	fn inc_ref(&self) {
	// SAFETY: The existence of a shared reference means that the refcount is nonzero.
	unsafe { bindings::get_file(self.0.get()) };
	}
	unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
	// SAFETY: The safety requirements guarantee that the refcount is nonzero.
	unsafe { bindings::fput(obj.cast().as_ptr()) }
	}
	}

You can then remove the Send, Sync and Drop impls.

[vvarma]

thanks @bjorn3 for the review! Is it correct to reason that this is not required for DeviceNode since it's ptr is not public? And hence its safe to rely on rust's reference counting completely.

[bjorn3]

fwnode_handle is reference counted too, so DeviceNode should probably do the same. Also dev_fwnode seems to return a pointer without incrementing reference count. As such I think DeviceNode::from should return &'a DeviceNode where 'a is the lifetime of the device.

[bjorn3]

Also I don't know if you endpoints of nodes can have endpoints themself, but if so maybe DeviceNode should be merged into Node.

[vvarma]

if you endpoints of nodes can have endpoints themself,

I took your recommendation and merged DeviceNode and Node. Based on the documentation endpoints should not have endpoints themselves. However, calling endpoints() returns an Iterator which may be empty as well, so should be safe for consumption.

dev_fwnode seems to return a pointer without incrementing reference count.

I have added a fwnode_handle_get call to ensure that the refcount is incremented before passing it to the ARef. Since I am explicitly incrementing the refcount in Node::from I am not associating the lifetime of Node with the device. Am i missing anything?

[bjorn3]

dev_fwnode seems to return a pointer without incrementing reference count.

I have added a fwnode_handle_get call to ensure that the refcount is incremented before passing it to the ARef. Since I am explicitly incrementing the refcount in Node::from I am not associating the lifetime of Node with the device. Am i missing anything?

Should be fine now.

[bjorn3]

Based on what I know about this interface, it looks good. I'm not sure if Node should be called this or something closer to fwnode_handle though like FwNodeHandle.

[bjorn3]

Here and elsewhere there are still references to DeviceNode.

[vvarma]

My bad! I have fixed them now.

[vvarma]

Based on what I know about this interface, it looks good. I'm not sure if Node should be called this or something closer to fwnode_handle though like FwNodeHandle.

Since the module name is called fwnode wont it be repetitive to call it fwnode::FwNodeHandle ? However, if thats the convention I could rename it to that.

I have also added two methods parent and children. I have reverted to passing a function to the NodeIterator (like i had in an earlier version of this PR), to reuse the iterator for the endpoints and the children methods, both of which have the same pattern.