netdev - Re: [PATCH net-next v5 1/5] rust: core abstractions for network PHY drivers

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: <87sf65gpi0.fsf@metaspace.dk>
Date: Fri, 20 Oct 2023 19:26:50 +0200
From: "Andreas Hindborg (Samsung)" <nmi@...aspace.dk>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: netdev@...r.kernel.org, rust-for-linux@...r.kernel.org, andrew@...n.ch,
 miguel.ojeda.sandonis@...il.com, tmgross@...ch.edu, boqun.feng@...il.com,
 wedsonaf@...il.com, benno.lossin@...ton.me, greg@...ah.com
Subject: Re: [PATCH net-next v5 1/5] rust: core abstractions for network PHY
 drivers


Hi,

FUJITA Tomonori <fujita.tomonori@...il.com> writes:

<cut>

> +
> +    /// Returns true if the link is up.
> +    pub fn get_link(&self) -> bool {
> +        const LINK_IS_UP: u32 = 1;
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        let phydev = unsafe { *self.0.get() };
> +        phydev.link() == LINK_IS_UP
> +    }

I would prefer `is_link_up` or `link_is_up`.

> +
> +    /// Returns true if auto-negotiation is enabled.
> +    pub fn is_autoneg_enabled(&self) -> bool {
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        let phydev = unsafe { *self.0.get() };
> +        phydev.autoneg() == bindings::AUTONEG_ENABLE
> +    }
> +
> +    /// Returns true if auto-negotiation is completed.
> +    pub fn is_autoneg_completed(&self) -> bool {
> +        const AUTONEG_COMPLETED: u32 = 1;
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        let phydev = unsafe { *self.0.get() };
> +        phydev.autoneg_complete() == AUTONEG_COMPLETED
> +    }
> +
> +    /// Sets the speed of the PHY.
> +    pub fn set_speed(&mut self, speed: u32) {
> +        let phydev = self.0.get();
> +        // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`.
> +        unsafe { (*phydev).speed = speed as i32 };
> +    }

If this function is called with `u32::MAX` `(*phydev).speed` will become -1. Is that OK?

<cut>

> +
> +/// An instance of a PHY driver.
> +///
> +/// Wraps the kernel's `struct phy_driver`.
> +///
> +/// # Invariants
> +///
> +/// `self.0` is always in a valid state.
> +#[repr(transparent)]
> +pub struct DriverType(Opaque<bindings::phy_driver>);

I don't like the name `DriverType`. How about `DriverDesciptor` or
something like that?

<cut>

> +
> +/// Corresponds to functions in `struct phy_driver`.
> +///
> +/// This is used to register a PHY driver.
> +#[vtable]
> +pub trait Driver {
> +    /// Defines certain other features this PHY supports.
> +    /// It is a combination of the flags in the [`flags`] module.
> +    const FLAGS: u32 = 0;
> +
> +    /// The friendly name of this PHY type.
> +    const NAME: &'static CStr;
> +
> +    /// This driver only works for PHYs with IDs which match this field.
> +    /// The default id and mask are zero.
> +    const PHY_DEVICE_ID: DeviceId = DeviceId::new_with_custom_mask(0, 0);
> +
> +    /// Issues a PHY software reset.
> +    fn soft_reset(_dev: &mut Device) -> Result {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Probes the hardware to determine what abilities it has.
> +    fn get_features(_dev: &mut Device) -> Result {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Returns true if this is a suitable driver for the given phydev.
> +    /// If not implemented, matching is based on [`Driver::PHY_DEVICE_ID`].
> +    fn match_phy_device(_dev: &Device) -> bool {
> +        false
> +    }
> +
> +    /// Configures the advertisement and resets auto-negotiation
> +    /// if auto-negotiation is enabled.
> +    fn config_aneg(_dev: &mut Device) -> Result {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Determines the negotiated speed and duplex.
> +    fn read_status(_dev: &mut Device) -> Result<u16> {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Suspends the hardware, saving state if needed.
> +    fn suspend(_dev: &mut Device) -> Result {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// Resumes the hardware, restoring state if needed.
> +    fn resume(_dev: &mut Device) -> Result {
> +        Err(code::ENOTSUPP)
> +    }
> +
> +    /// 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)
> +    }
> +
> +    /// 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)
> +    }
> +
> +    /// Callback for notification of link change.
> +    fn link_change_notify(_dev: &mut Device) {}

It is probably an error if these functions are called, and so BUG() would be
appropriate? See the discussion in [1].

[1] https://lore.kernel.org/rust-for-linux/20231019171540.259173-1-benno.lossin@proton.me/

<cut>

> +
> +    // macro use only
> +    #[doc(hidden)]
> +    pub const fn mdio_device_id(&self) -> bindings::mdio_device_id {
> +        bindings::mdio_device_id {
> +            phy_id: self.id,
> +            phy_id_mask: self.mask.as_int(),
> +        }
> +    }

Would it make sense to move this function to the macro patch?

Best regards,
Andreas