| Message ID | 20250326171411.590681-1-remo@buenzli.dev |
|---|---|
| Headers | show |
| Series | More Rust bindings for device property reads | expand |
On Wed, Mar 26, 2025 at 03:51:06PM -0500, Rob Herring wrote: > On Wed, Mar 26, 2025 at 06:13:40PM +0100, Remo Senekowitsch wrote: > > Not all property-related APIs can be exposed directly on a device. > > For example, iterating over child nodes of a device will yield > > fwnode_handle. Thus, in order to access properties on these child nodes, > > the APIs has to be duplicated on a fwnode as they are in C. > > s/has/have/ > > > > > A related discussion can be found on the R4L Zulip[1]. > > > > [1] https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/DS90UB954.20driver.20done.2C.20ready.20to.20upstream.3F/near/505415697 > > Useful below the '---', but I don't think we want to keep this link > forever. And who knows how long it will be valid? The commit msg needs > to stand on its own, and I think it does. > > > > > Signed-off-by: Remo Senekowitsch <remo@buenzli.dev> > > --- > > rust/helpers/helpers.c | 1 + > > rust/helpers/property.c | 13 ++++++++ > > rust/kernel/device.rs | 7 ---- > > rust/kernel/lib.rs | 1 + > > rust/kernel/property.rs | 73 +++++++++++++++++++++++++++++++++++++++++ > > 5 files changed, 88 insertions(+), 7 deletions(-) > > create mode 100644 rust/helpers/property.c > > create mode 100644 rust/kernel/property.rs Also, property.rs needs to be added to MAINTAINERS. I guess it goes under driver core with Greg. Rob
On Wed, Mar 26, 2025 at 06:13:40PM +0100, Remo Senekowitsch wrote: > Not all property-related APIs can be exposed directly on a device. > For example, iterating over child nodes of a device will yield > fwnode_handle. Thus, in order to access properties on these child nodes, > the APIs has to be duplicated on a fwnode as they are in C. > > A related discussion can be found on the R4L Zulip[1]. > > [1] https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/DS90UB954.20driver.20done.2C.20ready.20to.20upstream.3F/near/505415697 You can make the above to be a Link tag like Link: ... [1] > Signed-off-by: Remo Senekowitsch <remo@buenzli.dev> ... > +struct fwnode_handle *rust_helper_dev_fwnode(struct device *dev) > +{ > + return dev_fwnode(dev); > +} Why not const? For most of the property retrieval APIs the parameter is const.
On Wed, Mar 26, 2025 at 03:54:09PM -0500, Andrew Ballance wrote: > Hi thanks for sending these in. > > On Wed, Mar 26, 2025 at 12:13 PM Remo Senekowitsch Wrote: > > This is my first time posting to the mailing list, please let me know if > > I did anything wrong. > > you probably will have to resubmit in the patch canonical format[1] > > I would recomend reading the docs but in summary: > - all of your patches should have the subsystem in the subject line. > - patch 6 is missing a commit message > - commit messages should be written in the imperitive tone > - a couple of your commit messages give a reason for the changes but > do not have a summary of the changes > - for your v2 you should add a diffstat to your cover letter running > `git format-patch --base=auto --cover-letter` does this automatically > for you Also -v<X> gives the proper versioning. > [1] https://www.kernel.org/doc/html/latest/process/submitting-patches.html#the-canonical-patch-format
On Thu, Mar 27, 2025 at 3:37 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Wed, Mar 26, 2025 at 06:13:40PM +0100, Remo Senekowitsch wrote: > > Not all property-related APIs can be exposed directly on a device. > > For example, iterating over child nodes of a device will yield > > fwnode_handle. Thus, in order to access properties on these child nodes, > > the APIs has to be duplicated on a fwnode as they are in C. > > > > A related discussion can be found on the R4L Zulip[1]. > > > > [1] https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/DS90UB954.20driver.20done.2C.20ready.20to.20upstream.3F/near/505415697 > > You can make the above to be a Link tag like > > Link: ... [1] > > > Signed-off-by: Remo Senekowitsch <remo@buenzli.dev> > > ... > > > +struct fwnode_handle *rust_helper_dev_fwnode(struct device *dev) > > +{ > > + return dev_fwnode(dev); > > +} > > Why not const? For most of the property retrieval APIs the parameter is const. Because you might need to modify the refcount. Though the rust code should probably use __dev_fwnode() and/or __dev_fwnode_const() directly and avoid the need for the helper here. Rob
On Wed, Mar 26, 2025 at 06:13:47PM +0100, Remo Senekowitsch wrote: > Signed-off-by: Remo Senekowitsch <remo@buenzli.dev> > --- > rust/kernel/property.rs | 63 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 63 insertions(+) > > diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs > index dc927ad93..f1d0a33ba 100644 > --- a/rust/kernel/property.rs > +++ b/rust/kernel/property.rs > @@ -8,6 +8,7 @@ > > use crate::{ > alloc::KVec, > + arrayvec::ArrayVec, > bindings, > device::Device, > error::{to_result, Result}, > @@ -64,6 +65,20 @@ pub fn get_child_by_name(&self, name: &CStr) -> Option<ARef<FwNode>> { > pub fn children<'a>(&'a self) -> impl Iterator<Item = ARef<FwNode>> + 'a { > self.fwnode().children() > } > + > + /// Finds a reference with arguments. > + pub fn property_get_reference_args( > + &self, > + prop: &CStr, > + nargs: NArgs<'_>, > + index: u32, > + ) -> Result<( > + ARef<FwNode>, > + ArrayVec<{ bindings::NR_FWNODE_REFERENCE_ARGS as usize }, u64>, > + )> { > + self.fwnode() > + .property_get_reference_args(prop, nargs, index) > + } > } > > /// A reference-counted fwnode_handle. > @@ -226,6 +241,45 @@ pub fn children<'a>(&'a self) -> impl Iterator<Item = ARef<FwNode>> + 'a { > Some(next) > }) > } > + > + /// Finds a reference with arguments. > + pub fn property_get_reference_args( > + &self, > + prop: &CStr, > + nargs: NArgs<'_>, > + index: u32, > + ) -> Result<( > + ARef<Self>, > + ArrayVec<{ bindings::NR_FWNODE_REFERENCE_ARGS as usize }, u64>, > + )> { > + let mut out_args = bindings::fwnode_reference_args::default(); > + > + let (nargs_prop, nargs) = match nargs { > + NArgs::Prop(nargs_prop) => (nargs_prop.as_char_ptr(), 0), > + NArgs::N(nargs) => (ptr::null(), nargs), > + }; > + > + let ret = unsafe { > + bindings::fwnode_property_get_reference_args( > + self.0.get(), > + prop.as_char_ptr(), > + nargs_prop, > + nargs, > + index, > + &mut out_args, > + ) > + }; > + to_result(ret)?; > + > + let node = unsafe { FwNode::from_raw(out_args.fwnode) }; > + let mut args = ArrayVec::default(); > + > + for i in 0..out_args.nargs { > + args.push(out_args.args[i as usize]); > + } Why the copy? Can't you just write an abstraction for struct fwnode_reference_args and just return an instance of that?
On Thu, Mar 27, 2025 at 08:55:42AM -0500, Rob Herring wrote: > On Thu, Mar 27, 2025 at 3:37 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Wed, Mar 26, 2025 at 06:13:40PM +0100, Remo Senekowitsch wrote: ... > > > +struct fwnode_handle *rust_helper_dev_fwnode(struct device *dev) > > > +{ > > > + return dev_fwnode(dev); > > > +} > > > > Why not const? For most of the property retrieval APIs the parameter is const. > > Because you might need to modify the refcount. > > Though the rust code should probably use __dev_fwnode() and/or > __dev_fwnode_const() directly and avoid the need for the helper here. Indeed, that would help at least to understand the intention.
On Wed Mar 26, 2025 at 9:51 PM CET, Rob Herring wrote: > On Wed, Mar 26, 2025 at 06:13:40PM +0100, Remo Senekowitsch wrote: >> >> +impl Device { >> + /// Obtain the fwnode corresponding to the device. >> + fn fwnode(&self) -> &FwNode { >> + // SAFETY: `self` is valid. >> + let fwnode_handle = unsafe { bindings::dev_fwnode(self.as_raw()) }; >> + if fwnode_handle.is_null() { >> + panic!("fwnode_handle cannot be null"); > > It's usually not a good idea to panic the kernel especially with > something a driver calls as that's probably recoverable. > > Users/drivers testing fwnode_handle/of_node for NULL is pretty common. > Though often that's a legacy code path, so maybe not allowing NULL is > fine for now. Just to be clear on this, should I keep this as is, or return a result? In the latter case, all the duplicated methods on `Device` that just call `self.fwnode().same_method()` would have a result in their function signatur as well. That includes `property_present`, `read_property` and `children`. >> + } >> + // SAFETY: `fwnode_handle` is valid. Its lifetime is tied to `&self`. We >> + // return a reference instead of an `ARef<FwNode>` because `dev_fwnode()` >> + // doesn't increment the refcount. >> + unsafe { &*fwnode_handle.cast() } >> + } >> + >> + /// Checks if property is present or not. >> + pub fn property_present(&self, name: &CStr) -> bool { >> + self.fwnode().property_present(name) >> + } >> +} > > The C developer in me wants to put this after the FwNode stuff since > this uses it. Is that just a comment or a call to action? :-) Remo