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: <20231007.002609.681250079112313735.fujita.tomonori@gmail.com>
Date: Sat, 07 Oct 2023 00:26:09 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: andrew@...n.ch
Cc: fujita.tomonori@...il.com, 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

On Fri, 6 Oct 2023 16:35:28 +0200
Andrew Lunn <andrew@...n.ch> wrote:

>> The Kconfig file would be like the following. AX88796B_RUST_PHY
>> depends on AX88796B_PHY so the description of AX88796B_PHY is enough?
>> I'll add the name of the module.
>> 
>> 
>> config AX88796B_PHY
>> 	tristate "Asix PHYs"
>> 	help
>> 	  Currently supports the Asix Electronics PHY found in the X-Surf 100
>> 	  AX88796B package.
> 
> I _think_ you can add
> 
> 	depends on !AX88796B_RUST_PHY
> 
>> config AX88796B_RUST_PHY
>> 	bool "Rust reference driver"
>> 	depends on RUST && AX88796B_PHY
> 
> And then this becomes
> 
>     	depends on RUST && !AX88796B_PHY
> 
>> 	default n
>> 	help
>> 	  Uses the Rust version driver for Asix PHYs.
> 
> You then express the mutual exclusion in Kconfig, so that only one of
> AX88796B_PHY and AX88796B_RUST_PHY is ever enabled.
> 
> I've not actually tried this, so it might not work. Ideally you need
> to be able disable both, so that you can enable one.

This doesn't work.

ubuntu@...172-30-47-114:~/git/linux$ make LLVM=1 -j 32 menuconfig
drivers/net/phy/Kconfig:111:error: recursive dependency detected!
drivers/net/phy/Kconfig:111:    symbol AX88796B_RUST_PHY depends on AX88796B_PHY
drivers/net/phy/Kconfig:104:    symbol AX88796B_PHY depends on AX88796B_RUST_P

The following gurantees that only one is built but we hit the `select
AX88796B_PHY` problem in my previous mail.

config AX88796B_PHY
        tristate "Asix PHYs"
        help
          Currently supports the Asix Electronics PHY found in the X-Surf 100
          AX88796B package.

config AX88796B_RUST_PHY
        bool "Rust reference driver"
        depends on RUST && AX88796B_PHY=n
        help
          Uses the Rust version driver for Asix PHYs.


Greg, Sorry. I messed up copy-and-paste in the previous mail. I think that you meant the above.

> There is good documentation in
> 
> Documentation/kbuild/kconfig-language.rst
> 
>> >> +ifdef CONFIG_AX88796B_RUST_PHY
>> >> +  obj-$(CONFIG_AX88796B_PHY)	+= ax88796b_rust.o
>> >> +else
>> >> +  obj-$(CONFIG_AX88796B_PHY)	+= ax88796b.o
>> >> +endif
>> > 
>> > This can be expressed in Kconfig, no need to put this here, right?
>> 
>> Not sure. Is it possible? If we allow both modules to be built, I
>> guess it's possible though.
> 
> If what i suggested above works, you don't need the ifdef, just list
> the two drivers are normal and let Kconfig only enable one at most.
> Or go back to your idea of using choice. Maybe something like
> 
> choice
> 	tristate "AX88796B PHY driver"
> 
> 	config CONFIG_AX88796B_PHY
> 		bool "C driver"
> 
> 	config CONFIG_AX88796B_RUST_PHY
> 	        bool "Rust driver"
> 		depends on RUST
> endchoice
> 
> totally untested....

Now I'm thinking that this is the best option. Kconfig would be the following:

config AX88796B_PHY
        tristate "Asix PHYs"
        help
         Currently supports the Asix Electronics PHY found in the X-Surf 100
         AX88796B package.

choice
        prompt "Implementation options"
        depends on AX88796B_PHY
        help
         There are two implementations for a driver for Asix PHYs; C and Rust.
         If not sure, choose C.

config AX88796B_C_PHY
        bool "The C version driver for Asix PHYs"

config AX88796B_RUST_PHY
        bool "The Rust version driver for Asix PHYs"
        depends on RUST

endchoice


No hack in Makefile:

obj-$(CONFIG_AX88796B_C_PHY)    += ax88796b.o
obj-$(CONFIG_AX88796B_RUST_PHY) += ax88796b_rust.o