netdev - Re: [PATCH net-next v7 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: <20231119.225114.390963370394344723.fujita.tomonori@gmail.com>
Date: Sun, 19 Nov 2023 22:51:14 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: aliceryhl@...gle.com
Cc: fujita.tomonori@...il.com, andrew@...n.ch, benno.lossin@...ton.me,
 miguel.ojeda.sandonis@...il.com, netdev@...r.kernel.org,
 rust-for-linux@...r.kernel.org, tmgross@...ch.edu, wedsonaf@...il.com
Subject: Re: [PATCH net-next v7 1/5] rust: core abstractions for network
 PHY drivers

On Fri, 17 Nov 2023 09:39:05 +0000
Alice Ryhl <aliceryhl@...gle.com> wrote:

> FUJITA Tomonori <fujita.tomonori@...il.com> writes:
>> This patch adds abstractions to implement network PHY drivers; the
>> driver registration and bindings for some of callback functions in
>> struct phy_driver and many genphy_ functions.
>> 
>> This feature is enabled with CONFIG_RUST_PHYLIB_ABSTRACTIONS=y.
>> 
>> This patch enables unstable const_maybe_uninit_zeroed feature for
>> kernel crate to enable unsafe code to handle a constant value with
>> uninitialized data. With the feature, the abstractions can initialize
>> a phy_driver structure with zero easily; instead of initializing all
>> the members by hand. It's supposed to be stable in the not so distant
>> future.
>> 
>> Link: https://github.com/rust-lang/rust/pull/116218
>> 
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@...il.com>
> 
> In this reply, I go through my minor nits:
> 
>> +use crate::{
>> +    prelude::{vtable, Pin},
>> +};
> 
> Normally, if you're importing specific prelude items by name instead of
> using prelude::*, then you would import them using their non-prelude
> path.

Understood. I have no reason to import specific prelude items so I use
`prelude::*` instead.


>> +#[derive(PartialEq)]
>> +pub enum DeviceState {
> 
> If you add PartialEq and you can add Eq, then also add Eq.

Added Eq.


>> +/// An adapter for the registration of a PHY driver.
>> +struct Adapter<T: Driver> {
>> +    _p: PhantomData<T>,
>> +}
> 
> You don't need this struct. The methods can be top-level functions.
> 
> But I know that others have used the same style of defining a useless
> struct, so it's fine with me.

You meant like the following?

unsafe extern "C" fn soft_reset_callback<T: Driver>(
    phydev: *mut bindings::phy_device,
) -> core::ffi::c_int {
    from_result(|| {
        // SAFETY: Preconditions ensure `phydev` is valid.
        let dev = unsafe { Device::from_raw(phydev) };
        T::soft_reset(dev)?;
        Ok(0)
    })
}

pub const fn create_phy_driver<T: Driver>() -> DriverVTable {
    DriverVTable(Opaque::new(bindings::phy_driver {
	soft_reset: if T::HAS_SOFT_RESET {
            Some(soft_reset_callback::<T>)
        } else {
            None
	}
    }
}

I thought that a struct is used to group callbacks. Either is fine by
me though.


>> +    /// Defines certain other features this PHY supports.
>> +    /// It is a combination of the flags in the [`flags`] module.
>> +    const FLAGS: u32 = 0;
> 
> You need an empty line between the two lines if you intend for them to
> be separate lines in rendered documentation.

I don't intend to make them separate lines. If I make thme one line,
it's too long (over 100) so I made them two lines.


>> +#[vtable]
>> +pub trait Driver {
>> +    /// Issues a PHY software reset.
>> +    fn soft_reset(_dev: &mut Device) -> Result {
>> +        Err(code::ENOTSUPP)
>> +    }
>>      [...]
>> +}
> 
> I believe that the guidance for what to put in optional vtable-trait
> methods was changed in:
> 
> https://lore.kernel.org/all/20231026201855.1497680-1-benno.lossin@proton.me/

But VTABLE_DEFAULT_ERROR is defined in that patch, which isn't
merged. I'll update the code once that patch is merged.


>> +// SAFETY: `Registration` does not expose any of its state across threads.
>> +unsafe impl Send for Registration {}
> 
> I would change this to "it's okay to call phy_drivers_unregister from a
> different thread than the one in which phy_drivers_register was called".
> 
>> +// SAFETY: `Registration` does not expose any of its state across threads.
>> +unsafe impl Sync for Registration {}
> 
> Here, you can say "Registration has no &self methods, so immutable
> references to it are useless".

I'll reply to Trevor mail on this issue.


>> +    // 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(),
>> +        }
>> +    }
> 
> This is fine, but I probably would just expose it for everyone. It's not
> like it hurts if non-macro code can call this method, even if it doesn't
> have a reason to do so.

IIRC, someone said that drivers should not use bindings directly so
this function that returns bindings::mdio_device_id isn't exported.

Thanks!