| Message ID | 20231026001050.1720612-4-fujita.tomonori@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Rust abstractions for network PHY drivers | expand |
On Thu, Oct 26, 2023 at 2:16 AM FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > > From: Miguel Ojeda <ojeda@kernel.org> > > This patch makes sure that the C's enum is sync with Rust sides. If > the enum is out of sync, compiling fails with an error like the > following. > > Note that this is a temporary solution. It will be replaced with > bindgen when it supports generating the enum conversion code. > Signed-off-by: Miguel Ojeda <ojeda@kernel.org> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> Please do not modify patches from others without warning that you did so. I did not write this commit message nor agreed to this, but it looks as if I did. I even explicitly said I would send the patch independently. As I recently told you, if you want to pick it up in your series to showcase how it would work, you should have at least kept the WIP, put it at the end of the series and added RFC since it is not intended to be merged with your other patches. Cheers, Miguel
On Thu, 26 Oct 2023 13:02:57 +0200 Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > On Thu, Oct 26, 2023 at 2:16 AM FUJITA Tomonori > <fujita.tomonori@gmail.com> wrote: >> >> From: Miguel Ojeda <ojeda@kernel.org> >> >> This patch makes sure that the C's enum is sync with Rust sides. If >> the enum is out of sync, compiling fails with an error like the >> following. >> >> Note that this is a temporary solution. It will be replaced with >> bindgen when it supports generating the enum conversion code. > >> Signed-off-by: Miguel Ojeda <ojeda@kernel.org> >> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> > > Please do not modify patches from others without warning that you did > so. I did not write this commit message nor agreed to this, but it > looks as if I did. I even explicitly said I would send the patch > independently. > > As I recently told you, if you want to pick it up in your series to > showcase how it would work, you should have at least kept the WIP, put > it at the end of the series and added RFC since it is not intended to > be merged with your other patches. Sorry, I totally misunderstand your intention. I thought that the PHY abstractions needs to be merged with your patch together. I'll drop your patch in the next version and focus on my patches.
On Thu, Oct 26, 2023 at 1:54 PM FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > > Sorry, I totally misunderstand your intention. I thought that the PHY > abstractions needs to be merged with your patch together. > > I'll drop your patch in the next version and focus on my patches. No harm done! I understand you were trying to help, and I apologize if I sounded too harsh. Your abstractions are not blocked on this patch -- they could go in without this, that is why I suggested marking this one as RFC and putting it at the end of the series. The exhaustiveness check here is an extra feature that prevents a class of bugs, which is great, but it does not really affect the abstractions, i.e. there is no unsoundness in your code whether this patch is in or not. I will send the patch soon, and assuming it lands, then you can start using the feature if you wish. I would recommend basing your patches on top of that patch (or `rust-next` when the patch lands), so that your PHY series contains the addition of `check_phy_state`. Cheers, Miguel
On Thu, Oct 26, 2023 at 02:22:23PM +0200, Miguel Ojeda wrote: > On Thu, Oct 26, 2023 at 1:54 PM FUJITA Tomonori > <fujita.tomonori@gmail.com> wrote: > > > > Sorry, I totally misunderstand your intention. I thought that the PHY > > abstractions needs to be merged with your patch together. > > > > I'll drop your patch in the next version and focus on my patches. > > No harm done! I understand you were trying to help, and I apologize if > I sounded too harsh. > > Your abstractions are not blocked on this patch -- they could go in > without this, that is why I suggested marking this one as RFC and > putting it at the end of the series. That is not how netdev works. It messes up the patch flow, since the machinery expects to commit all or nothing. The best way forwards is you create a stable branch with this patch. The netdev Maintainer can then pull that branch into netdev, and Tomonori can then add his patches using it on top. When everything meets up in linux-next, git then recognises it has the same patch twice and drops one of them, depending on the order of the merge. > I will send the patch soon, and assuming it lands, then you can start > using the feature if you wish. I would recommend basing your patches > on top of that patch (or `rust-next` when the patch lands), That does not work. Networking patches need to be on net-next. The stable branch solves that when we have cross subsystem dependencies. Andrew
On Fri, Oct 27, 2023 at 2:07 AM Andrew Lunn <andrew@lunn.ch> wrote: > > That is not how netdev works. It messes up the patch flow, since the > machinery expects to commit all or nothing. Then please simply drop this patch (or improve the machinery :) > The best way forwards is you create a stable branch with this > patch. The netdev Maintainer can then pull that branch into netdev, > and Tomonori can then add his patches using it on top. When everything > meets up in linux-next, git then recognises it has the same patch > twice and drops one of them, depending on the order of the merge. That is only needed if you want to land all this in the next cycle, do you? Moreover, it also assumes this exhaustiveness check lands -- it has not been posted/discussed/agreed yet. Thus, if either of those are false, then this bit (or the entire series) could just wait one cycle. > That does not work. Networking patches need to be on net-next. I have not said the patches should not go through net-next, though. And what I suggested definitely works. > The > stable branch solves that when we have cross subsystem dependencies. Yes, we are aware of that, thank you. We are even doing it right now with another subsystem. Cheers, Miguel
diff --git a/rust/.gitignore b/rust/.gitignore index d3829ffab80b..1a76ad0d6603 100644 --- a/rust/.gitignore +++ b/rust/.gitignore @@ -1,6 +1,7 @@ # SPDX-License-Identifier: GPL-2.0 bindings_generated.rs +bindings_generated_enum_check.rs bindings_helpers_generated.rs doctests_kernel_generated.rs doctests_kernel_generated_kunit.c diff --git a/rust/Makefile b/rust/Makefile index 87958e864be0..a622111c8c50 100644 --- a/rust/Makefile +++ b/rust/Makefile @@ -15,6 +15,7 @@ always-$(CONFIG_RUST) += libmacros.so no-clean-files += libmacros.so always-$(CONFIG_RUST) += bindings/bindings_generated.rs bindings/bindings_helpers_generated.rs +always-$(CONFIG_RUST) += bindings/bindings_generated_enum_check.rs obj-$(CONFIG_RUST) += alloc.o bindings.o kernel.o always-$(CONFIG_RUST) += exports_alloc_generated.h exports_bindings_generated.h \ exports_kernel_generated.h @@ -341,6 +342,19 @@ $(obj)/bindings/bindings_generated.rs: $(src)/bindings/bindings_helper.h \ $(src)/bindgen_parameters FORCE $(call if_changed_dep,bindgen) +$(obj)/bindings/bindings_generated_enum_check.rs: private bindgen_target_flags = \ + $(shell grep -v '^#\|^$$' $(srctree)/$(src)/bindgen_parameters) \ + --default-enum-style rust +$(obj)/bindings/bindings_generated_enum_check.rs: private bindgen_target_extra = ; \ + OBJTREE=$(abspath $(objtree)) $(RUSTC_OR_CLIPPY) $(rust_flags) $(rustc_target_flags) \ + --crate-type rlib -L$(objtree)/$(obj) \ + --emit=dep-info=$(obj)/bindings/.bindings_enum_check.rs.d \ + --emit=metadata=$(obj)/bindings/libbindings_enum_check.rmeta \ + --crate-name enum_check $(srctree)/$(src)/bindings/bindings_enum_check.rs +$(obj)/bindings/bindings_generated_enum_check.rs: $(src)/bindings/bindings_helper.h \ + $(src)/bindings/bindings_enum_check.rs $(src)/bindgen_parameters FORCE + $(call if_changed_dep,bindgen) + $(obj)/uapi/uapi_generated.rs: private bindgen_target_flags = \ $(shell grep -v '^#\|^$$' $(srctree)/$(src)/bindgen_parameters) $(obj)/uapi/uapi_generated.rs: $(src)/uapi/uapi_helper.h \ diff --git a/rust/bindings/bindings_enum_check.rs b/rust/bindings/bindings_enum_check.rs new file mode 100644 index 000000000000..eef7e9ca3c54 --- /dev/null +++ b/rust/bindings/bindings_enum_check.rs @@ -0,0 +1,36 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Bindings' enum exhaustiveness check. +//! +//! Eventually, this should be replaced by a safe version of `--rustified-enum`, see +//! https://github.com/rust-lang/rust-bindgen/issues/2646. + +#![no_std] +#![allow( + clippy::all, + dead_code, + missing_docs, + non_camel_case_types, + non_upper_case_globals, + non_snake_case, + improper_ctypes, + unreachable_pub, + unsafe_op_in_unsafe_fn +)] + +include!(concat!( + env!("OBJTREE"), + "/rust/bindings/bindings_generated_enum_check.rs" +)); + +fn check_phy_state( + (phy_state::PHY_DOWN + | phy_state::PHY_READY + | phy_state::PHY_HALTED + | phy_state::PHY_ERROR + | phy_state::PHY_UP + | phy_state::PHY_RUNNING + | phy_state::PHY_NOLINK + | phy_state::PHY_CABLETEST): phy_state, +) { +}