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

[Benno Lossin <benno.lossin@proton.me>]

On 9/18/23 15:09, FUJITA Tomonori wrote:
> 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
> 
> ?

I would prefer https://crates.io/crates/bitflags
because this library does not create a separate type for multiple flags.

I will look into this and see with what I come up.

> 
>>   > +/// 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?
> 

Pushing a new value is a lot simpler, since it only is a single function 
call and not a loop. You also do not need a generic parameter.

> 
>>   > +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.

Actually, I talked with Gary and what I suggested is actually not 
sufficient, because `Box` means that we have a unique pointer, even 
though it contains an Opaque. So what we would suggest would be to use 
`Box::into_raw` then both store that pointer and give it to 
`phy_driver_register`.

> 
>> 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?
> 

If you do what I wrote above, then this is not a problem. The rationale 
behind this suggestion was to make it impossible to move the value in a 
future modification of this file.