Re: [PATCH v3 1/2] rust: net::phy unified read/write API for C22 and C45 registers

[Trevor Gross <tmgross@umich.edu>]

On Thu, Jun 6, 2024 at 7:03 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> On Thu, 6 Jun 2024 03:41:18 -0500
> Trevor Gross <tmgross@umich.edu> wrote:
>
> > On Sun, Jun 2, 2024 at 6:18 PM FUJITA Tomonori
> > <fujita.tomonori@gmail.com> wrote:
> [..]
> >> +use super::Device;
> >> +use crate::build_assert;
> >> +use crate::error::*;
> >> +use crate::uapi;
> >> +
> >> +/// Accesses PHY registers.
> >> +///
> >> +/// This trait is used to implement the unified interface to access
> >> +/// C22 and C45 PHY registers.
> >> +pub trait Register {
> >
> > Since we don't use this anywhere could it be sealed?
>
> Yeah. Only reg module implements it. How it can be done?
>
> diff --git a/rust/kernel/net/phy/reg.rs b/rust/kernel/net/phy/reg.rs
> index 7628eb09e902..0dfae4677574 100644
> --- a/rust/kernel/net/phy/reg.rs
> +++ b/rust/kernel/net/phy/reg.rs
> @@ -12,11 +12,18 @@
>  use crate::error::*;
>  use crate::uapi;
>
> +mod private {
> +    pub trait Sealed {}
> +
> +    impl Sealed for super::C22 {}
> +    impl Sealed for super::C45 {}
> +}
> +
>  /// Accesses PHY registers.
>  ///
>  /// This trait is used to implement the unified interface to access
>  /// C22 and C45 PHY registers.
> -pub trait Register {
> +pub trait Register: private::Sealed {
>      /// Reads a PHY register.
>      fn read(&self, dev: &mut Device) -> Result<u16>;
>
>
> > This might not even need to be public since it should probably only be
> > used as a method on `Device` (i.e. `my_device.read(C22::BMCR)` rather
> > than importing the trait and using `C22::BCMR::read(my_device)`).
>
> If I drop public, I got the following error:
>
> error[E0603]: trait `Register` is private
>    --> rust/kernel/net/phy.rs:181:25
>     |
> 181 |     pub fn read<R: reg::Register>(&mut self, reg: R) -> Result<u16> {
>     |                         ^^^^^^^^ private trait
>     |
> note: the trait `Register` is defined here
>    --> rust/kernel/net/phy/reg.rs:19:1
>     |
> 19  | trait Register {
>     | ^^^^^^^^^^^^^^:
>
> (snip)

Oh, I did mean `pub(crate)`.

If that doesn't work, usually the sealed pattern looks like this:

    /* lib.rs */
    mod private {
        /// Marker that a trait cannot be implemented outside of this crate
        trait Sealed {}
    }

    /* reg.rs or other modules */
    use crate::private::Sealed;

    // Traits that aren't meant to be implemented outside of the crate
    // get `Sealed` as a bound
    pub trait Register: Sealed { /* ... */ }

    // Anything with a `Register` impl now also needs `Sealed`
    impl Sealed for C22 {}
    impl Register for C22 { /* ... */ }

    // And then nobody from a different crate can implement `Register` on
    // some random type, because they have no way to access/implement
    // private `Sealed`

I also asked about this because I'm a bit surprised it hasn't come up yet [1].

But if `pub(crate)` works then that is easier, of course.

- Trevor

[1]: https://rust-for-linux.zulipchat.com/#narrow/stream/288089-General/topic/Sealed.20traits/near/443180412