| Message ID | 20241117-simplify-result-v1-1-5b01bd230a6b@iiitd.ac.in (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | rust: simplify Result<()> uses | expand |
On Sun, Nov 17, 2024 at 08:41:47PM +0530, Manas via B4 Relay wrote: > From: Manas <manas18244@iiitd.ac.in> > > This patch replaces `Result<()>` with `Result`. > > Suggested-by: Miguel Ojeda <ojeda@kernel.org> > Link: https://github.com/Rust-for-Linux/linux/issues/1128 > Signed-off-by: Manas <manas18244@iiitd.ac.in> > --- > drivers/net/phy/qt2025.rs | 2 +- > rust/kernel/block/mq/gen_disk.rs | 2 +- > rust/kernel/uaccess.rs | 2 +- > rust/macros/lib.rs | 6 +++--- Please split these patches up per subsystem, and submit them individually to the appropriate subsystems. Andrew --- pw-bot: cr
On Sun, Nov 17, 2024 at 07:25:48PM +0100, Andrew Lunn wrote: > On Sun, Nov 17, 2024 at 08:41:47PM +0530, Manas via B4 Relay wrote: > > From: Manas <manas18244@iiitd.ac.in> > > > > This patch replaces `Result<()>` with `Result`. > > > > Suggested-by: Miguel Ojeda <ojeda@kernel.org> > > Link: https://github.com/Rust-for-Linux/linux/issues/1128 > > Signed-off-by: Manas <manas18244@iiitd.ac.in> > > --- > > drivers/net/phy/qt2025.rs | 2 +- > > rust/kernel/block/mq/gen_disk.rs | 2 +- > > rust/kernel/uaccess.rs | 2 +- > > rust/macros/lib.rs | 6 +++--- > > Please split these patches up per subsystem, and submit them > individually to the appropriate subsystems. In addition, it would be good if the commit stated the rationale for the change, rather than what the change is (which we can see from the patch itself.)
Let's try that again... with Manas' address pasted in _before_ Andrew's "A" ! On Sun, Nov 17, 2024 at 06:56:31PM +0000, Russell King (Oracle) wrote: > On Sun, Nov 17, 2024 at 07:25:48PM +0100, Andrew Lunn wrote: > > On Sun, Nov 17, 2024 at 08:41:47PM +0530, Manas via B4 Relay wrote: > > > From: Manas <manas18244@iiitd.ac.in> > > > > > > This patch replaces `Result<()>` with `Result`. > > > > > > Suggested-by: Miguel Ojeda <ojeda@kernel.org> > > > Link: https://github.com/Rust-for-Linux/linux/issues/1128 > > > Signed-off-by: Manas <manas18244@iiitd.ac.in> > > > --- > > > drivers/net/phy/qt2025.rs | 2 +- > > > rust/kernel/block/mq/gen_disk.rs | 2 +- > > > rust/kernel/uaccess.rs | 2 +- > > > rust/macros/lib.rs | 6 +++--- > > > > Please split these patches up per subsystem, and submit them > > individually to the appropriate subsystems. > > In addition, it would be good if the commit stated the rationale for > the change, rather than what the change is (which we can see from the > patch itself.) > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Sun, Nov 17, 2024 at 7:26 PM Andrew Lunn <andrew@lunn.ch> wrote: > > Please split these patches up per subsystem, and submit them > individually to the appropriate subsystems. That is good advice, although if you and block are Ok with an Acked-by (assuming a good v2), we could do that too. Manas: I forgot to mention in the issue that this could be a good case for a `checkpatch.pl` check (I added it now). It would be great if you could add that in a different (and possibly independent) patch. Of course, it is not a requirement, but it would be a nice opportunity to contribute something extra related to this :) Thanks! Cheers, Miguel
On 17.11.2024 21:49, Miguel Ojeda wrote: >On Sun, Nov 17, 2024 at 7:26 PM Andrew Lunn <andrew@lunn.ch> wrote: >> >> Please split these patches up per subsystem, and submit them >> individually to the appropriate subsystems. > >That is good advice, although if you and block are Ok with an Acked-by >(assuming a good v2), we could do that too. > >Manas: I forgot to mention in the issue that this could be a good case >for a `checkpatch.pl` check (I added it now). It would be great if you >could add that in a different (and possibly independent) patch. > >Of course, it is not a requirement, but it would be a nice opportunity >to contribute something extra related to this :) > On 17.11.2024 18:56, Russell King (Oracle) wrote: >On Sun, Nov 17, 2024 at 07:25:48PM +0100, Andrew Lunn wrote: >> On Sun, Nov 17, 2024 at 08:41:47PM +0530, Manas via B4 Relay wrote: >> > From: Manas <manas18244@iiitd.ac.in> >> > >> > This patch replaces `Result<()>` with `Result`. >> > >> > Suggested-by: Miguel Ojeda <ojeda@kernel.org> >> > Link: https://github.com/Rust-for-Linux/linux/issues/1128 >> > Signed-off-by: Manas <manas18244@iiitd.ac.in> >> > --- >> > drivers/net/phy/qt2025.rs | 2 +- >> > rust/kernel/block/mq/gen_disk.rs | 2 +- >> > rust/kernel/uaccess.rs | 2 +- >> > rust/macros/lib.rs | 6 +++--- >> >> Please split these patches up per subsystem, and submit them >> individually to the appropriate subsystems. > >In addition, it would be good if the commit stated the rationale for >the change, rather than what the change is (which we can see from the >patch itself.) > Thanks Andrew, Rusell and Miguel for the feedback. Russell: I will edit the commit message to say something like, use the standard way of `Result<()>` which is `Result` and keep things consistent wrt other parts of codebase. Andrew, Miguel: I can split it in the following subsystems: rust: block: rust: uaccess: rust: macros: net: phy: qt2025: Should I do a patch series for first three, and put an individual patch for qt2025? Also, I can work on the checkpatch.pl after this.
On Mon, Nov 18, 2024 at 1:11 AM Manas <manas18244@iiitd.ac.in> wrote: > > I can split it in the following subsystems: > > rust: block: > rust: uaccess: > rust: macros: > net: phy: qt2025: > > Should I do a patch series for first three, and put an individual patch for > qt2025? Up to Andrew et al., either way is fine for me. > Also, I can work on the checkpatch.pl after this. That is great, thanks a lot! By the way, for the purposes of the Signed-off-by, is "Manas" a "known identity" (please see [1])? [1] https://docs.kernel.org/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin Cheers, Miguel
On 18.11.2024 01:54, Miguel Ojeda wrote: >By the way, for the purposes of the Signed-off-by, is "Manas" a "known >identity" (please see [1])? > >[1] https://docs.kernel.org/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin > Yes.
On Mon, Nov 18, 2024 at 2:12 AM Manas <manas18244@iiitd.ac.in> wrote: > > Yes. I was asking because the kernel does not accept anonymous contributions, so when someone uses a single (common, I think?) word as their identity, it may not be enough to find the contributor again in the future if the email changes. Thanks! Cheers, Miguel
> Andrew, Miguel: > > I can split it in the following subsystems: > > rust: block: > rust: uaccess: > rust: macros: > net: phy: qt2025: > > Should I do a patch series for first three, and put an individual patch for > qt2025? qt2025 should be an individual patch. How active is the block Maintainer with Rust patches? It might be he also wants an individual patch. Please also note that the merge window just opened, so no patches will be accepted for the next two weeks. Andrew
On 18.11.2024 14:29, Andrew Lunn wrote: >> Andrew, Miguel: >> >> I can split it in the following subsystems: >> >> rust: block: >> rust: uaccess: >> rust: macros: >> net: phy: qt2025: >> >> Should I do a patch series for first three, and put an individual patch for >> qt2025? > >qt2025 should be an individual patch. How active is the block >Maintainer with Rust patches? It might be he also wants an individual >patch. > Last commit in block was in September. I haven't heard any objections from Andreas. >Please also note that the merge window just opened, so no patches will >be accepted for the next two weeks. > > Andrew
diff --git a/drivers/net/phy/qt2025.rs b/drivers/net/phy/qt2025.rs index 1ab065798175b4f54c5f2fd6c871ba2942c48bf1..25c12a02baa255d3d5952e729a890b3ccfe78606 100644 --- a/drivers/net/phy/qt2025.rs +++ b/drivers/net/phy/qt2025.rs @@ -39,7 +39,7 @@ impl Driver for PhyQT2025 { const NAME: &'static CStr = c_str!("QT2025 10Gpbs SFP+"); const PHY_DEVICE_ID: phy::DeviceId = phy::DeviceId::new_with_exact_mask(0x0043a400); - fn probe(dev: &mut phy::Device) -> Result<()> { + fn probe(dev: &mut phy::Device) -> Result { // Check the hardware revision code. // Only 0x3b works with this driver and firmware. let hw_rev = dev.read(C45::new(Mmd::PMAPMD, 0xd001))?; diff --git a/rust/kernel/block/mq/gen_disk.rs b/rust/kernel/block/mq/gen_disk.rs index 708125dce96a934f32caab44d5e6cff14c4321a9..798c4ae0bdedd58221b5851a630c0e1052e0face 100644 --- a/rust/kernel/block/mq/gen_disk.rs +++ b/rust/kernel/block/mq/gen_disk.rs @@ -45,7 +45,7 @@ pub fn rotational(mut self, rotational: bool) -> Self { /// Validate block size by verifying that it is between 512 and `PAGE_SIZE`, /// and that it is a power of two. - fn validate_block_size(size: u32) -> Result<()> { + fn validate_block_size(size: u32) -> Result { if !(512..=bindings::PAGE_SIZE as u32).contains(&size) || !size.is_power_of_two() { Err(error::code::EINVAL) } else { diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs index 05b0b8d13b10da731af62be03e1c2c13ced3f706..7c21304344ccd943816e38119a5be2ccf8d8e154 100644 --- a/rust/kernel/uaccess.rs +++ b/rust/kernel/uaccess.rs @@ -49,7 +49,7 @@ /// use kernel::error::Result; /// use kernel::uaccess::{UserPtr, UserSlice}; /// -/// fn bytes_add_one(uptr: UserPtr, len: usize) -> Result<()> { +/// fn bytes_add_one(uptr: UserPtr, len: usize) -> Result { /// let (read, mut write) = UserSlice::new(uptr, len).reader_writer(); /// /// let mut buf = KVec::new(); diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs index 4ab94e44adfe3206faad159e81417ea41a35815b..463920353ca9c408f5d69e2626c13a173bae98d7 100644 --- a/rust/macros/lib.rs +++ b/rust/macros/lib.rs @@ -144,11 +144,11 @@ pub fn module(ts: TokenStream) -> TokenStream { /// // Declares a `#[vtable]` trait /// #[vtable] /// pub trait Operations: Send + Sync + Sized { -/// fn foo(&self) -> Result<()> { +/// fn foo(&self) -> Result { /// kernel::build_error(VTABLE_DEFAULT_ERROR) /// } /// -/// fn bar(&self) -> Result<()> { +/// fn bar(&self) -> Result { /// kernel::build_error(VTABLE_DEFAULT_ERROR) /// } /// } @@ -158,7 +158,7 @@ pub fn module(ts: TokenStream) -> TokenStream { /// // Implements the `#[vtable]` trait /// #[vtable] /// impl Operations for Foo { -/// fn foo(&self) -> Result<()> { +/// fn foo(&self) -> Result { /// # Err(EINVAL) /// // ... /// }