rust: simplify Result<()> uses

Commit Message

Manas via B4 Relay Nov. 17, 2024, 3:11 p.m. UTC

From: Manas <[e-mail]>

This patch replaces `Result<()>` with `Result`.

Suggested-by: Miguel Ojeda <[e-mail]>
Link: https://github.com/Rust-for-Linux/linux/issues/1128
Signed-off-by: Manas <[e-mail]>
---
 drivers/net/phy/qt2025.rs        | 2 +-
 rust/kernel/block/mq/gen_disk.rs | 2 +-
 rust/kernel/uaccess.rs           | 2 +-
 rust/macros/lib.rs               | 6 +++---
 4 files changed, 6 insertions(+), 6 deletions(-)


---
base-commit: b2603f8ac8217bc59f5c7f248ac248423b9b99cb
change-id: 20241117-simplify-result-cad98af090c9

Best regards,

Comments

Andrew Lunn Nov. 17, 2024, 6:25 p.m. UTC | #1

On Sun, Nov 17, 2024 at 08:41:47PM +0530, Manas via B4 Relay wrote:
> From: Manas <[e-mail]>
> 
> This patch replaces `Result<()>` with `Result`.
> 
> Suggested-by: Miguel Ojeda <[e-mail]>
> Link: https://github.com/Rust-for-Linux/linux/issues/1128
> Signed-off-by: Manas <[e-mail]>
> ---
>  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

Russell King (Oracle) Nov. 17, 2024, 6:56 p.m. UTC | #2

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 <[e-mail]>
> > 
> > This patch replaces `Result<()>` with `Result`.
> > 
> > Suggested-by: Miguel Ojeda <[e-mail]>
> > Link: https://github.com/Rust-for-Linux/linux/issues/1128
> > Signed-off-by: Manas <[e-mail]>
> > ---
> >  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.)

Russell King (Oracle) Nov. 17, 2024, 7:12 p.m. UTC | #3

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 <[e-mail]>
> > > 
> > > This patch replaces `Result<()>` with `Result`.
> > > 
> > > Suggested-by: Miguel Ojeda <[e-mail]>
> > > Link: https://github.com/Rust-for-Linux/linux/issues/1128
> > > Signed-off-by: Manas <[e-mail]>
> > > ---
> > >  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!

Miguel Ojeda Nov. 17, 2024, 8:49 p.m. UTC | #4

On Sun, Nov 17, 2024 at 7:26 PM Andrew Lunn <[e-mail]> 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

Manas Nov. 18, 2024, 12:11 a.m. UTC | #5

On 17.11.2024 21:49, Miguel Ojeda wrote:
>On Sun, Nov 17, 2024 at 7:26 PM Andrew Lunn <[e-mail]> 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 <[e-mail]>
>> >
>> > This patch replaces `Result<()>` with `Result`.
>> >
>> > Suggested-by: Miguel Ojeda <[e-mail]>
>> > Link: https://github.com/Rust-for-Linux/linux/issues/1128
>> > Signed-off-by: Manas <[e-mail]>
>> > ---
>> >  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.

Miguel Ojeda Nov. 18, 2024, 12:54 a.m. UTC | #6

On Mon, Nov 18, 2024 at 1:11 AM Manas <[e-mail]> 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

Manas Nov. 18, 2024, 1:11 a.m. UTC | #7

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.

Miguel Ojeda Nov. 18, 2024, 9:09 a.m. UTC | #8

On Mon, Nov 18, 2024 at 2:12 AM Manas <[e-mail]> 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 Lunn Nov. 18, 2024, 1:29 p.m. UTC | #9

> 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

Manas Nov. 18, 2024, 2:44 p.m. UTC | #10

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

Patch

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)
 ///         // ...
 ///     }