Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] The abstraction methods `io_read` and `io_write` for `ConfigSpaceBackend` are missing the `#[inline]` annotation. --
commit 4b02e1c6f87f5cb689ea18ffd09a2fb450c97ee7 Author: Gary Guo <[email protected]> Subject: rust: io: use view types instead of addresses for Io This commit introduces a new IoBackend trait to avoid monomorphization duplication and defines operations on it using view types instead of addresses. IoCapable implementations are moved to backends, and io_read and io_write are made safe by relying on view invariants. > diff --git a/rust/kernel/pci/io.rs b/rust/kernel/pci/io.rs > index 89f4bb483a7f6..e67c1e3694fb8 100644 > --- a/rust/kernel/pci/io.rs > +++ b/rust/kernel/pci/io.rs [ ... ] > @@ -112,25 +138,24 @@ unsafe fn io_write(self, value: $ty, address: usize) { > /// Implements [`IoCapable`] on [`ConfigSpace`] for `$ty` using `$read_fn` > and `$write_fn`. > macro_rules! impl_config_space_io_capable { > ($ty:ty, $read_fn:ident, $write_fn:ident) => { > - impl<'a, T: ?Sized> IoCapable<$ty> for ConfigSpace<'a, T> { > - unsafe fn io_read(self, address: usize) -> $ty { > + impl IoCapable<$ty> for ConfigSpaceBackend { > + fn io_read(view: ConfigSpace<'_, $ty>) -> $ty { [Severity: Low] this isn't a bug, but should this small forwarding function be annotated with #[inline]? The Rust subsystem guidelines state that functions in abstractions that are small or forwarding to a binding call should be annotated with #[inline]. > + // CAST: The offset is cast to `i32` because the C functions > expect a 32-bit > + // signed offset parameter. PCI configuration space size is > at most 4096 bytes, > + // so the value always fits within `i32` without truncation > or sign change. > + let addr = view.ptr.addr() as i32; > + > let mut val: $ty = 0; > > // Return value from C function is ignored in infallible > accessors. > - let _ret = > - // SAFETY: By the type invariant `self.pdev` is a valid > address. > - // CAST: The offset is cast to `i32` because the C > functions expect a 32-bit > - // signed offset parameter. PCI configuration space size > is at most 4096 bytes, > - // so the value always fits within `i32` without > truncation or sign change. > - unsafe { bindings::$read_fn(self.pdev.as_raw(), address > as i32, &mut val) }; > - > + // SAFETY: By the type invariant `pdev` is a valid address. > + let _ = unsafe { bindings::$read_fn(view.pdev.as_raw(), > addr, &mut val) }; > val > } > > - unsafe fn io_write(self, value: $ty, address: usize) { > + fn io_write(view: ConfigSpace<'_, $ty>, value: $ty) { [Severity: Low] this isn't a bug, but should this function also have the #[inline] annotation? > + // CAST: The offset is cast to `i32` because the C functions > expect a 32-bit > + // signed offset parameter. PCI configuration space size is > at most 4096 bytes, > + // so the value always fits within `i32` without truncation > or sign change. > + let addr = view.ptr.addr() as i32; > + > // Return value from C function is ignored in infallible > accessors. > - let _ret = > - // SAFETY: By the type invariant `self.pdev` is a valid > address. > - // CAST: The offset is cast to `i32` because the C > functions expect a 32-bit > - // signed offset parameter. PCI configuration space size > is at most 4096 bytes, > - // so the value always fits within `i32` without > truncation or sign change. > - unsafe { bindings::$write_fn(self.pdev.as_raw(), address > as i32, value) }; > + // SAFETY: By the type invariant `pdev` is a valid address. > + let _ = unsafe { bindings::$write_fn(view.pdev.as_raw(), > addr, value) }; > } > } > }; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=9
