| Message ID | 20240125014502.3527275-2-fujita.tomonori@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 599b75a3b753ad06444a4e9667cb0b7545c297c3 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] rust: phy: use VTABLE_DEFAULT_ERROR | expand |
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>
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
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
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!
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.
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(-)