Re: [PATCH v16 1/4] rust: remove redundant `as _` casts

[Danilo Krummrich <dakr@kernel.org>]

On Fri, Feb 07, 2025 at 08:58:24AM -0500, Tamir Duberstein wrote:
> Remove redundant casts added in commit 1bd8b6b2c5d3 ("rust: pci: add
> basic PCI device / driver abstractions") and commit 683a63befc73 ("rust:
> platform: add basic platform device / driver abstractions")

I thought of doing it the other way around: Do only the *required* changes in
commit "rust: types: add `ForeignOwnable::PointedTo`", i.e. no need to touch
this code at all. And then switch to cast() in a subsequent patch.

This way you don't need to remove (previously unnecessary) casts and then add
them back in.

> 
> While I'm churning this line, move the `.into_foreign()` call to its own
> statement to avoid churn in the next commit which adds a `.cast()` call.
> 
> Fixes: 1bd8b6b2c5d3 ("rust: pci: add basic PCI device / driver abstractions")
> Fixes: 683a63befc73 ("rust: platform: add basic platform device / driver abstractions")

No, at the time those casts were indeed necessary, because the types differed in
mutability.

"A Fixes: tag indicates that the patch fixes an issue in a previous commit." [1]

Even if the cast was unnecessary in the first place, it is at least questionable
to me whether this falls under "fixing an issue".

[1] https://docs.kernel.org/process/submitting-patches.html#reviewer-s-statement-of-oversight

> Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> ---
>  rust/kernel/pci.rs      | 3 ++-
>  rust/kernel/platform.rs | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 4c98b5b9aa1e..6c3bc14b42ad 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -72,10 +72,11 @@ extern "C" fn probe_callback(
>  
>          match T::probe(&mut pdev, info) {
>              Ok(data) => {
> +                let data = data.into_foreign();
>                  // Let the `struct pci_dev` own a reference of the driver's private data.
>                  // SAFETY: By the type invariant `pdev.as_raw` returns a valid pointer to a
>                  // `struct pci_dev`.
> -                unsafe { bindings::pci_set_drvdata(pdev.as_raw(), data.into_foreign() as _) };
> +                unsafe { bindings::pci_set_drvdata(pdev.as_raw(), data) };

Please do not factor this out, it is unnecessary for what you want to accomplish
with your commit.

>              }
>              Err(err) => return Error::to_errno(err),
>          }
> diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
> index 50e6b0421813..dea104563fa9 100644
> --- a/rust/kernel/platform.rs
> +++ b/rust/kernel/platform.rs
> @@ -63,10 +63,11 @@ extern "C" fn probe_callback(pdev: *mut bindings::platform_device) -> kernel::ff
>          let info = <Self as driver::Adapter>::id_info(pdev.as_ref());
>          match T::probe(&mut pdev, info) {
>              Ok(data) => {
> +                let data = data.into_foreign();
>                  // Let the `struct platform_device` own a reference of the driver's private data.
>                  // SAFETY: By the type invariant `pdev.as_raw` returns a valid pointer to a
>                  // `struct platform_device`.
> -                unsafe { bindings::platform_set_drvdata(pdev.as_raw(), data.into_foreign() as _) };
> +                unsafe { bindings::platform_set_drvdata(pdev.as_raw(), data) };
>              }
>              Err(err) => return Error::to_errno(err),
>          }
> 
> -- 
> 2.48.1
>