Re: [RFC PATCH v1 0/4] Rust abstractions for network PHY drivers

[Trevor Gross <tmgross@umich.edu>]

Some meta design comments - why does registering need to allocate at
all? We should be able to keep the `phy_driver` in statics, like the C
side does.

Also, the size-generic `Registration` seems more complex that is
needed, which Benno was getting at. The C side already provides
`phy_drivers_register` and `phy_drivers_unregister` (drivers plural)
that can handle >1, so I don't think we need to create anything
special here.

I think the API could be simplified quite a bit with these things in
mind. A `Driver` can probably take over the `phy_id`, `phy_id_mask`,
and `name` fields which will more closely mirror `phy_driver` [1]:

    /// Used to do the same thing as `PHY_ID_MATCH_{EXACT, MODEL, VENDOR}`
    enum PhyIdMask {
        Exact,
        Model,
        Vendor,
        Custom(u32)
    }

    trait Driver {
        const PHY_ID: u32,
        const PHY_ID_MASK: PhyIdMask,
        const NAME: &'static CStr;
        // ...
    }

Then you need two const functions that can construct the
`mdio_device_id` table and the `phy_driver` structs (const functions
or macros, but I think functions are easier)

    const fn make_mdio_id<T: Driver>() -> Opaque<bindings::mdio_device_id> {
        let phy_id_mask = /* copy logic from existing C macros */;
        bindings::mdio_device_id { phy_id: T::PHY_ID, phy_id_mask }
    }

    const fn make_phy_device<T: Driver>() -> Opaque<bindings::phy_device> {
        // take this implementation from Adapter::driver
    }

And a type that can be used to unregister the devices on drop. This
doesn't need to be generic over N devices since it only ever needs to
call a single function.

    struct RegisteredHandle {
        unregister: unsafe fn()
    }

    impl Drop for RegisteredHandle {
        fn drop(&mut self) {
            // SAFETY: cannot be called more than once
            unsafe { self.0() };
        }
    }

Then, usage looks like this:

    struct RustAsixPhy {
        _reg: phy::RegisteredHandle,
    }

    impl kernel::Module for RustAsixPhy {
        fn init(module: &'static ThisModule) -> Result<Self> {
            pr_info!("Rust Asix phy driver\n");
            Ok(Self { _reg: register_fn_name()? } )
        }
    }

    // Takes one or more `impl Driver` structs. `modname` would just
be `asix` in real use
    phy_drivers!(modname, register_fn_name,  AsixAx8872a, AsixAx8872c,
AsixAc88796b);

The macro will expand to all of the below:

    /// A function that the user will call to register these modules.
Returns a handle that
    /// will unregister them on drop
    fn register_fn_name(module: &'static ThisModule) ->
Result<RegisteredHandle> {
        // We can use the existing functions that register multiple phys at once
        bindings::phy_drivers_register(
            &__modname_drivers,
            __modname_drivers.len(),
            module
        )?; // <- actually map this error...

        Ok(RegisteredHandle{
            unregister: || bindings::phy_drivers_unregister(
                &__modname_drivers, __modname_drivers.len()
            )
        })
    }

    /// Our `phy_device`s are stored here rather than in an allocation
    /// Don't need to be `#[no_mangle]` since C will never see this directly
    /// Does need to be mutable based on Andrew's comments
    pub static __modname_drivers: [Opaque<bindings::phy_device>; 3] = [
        make_phy_device::<AsixAx8872a>(),
        make_phy_device::<AsixAx8872c>(),
        make_phy_device::<AsixAx88796b>(),
    ];

    /// This will be the same as what exists - I assume this is read by
    /// the C side somehow?
    #[no_mangle]
    pub static __mod_mdio__modname_device_table:
        [Opaque<bindings::mdio_device_id>; 4] = [
        make_mdio_id::<AsixAx8872a>(),
        make_mdio_id::<AsixAx8872c>(),
        make_mdio_id::<AsixAx88796b>(),
        bindings::mdio_device_id { phy_id: 0, phy_id_mask: 0 }
    ];

    /// Bonus: we should be able to write a function that verifies the
phy IDs and
    /// their masks can actually all get matched
    const _: () = assert_correct_phy_ordering(__mod_mdio__modname_device_table);

This eliminates the need for allocation, a const-generic registration
struct, and the `Adapter` type (the associated functions on `Adapter`
could probably move into a `mod adapters` or similar). And
registration gets a bit more straightforward. I think this is a lot
more similar to what happens on the C side.

Any reasons the above wouldn't work?

[1]: @Fujita I know you have mentioned there are times where a driver
does not always have one mask. I can't find anything and am not sure
how this would work since `phy_driver` has `phy_id` and `phy_id_mask`
fields - do you mind linking an example? The above could probably be
adjusted to work with this