[PATCH net-next] rust: phy: use `srctree`-relative links

[PATCH net-next] rust: phy: use `srctree`-relative links

[FUJITA Tomonori]

The relative paths like the following are bothersome and don't work
with `O=` builds:

//! C headers: [`include/linux/phy.h`](../../../../../../../include/linux/phy.h).

This updates such links by using the `srctree`-relative link feature
introduced in 6.8-rc1 like:

//! C headers: [`include/linux/phy.h`](srctree/include/linux/phy.h).

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/kernel/net/phy.rs | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index e457b3c7cb2f..203918192a1f 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -4,7 +4,7 @@
 
 //! Network PHY device.
 //!
-//! C headers: [`include/linux/phy.h`](../../../../../../../include/linux/phy.h).
+//! C headers: [`include/linux/phy.h`](srctree/include/linux/phy.h).
 
 use crate::{bindings, error::*, prelude::*, str::CStr, types::Opaque};
 
@@ -16,7 +16,7 @@
 ///
 /// Some of PHY drivers access to the state of PHY's software state machine.
 ///
-/// [`enum phy_state`]: ../../../../../../../include/linux/phy.h
+/// [`enum phy_state`]: srctree/include/linux/phy.h
 #[derive(PartialEq, Eq)]
 pub enum DeviceState {
     /// PHY device and driver are not ready for anything.
@@ -61,7 +61,7 @@ pub enum DuplexMode {
 /// Referencing a `phy_device` using this struct asserts that you are in
 /// a context where all methods defined on this struct are safe to call.
 ///
-/// [`struct phy_device`]: ../../../../../../../include/linux/phy.h
+/// [`struct phy_device`]: srctree/include/linux/phy.h
 // During the calls to most functions in [`Driver`], the C side (`PHYLIB`) holds a lock that is
 // unique for every instance of [`Device`]. `PHYLIB` uses a different serialization technique for
 // [`Driver::resume`] and [`Driver::suspend`]: `PHYLIB` updates `phy_device`'s state with
@@ -486,7 +486,7 @@ impl<T: Driver> Adapter<T> {
 ///
 /// `self.0` is always in a valid state.
 ///
-/// [`struct phy_driver`]: ../../../../../../../include/linux/phy.h
+/// [`struct phy_driver`]: srctree/include/linux/phy.h
 #[repr(transparent)]
 pub struct DriverVTable(Opaque<bindings::phy_driver>);
 

base-commit: fa47527c71dceb2fd4fb3b17104df53f7aed8d49
-- 
2.34.1

[PATCH net-next] rust: phy: use VTABLE_DEFAULT_ERROR

[FUJITA Tomonori]

Since 6.8-rc1, using VTABLE_DEFAULT_ERROR for optional functions
(never called) in #[vtable] is the recommended way.

Note that no functional changes in this patch.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/kernel/net/phy.rs | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
index 203918192a1f..96e09c6e8530 100644
--- a/rust/kernel/net/phy.rs
+++ b/rust/kernel/net/phy.rs
@@ -580,12 +580,12 @@ pub trait Driver {
 
     /// Issues a PHY software reset.
     fn soft_reset(_dev: &mut Device) -> Result {
-        Err(code::ENOTSUPP)
+        kernel::build_error(VTABLE_DEFAULT_ERROR)
     }
 
     /// Probes the hardware to determine what abilities it has.
     fn get_features(_dev: &mut Device) -> Result {
-        Err(code::ENOTSUPP)
+        kernel::build_error(VTABLE_DEFAULT_ERROR)
     }
 
     /// Returns true if this is a suitable driver for the given phydev.
@@ -597,32 +597,32 @@ fn match_phy_device(_dev: &Device) -> bool {
     /// Configures the advertisement and resets auto-negotiation
     /// if auto-negotiation is enabled.
     fn config_aneg(_dev: &mut Device) -> Result {
-        Err(code::ENOTSUPP)
+        kernel::build_error(VTABLE_DEFAULT_ERROR)
     }
 
     /// Determines the negotiated speed and duplex.
     fn read_status(_dev: &mut Device) -> Result<u16> {
-        Err(code::ENOTSUPP)
+        kernel::build_error(VTABLE_DEFAULT_ERROR)
     }
 
     /// Suspends the hardware, saving state if needed.
     fn suspend(_dev: &mut Device) -> Result {
-        Err(code::ENOTSUPP)
+        kernel::build_error(VTABLE_DEFAULT_ERROR)
     }
 
     /// Resumes the hardware, restoring state if needed.
     fn resume(_dev: &mut Device) -> Result {
-        Err(code::ENOTSUPP)
+        kernel::build_error(VTABLE_DEFAULT_ERROR)
     }
 
     /// Overrides the default MMD read function for reading a MMD register.
     fn read_mmd(_dev: &mut Device, _devnum: u8, _regnum: u16) -> Result<u16> {
-        Err(code::ENOTSUPP)
+        kernel::build_error(VTABLE_DEFAULT_ERROR)
     }
 
     /// Overrides the default MMD write function for writing a MMD register.
     fn write_mmd(_dev: &mut Device, _devnum: u8, _regnum: u16, _val: u16) -> Result {
-        Err(code::ENOTSUPP)
+        kernel::build_error(VTABLE_DEFAULT_ERROR)
     }
 
     /// Callback for notification of link change.
-- 
2.34.1

Re: [PATCH net-next] rust: phy: use `srctree`-relative links

[Trevor Gross]

On Wed, Jan 24, 2024 at 8:47 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> The relative paths like the following are bothersome and don't work
> with `O=` builds:
>
> //! C headers: [`include/linux/phy.h`](../../../../../../../include/linux/phy.h).
>
> This updates such links by using the `srctree`-relative link feature
> introduced in 6.8-rc1 like:
>
> //! C headers: [`include/linux/phy.h`](srctree/include/linux/phy.h).
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

Link: https://lore.kernel.org/lkml/20231215235428.243211-1-ojeda@kernel.org/

Reviewed-by: Trevor Gross <tmgross@umich.edu>

Re: [PATCH net-next] rust: phy: use VTABLE_DEFAULT_ERROR

[Trevor Gross]

On Wed, Jan 24, 2024 at 8:47 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Since 6.8-rc1, using VTABLE_DEFAULT_ERROR for optional functions
> (never called) in #[vtable] is the recommended way.
>
> Note that no functional changes in this patch.
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

Link: https://lore.kernel.org/lkml/20231026201855.1497680-1-benno.lossin@proton.me/

Reviewed-by: Trevor Gross <tmgross@umich.edu>

Re: [PATCH net-next] rust: phy: use VTABLE_DEFAULT_ERROR

[Andrew Lunn]

On Thu, Jan 25, 2024 at 10:45:02AM +0900, FUJITA Tomonori wrote:
> Since 6.8-rc1, using VTABLE_DEFAULT_ERROR for optional functions
> (never called) in #[vtable] is the recommended way.
> 
> Note that no functional changes in this patch.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/kernel/net/phy.rs | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs
> index 203918192a1f..96e09c6e8530 100644
> --- a/rust/kernel/net/phy.rs
> +++ b/rust/kernel/net/phy.rs
> @@ -580,12 +580,12 @@ pub trait Driver {
>  
>      /// Issues a PHY software reset.
>      fn soft_reset(_dev: &mut Device) -> Result {
> -        Err(code::ENOTSUPP)
> +        kernel::build_error(VTABLE_DEFAULT_ERROR)
>      }

Dumb question, which i should probably duckduckgo myself.

Why is it called VTABLE_DEFAULT_ERROR and not
VTABLE_NOT_SUPPORTED_ERROR?

Looking through the C code my guess would be -EINVAL would be the
default, or maybe 0.

The semantics of ENOTSUPP can also vary. Its often not an actual
error, it just a means to indicate its not supported, and the caller
might decide to do something different as a result. One typical use in
the network stack is offloading functionality to hardware.
e.g. blinking the LEDs on the RJ45 connected. The driver could be
asked to blink to show activity of a received packet. Some hardware
can only indicate activity for both receive and transmit, so the
driver would return -EOPNOTSUPP. Software blinking would then be used
instead of hardware blinking.

	 Andrew

Re: [PATCH net-next] rust: phy: use VTABLE_DEFAULT_ERROR

[Trevor Gross]

On Thu, Jan 25, 2024 at 11:42 AM Andrew Lunn <andrew@lunn.ch> wrote:
> > @@ -580,12 +580,12 @@ pub trait Driver {
> >
> >      /// Issues a PHY software reset.
> >      fn soft_reset(_dev: &mut Device) -> Result {
> > -        Err(code::ENOTSUPP)
> > +        kernel::build_error(VTABLE_DEFAULT_ERROR)
> >      }
>
> Dumb question, which i should probably duckduckgo myself.
>
> Why is it called VTABLE_DEFAULT_ERROR and not
> VTABLE_NOT_SUPPORTED_ERROR?
>
> Looking through the C code my guess would be -EINVAL would be the
> default, or maybe 0.
>
> The semantics of ENOTSUPP can also vary. Its often not an actual
> error, it just a means to indicate its not supported, and the caller
> might decide to do something different as a result. One typical use in
> the network stack is offloading functionality to hardware.
> e.g. blinking the LEDs on the RJ45 connected. The driver could be
> asked to blink to show activity of a received packet. Some hardware
> can only indicate activity for both receive and transmit, so the
> driver would return -EOPNOTSUPP. Software blinking would then be used
> instead of hardware blinking.
>
>          Andrew

`build_error` is a bit special and is implemented as follows:

1. If called in a `const fn`, it will fail the build
2. If the build_error symbol is in a final binary, tooling should
detect this and raise some kind of error (config
RUST_BUILD_ASSERT_ALLOW, not exactly positive how this is implemented)
3. If called at runtime, something is very unexpected so we panic

To make a trait function optional in rust, you need to provide a
default (since all functions in a trait must always be callable). But
if the C side can handle a null function pointer, it's better to just
set that instead of reimplementing the default from Rust.

So it's up to the abstraction to set these fields to NULL/None if not
implemented, e.g. from create_phy_driver in phy.rs:

        soft_reset: if T::HAS_SOFT_RESET {
            Some(Adapter::<T>::soft_reset_callback)
        } else {
            None
        },

If an abstraction does this wrong and tries to assign a default
function when it would be better to get a null pointer, then we get
the build error. So this is something that would pop up if the
abstraction is done wrong, not just if somebody writes a bad phy
driver.

VTABLE_DEFAULT_ERROR isn't a kernel errno, it's a string printed to be
printed by build_error or the tooling.

- Trevor

Re: [PATCH net-next] rust: phy: use `srctree`-relative links

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:

On Thu, 25 Jan 2024 10:45:01 +0900 you wrote:
> The relative paths like the following are bothersome and don't work
> with `O=` builds:
> 
> //! C headers: [`include/linux/phy.h`](../../../../../../../include/linux/phy.h).
> 
> This updates such links by using the `srctree`-relative link feature
> introduced in 6.8-rc1 like:
> 
> [...]

Here is the summary with links:
  - [net-next] rust: phy: use `srctree`-relative links
    https://git.kernel.org/netdev/net-next/c/1d4046b57142

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next] rust: phy: use VTABLE_DEFAULT_ERROR

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:

On Thu, 25 Jan 2024 10:45:02 +0900 you wrote:
> Since 6.8-rc1, using VTABLE_DEFAULT_ERROR for optional functions
> (never called) in #[vtable] is the recommended way.
> 
> Note that no functional changes in this patch.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> 
> [...]

Here is the summary with links:
  - [net-next] rust: phy: use VTABLE_DEFAULT_ERROR
    https://git.kernel.org/netdev/net-next/c/599b75a3b753

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html