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

FUJITA Tomonori
2023-11-19T13:51:16Z

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

> FUJITA Tomonori <fujita.tomonori@gmail.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@gmail.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!