netdev - Re: [PATCH v2 3/3] net: phy: add Rust Asix PHY driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a8625fd5-6083-4a4d-872a-c755c214b891@lunn.ch>
Date: Fri, 6 Oct 2023 18:55:23 +0200
From: Andrew Lunn <andrew@...n.ch>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: gregkh@...uxfoundation.org, netdev@...r.kernel.org,
	rust-for-linux@...r.kernel.org, miguel.ojeda.sandonis@...il.com
Subject: Re: [PATCH v2 3/3] net: phy: add Rust Asix PHY driver

> How about adding CONFIG_RUST_PHYLIB to the first patch. Not
> selectable, it's just a flag for Rust PHYLIB support.

We have to be careful with names. To some extent, CONFIG_PHYLIB means
the core of phylib. So it could be that CONFIG_RUST_PHYLIB means the
core of phylib written in rust? I doubt that will ever happen, but we
are setting a naming scheme here which i expect others will blindly
cut/paste. What we actually want is a symbol which represents the Rust
binding onto the phylib core. So i think it should have BINDING, or
WRAPPER or something like that in the name.

> diff --git a/init/Kconfig b/init/Kconfig
> index 4b4e3df1658d..2b6627aeb98c 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1889,7 +1889,7 @@ config RUST
>  	depends on !GCC_PLUGINS
>  	depends on !RANDSTRUCT
>  	depends on !DEBUG_INFO_BTF || PAHOLE_HAS_LANG_EXCLUDE
> 	depends on PHYLIB=y
> +	select RUST_PHYLIB

I know the rust build system is rather limited at the moment, but is
this required? Is it possible to build the rust code without the
phylib binding? Can your `RUST_PHYLIB` add phylib.rs to a Makefile
target only if it is enabled?

>  	select CONSTRUCTORS
>  	help
>  	  Enables Rust support in the kernel.
> @@ -1904,6 +1904,10 @@ config RUST
>  
>  	  If unsure, say N.
>  
> +config RUST_PHYLIB
> +	bool

This is where the depends on PHYLIB should be. It is the Rust binding
on phylib which has the dependency on phylib, not the core rust code.

What i think the end state should be, once the Rust build system is
better is that in drivers/net/phy/Kconfig we have:

if PHYLIB

config RUST_PHYLIB_BINDING
    bool
    depends on RUST
    help
      Adds support needed for PHY drivers written in Rust. It provides
      a wrapper around the C phlib core.

and the Makefile when uses this to build the binding as a kernel
module.

	Andrew