Re: [RFC PATCH v1 1/4] rust: core abstractions for network PHY drivers

[FUJITA Tomonori <fujita.tomonori@gmail.com>]

Hi,

On Mon, 18 Sep 2023 10:22:22 +0000
Benno Lossin <benno.lossin@proton.me> wrote:

>  > +/// PHY is internal.
>  > +pub const PHY_IS_INTERNAL: u32 = 0x00000001;
>  > +/// PHY needs to be reset after the refclk is enabled.
>  > +pub const PHY_RST_AFTER_CLK_EN: u32 = 0x00000002;
>  > +/// Polling is used to detect PHY status changes.
>  > +pub const PHY_POLL_CABLE_TEST: u32 = 0x00000004;
>  > +/// Don't suspend.
>  > +pub const PHY_ALWAYS_CALL_SUSPEND: u32 = 0x00000008;
> 
> I think we should create a macro for creating flags, it should create a 
> newtype and
> work similar to an enum declaration. On the newtype it should implement 
> the `Or` trait
> for allowing `PHY_IS_INTERNAL | PHY_RST_AFTER_CLK_EN`.

You are thinking about something like:
https://crates.io/crates/enumflags2

?

>  > +/// Registration structure for a PHY driver.
>  > +///
>  > +/// `Registration` can keep multiple `phy_drivers` object because
>  > +/// commonly one module implements multiple PHYs drivers.
>  > +pub struct Registration<const N: usize> {
>  > +    module: &'static crate::ThisModule,
>  > +    drivers: [Option<Box<bindings::phy_driver>>; N],
> 
> Why is this not a vector? You have to allocate in `register` anyways.
> You could also use `Vec::try_with_capacity(N)` to initialize the vector with
> capacity for N elements.

I thought that this is simpler. What are advantages of Vec over this?


>  > +impl<const N: usize> Registration<{ N }> {
>  > +    /// Creates a new `Registration` instance.
>  > +    pub fn new(module: &'static crate::ThisModule) -> Self {
>  > +        const NONE: Option<Box<bindings::phy_driver>> = None;
>  > +        Registration {
>  > +            module,
>  > +            drivers: [NONE; N],
>  > +        }
>  > +    }
>  > +
>  > +    /// Registers a PHY driver.
>  > +    pub fn register<T: Driver>(&mut self, adapter: &Adapter<T>) -> 
> Result {
> 
> The way this function is used in the driver makes me think that the 
> `Adapter`
> type does not have to be public. So I would suggest to change the 
> signature to
> `pub fn register<T: Driver>(&mut self, name: &'static CStr) -> Result`.

Ah, right. I'll in the next round.

> 
>  > +        for i in 0..N {
>  > +            if self.drivers[i].is_none() {
>  > +                let mut drv = Box::try_new(adapter.driver())?;
>  > +                // SAFETY: Just an FFI call.
>  > +                let ret = unsafe { 
> bindings::phy_driver_register(drv.as_mut(), self.module.0) };
> 
> This call exposes the driver to the C side and thus it is able to be 
> modified at any time,
> which means that in Rust it should be put into an `UnsafeCell`, better 
> even in this case it
> should just be `Opaque`, so the `drivers` fields should be of type
> `[Option<Box<Opaque<bindings::phy_driver>>>]`.

Understood.

> It would also be a good idea to pin it, since the C side will rely on 
> the pointee not moving
> and it will prevent accidentally moving it.

The above drv is touched by only this file. What possible cases where
it would be moved?