Date: Fri, 13 Oct 2023 13:59:12 +0200
From: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: gregkh@...uxfoundation.org, netdev@...r.kernel.org, 
	rust-for-linux@...r.kernel.org, andrew@...n.ch, tmgross@...ch.edu, 
	wedsonaf@...il.com
Subject: Re: [PATCH net-next v3 1/3] rust: core abstractions for network PHY drivers

On Thu, Oct 12, 2023 at 1:18 AM FUJITA Tomonori
<fujita.tomonori@...il.com> wrote:
>
> IIRC, Andrew prefers to avoid creating a temporary rust variant (Greg
> does too, I understand). I guess that only solusion that both Rust and
> C devs would be happy with is generating safe Rust code from C. The

As far as I understand, the workaround I just suggested in the
previous reply was not discussed so far. I am not sure which of the
alternatives you mean by the "temporary rust variant", so I may be
misunderstanding your message.

> solution is still a prototype and I don't know when it will be
> available (someone knows?).

If no alternative is good enough, and you do not have time to
implement one of the better solutions, then we need to wait until one
of us (or somebody else) implements it. I understand that can be
frustrating, but we cannot really agree to start using
`--rustified-enum` or, in general, ways to introduce UB where we
already have known solutions.

Instead, we prefer to spend some time iterating on this sort of
problem. It is also not the first time at all we have done this, e.g.
see `pin-init`. It is all about trying to avoid compromising, unless
the solution is really far away.

Having said that, to try to unblock things, I spent some time
prototyping the workaround I suggested, see below [1]. That catches
the "new C variant added" desync between Rust and C.

For instance, if I add a `PHY_NEW` variant, then I get:

    error[E0005]: refutable pattern in function argument
         --> rust/bindings/bindings_enum_check.rs:29:6
          |
    29    |       (phy_state::PHY_DOWN
          |  ______^
    30    | |     | phy_state::PHY_READY
    31    | |     | phy_state::PHY_HALTED
    32    | |     | phy_state::PHY_ERROR
    ...     |
    35    | |     | phy_state::PHY_NOLINK
    36    | |     | phy_state::PHY_CABLETEST): phy_state,
          | |______________________________^ pattern
`phy_state::PHY_NEW` not covered
          |
    note: `phy_state` defined here
         --> rust/bindings/bindings_generated_enum_check.rs:60739:10
          |
    60739 | pub enum phy_state {
          |          ^^^^^^^^^
    ...
    60745 |     PHY_NEW = 5,
          |     ------- not covered
          = note: the matched value is of type `phy_state`

It seems to work fine and would allow us to use the wildcard `_`
without risk of desync, and without needing changes on the C enum.

> Sorry, there's no excuse. I should have done better. I'll make sure
> that the code is warning-free.

No problem at all -- it is all fine. I hope the workaround is suitable
and unblocks you. Please let me know and I can send it as a patch.

However, I would still prefer that it is done in `bindgen` -- when we
talked about that possibility in the weekly meeting a couple days ago,
we thought it could be doable to have it ready for the next kernel
version if somebody steps up to do it now. I could do it, but not
before LPC.

Cheers,
Miguel

[1]

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..4a1c7a48dfad 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 $(obj)/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..7c62bab12ea1
--- /dev/null
+++ b/rust/bindings/bindings_enum_check.rs
@@ -0,0 +1,38 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Bindings exhaustiveness enum 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,
+) {
+}

Powered by blists - more mailing lists