| Message ID | 20250207-rust-xarray-bindings-v16-1-256b0cf936bd@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | rust: xarray: Add a minimal abstraction for XArray | expand |
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 >
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) }; } 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), }
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") 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") 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(-)